Hello Keir, could you apply the following cleanup patch? Cheers Gregor _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Hi Grzegorz, I''ve made a few coding-style comments inline. -- Horms H: http://www.vergenet.net/~horms/ W: http://www.valinux.co.jp/en/ On Wed, 5 Jul 2006 11:22:20 +0100, Grzegorz Milos wrote:> > --Apple-Mail-3--158434814 > Content-Transfer-Encoding: 7bit > Content-Type: text/plain; > charset=US-ASCII; > format=flowed > > Hello Keir, could you apply the following cleanup patch? > > Cheers > Gregor > > > --Apple-Mail-3--158434814 > Content-Transfer-Encoding: 7bit > Content-Type: application/octet-stream; x-unix-mode=0644; > name="minios.events.patch" > Content-Disposition: attachment; > filename=minios.events.patch > > # HG changeset patch > # User gm281@brent.cl.cam.ac.uk > # Node ID ef01229d6344d7ff1c8970512fdb812445ec3430 > # Parent c3d6610fc0f0510a09bd3d6c63a4e8a3f78312cc > Mini-OS: Events handling cleaned up. The interface extended to provide void* pointer to handlers. > > Signed-off-by: Steven Smith <sos22@cam.ac.uk> > Signed-off-by: Grzegorz Milos <gm281@cam.ac.uk> > > diff -r c3d6610fc0f0 -r ef01229d6344 extras/mini-os/console/xencons_ring.c > --- a/extras/mini-os/console/xencons_ring.c Wed Jul 5 10:06:14 2006 > +++ b/extras/mini-os/console/xencons_ring.c Wed Jul 5 10:20:15 2006 > @@ -53,7 +53,7 @@ > > > > -static void handle_input(int port, struct pt_regs *regs) > +static void handle_input(int port, struct pt_regs *regs, void *ign) > { > struct xencons_interface *intf = xencons_interface(); > XENCONS_RING_IDX cons, prod; > @@ -83,7 +83,8 @@ > if (!start_info.console_evtchn) > return 0; > > - err = bind_evtchn(start_info.console_evtchn, handle_input); > + err = bind_evtchn(start_info.console_evtchn, handle_input, > + NULL);Could you move the NULL ont the same line as bind_evtchn( ?> if (err <= 0) { > printk("XEN console request chn bind failed %i\n", err); > return err; > diff -r c3d6610fc0f0 -r ef01229d6344 extras/mini-os/events.c > --- a/extras/mini-os/events.c Wed Jul 5 10:06:14 2006 > +++ b/extras/mini-os/events.c Wed Jul 5 10:20:15 2006 > @@ -22,9 +22,18 @@ > #include <events.h> > #include <lib.h> > > +#define NR_EVS 1024 > + > +/* this represents a event handler. Chaining or sharing is not allowed */ > +typedef struct _ev_action_t { > + void (*handler)(int, struct pt_regs *, void *); > + void *data; > + u32 count; > +} ev_action_t; > +Tabs and spaces are used inconsistently in the above structure definition.> static ev_action_t ev_actions[NR_EVS]; > -void default_handler(int port, struct pt_regs *regs); > +void default_handler(int port, struct pt_regs *regs, void *data); > > > /* > @@ -35,42 +44,33 @@ > ev_action_t *action; > if (port >= NR_EVS) { > printk("Port number too large: %d\n", port); > - goto out; > + goto out; > } > > action = &ev_actions[port]; > action->count++; > > - if (!action->handler) > - { > - printk("Spurious event on port %d\n", port); > - goto out; > - } > - > - if (action->status & EVS_DISABLED) > - { > - printk("Event on port %d disabled\n", port); > - goto out; > - } > - > /* call the handler */ > - action->handler(port, regs); > - > + action->handler(port, regs, action->data); > + > out: > clear_evtchn(port); > + > return 1; > > } > > -int bind_evtchn( u32 port, void (*handler)(int, struct pt_regs *) ) > +int bind_evtchn( u32 port, void (*handler)(int, struct pt_regs *, void *), > + void *data ) > { > if(ev_actions[port].handler != default_handler) > printk("WARN: Handler for port %d already registered, replacing\n", > port); > > + ev_actions[port].data = data; > + wmb(); > ev_actions[port].handler = handler; > - ev_actions[port].status &= ~EVS_DISABLED; > - > + > /* Finally unmask the port */ > unmask_evtchn(port); > > @@ -82,13 +82,14 @@ > if (ev_actions[port].handler == default_handler) > printk("WARN: No handler for port %d when unbinding\n", port); > ev_actions[port].handler = default_handler; > - ev_actions[port].status |= EVS_DISABLED; > + wmb(); > + ev_actions[port].data = NULL; > }I''m not expert here, but the wmb() additions seem a bit odd. Are the neccessary?> > -int bind_virq( u32 virq, void (*handler)(int, struct pt_regs *) ) > +int bind_virq( u32 virq, void (*handler)(int, struct pt_regs *, void *data), > + void *data) > { > evtchn_op_t op; > - int ret = 0; > > /* Try to bind the virq to a port */ > op.cmd = EVTCHNOP_bind_virq; > @@ -97,13 +98,11 @@ > > if ( HYPERVISOR_event_channel_op(&op) != 0 ) > { > - ret = 1; > printk("Failed to bind virtual IRQ %d\n", virq); > - goto out; > + return 1; > } > - bind_evtchn(op.u.bind_virq.port, handler); > -out: > - return ret; > + bind_evtchn(op.u.bind_virq.port, handler, data); > + return 0; > } > > void unbind_virq( u32 port ) > @@ -137,13 +136,38 @@ > #endif > /* inintialise event handler */ > for ( i = 0; i < NR_EVS; i++ ) > - { > - ev_actions[i].status = EVS_DISABLED; > + { > ev_actions[i].handler = default_handler; > mask_evtchn(i); > } > } > > -void default_handler(int port, struct pt_regs *regs) { > +void default_handler(int port, struct pt_regs *regs, void *ignore) > +{ > printk("[Port %d] - event received\n", port); > } > + > +/* Unfortunate confusion of terminology: the port is unbound as far > + as Xen is concerned, but we automatically bind a handler to it > + from inside mini-os. */ > +int evtchn_alloc_unbound(void (*handler)(int, struct pt_regs *regs, > + void *data), > + void *data) > +{ > + u32 port; > + evtchn_op_t op; > + int err; > + > + op.cmd = EVTCHNOP_alloc_unbound; > + op.u.alloc_unbound.dom = DOMID_SELF; > + op.u.alloc_unbound.remote_dom = 0; > + > + err = HYPERVISOR_event_channel_op(&op); > + if (err) { > + printk("Failed to alloc unbound evtchn: %d.\n", err); > + return -1; > + } > + port = op.u.alloc_unbound.port; > + bind_evtchn(port, handler, data); > + return port; > +} > diff -r c3d6610fc0f0 -r ef01229d6344 extras/mini-os/include/events.h > --- a/extras/mini-os/include/events.h Wed Jul 5 10:06:14 2006 > +++ b/extras/mini-os/include/events.h Wed Jul 5 10:20:15 2006 > @@ -22,28 +22,18 @@ > #include<traps.h> > #include <xen/event_channel.h> > > -#define NR_EVS 1024 > - > -/* ev handler status */ > -#define EVS_INPROGRESS 1 /* Event handler active - do not enter! */ > -#define EVS_DISABLED 2 /* Event disabled - do not enter! */ > -#define EVS_PENDING 4 /* Event pending - replay on enable */ > -#define EVS_REPLAY 8 /* Event has been replayed but not acked yet */ > - > -/* this represents a event handler. Chaining or sharing is not allowed */ > -typedef struct _ev_action_t { > - void (*handler)(int, struct pt_regs *); > - unsigned int status; /* IRQ status */ > - u32 count; > -} ev_action_t; > - > /* prototypes */ > int do_event(u32 port, struct pt_regs *regs); > -int bind_virq( u32 virq, void (*handler)(int, struct pt_regs *) ); > -int bind_evtchn( u32 virq, void (*handler)(int, struct pt_regs *) ); > +int bind_virq( u32 virq, void (*handler)(int, struct pt_regs *, void *data), > + void *data); > +int bind_evtchn( u32 virq, void (*handler)(int, struct pt_regs *, void *data), > + void *data ); > void unbind_evtchn( u32 port ); > void init_events(void); > void unbind_virq( u32 port ); > +int evtchn_alloc_unbound(void (*handler)(int, struct pt_regs *regs, > + void *data), > + void *data); > > static inline int notify_remote_via_evtchn(int port) > { > diff -r c3d6610fc0f0 -r ef01229d6344 extras/mini-os/include/lib.h > --- a/extras/mini-os/include/lib.h Wed Jul 5 10:06:14 2006 > +++ b/extras/mini-os/include/lib.h Wed Jul 5 10:20:15 2006 > @@ -89,6 +89,7 @@ > char *strchr(const char *s, int c); > char *strstr(const char *s1, const char *s2); > char * strcat(char * dest, const char * src); > +char *strdup(const char *s);Is this strdup addition related to the rest of the patch?> #define ARRAY_SIZE(x) (sizeof(x) / sizeof((x)[0])) > @@ -98,6 +99,18 @@ > size_t iov_len; > }; > > +#define ASSERT(x) \ > +do { \ > + if (!(x)) { \ > + printk("ASSERTION FAILED: %s at %s:%d.\n", \ > + # x , \ > + __FILE__, \ > + __LINE__); \Could the above 4 lines be merged into one or two? Also, is the do {} while(0) blocl neccessary if evertthing is inside an if() { } block ?> + BUG(); \ > + } \ > +} while(0) > > +/* Consistency check as much as possible. */ > +void sanity_check(void); > > #endif /* _LIB_H_ */ > diff -r c3d6610fc0f0 -r ef01229d6344 extras/mini-os/lib/string.c > --- a/extras/mini-os/lib/string.c Wed Jul 5 10:06:14 2006 > +++ b/extras/mini-os/lib/string.c Wed Jul 5 10:20:15 2006 > @@ -23,6 +23,7 @@ > #include <os.h> > #include <types.h> > #include <lib.h> > +#include <xmalloc.h> > > int memcmp(const void * cs,const void * ct,size_t count) > { > @@ -156,4 +157,13 @@ > return NULL; > } > > +char *strdup(const char *x) > +{ > + int l = strlen(x); > + char *res = malloc(l + 1); > + if (!res) return NULL; > + memcpy(res, x, l + 1); > + return res; > +} > + > #endifAgain, strdup.> diff -r c3d6610fc0f0 -r ef01229d6344 extras/mini-os/time.c > --- a/extras/mini-os/time.c Wed Jul 5 10:06:14 2006 > +++ b/extras/mini-os/time.c Wed Jul 5 10:20:15 2006 > @@ -215,7 +215,7 @@ > /* > * Just a dummy > */ > -static void timer_handler(int ev, struct pt_regs *regs) > +static void timer_handler(int ev, struct pt_regs *regs, void *ign) > { > static int i; > > @@ -233,5 +233,5 @@ > void init_time(void) > { > printk("Initialising timer interface\n"); > - bind_virq(VIRQ_TIMER, &timer_handler); > -} > + bind_virq(VIRQ_TIMER, &timer_handler, NULL); > +} > diff -r c3d6610fc0f0 -r ef01229d6344 extras/mini-os/xenbus/xenbus.c > --- a/extras/mini-os/xenbus/xenbus.c Wed Jul 5 10:06:14 2006 > +++ b/extras/mini-os/xenbus/xenbus.c Wed Jul 5 10:20:15 2006 > @@ -112,7 +112,7 @@ > } > } > > -static void xenbus_evtchn_handler(int port, struct pt_regs *regs) > +static void xenbus_evtchn_handler(int port, struct pt_regs *regs, void *ign) > { > wake_up(&xb_waitq); > } > @@ -174,7 +174,8 @@ > create_thread("xenstore", xenbus_thread_func, NULL); > DEBUG("buf at %p.\n", xenstore_buf); > err = bind_evtchn(start_info.store_evtchn, > - xenbus_evtchn_handler); > + xenbus_evtchn_handler, > + NULL);Again, could the NULL be appended to the previous line?> DEBUG("xenbus on irq %d\n", err); > } > > > --Apple-Mail-3--158434814 > Content-Type: text/plain; charset="us-ascii" > MIME-Version: 1.0 > Content-Transfer-Encoding: 7bit > Content-Disposition: inline > > _______________________________________________ > Xen-devel mailing list > Xen-devel@lists.xensource.com > http://lists.xensource.com/xen-devel > > --Apple-Mail-3--158434814-- > >_______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Steven Smith
2006-Jul-11 13:47 UTC
[Xen-devel] Re: [PATCH] Mini-OS events handling cleanup
> > port); > > > > + ev_actions[port].data = data; > > + wmb(); > > ev_actions[port].handler = handler; > > - ev_actions[port].status &= ~EVS_DISABLED; > > > > /* Finally unmask the port */ > > unmask_evtchn(port); > > > > @@ -82,13 +82,14 @@ > > if (ev_actions[port].handler == default_handler) > > printk("WARN: No handler for port %d when unbinding\n", port); > > ev_actions[port].handler = default_handler; > > - ev_actions[port].status |= EVS_DISABLED; > > + wmb(); > > + ev_actions[port].data = NULL; > > } > I''m not expert here, but the wmb() additions seem a bit odd. > Are the neccessary?When I wrote this, I was worried that the compiler might decide to reorder the writes to handler and data, in which case there might be a window during which you could get an interrupt which would call the user-supplied handler with the wrong data. The wmb()s protect against this.> > diff -r c3d6610fc0f0 -r ef01229d6344 extras/mini-os/include/lib.h > > --- a/extras/mini-os/include/lib.h Wed Jul 5 10:06:14 2006 > > +++ b/extras/mini-os/include/lib.h Wed Jul 5 10:20:15 2006 > > @@ -89,6 +89,7 @@ > > char *strchr(const char *s, int c); > > char *strstr(const char *s1, const char *s2); > > char * strcat(char * dest, const char * src); > > +char *strdup(const char *s); > Is this strdup addition related to the rest of the patch?No, sorry, that was supposed to be part of a different patch.> > #define ARRAY_SIZE(x) (sizeof(x) / sizeof((x)[0])) > > @@ -98,6 +99,18 @@ > > size_t iov_len; > > }; > > > > +#define ASSERT(x) \ > > +do { \ > > + if (!(x)) { \ > > + printk("ASSERTION FAILED: %s at %s:%d.\n", \ > > + # x , \ > > + __FILE__, \ > > + __LINE__); \ > Could the above 4 lines be merged into one or two?They could be, but I think the current definition is a bit clearer.> Also, is the do {} while(0) blocl neccessary if evertthing is > inside an if() { } block ?Yes: if () {} statements don''t need a semicolon at the end, whereas do {} while(0)s do, and we want ASSERT() to need a trailing semicolon. More explicitly: if (x) ASSERT(y); else z; should be equivalent to if (x) { ASSERT(y); } else { z; } and, with the current definition, it is. If you don''t have the while(0) business, it would macro expand to if (x) if (!(y)) { printk(...); BUG(); }; else z; which is a parse error because of the extra semicolon. The general formatting comments I''ll leave to someone a bit more familiar with the mini-os coding standards, such as they are. Steven. _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
On Tue, 11 Jul 2006 14:47:04 +0100, Steven Smith wrote:> [-- multipart/signed, encoding 7bit, 2 lines --] > > [-- text/plain, encoding quoted-printable, charset: us-ascii, 85 lines --] > >> > port); >> > >> > + ev_actions[port].data = data; >> > + wmb(); >> > ev_actions[port].handler = handler; >> > - ev_actions[port].status &= ~EVS_DISABLED; >> > >> > /* Finally unmask the port */ >> > unmask_evtchn(port); >> > >> > @@ -82,13 +82,14 @@ >> > if (ev_actions[port].handler == default_handler) >> > printk("WARN: No handler for port %d when unbinding\n", port); >> > ev_actions[port].handler = default_handler; >> > - ev_actions[port].status |= EVS_DISABLED; >> > + wmb(); >> > + ev_actions[port].data = NULL; >> > } >> I''m not expert here, but the wmb() additions seem a bit odd. >> Are the neccessary? > When I wrote this, I was worried that the compiler might decide to > reorder the writes to handler and data, in which case there might be a > window during which you could get an interrupt which would call the > user-supplied handler with the wrong data. The wmb()s protect against > this.Understood, thanks for the clarification.>> > diff -r c3d6610fc0f0 -r ef01229d6344 extras/mini-os/include/lib.h >> > --- a/extras/mini-os/include/lib.h Wed Jul 5 10:06:14 2006 >> > +++ b/extras/mini-os/include/lib.h Wed Jul 5 10:20:15 2006 >> > @@ -89,6 +89,7 @@ >> > char *strchr(const char *s, int c); >> > char *strstr(const char *s1, const char *s2); >> > char * strcat(char * dest, const char * src); >> > +char *strdup(const char *s); >> Is this strdup addition related to the rest of the patch? > No, sorry, that was supposed to be part of a different patch. > >> > #define ARRAY_SIZE(x) (sizeof(x) / sizeof((x)[0])) >> > @@ -98,6 +99,18 @@ >> > size_t iov_len; >> > }; >> > >> > +#define ASSERT(x) \ >> > +do { \ >> > + if (!(x)) { \ >> > + printk("ASSERTION FAILED: %s at %s:%d.\n", \ >> > + # x , \ >> > + __FILE__, \ >> > + __LINE__); \ >> Could the above 4 lines be merged into one or two? > They could be, but I think the current definition is a bit clearer. > >> Also, is the do {} while(0) blocl neccessary if evertthing is >> inside an if() { } block ? > Yes: if () {} statements don''t need a semicolon at the end, whereas > do {} while(0)s do, and we want ASSERT() to need a trailing semicolon. > More explicitly: > > if (x) > ASSERT(y); > else > z; > > should be equivalent to > > if (x) { ASSERT(y); } else { z; } > > and, with the current definition, it is. If you don''t have the > while(0) business, it would macro expand to > > if (x) > if (!(y)) { > printk(...); > BUG(); > }; > else > z; > > which is a parse error because of the extra semicolon.Thanks, I missed the trailing ASSERT(). -- Horms H: http://www.vergenet.net/~horms/ W: http://www.valinux.co.jp/en/ _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Grzegorz Milos
2006-Jul-12 07:24 UTC
[Xen-devel] Re: [PATCH] Mini-OS events handling cleanup
Hi Grzegorz, I''ve made a few coding-style comments inline. Steven already answerd the most difficult questions, here is the reminder:>> >> --Apple-Mail-3--158434814 >> Content-Transfer-Encoding: 7bit >> Content-Type: application/octet-stream; x-unix-mode=0644; >> name="minios.events.patch" >> Content-Disposition: attachment; >> filename=minios.events.patch >> >> # HG changeset patch >> # User gm281@brent.cl.cam.ac.uk >> # Node ID ef01229d6344d7ff1c8970512fdb812445ec3430 >> # Parent c3d6610fc0f0510a09bd3d6c63a4e8a3f78312cc >> Mini-OS: Events handling cleaned up. The interface extended to >> provide void* pointer to handlers. >> >> Signed-off-by: Steven Smith <sos22@cam.ac.uk> >> Signed-off-by: Grzegorz Milos <gm281@cam.ac.uk> >> >> diff -r c3d6610fc0f0 -r ef01229d6344 extras/mini-os/console/ >> xencons_ring.c >> --- a/extras/mini-os/console/xencons_ring.c Wed Jul 5 10:06:14 2006 >> +++ b/extras/mini-os/console/xencons_ring.c Wed Jul 5 10:20:15 2006 >> @@ -53,7 +53,7 @@ >> >> >> >> -static void handle_input(int port, struct pt_regs *regs) >> +static void handle_input(int port, struct pt_regs *regs, void *ign) >> { >> struct xencons_interface *intf = xencons_interface(); >> XENCONS_RING_IDX cons, prod; >> @@ -83,7 +83,8 @@ >> if (!start_info.console_evtchn) >> return 0; >> >> - err = bind_evtchn(start_info.console_evtchn, handle_input); >> + err = bind_evtchn(start_info.console_evtchn, handle_input, >> + NULL); > > Could you move the NULL ont the same line as bind_evtchn( ? >Would the line still be shorter then 80 characters? (that was the reason to put NULL on the next line).>> if (err <= 0) { >> printk("XEN console request chn bind failed %i\n", err); >> return err; >> diff -r c3d6610fc0f0 -r ef01229d6344 extras/mini-os/events.c >> --- a/extras/mini-os/events.c Wed Jul 5 10:06:14 2006 >> +++ b/extras/mini-os/events.c Wed Jul 5 10:20:15 2006 >> @@ -22,9 +22,18 @@ >> #include <events.h> >> #include <lib.h> >> >> +#define NR_EVS 1024 >> + >> +/* this represents a event handler. Chaining or sharing is not >> allowed */ >> +typedef struct _ev_action_t { >> + void (*handler)(int, struct pt_regs *, void *); >> + void *data; >> + u32 count; >> +} ev_action_t; >> + > > Tabs and spaces are used inconsistently in the above structure > definition.Ok - will change that.> >> static ev_action_t ev_actions[NR_EVS]; >> -void default_handler(int port, struct pt_regs *regs); >> +void default_handler(int port, struct pt_regs *regs, void *data); >> >> >> /* >> @@ -35,42 +44,33 @@ >> ev_action_t *action; >> if (port >= NR_EVS) { >> printk("Port number too large: %d\n", port); >> - goto out; >> + goto out; >> } >> >> action = &ev_actions[port]; >> action->count++; >> >> - if (!action->handler) >> - { >> - printk("Spurious event on port %d\n", port); >> - goto out; >> - } >> - >> - if (action->status & EVS_DISABLED) >> - { >> - printk("Event on port %d disabled\n", port); >> - goto out; >> - } >> - >> /* call the handler */ >> - action->handler(port, regs); >> - >> + action->handler(port, regs, action->data); >> + >> out: >> clear_evtchn(port); >> + >> return 1; >> >> } >> >> -int bind_evtchn( u32 port, void (*handler)(int, struct pt_regs *) ) >> +int bind_evtchn( u32 port, void (*handler)(int, struct pt_regs *, >> void *), >> + void *data ) >> { >> if(ev_actions[port].handler != default_handler) >> printk("WARN: Handler for port %d already registered, >> replacing\n", >> port); >> >> + ev_actions[port].data = data; >> + wmb(); >> ev_actions[port].handler = handler; >> - ev_actions[port].status &= ~EVS_DISABLED; >> - >> + >> /* Finally unmask the port */ >> unmask_evtchn(port); >> >> @@ -82,13 +82,14 @@ >> if (ev_actions[port].handler == default_handler) >> printk("WARN: No handler for port %d when unbinding\n", port); >> ev_actions[port].handler = default_handler; >> - ev_actions[port].status |= EVS_DISABLED; >> + wmb(); >> + ev_actions[port].data = NULL; >> } >> >> -static void xenbus_evtchn_handler(int port, struct pt_regs *regs) >> +static void xenbus_evtchn_handler(int port, struct pt_regs *regs, >> void *ign) >> { >> wake_up(&xb_waitq); >> } >> @@ -174,7 +174,8 @@ >> create_thread("xenstore", xenbus_thread_func, NULL); >> DEBUG("buf at %p.\n", xenstore_buf); >> err = bind_evtchn(start_info.store_evtchn, >> - xenbus_evtchn_handler); >> + xenbus_evtchn_handler, >> + NULL); > > Again, could the NULL be appended to the previous line?If the lines become too long to put more then one argument I generally separate each argument into a new line. This seems to be slightly more readable when you trying to find nth argument. Cheers Gregor _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
On Wed, Jul 12, 2006 at 08:24:28AM +0100, Grzegorz Milos wrote:> Hi Grzegorz, > > I''ve made a few coding-style comments inline. > > > Steven already answerd the most difficult questions, here is the > reminder: > >> > >>--Apple-Mail-3--158434814 > >>Content-Transfer-Encoding: 7bit > >>Content-Type: application/octet-stream; x-unix-mode=0644; > >> name="minios.events.patch" > >>Content-Disposition: attachment; > >> filename=minios.events.patch > >> > >># HG changeset patch > >># User gm281@brent.cl.cam.ac.uk > >># Node ID ef01229d6344d7ff1c8970512fdb812445ec3430 > >># Parent c3d6610fc0f0510a09bd3d6c63a4e8a3f78312cc > >>Mini-OS: Events handling cleaned up. The interface extended to > >>provide void* pointer to handlers. > >> > >>Signed-off-by: Steven Smith <sos22@cam.ac.uk> > >>Signed-off-by: Grzegorz Milos <gm281@cam.ac.uk> > >> > >>diff -r c3d6610fc0f0 -r ef01229d6344 extras/mini-os/console/ > >>xencons_ring.c > >>--- a/extras/mini-os/console/xencons_ring.c Wed Jul 5 10:06:14 2006 > >>+++ b/extras/mini-os/console/xencons_ring.c Wed Jul 5 10:20:15 2006 > >>@@ -53,7 +53,7 @@ > >> > >> > >> > >>-static void handle_input(int port, struct pt_regs *regs) > >>+static void handle_input(int port, struct pt_regs *regs, void *ign) > >>{ > >> struct xencons_interface *intf = xencons_interface(); > >> XENCONS_RING_IDX cons, prod; > >>@@ -83,7 +83,8 @@ > >> if (!start_info.console_evtchn) > >> return 0; > >> > >>- err = bind_evtchn(start_info.console_evtchn, handle_input); > >>+ err = bind_evtchn(start_info.console_evtchn, handle_input, > >>+ NULL); > > > >Could you move the NULL ont the same line as bind_evtchn( ? > > > > Would the line still be shorter then 80 characters? (that was the > reason to put NULL on the next line).It looks like it would be, though if not the newline is the way to go.> >> if (err <= 0) { > >> printk("XEN console request chn bind failed %i\n", err); > >> return err; > >>diff -r c3d6610fc0f0 -r ef01229d6344 extras/mini-os/events.c > >>--- a/extras/mini-os/events.c Wed Jul 5 10:06:14 2006 > >>+++ b/extras/mini-os/events.c Wed Jul 5 10:20:15 2006 > >>@@ -22,9 +22,18 @@ > >>#include <events.h> > >>#include <lib.h> > >> > >>+#define NR_EVS 1024 > >>+ > >>+/* this represents a event handler. Chaining or sharing is not > >>allowed */ > >>+typedef struct _ev_action_t { > >>+ void (*handler)(int, struct pt_regs *, void *); > >>+ void *data; > >>+ u32 count; > >>+} ev_action_t; > >>+ > > > >Tabs and spaces are used inconsistently in the above structure > >definition. > > Ok - will change that. > > > > >>static ev_action_t ev_actions[NR_EVS]; > >>-void default_handler(int port, struct pt_regs *regs); > >>+void default_handler(int port, struct pt_regs *regs, void *data); > >> > >> > >>/* > >>@@ -35,42 +44,33 @@ > >> ev_action_t *action; > >> if (port >= NR_EVS) { > >> printk("Port number too large: %d\n", port); > >>- goto out; > >>+ goto out; > >> } > >> > >> action = &ev_actions[port]; > >> action->count++; > >> > >>- if (!action->handler) > >>- { > >>- printk("Spurious event on port %d\n", port); > >>- goto out; > >>- } > >>- > >>- if (action->status & EVS_DISABLED) > >>- { > >>- printk("Event on port %d disabled\n", port); > >>- goto out; > >>- } > >>- > >> /* call the handler */ > >>- action->handler(port, regs); > >>- > >>+ action->handler(port, regs, action->data); > >>+ > >> out: > >> clear_evtchn(port); > >>+ > >> return 1; > >> > >>} > >> > >>-int bind_evtchn( u32 port, void (*handler)(int, struct pt_regs *) ) > >>+int bind_evtchn( u32 port, void (*handler)(int, struct pt_regs *, > >>void *), > >>+ void *data ) > >>{ > >> if(ev_actions[port].handler != default_handler) > >> printk("WARN: Handler for port %d already registered, > >>replacing\n", > >> port); > >> > >>+ ev_actions[port].data = data; > >>+ wmb(); > >> ev_actions[port].handler = handler; > >>- ev_actions[port].status &= ~EVS_DISABLED; > >>- > >>+ > >> /* Finally unmask the port */ > >> unmask_evtchn(port); > >> > >>@@ -82,13 +82,14 @@ > >> if (ev_actions[port].handler == default_handler) > >> printk("WARN: No handler for port %d when unbinding\n", > >> port); > >> ev_actions[port].handler = default_handler; > >>- ev_actions[port].status |= EVS_DISABLED; > >>+ wmb(); > >>+ ev_actions[port].data = NULL; > >>} > >> > >>-static void xenbus_evtchn_handler(int port, struct pt_regs *regs) > >>+static void xenbus_evtchn_handler(int port, struct pt_regs *regs, > >>void *ign) > >>{ > >> wake_up(&xb_waitq); > >>} > >>@@ -174,7 +174,8 @@ > >> create_thread("xenstore", xenbus_thread_func, NULL); > >> DEBUG("buf at %p.\n", xenstore_buf); > >> err = bind_evtchn(start_info.store_evtchn, > >>- xenbus_evtchn_handler); > >>+ xenbus_evtchn_handler, > >>+ NULL); > > > >Again, could the NULL be appended to the previous line? > > If the lines become too long to put more then one argument I > generally separate each argument into a new line. This seems to be > slightly more readable when you trying to find nth argument.Ok, I''m not sure if there are any hard and fast rules there for xen coding style, which I assume mini-os follows. Perhaps someone else can comment. -- Horms H: http://www.vergenet.net/~horms/ W: http://www.valinux.co.jp/en/ _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel