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

On 01/06/2011 08:43 AM, Osier Yang wrote: > Add VM name/UUID in log for domain related APIs. > Format: "dom=%p, (VM: name=%s, uuid=%s), param0=%s, param1=%s > > *src/libvirt.c (introduce two macros: VIR_DOMAIN_DEBUG, and > VIR_DOMAIN_DEBUG0) > --- > src/libvirt.c | 243 +++++++++++++++++++++++++++++++++++++++----------------- > 1 files changed, 169 insertions(+), 74 deletions(-) > > diff --git a/src/libvirt.c b/src/libvirt.c > index ee2495a..bdf9896 100644 > --- a/src/libvirt.c > +++ b/src/libvirt.c > @@ -312,6 +312,39 @@ static struct gcry_thread_cbs virTLSThreadImpl = { > NULL, NULL, NULL, NULL, NULL, NULL, NULL, NULL > }; > > +#define VIR_DOMAIN_DEBUG(dom, fmt, ...) { \ > + do { \ 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. > + virUUIDFormat(dom->uuid, uuidstr); \ 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). > +#define VIR_DOMAIN_DEBUG0(dom) { \ 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).

> @@ -2100,7 +2133,10 @@ error: > virDomainPtr > virDomainLookupByUUID(virConnectPtr conn, const unsigned char *uuid) > { > - DEBUG("conn=%p, uuid=%s", conn, uuid); > + char uuidstr[VIR_UUID_STRING_BUFLEN]; > + virUUIDFormat(uuid, uuidstr); > + > + DEBUG("conn=%p, uuid=%s", conn, uuidstr); 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.