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

[Horms <horms@xxxxxxxxxxxx>]

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@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).

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@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel