[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

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.

diff --git -b i/src/libvirt.c w/src/libvirt.c
index 68873cf..89b37c5 100644
--- i/src/libvirt.c
+++ w/src/libvirt.c
@@ -2,7 +2,7 @@
  * libvirt.c: Main interfaces for the libvirt library to handle
virtualization
  *           domains from a process running in domain 0
  *
- * Copyright (C) 2005-2006, 2008-2010 Red Hat, Inc.
+ * Copyright (C) 2005-2006, 2008-2011 Red Hat, Inc.
  *
  * See COPYING.LIB for the License of this software
  *
@@ -312,39 +312,24 @@ static struct gcry_thread_cbs virTLSThreadImpl = {
     NULL, NULL, NULL, NULL, NULL, NULL, NULL, NULL
 };

-#define VIR_DOMAIN_DEBUG(dom, fmt, ...) {                     \
-    do {                                                      \
-        char uuidstr[VIR_UUID_STRING_BUFLEN];                 \
-        const char *domname = NULL;                           \
+/* Helper macro to print debugging information about a domain DOM,
+ * followed by a literal string FMT and any other printf arguments.
+ */
+#define VIR_DOMAIN_DEBUG(dom, fmt, ...)                   \
+    char _uuidstr[VIR_UUID_STRING_BUFLEN];                \
+    const char *_domname = NULL;                          \
                                                               \
         if (!VIR_IS_DOMAIN(dom)) {                            \
-            memset(uuidstr, 0, sizeof(uuidstr));              \
+        memset(_uuidstr, 0, sizeof(_uuidstr));            \
         } else {                                              \
-            virUUIDFormat(dom->uuid, uuidstr);                \
-            domname = dom->name;                              \
+        virUUIDFormat((dom)->uuid, _uuidstr);             \
+        _domname = (dom)->name;                           \
         }                                                     \
                                                               \
         DEBUG("dom=%p, (VM: name=%s, uuid=%s), " fmt,         \
-              dom, NULLSTR(domname), uuidstr, __VA_ARGS__);   \
-    } while (0);                                              \
-}
+          dom, NULLSTR(_domname), _uuidstr, __VA_ARGS__)

-#define VIR_DOMAIN_DEBUG0(dom) {                              \
-    do {                                                      \
-        char uuidstr[VIR_UUID_STRING_BUFLEN];                 \
-        const char *name = NULL;                              \
-                                                              \
-        if (!VIR_IS_DOMAIN(dom)) {                            \
-            memset(uuidstr, 0, sizeof(uuidstr));              \
-        } else {                                              \
-            virUUIDFormat(dom->uuid, uuidstr);                \
-            name = dom->name;                                 \
-        }                                                     \
-                                                              \
-        DEBUG("dom=%p, (VM: name=%s, uuid=%s)",               \
-              dom, NULLSTR(name), uuidstr);                   \
-    } while (0);                                              \
-}
+#define VIR_DOMAIN_DEBUG0(dom) VIR_DOMAIN_DEBUG(dom, "%s", "")

 /**
  * virInitialize:
@@ -4985,7 +4970,7 @@ int
 virDomainUndefine(virDomainPtr domain) {
     virConnectPtr conn;

-    VIR_DOMAIN_DEBUG0(domain)
+    VIR_DOMAIN_DEBUG0(domain);

     virResetLastError();



-- 
Eric Blake   eblake redhat com    +1-801-349-2682
Libvirt virtualization library http://libvirt.org

Attachment: signature.asc
Description: OpenPGP digital signature


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]