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

[Daniel P. Berrange]

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)

NB, this relies on 'fmt' being a literal string in the
caller, to avoid malloc'ing, but that's already true.
This should be low enough overhead that we don't need
to have "if ! DEBUG then return" - at least not seriously
worse than our existing overhead - its only adding a
single virUUIDFormat call

So

int
virDomainSetMaxMemory(virDomainPtr domain, unsigned long memory)
{
    virConnectPtr conn;
    DEBUG("domain=%p, memory=%lu", domain, memory);

would thus become

int
virDomainSetMaxMemory(virDomainPtr domain, unsigned long memory)
{
    virConnectPtr conn;
    DEBUG_DOMAIN(domain, "memory=%lu", memory);



Daniel