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

[Osier Yang]

于 2011年01月06日 00:55, 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)
>

Cool, I prefer this one, personally, as with it the work is
easier, don't need to introduce new statements, and don't need
to worry about malloc'ing.

Thanks, Eric, and Daniel.

Regards
Osier