thr3ads.net - Xen devel - [Xen-devel] [PATCH] xl: make libxl_uuid2string internal to libxenlight [Aug 2010]

If this information is useful, please help other people find it:
Share via:

Gianni Tedesco

2010-Aug-13 14:33 UTC

[Xen-devel] [PATCH] xl: make libxl_uuid2string internal to libxenlight

libxenlight exports a function libxl_uuid2string which is used
internally in several places but has one external caller in xl. The
function mainly implements policy so should not be part of the
libxenlight API. The extent to which it can be considered mechanism it
is not a xen mechanism since UUID''s are not a concept exlusive to xen.

The one caller in xl seems to be an "odd-one-out" since xl has its own
UUID formatting macros in any case.

This change fixes the leaks in libxl internal callers which were not
expecting to have to free() the UUID since the per-api-call-gc-lifetime
patch.

Signed-off-by: Gianni Tedesco <gianni.tedesco@citrix.com>

diff -r dc335ebde3b5 tools/libxl/libxl.c
--- a/tools/libxl/libxl.c	Thu Aug 12 18:03:23 2010 +0100
+++ b/tools/libxl/libxl.c	Fri Aug 13 15:32:31 2010 +0100
@@ -90,7 +90,7 @@ int libxl_domain_make(libxl_ctx *ctx, li
     xs_transaction_t t;
     xen_domain_handle_t handle;
 
-    uuid_string = libxl_uuid2string(ctx, info->uuid);
+    uuid_string = libxl_uuid2string(&gc, info->uuid);
     if (!uuid_string) {
         libxl_free_all(&gc);
         return ERROR_NOMEM;
@@ -453,7 +453,7 @@ int libxl_domain_preserve(libxl_ctx *ctx
         return ERROR_NOMEM;
     }
 
-    uuid_string = libxl_uuid2string(ctx, new_uuid);
+    uuid_string = libxl_uuid2string(&gc, new_uuid);
     if (!uuid_string) {
         libxl_free_all(&gc);
         return ERROR_NOMEM;
@@ -2785,7 +2785,7 @@ int libxl_set_memory_target(libxl_ctx *c
     if (rc != 1 || info.domain != domid)
         goto out;
     xcinfo2xlinfo(&info, &ptr);
-    uuid = libxl_uuid2string(ctx, ptr.uuid);
+    uuid = libxl_uuid2string(&gc, ptr.uuid);
     libxl_xs_write(&gc, XBT_NULL, libxl_sprintf(&gc,
"/vm/%s/memory", uuid), "%"PRIu32, target_memkb / 1024);
 
     if (enforce || !domid)
diff -r dc335ebde3b5 tools/libxl/libxl.h
--- a/tools/libxl/libxl.h	Thu Aug 12 18:03:23 2010 +0100
+++ b/tools/libxl/libxl.h	Fri Aug 13 15:32:31 2010 +0100
@@ -362,7 +362,6 @@ int libxl_run_bootloader(libxl_ctx *ctx,
                          libxl_device_disk *disk,
                          uint32_t domid);
 
-char *libxl_uuid2string(libxl_ctx *ctx, const libxl_uuid uuid);
   /* 0 means ERROR_ENOMEM, which we have logged */
 
 /* events handling */
diff -r dc335ebde3b5 tools/libxl/libxl_dom.c
--- a/tools/libxl/libxl_dom.c	Thu Aug 12 18:03:23 2010 +0100
+++ b/tools/libxl/libxl_dom.c	Fri Aug 13 15:32:31 2010 +0100
@@ -442,19 +442,12 @@ int save_device_model(libxl_ctx *ctx, ui
     return 0;
 }
 
-char *libxl_uuid2string(libxl_ctx *ctx, const libxl_uuid uuid)
+char *libxl_uuid2string(libxl_gc *gc, const libxl_uuid uuid)
 {
-    libxl_gc gc = LIBXL_INIT_GC(ctx);
-    char *s = string_of_uuid(&gc, uuid);
-    char *ret;
-    if (!s) {
-        XL_LOG(ctx, XL_LOG_ERROR, "cannot allocate for uuid");
-        ret = NULL;
-    }else{
-        ret = strdup(s);
-    }
-    libxl_free_all(&gc);
-    return ret;
+    char *s = string_of_uuid(gc, uuid);
+    if (!s)
+        XL_LOG(libxl_gc_owner(gc), XL_LOG_ERROR, "cannot allocate for
uuid");
+    return s;
 }
 
 static const char *userdata_path(libxl_gc *gc, uint32_t domid,
diff -r dc335ebde3b5 tools/libxl/libxl_internal.h
--- a/tools/libxl/libxl_internal.h	Thu Aug 12 18:03:23 2010 +0100
+++ b/tools/libxl/libxl_internal.h	Fri Aug 13 15:32:31 2010 +0100
@@ -249,4 +249,6 @@ _hidden char *libxl_abs_path(libxl_gc *g
 _hidden char *_libxl_domid_to_name(libxl_gc *gc, uint32_t domid);
 _hidden char *_libxl_poolid_to_name(libxl_gc *gc, uint32_t poolid);
 
+_hidden char *libxl_uuid2string(libxl_gc *gc, const libxl_uuid uuid);
+
 #endif
diff -r dc335ebde3b5 tools/libxl/xl_cmdimpl.c
--- a/tools/libxl/xl_cmdimpl.c	Thu Aug 12 18:03:23 2010 +0100
+++ b/tools/libxl/xl_cmdimpl.c	Fri Aug 13 15:32:31 2010 +0100
@@ -41,6 +41,10 @@
 #include "xl.h"
 
 #define UUID_FMT
"%02hhx%02hhx%02hhx%02hhx-%02hhx%02hhx-%02hhx%02hhx-%02hhx%02hhx-%02hhx%02hhx%02hhx%02hhx%02hhx%02hhx"
+#define UUID_BYTES(uuid) uuid[0], uuid[1], uuid[2], uuid[3], \
+            uuid[4], uuid[5], uuid[6], uuid[7], \
+            uuid[8], uuid[9], uuid[10], uuid[11], \
+            uuid[12], uuid[13], uuid[14], uuid[15] \
 
 #define CHK_ERRNO( call ) ({                                            \
         int chk_errno = (call);                                         \
@@ -2169,10 +2173,8 @@ void list_domains(int verbose, const lib
                 info[i].dying ? ''d'' : ''-'',
                 ((float)info[i].cpu_time / 1e9));
         free(domname);
-        if (verbose) {
-            char *uuid = libxl_uuid2string(&ctx, info[i].uuid);
-            printf(" %s", uuid);
-        }
+        if (verbose)
+            printf(" " UUID_FMT, UUID_BYTES(info[i].uuid));
         putchar(''\n'');
     }
 }
@@ -2192,11 +2194,7 @@ void list_vm(void)
     printf("UUID                                  ID    name\n");
     for (i = 0; i < nb_vm; i++) {
         domname = libxl_domid_to_name(&ctx, info[i].domid);
-        printf(UUID_FMT "  %d    %-30s\n",
-            info[i].uuid[0], info[i].uuid[1], info[i].uuid[2], info[i].uuid[3],
-            info[i].uuid[4], info[i].uuid[5], info[i].uuid[6], info[i].uuid[7],
-            info[i].uuid[8], info[i].uuid[9], info[i].uuid[10],
info[i].uuid[11],
-            info[i].uuid[12], info[i].uuid[13], info[i].uuid[14],
info[i].uuid[15],
+        printf(UUID_FMT "  %d    %-30s\n", UUID_BYTES(info[i].uuid),
             info[i].domid, domname);
         free(domname);
     }



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Ian Campbell

2010-Aug-13 14:45 UTC

head link

[Xen-devel] Re: [PATCH] xl: make libxl_uuid2string internal to libxenlight

I''m happy for this to replace patches 1+2 of my previous series
(although I think #3 stands alone and is still useful)

However I think this makes string_of_uuid a bit pointless -- it adds
nothing to libxl_uuid2string and only has one caller which could just as
well use libxl_uuid2string.

Ian.


On Fri, 2010-08-13 at 15:33 +0100, Gianni Tedesco (3P)
wrote:> libxenlight exports a function libxl_uuid2string which is used
> internally in several places but has one external caller in xl. The
> function mainly implements policy so should not be part of the
> libxenlight API. The extent to which it can be considered mechanism it
> is not a xen mechanism since UUID''s are not a concept exlusive to
xen.
> 
> The one caller in xl seems to be an "odd-one-out" since xl has
its own
> UUID formatting macros in any case.
> 
> This change fixes the leaks in libxl internal callers which were not
> expecting to have to free() the UUID since the per-api-call-gc-lifetime
> patch.
> 
> Signed-off-by: Gianni Tedesco <gianni.tedesco@citrix.com>
> 
> diff -r dc335ebde3b5 tools/libxl/libxl.c
> --- a/tools/libxl/libxl.c	Thu Aug 12 18:03:23 2010 +0100
> +++ b/tools/libxl/libxl.c	Fri Aug 13 15:32:31 2010 +0100
> @@ -90,7 +90,7 @@ int libxl_domain_make(libxl_ctx *ctx, li
>      xs_transaction_t t;
>      xen_domain_handle_t handle;
>  
> -    uuid_string = libxl_uuid2string(ctx, info->uuid);
> +    uuid_string = libxl_uuid2string(&gc, info->uuid);
>      if (!uuid_string) {
>          libxl_free_all(&gc);
>          return ERROR_NOMEM;
> @@ -453,7 +453,7 @@ int libxl_domain_preserve(libxl_ctx *ctx
>          return ERROR_NOMEM;
>      }
>  
> -    uuid_string = libxl_uuid2string(ctx, new_uuid);
> +    uuid_string = libxl_uuid2string(&gc, new_uuid);
>      if (!uuid_string) {
>          libxl_free_all(&gc);
>          return ERROR_NOMEM;
> @@ -2785,7 +2785,7 @@ int libxl_set_memory_target(libxl_ctx *c
>      if (rc != 1 || info.domain != domid)
>          goto out;
>      xcinfo2xlinfo(&info, &ptr);
> -    uuid = libxl_uuid2string(ctx, ptr.uuid);
> +    uuid = libxl_uuid2string(&gc, ptr.uuid);
>      libxl_xs_write(&gc, XBT_NULL, libxl_sprintf(&gc,
"/vm/%s/memory", uuid), "%"PRIu32, target_memkb / 1024);
>  
>      if (enforce || !domid)
> diff -r dc335ebde3b5 tools/libxl/libxl.h
> --- a/tools/libxl/libxl.h	Thu Aug 12 18:03:23 2010 +0100
> +++ b/tools/libxl/libxl.h	Fri Aug 13 15:32:31 2010 +0100
> @@ -362,7 +362,6 @@ int libxl_run_bootloader(libxl_ctx *ctx,
>                           libxl_device_disk *disk,
>                           uint32_t domid);
>  
> -char *libxl_uuid2string(libxl_ctx *ctx, const libxl_uuid uuid);
>    /* 0 means ERROR_ENOMEM, which we have logged */
>  
>  /* events handling */
> diff -r dc335ebde3b5 tools/libxl/libxl_dom.c
> --- a/tools/libxl/libxl_dom.c	Thu Aug 12 18:03:23 2010 +0100
> +++ b/tools/libxl/libxl_dom.c	Fri Aug 13 15:32:31 2010 +0100
> @@ -442,19 +442,12 @@ int save_device_model(libxl_ctx *ctx, ui
>      return 0;
>  }
>  
> -char *libxl_uuid2string(libxl_ctx *ctx, const libxl_uuid uuid)
> +char *libxl_uuid2string(libxl_gc *gc, const libxl_uuid uuid)
>  {
> -    libxl_gc gc = LIBXL_INIT_GC(ctx);
> -    char *s = string_of_uuid(&gc, uuid);
> -    char *ret;
> -    if (!s) {
> -        XL_LOG(ctx, XL_LOG_ERROR, "cannot allocate for uuid");
> -        ret = NULL;
> -    }else{
> -        ret = strdup(s);
> -    }
> -    libxl_free_all(&gc);
> -    return ret;
> +    char *s = string_of_uuid(gc, uuid);
> +    if (!s)
> +        XL_LOG(libxl_gc_owner(gc), XL_LOG_ERROR, "cannot allocate for
uuid");
> +    return s;
>  }
>  
>  static const char *userdata_path(libxl_gc *gc, uint32_t domid,
> diff -r dc335ebde3b5 tools/libxl/libxl_internal.h
> --- a/tools/libxl/libxl_internal.h	Thu Aug 12 18:03:23 2010 +0100
> +++ b/tools/libxl/libxl_internal.h	Fri Aug 13 15:32:31 2010 +0100
> @@ -249,4 +249,6 @@ _hidden char *libxl_abs_path(libxl_gc *g
>  _hidden char *_libxl_domid_to_name(libxl_gc *gc, uint32_t domid);
>  _hidden char *_libxl_poolid_to_name(libxl_gc *gc, uint32_t poolid);
>  
> +_hidden char *libxl_uuid2string(libxl_gc *gc, const libxl_uuid uuid);
> +
>  #endif
> diff -r dc335ebde3b5 tools/libxl/xl_cmdimpl.c
> --- a/tools/libxl/xl_cmdimpl.c	Thu Aug 12 18:03:23 2010 +0100
> +++ b/tools/libxl/xl_cmdimpl.c	Fri Aug 13 15:32:31 2010 +0100
> @@ -41,6 +41,10 @@
>  #include "xl.h"
>  
>  #define UUID_FMT
"%02hhx%02hhx%02hhx%02hhx-%02hhx%02hhx-%02hhx%02hhx-%02hhx%02hhx-%02hhx%02hhx%02hhx%02hhx%02hhx%02hhx"
> +#define UUID_BYTES(uuid) uuid[0], uuid[1], uuid[2], uuid[3], \
> +            uuid[4], uuid[5], uuid[6], uuid[7], \
> +            uuid[8], uuid[9], uuid[10], uuid[11], \
> +            uuid[12], uuid[13], uuid[14], uuid[15] \
>  
>  #define CHK_ERRNO( call ) ({                                            \
>          int chk_errno = (call);                                         \
> @@ -2169,10 +2173,8 @@ void list_domains(int verbose, const lib
>                  info[i].dying ? ''d'' :
''-'',
>                  ((float)info[i].cpu_time / 1e9));
>          free(domname);
> -        if (verbose) {
> -            char *uuid = libxl_uuid2string(&ctx, info[i].uuid);
> -            printf(" %s", uuid);
> -        }
> +        if (verbose)
> +            printf(" " UUID_FMT, UUID_BYTES(info[i].uuid));
>          putchar(''\n'');
>      }
>  }
> @@ -2192,11 +2194,7 @@ void list_vm(void)
>      printf("UUID                                  ID   
name\n");
>      for (i = 0; i < nb_vm; i++) {
>          domname = libxl_domid_to_name(&ctx, info[i].domid);
> -        printf(UUID_FMT "  %d    %-30s\n",
> -            info[i].uuid[0], info[i].uuid[1], info[i].uuid[2],
info[i].uuid[3],
> -            info[i].uuid[4], info[i].uuid[5], info[i].uuid[6],
info[i].uuid[7],
> -            info[i].uuid[8], info[i].uuid[9], info[i].uuid[10],
info[i].uuid[11],
> -            info[i].uuid[12], info[i].uuid[13], info[i].uuid[14],
info[i].uuid[15],
> +        printf(UUID_FMT "  %d    %-30s\n",
UUID_BYTES(info[i].uuid),
>              info[i].domid, domname);
>          free(domname);
>      }
> 
> 


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Ian Campbell

2010-Aug-13 14:46 UTC

head link

Re: [Xen-devel] Re: [PATCH] xl: make libxl_uuid2string internal to libxenlight

On Fri, 2010-08-13 at 15:45 +0100, Ian Campbell wrote:> (although I think #3 stands alone and is still useful)
oh wait, it isn''t since xl no longer dynamically allocates the uuid
string.

So all OK by me.


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Ian Jackson

2010-Aug-13 14:55 UTC

head link

[Xen-devel] Re: [PATCH] xl: make libxl_uuid2string internal to libxenlight

Gianni Tedesco (3P) writes ("[PATCH] xl: make libxl_uuid2string internal to
libxenlight"):> libxenlight exports a function libxl_uuid2string which is used
> internally in several places but has one external caller in xl. The
> function mainly implements policy so should not be part of the
> libxenlight API. The extent to which it can be considered mechanism it
> is not a xen mechanism since UUID''s are not a concept exlusive to
xen.
> 
> The one caller in xl seems to be an "odd-one-out" since xl has
its own
> UUID formatting macros in any case.
This is not a good idea because it duplicates code - even if it''s just
the macros for formatting uuids.  How to format a UUID is not
"policy", it is a fact about UUIDs.

I see that xl already has some duplicated uuid formatting code.  This
should be removed, not extended.  One of the cardinal sins in
programming is duplicating code.

Nacked-by: Ian Jackson <ian.jackson@eu.citrix.com>

Ian.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Reasonably Related Threads

Search for more maybe matching threads

Xen devel - Aug 2010 - [PATCH] xl: make libxl_uuid2string internal to libxenlight

[Xen-devel] [PATCH] xl: make libxl_uuid2string internal to libxenlight

[Xen-devel] Re: [PATCH] xl: make libxl_uuid2string internal to libxenlight

Re: [Xen-devel] Re: [PATCH] xl: make libxl_uuid2string internal to libxenlight

[Xen-devel] Re: [PATCH] xl: make libxl_uuid2string internal to libxenlight

Reasonably Related Threads