WARNING - OLD ARCHIVES

This is an archived copy of the Xen.org mailing list, which we have preserved to ensure that existing links to archives are not broken. The live archive, which contains the latest emails, can be found at http://lists.xen.org/
   
 
 
Xen 
 
Home Products Support Community News
 
   
 

xen-devel

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

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