[Xen-devel] Re: [PATCH] Mini-OS events handling cleanup

[Grzegorz Milos <gm281@xxxxxxxxx>]

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@xxxxxxxxxxxxxxxxxx
> > # 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@xxxxxxxxx>
> > Signed-off-by: Grzegorz Milos <gm281@xxxxxxxxx>
> >
> > 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@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel