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

[Horms <horms@xxxxxxxxxxxx>]

On Tue, 11 Jul 2006 14:47:04 +0100, Steven Smith wrote:
> [-- multipart/signed, encoding 7bit, 2 lines --]
> 
>    [-- text/plain, encoding quoted-printable, charset: us-ascii, 85 lines --]
> 
>> >                             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.

Understood, thanks for the clarification.

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

Thanks, I missed the trailing ASSERT().

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