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

On Wed, Jan 05, 2011 at 09:34:20AM -0700, Eric Blake wrote: > On 01/05/2011 03:40 AM, Osier Yang wrote: >>> Then maybe the patch should be altered to output both name and UUID >>> (probably by introducing a helper function, which when given a >>> virDomainPtr outputs all three pieces of debug information rather than >>> the current %p). I like the idea behind the patch, but only if we can >>> come up with the correct way of getting the extra information when >>> debugging is enabled, and without adding extra time when debugging is >>> off. I'm not even sure what the right internal APIs for the job would >>> be, or if they even exist yet. >>> >> >> Hi, Eric, >> >> I implemented a helper function like so in src/libvirt.c: >> >> static const char * >> virDomainNameUUID(virDomainPtr domain) { >> char *entry = NULL; >> const char *name = NULL; >> char uuidstr[VIR_UUID_STRING_BUFLEN]; >> >> if (!VIR_IS_DOMAIN(domain)) { >> entry = strdup("(VM: invalid domain)"); > > malloc'd return... > >> return entry; >> } >> >> name = domain->name; >> virUUIDFormat(domain->uuid, uuidstr); >> >> if(virAsprintf(&entry, "(VM: name=%s, uuid=%s)", name, >> uuidstr)< 0) >> virReportOOMError(); >> >> return NULLSTR(entry); > > and static return. Ouch - that means the caller doesn't know whether to > call free or not. You have to be consistent (the result is either NULL > or malloc'd; or the result is always static). > >> } >> >> But how to avoid extra time when debugging is off? Following >> is not correct way definitely, as malloc'ed "entry" need to be >> freed. >> >> virConnectPtr >> virDomainGetConnect (virDomainPtr dom) >> { >> DEBUG("dom=%p, %s", dom, virDomainNameUUID(dom)); > > When I envisioned a helper function, I'm thinking more like callers > using a simpler: > > DEBUG_DOMAIN(dom) > > and a helper like: > > void > DEBUG_DOMAIN (virDomainPtr dom) > { > if !DEBUG then early exit > gather data > DEBUG("dom=%p, %s", dom, virDomainNameUUID(dom)); > free any gathered data if needed > } > > That would mean that your new helper function would need to look at the > guts of DEBUG to mirror the same decision of whether DEBUG will result > in any output. Ideally it'd be a macro that accepts extra args too eg perhaps #define DEBUG_DOMAIN(dom, fmt, ...) \ do { char uuidstr[VIR_UUID_STRING_BUFLEN]; char *name; if (VIR_IS_DOMAIN(dom) { virUUIDFormat(uuidstr, dom->uuid); name=dom->name; } else { memset(uuidstr, 0, sizeof(uuidstr)); } DEBUG("dom=%p (name=%s, uuid=%s) " fmt, NULLSTR(name), uuidstr, __VA_ARGS__) } while(0)