From mboxrd@z Thu Jan 1 00:00:00 1970 From: Daniel De Graaf Subject: Re: [PATCH v2] libxl: Fix incorrect return of OSEVENT_HOOK macro Date: Wed, 09 May 2012 12:42:45 -0400 Message-ID: <4FAA9E85.9010007@tycho.nsa.gov> References: <1336418109-6335-1-git-send-email-dgdegra@tycho.nsa.gov> <20394.28369.936242.527916@mariner.uk.xensource.com> <4FAA7354.5070802@tycho.nsa.gov> <20394.36387.49258.141494@mariner.uk.xensource.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <20394.36387.49258.141494@mariner.uk.xensource.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Ian Jackson Cc: Ian Campbell , "xen-devel@lists.xen.org" List-Id: xen-devel@lists.xenproject.org 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 --- 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