> > 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.
signature.asc
Description: Digital signature
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel
|
Home