Re: [libvirt] [v3] API: Improve log for domain related APIs

On 01/06/2011 08:43 AM, Osier Yang wrote: No need to use both an outer '{}' and an inner 'do {}while(0);'. The more idiomatic expression is do{}while(0) (note the missing trailing ; in the macro definition, since it is supplied instead by the statement where the macro is used - you had one caller that missed this) and no extra {} (if there is any worry about ever using this as a one-line statement in a brace-less if statement, then it is essential to let the caller supply a trailing ; without it expanding into two statements). But in this case, we can go one step further - VIR_DOMAIN_DEBUG is always called at the top level (never part of a compound statement), and we require C99 (decl-after-statement is not an issue even if VIR_DOMAIN_DEBUG(); is not the first statement), so we can avoid introducing a new scope altogether. We already use compiler warnings to ensure we don't accidentally shadow any variables, but we can be even safer by making the macro-specific declarations less likely to collide. Just in case dom is complex, we're safer to properly parenthesize within the macro (although I don't think any of the existing uses of dom are complex). Shorter to define this as as a wrapper around VIR_DOMAIN_DEBUG, rather than repeating logic (plus it means fewer places to touch if we change the layout in the future). Good catch. Maybe we should favor VIR_DEBUG over DEBUG, (they are identical macros, but the latter is marked as legacy in logging.h), but that doesn't impact this patch. Everything else looks like straight-forward mechanical conversions. Therefore, ACK with this squashed in (I used diff -b, to focus on the non-indentation changes; the final patch looks better), and I'm pushing it. diff --git -b i/src/libvirt.c w/src/libvirt.c index 68873cf..89b37c5 100644 --- i/src/libvirt.c +++ w/src/libvirt.c @@ -2,7 +2,7 @@ * libvirt.c: Main interfaces for the libvirt library to handle virtualization * domains from a process running in domain 0 * - * Copyright (C) 2005-2006, 2008-2010 Red Hat, Inc. + * Copyright (C) 2005-2006, 2008-2011 Red Hat, Inc. * * See COPYING.LIB for the License of this software * @@ -312,39 +312,24 @@ static struct gcry_thread_cbs virTLSThreadImpl = { NULL, NULL, NULL, NULL, NULL, NULL, NULL, NULL }; -#define VIR_DOMAIN_DEBUG(dom, fmt, ...) { \ - do { \ - char uuidstr[VIR_UUID_STRING_BUFLEN]; \ - const char *domname = NULL; \ +/* Helper macro to print debugging information about a domain DOM, + * followed by a literal string FMT and any other printf arguments. + */ +#define VIR_DOMAIN_DEBUG(dom, fmt, ...) \ + char _uuidstr[VIR_UUID_STRING_BUFLEN]; \ + const char *_domname = NULL; \ \ if (!VIR_IS_DOMAIN(dom)) { \ - memset(uuidstr, 0, sizeof(uuidstr)); \ + memset(_uuidstr, 0, sizeof(_uuidstr)); \ } else { \ - virUUIDFormat(dom->uuid, uuidstr); \ - domname = dom->name; \ + virUUIDFormat((dom)->uuid, _uuidstr); \ + _domname = (dom)->name; \ } \ \ DEBUG("dom=%p, (VM: name=%s, uuid=%s), " fmt, \ - dom, NULLSTR(domname), uuidstr, __VA_ARGS__); \ - } while (0); \ -} + dom, NULLSTR(_domname), _uuidstr, __VA_ARGS__) -#define VIR_DOMAIN_DEBUG0(dom) { \ - do { \ - char uuidstr[VIR_UUID_STRING_BUFLEN]; \ - const char *name = NULL; \ - \ - if (!VIR_IS_DOMAIN(dom)) { \ - memset(uuidstr, 0, sizeof(uuidstr)); \ - } else { \ - virUUIDFormat(dom->uuid, uuidstr); \ - name = dom->name; \ - } \ - \ - DEBUG("dom=%p, (VM: name=%s, uuid=%s)", \ - dom, NULLSTR(name), uuidstr); \ - } while (0); \ -} +#define VIR_DOMAIN_DEBUG0(dom) VIR_DOMAIN_DEBUG(dom, "%s", "") /** * virInitialize: @@ -4985,7 +4970,7 @@ int virDomainUndefine(virDomainPtr domain) { virConnectPtr conn; - VIR_DOMAIN_DEBUG0(domain) + VIR_DOMAIN_DEBUG0(domain); virResetLastError(); -- Eric Blake eblake(a)redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org