Re: [Xen-devel] [PATCH v2] libxl: Fix incorrect return of OSEVENT_HOOK macro

[Daniel De Graaf <dgdegra@xxxxxxxxxxxxx>]

On 05/09/2012 11:32 AM, Ian Jackson wrote:
> Daniel De Graaf writes ("[PATCH v2] libxl: Fix incorrect return of 
> OSEVENT_HOOK macro"):
>> Agreed, using the statement expression syntax seems cleaner here.
> ...
>> +#define OSEVENT_HOOK(hookname,...) ({                                       
>> \
>> +    int rc;                                                                 
>> \
>> +    if (CTX->osevent_hooks) {                                               
>> \
>> +        CTX->osevent_in_hook++;                                             
>> \
>> +        rc = CTX->osevent_hooks->hookname(CTX->osevent_user, __VA_ARGS__);  
>> \
>> +        CTX->osevent_in_hook--;                                             
>> \
>> +    } else {                                                                
>> \
>> +        rc = 0;                                                             
>> \
>> +    }                                                                       
>> \
>> +    rc;                                                                     
>> \
>> +})
> 
> Thanks.  However, you need to use a different variable name to "rc".
> "rc" might theoretically be present int __VA_ARGS__ in which case this
> will go wrong.  Also if we ever enable -Wshadow, this construction
> will cause a warning.  I suggest   int osevent_hook_rc;
> 
> You could preserve the unification of the two macros by having
> 
>  #define OSEVENT_HOOK(hookname,...)                                           
> \
>      OSEVENT_HOOK_INTERN(osevent_hook_rc =, /*empty*/, hookname, __VA_ARGS__)
> 
>  #define OSEVENT_HOOK_VOID(hookname,...)                                      
> \
>      OSEVENT_HOOK_INTERN(/*empty*/,         (void),    hookname, __VA_ARGS__)
> 
> or some such.  I don't know whether you like that idea; if you do
> please go ahead and do it.  If not then I'm happy to take the patch
> which duplicates the macro.
> 
> Ian.
> 

I like that method (or this slight tweak to it):

-----------8<-------------------------

The OSEVENT_HOOK_INTERN macro incorrectly returned the value of the
expression CTX->osevent_in_hook-- (usually 1) instead of the value of
the function call it made. Fix the macro to return the proper value.

Signed-off-by: Daniel De Graaf <dgdegra@xxxxxxxxxxxxx>
---
 tools/libxl/libxl_event.c |   28 ++++++++++++++++------------
 1 files changed, 16 insertions(+), 12 deletions(-)

diff --git a/tools/libxl/libxl_event.c b/tools/libxl/libxl_event.c
index 638b9ab..a31aec7 100644
--- a/tools/libxl/libxl_event.c
+++ b/tools/libxl/libxl_event.c
@@ -27,18 +27,22 @@
  * these macros, with the ctx locked.  Likewise all the "occurred"
  * entrypoints from the application should assert(!in_hook);
  */
-#define OSEVENT_HOOK_INTERN(defval, hookname, ...)                      \
-    (CTX->osevent_hooks                                                 \
-     ? (CTX->osevent_in_hook++,                                         \
-        CTX->osevent_hooks->hookname(CTX->osevent_user, __VA_ARGS__),   \
-        CTX->osevent_in_hook--)                                         \
-     : defval)
-
-#define OSEVENT_HOOK(hookname,...)                      \
-    OSEVENT_HOOK_INTERN(0, hookname, __VA_ARGS__)
-
-#define OSEVENT_HOOK_VOID(hookname,...)                 \
-    OSEVENT_HOOK_INTERN((void)0, hookname, __VA_ARGS__)
+#define OSEVENT_HOOK_INTERN(retval, hookname, ...) do {                      \
+    if (CTX->osevent_hooks) {                                                \
+        CTX->osevent_in_hook++;                                              \
+        retval CTX->osevent_hooks->hookname(CTX->osevent_user, __VA_ARGS__); \
+        CTX->osevent_in_hook--;                                              \
+    }                                                                        \
+} while (0)
+
+#define OSEVENT_HOOK(hookname, ...) ({                                       \
+    int osevent_hook_rc = 0;                                                 \
+    OSEVENT_HOOK_INTERN(osevent_hook_rc = , hookname, __VA_ARGS__);          \
+    osevent_hook_rc;                                                         \
+})
+
+#define OSEVENT_HOOK_VOID(hookname, ...) \
+    OSEVENT_HOOK_INTERN(/* void */, hookname, __VA_ARGS__)
 
 /*
  * fd events


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel