[libvirt] [v3] API: Improve log for domain related APIs
Osier Yang
jyang at redhat.com
Fri Jan 7 06:12:35 UTC 2011
于 2011年01月07日 00:50, Eric Blake 写道:
> 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).
>
Thanks for the detailed knowledge above.
>> @@ -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.
>
Great, thanks.
Regards
Osier
More information about the libvir-list
mailing list