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
|
Home