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

[Steven Smith <sos22@xxxxxxxxx>]

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

Attachment: signature.asc
Description: Digital signature

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel