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