Skip to contentSkip to conversation

xl: bug-fix patches implementing libxl allocation policy

PATCH New Gianni Tedesco 2 participants 12 messages Aug 10, 2010 – Aug 11, 2010

lore
Share & tools
lore
#0 lore raw

PATCH 1/1 xl: bug-fix patches implementing libxl allocation policy

This patch series implements several bug-fixes and implements "the memory allocation policy" of libxl. All touched functions have been tested with valgrind and are free of leaks, double-free and free-non-malloce'd pointer errors. This patch-set prepares for another patch to actually implement per-function-call lifetime for libxl garbage collection which is coming RealSoonNow(tm) - but touches almost every code path so requires more verification. This is all a step towards allowing demons to use libxl.

#1 lore raw

PATCH 1/1 xl: Return void from libxl_free() and libxl_free_all()

 tools/libxl/libxl_internal.c |  11 +++++------
 tools/libxl/libxl_internal.h |   4 ++--
 2 files changed, 7 insertions(+), 8 deletions(-)

Also abort() if a pointer passed in to libxl_free() cannot be found in the mini-gc structures. This exposes several real bugs (ie. not just the libxl_free() something that was allocated via malloc variety).

tools/libxl/libxl_internal.c +5 -6
diff -r 8992134dcfd0 -r 1b538b12751c tools/libxl/libxl_internal.c
@@ -59,26 +59,26 @@ int libxl_ptr_add(libxl_ctx *ctx, void *
     return 0;
 }
 
-int libxl_free(libxl_ctx *ctx, void *ptr)
+void libxl_free(libxl_ctx *ctx, void *ptr)
 {
     int i;
 
     if (!ptr)
-        return 0;
+        return;
 
     /* remove the pointer from the tracked ptrs */
     for (i = 0; i < ctx->alloc_maxsize; i++) {
         if (ctx->alloc_ptrs[i] == ptr) {
             ctx->alloc_ptrs[i] = NULL;
             free(ptr);
-            return 0;
+            return;
         }
     }
     /* haven't find the pointer, really bad */
-    return -1;
+    abort();
 }
 
-int libxl_free_all(libxl_ctx *ctx)
+void libxl_free_all(libxl_ctx *ctx)
 {
     void *ptr;
     int i;
@@ -88,7 +88,6 @@ int libxl_free_all(libxl_ctx *ctx)
         ctx->alloc_ptrs[i] = NULL;
         free(ptr);
     }
-    return 0;
 }
 
 void *libxl_zalloc(libxl_ctx *ctx, int bytes)
tools/libxl/libxl_internal.h +2 -2
diff -r 8992134dcfd0 -r 1b538b12751c tools/libxl/libxl_internal.h
@@ -108,8 +108,8 @@ int xs_writev(struct xs_handle *xsh, xs_
 
 /* memory allocation tracking/helpers */
 int libxl_ptr_add(libxl_ctx *ctx, void *ptr);
-int libxl_free(libxl_ctx *ctx, void *ptr);
-int libxl_free_all(libxl_ctx *ctx);
+void libxl_free(libxl_ctx *ctx, void *ptr);
+void libxl_free_all(libxl_ctx *ctx);
 void *libxl_zalloc(libxl_ctx *ctx, int bytes);
 void *libxl_calloc(libxl_ctx *ctx, size_t nmemb, size_t size);
 char *libxl_sprintf(libxl_ctx *ctx, const char *fmt, ...) PRINTF_ATTRIBUTE(2, 3);
#2 lore raw

PATCH 1/1 xl: allocate version info strings with strdup()

In reply to Gianni Tedesco
 tools/libxl/libxl.c |  31 +++++++++++++++++++++++--------
 1 files changed, 23 insertions(+), 8 deletions(-)

libxl_strdup() will soon have a per-call rather than per-context lifetime so use strdup() to allocate version info and manually free in context destructor

tools/libxl/libxl.c +23 -8
diff -r 1b538b12751c -r 7ffe8e01c721 tools/libxl/libxl.c
@@ -70,11 +70,13 @@ int libxl_ctx_init(libxl_ctx *ctx, int v
     return 0;
 }
 
+static void do_free_version_info(libxl_version_info *info);
 int libxl_ctx_free(libxl_ctx *ctx)
 {
     libxl_free_all(ctx);
     free(ctx->alloc_ptrs);
     xc_interface_close(ctx->xch);
+    do_free_version_info(&ctx->version_info);
     if (ctx->xsh) xs_daemon_close(ctx->xsh); 
     return 0;
 }
@@ -2490,6 +2492,18 @@ int libxl_get_physinfo(libxl_ctx *ctx, l
     return 0;
 }
 
+static void do_free_version_info(libxl_version_info *info)
+{
+    free(info->xen_version_extra);
+    free(info->compiler);
+    free(info->compile_by);
+    free(info->compile_domain);
+    free(info->compile_date);
+    free(info->capabilities);
+    free(info->changeset);
+    free(info->commandline);
+}
+
 const libxl_version_info* libxl_get_version_info(libxl_ctx *ctx)
 {
     union {
@@ -2509,20 +2523,21 @@ const libxl_version_info* libxl_get_vers
     xen_version = xc_version(ctx->xch, XENVER_version, NULL);
     info->xen_version_major = xen_version >> 16;
     info->xen_version_minor = xen_version & 0xFF;
+
     xc_version(ctx->xch, XENVER_extraversion, &u.xen_extra);
-    info->xen_version_extra = libxl_strdup(ctx, u.xen_extra);
+    info->xen_version_extra = strdup(u.xen_extra);
 
     xc_version(ctx->xch, XENVER_compile_info, &u.xen_cc);
-    info->compiler = libxl_strdup(ctx, u.xen_cc.compiler);
-    info->compile_by = libxl_strdup(ctx, u.xen_cc.compile_by);
-    info->compile_domain = libxl_strdup(ctx, u.xen_cc.compile_domain);
-    info->compile_date = libxl_strdup(ctx, u.xen_cc.compile_date);
+    info->compiler = strdup(u.xen_cc.compiler);
+    info->compile_by = strdup(u.xen_cc.compile_by);
+    info->compile_domain = strdup(u.xen_cc.compile_domain);
+    info->compile_date = strdup(u.xen_cc.compile_date);
 
     xc_version(ctx->xch, XENVER_capabilities, &u.xen_caps);
-    info->capabilities = libxl_strdup(ctx, u.xen_caps);
+    info->capabilities = strdup(u.xen_caps);
 
     xc_version(ctx->xch, XENVER_changeset, &u.xen_chgset);
-    info->changeset = libxl_strdup(ctx, u.xen_chgset);
+    info->changeset = strdup(u.xen_chgset);
 
     xc_version(ctx->xch, XENVER_platform_parameters, &u.p_parms);
     info->virt_start = u.p_parms.virt_start;
@@ -2530,7 +2545,7 @@ const libxl_version_info* libxl_get_vers
     info->pagesize = xc_version(ctx->xch, XENVER_pagesize, NULL);
 
     xc_version(ctx->xch, XENVER_commandline, &u.xen_commandline);
-    info->commandline = libxl_strdup(ctx, u.xen_commandline);
+    info->commandline = strdup(u.xen_commandline);
 
     return info;
 }
#3 lore raw

PATCH 1/1 xl: don't use libxl allocator for vcpu_list

In reply to Gianni Tedesco
 tools/libxl/libxl.c      |  17 ++++++++++++++---
 tools/libxl/libxl.h      |   1 +
 tools/libxl/xl_cmdimpl.c |   8 +++++---
 3 files changed, 20 insertions(+), 6 deletions(-)

This also fixes a bug with an erroneous call to libxl_free(). A destructor for the vpcu list is also implemented which is called from xl.

tools/libxl/libxl.c +14 -3
diff -r 7ffe8e01c721 -r 4a877c3d722f tools/libxl/libxl.c
@@ -2557,6 +2557,8 @@ libxl_vcpuinfo *libxl_list_vcpu(libxl_ct
     xc_domaininfo_t domaininfo;
     xc_vcpuinfo_t vcpuinfo;
     xc_physinfo_t physinfo = { 0 };
+    uint64_t *cpumaps;
+    unsigned num_cpuwords;
 
     if (xc_domain_getinfolist(ctx->xch, domid, 1, &domaininfo) != 1) {
         XL_LOG_ERRNO(ctx, XL_LOG_ERROR, "getting infolist");
@@ -2567,14 +2569,16 @@ libxl_vcpuinfo *libxl_list_vcpu(libxl_ct
         return NULL;
     }
     *cpusize = physinfo.max_cpu_id + 1;
-    ptr = libxl_calloc(ctx, domaininfo.max_vcpu_id + 1, sizeof (libxl_vcpuinfo));
+    ret = ptr = calloc(domaininfo.max_vcpu_id + 1, sizeof (libxl_vcpuinfo));
     if (!ptr) {
         return NULL;
     }
 
-    ret = ptr;
+    num_cpuwords = ((physinfo.max_cpu_id + 64) / 64);
+    cpumaps = calloc(num_cpuwords * sizeof(*cpumaps), domaininfo.max_vcpu_id + 1);
+
     for (*nb_vcpu = 0; *nb_vcpu <= domaininfo.max_vcpu_id; ++*nb_vcpu, ++ptr) {
-        ptr->cpumap = libxl_calloc(ctx, (*cpusize + 63) / 64, sizeof (uint64_t));
+        ptr->cpumap = cpumaps + (num_cpuwords * *nb_vcpu);
         if (!ptr->cpumap) {
             return NULL;
         }
@@ -2596,6 +2600,13 @@ libxl_vcpuinfo *libxl_list_vcpu(libxl_ct
     return ret;
 }
 
+void libxl_free_vcpu_list(libxl_vcpuinfo *vcpu)
+{
+    if ( vcpu )
+        free(vcpu[0].cpumap);
+    free(vcpu);
+}
+
 int libxl_set_vcpuaffinity(libxl_ctx *ctx, uint32_t domid, uint32_t vcpuid,
                            uint64_t *cpumap, int cpusize)
 {
tools/libxl/libxl.h +1 -0
diff -r 7ffe8e01c721 -r 4a877c3d722f tools/libxl/libxl.h
@@ -595,6 +595,7 @@ typedef struct {
 int libxl_get_physinfo(libxl_ctx *ctx, libxl_physinfo *physinfo);
 libxl_vcpuinfo *libxl_list_vcpu(libxl_ctx *ctx, uint32_t domid,
                                        int *nb_vcpu, int *cpusize);
+void libxl_free_vcpu_list(libxl_vcpuinfo *vcpu);
 int libxl_set_vcpuaffinity(libxl_ctx *ctx, uint32_t domid, uint32_t vcpuid,
                            uint64_t *cpumap, int cpusize);
 int libxl_set_vcpucount(libxl_ctx *ctx, uint32_t domid, uint32_t count);
tools/libxl/xl_cmdimpl.c +5 -3
diff -r 7ffe8e01c721 -r 4a877c3d722f tools/libxl/xl_cmdimpl.c
@@ -3243,7 +3243,7 @@ static void print_vcpuinfo(uint32_t tdom
 void vcpulist(int argc, char **argv)
 {
     libxl_dominfo *dominfo;
-    libxl_vcpuinfo *vcpuinfo;
+    libxl_vcpuinfo *vcpuinfo, *list = NULL;
     libxl_physinfo physinfo;
     int nb_vcpu, nb_domain, cpusize;
 
@@ -3259,26 +3259,28 @@ void vcpulist(int argc, char **argv)
             goto vcpulist_out;
         }
         for (; nb_domain > 0; --nb_domain, ++dominfo) {
-            if (!(vcpuinfo = libxl_list_vcpu(&ctx, dominfo->domid, &nb_vcpu, &cpusize))) {
+            if (!(list = vcpuinfo = libxl_list_vcpu(&ctx, dominfo->domid, &nb_vcpu, &cpusize))) {
                 fprintf(stderr, "libxl_list_vcpu failed.\n");
                 goto vcpulist_out;
             }
             for (; nb_vcpu > 0; --nb_vcpu, ++vcpuinfo) {
                 print_vcpuinfo(dominfo->domid, vcpuinfo, physinfo.nr_cpus);
             }
+            libxl_free_vcpu_list(list);
         }
     } else {
         for (; argc > 0; ++argv, --argc) {
             if (domain_qualifier_to_domid(*argv, &domid, 0) < 0) {
                 fprintf(stderr, "%s is an invalid domain identifier\n", *argv);
             }
-            if (!(vcpuinfo = libxl_list_vcpu(&ctx, domid, &nb_vcpu, &cpusize))) {
+            if (!(vcpuinfo = list = libxl_list_vcpu(&ctx, domid, &nb_vcpu, &cpusize))) {
                 fprintf(stderr, "libxl_list_vcpu failed.\n");
                 goto vcpulist_out;
             }
             for (; nb_vcpu > 0; --nb_vcpu, ++vcpuinfo) {
                 print_vcpuinfo(domid, vcpuinfo, physinfo.nr_cpus);
             }
+            libxl_free_vcpu_list(list);
         }
     }
   vcpulist_out:
#4 lore raw

Re: [PATCH 3 of 7] xl: don't use libxl allocator for vcpu_list

Quoted 10 lines · On Tue, 10 Aug 2010, Gianni Tedesco (3P) wrote:
 tools/libxl/libxl.c      |  17 ++++++++++++++---
 tools/libxl/libxl.h      |   1 +
 tools/libxl/xl_cmdimpl.c |   8 +++++---
 3 files changed, 20 insertions(+), 6 deletions(-)

This also fixes a bug with an erroneous call to libxl_free(). A destructor for the vpcu list is also implemented which is called from xl.

I applied all the patches of this series apart from this one that doesn't apply correctly. Please update and resend.

Thanks,

Stefano

#5 lore raw

Re: [PATCH 3 of 7] xl: don't use libxl allocator for vcpu_list

Quoted 7 lines · Stefano Stabellini
Quoted 6 lines · On Tue, 10 Aug 2010, Gianni Tedesco (3P) wrote:
 tools/libxl/libxl.c      |  17 ++++++++++++++---
 tools/libxl/libxl.h      |   1 +
 tools/libxl/xl_cmdimpl.c |   8 +++++---
 3 files changed, 20 insertions(+), 6 deletions(-)
Quoted 7 lines
Quoted 4 lines

This also fixes a bug with an erroneous call to libxl_free(). A destructor for the vpcu list is also implemented which is called from xl.

I applied all the patches of this series apart from this one that doesn't apply correctly. Please update and resend.

tools/libxl/libxl.c +13 -3
diff -r fffedd3d70e1 tools/libxl/libxl.c
@@ -2722,6 +2722,8 @@ libxl_vcpuinfo *libxl_list_vcpu(libxl_ct
     xc_domaininfo_t domaininfo;
     xc_vcpuinfo_t vcpuinfo;
     xc_physinfo_t physinfo = { 0 };
+    uint64_t *cpumaps;
+    unsigned num_cpuwords;
 
     if (xc_domain_getinfolist(ctx->xch, domid, 1, &domaininfo) != 1) {
         XL_LOG_ERRNO(ctx, XL_LOG_ERROR, "getting infolist");
@@ -2732,14 +2734,15 @@ libxl_vcpuinfo *libxl_list_vcpu(libxl_ct
         return NULL;
     }
     *nrcpus = physinfo.max_cpu_id + 1;
-    ptr = libxl_calloc(ctx, domaininfo.max_vcpu_id + 1, sizeof (libxl_vcpuinfo));
+    ret = ptr = calloc(domaininfo.max_vcpu_id + 1, sizeof (libxl_vcpuinfo));
     if (!ptr) {
         return NULL;
     }
 
-    ret = ptr;
+    num_cpuwords = ((physinfo.max_cpu_id + 64) / 64);
+    cpumaps = calloc(num_cpuwords * sizeof(*cpumaps), domaininfo.max_vcpu_id + 1);
     for (*nb_vcpu = 0; *nb_vcpu <= domaininfo.max_vcpu_id; ++*nb_vcpu, ++ptr) {
-        ptr->cpumap = libxl_calloc(ctx, (*nrcpus + 63) / 64, sizeof (uint64_t));
+        ptr->cpumap = cpumaps + (num_cpuwords * *nb_vcpu);
         if (!ptr->cpumap) {
             return NULL;
         }
@@ -2762,6 +2765,13 @@ libxl_vcpuinfo *libxl_list_vcpu(libxl_ct
     return ret;
 }
 
+void libxl_free_vcpu_list(libxl_vcpuinfo *vcpu)
+{
+    if ( vcpu )
+        free(vcpu[0].cpumap);
+    free(vcpu);
+}
+
 int libxl_set_vcpuaffinity(libxl_ctx *ctx, uint32_t domid, uint32_t vcpuid,
                            uint64_t *cpumap, int nrcpus)
 {
tools/libxl/libxl.h +1 -0
diff -r fffedd3d70e1 tools/libxl/libxl.h
@@ -598,6 +598,7 @@ typedef struct {
 int libxl_get_physinfo(libxl_ctx *ctx, libxl_physinfo *physinfo);
 libxl_vcpuinfo *libxl_list_vcpu(libxl_ctx *ctx, uint32_t domid,
                                        int *nb_vcpu, int *nrcpus);
+void libxl_free_vcpu_list(libxl_vcpuinfo *vcpu);
 int libxl_set_vcpuaffinity(libxl_ctx *ctx, uint32_t domid, uint32_t vcpuid,
                            uint64_t *cpumap, int nrcpus);
 int libxl_set_vcpucount(libxl_ctx *ctx, uint32_t domid, uint32_t count);
tools/libxl/xl_cmdimpl.c +5 -3
diff -r fffedd3d70e1 tools/libxl/xl_cmdimpl.c
@@ -3268,7 +3268,7 @@ static void print_vcpuinfo(uint32_t tdom
 void vcpulist(int argc, char **argv)
 {
     libxl_dominfo *dominfo;
-    libxl_vcpuinfo *vcpuinfo;
+    libxl_vcpuinfo *vcpuinfo, *list = NULL;
     libxl_physinfo physinfo;
     int nb_vcpu, nb_domain, nrcpus;
 
@@ -3284,7 +3284,7 @@ void vcpulist(int argc, char **argv)
             goto vcpulist_out;
         }
         for (; nb_domain > 0; --nb_domain, ++dominfo) {
-            if (!(vcpuinfo = libxl_list_vcpu(&ctx, dominfo->domid, &nb_vcpu,
+            if (!(list = vcpuinfo = libxl_list_vcpu(&ctx, dominfo->domid, &nb_vcpu,
                 &nrcpus))) {
                 fprintf(stderr, "libxl_list_vcpu failed.\n");
                 goto vcpulist_out;
@@ -3292,19 +3292,21 @@ void vcpulist(int argc, char **argv)
             for (; nb_vcpu > 0; --nb_vcpu, ++vcpuinfo) {
                 print_vcpuinfo(dominfo->domid, vcpuinfo, physinfo.nr_cpus);
             }
+            libxl_free_vcpu_list(list);
         }
     } else {
         for (; argc > 0; ++argv, --argc) {
             if (domain_qualifier_to_domid(*argv, &domid, 0) < 0) {
                 fprintf(stderr, "%s is an invalid domain identifier\n", *argv);
             }
-            if (!(vcpuinfo = libxl_list_vcpu(&ctx, domid, &nb_vcpu, &nrcpus))) {
+            if (!(list = vcpuinfo = libxl_list_vcpu(&ctx, domid, &nb_vcpu, &nrcpus))) {
                 fprintf(stderr, "libxl_list_vcpu failed.\n");
                 goto vcpulist_out;
             }
             for (; nb_vcpu > 0; --nb_vcpu, ++vcpuinfo) {
                 print_vcpuinfo(domid, vcpuinfo, physinfo.nr_cpus);
             }
+            libxl_free_vcpu_list(list);
         }
     }
   vcpulist_out:
#6 lore raw

Re: [PATCH 3 of 7] xl: don't use libxl allocator for vcpu_list

applied, thanks

Quoted 119 lines · On Wed, 11 Aug 2010, Gianni Tedesco (3P) wrote:
Quoted 7 lines · Stefano Stabellini
Quoted 6 lines · On Tue, 10 Aug 2010, Gianni Tedesco (3P) wrote:
 tools/libxl/libxl.c      |  17 ++++++++++++++---
 tools/libxl/libxl.h      |   1 +
 tools/libxl/xl_cmdimpl.c |   8 +++++---
 3 files changed, 20 insertions(+), 6 deletions(-)
Quoted 7 lines
Quoted 4 lines

This also fixes a bug with an erroneous call to libxl_free(). A destructor for the vpcu list is also implemented which is called from xl.

I applied all the patches of this series apart from this one that doesn't apply correctly. Please update and resend.

diff -r fffedd3d70e1 tools/libxl/libxl.c

tools/libxl/libxl.c +13 -3
@@ -2722,6 +2722,8 @@ libxl_vcpuinfo *libxl_list_vcpu(libxl_ct
     xc_domaininfo_t domaininfo;
     xc_vcpuinfo_t vcpuinfo;
     xc_physinfo_t physinfo = { 0 };
+    uint64_t *cpumaps;
+    unsigned num_cpuwords;
 
     if (xc_domain_getinfolist(ctx->xch, domid, 1, &domaininfo) != 1) {
         XL_LOG_ERRNO(ctx, XL_LOG_ERROR, "getting infolist");
@@ -2732,14 +2734,15 @@ libxl_vcpuinfo *libxl_list_vcpu(libxl_ct
         return NULL;
     }
     *nrcpus = physinfo.max_cpu_id + 1;
-    ptr = libxl_calloc(ctx, domaininfo.max_vcpu_id + 1, sizeof (libxl_vcpuinfo));
+    ret = ptr = calloc(domaininfo.max_vcpu_id + 1, sizeof (libxl_vcpuinfo));
     if (!ptr) {
         return NULL;
     }
 
-    ret = ptr;
+    num_cpuwords = ((physinfo.max_cpu_id + 64) / 64);
+    cpumaps = calloc(num_cpuwords * sizeof(*cpumaps), domaininfo.max_vcpu_id + 1);
     for (*nb_vcpu = 0; *nb_vcpu <= domaininfo.max_vcpu_id; ++*nb_vcpu, ++ptr) {
-        ptr->cpumap = libxl_calloc(ctx, (*nrcpus + 63) / 64, sizeof (uint64_t));
+        ptr->cpumap = cpumaps + (num_cpuwords * *nb_vcpu);
         if (!ptr->cpumap) {
             return NULL;
         }
@@ -2762,6 +2765,13 @@ libxl_vcpuinfo *libxl_list_vcpu(libxl_ct
     return ret;
 }
 
+void libxl_free_vcpu_list(libxl_vcpuinfo *vcpu)
+{
+    if ( vcpu )
+        free(vcpu[0].cpumap);
+    free(vcpu);
+}
+
 int libxl_set_vcpuaffinity(libxl_ctx *ctx, uint32_t domid, uint32_t vcpuid,
                            uint64_t *cpumap, int nrcpus)
 {

diff -r fffedd3d70e1 tools/libxl/libxl.h

tools/libxl/libxl.h +1 -0
@@ -598,6 +598,7 @@ typedef struct {
 int libxl_get_physinfo(libxl_ctx *ctx, libxl_physinfo *physinfo);
 libxl_vcpuinfo *libxl_list_vcpu(libxl_ctx *ctx, uint32_t domid,
                                        int *nb_vcpu, int *nrcpus);
+void libxl_free_vcpu_list(libxl_vcpuinfo *vcpu);
 int libxl_set_vcpuaffinity(libxl_ctx *ctx, uint32_t domid, uint32_t vcpuid,
                            uint64_t *cpumap, int nrcpus);
 int libxl_set_vcpucount(libxl_ctx *ctx, uint32_t domid, uint32_t count);

diff -r fffedd3d70e1 tools/libxl/xl_cmdimpl.c

tools/libxl/xl_cmdimpl.c +5 -3
@@ -3268,7 +3268,7 @@ static void print_vcpuinfo(uint32_t tdom
 void vcpulist(int argc, char **argv)
 {
     libxl_dominfo *dominfo;
-    libxl_vcpuinfo *vcpuinfo;
+    libxl_vcpuinfo *vcpuinfo, *list = NULL;
     libxl_physinfo physinfo;
     int nb_vcpu, nb_domain, nrcpus;
 
@@ -3284,7 +3284,7 @@ void vcpulist(int argc, char **argv)
             goto vcpulist_out;
         }
         for (; nb_domain > 0; --nb_domain, ++dominfo) {
-            if (!(vcpuinfo = libxl_list_vcpu(&ctx, dominfo->domid, &nb_vcpu,
+            if (!(list = vcpuinfo = libxl_list_vcpu(&ctx, dominfo->domid, &nb_vcpu,
                 &nrcpus))) {
                 fprintf(stderr, "libxl_list_vcpu failed.\n");
                 goto vcpulist_out;
@@ -3292,19 +3292,21 @@ void vcpulist(int argc, char **argv)
             for (; nb_vcpu > 0; --nb_vcpu, ++vcpuinfo) {
                 print_vcpuinfo(dominfo->domid, vcpuinfo, physinfo.nr_cpus);
             }
+            libxl_free_vcpu_list(list);
         }
     } else {
         for (; argc > 0; ++argv, --argc) {
             if (domain_qualifier_to_domid(*argv, &domid, 0) < 0) {
                 fprintf(stderr, "%s is an invalid domain identifier\n", *argv);
             }
-            if (!(vcpuinfo = libxl_list_vcpu(&ctx, domid, &nb_vcpu, &nrcpus))) {
+            if (!(list = vcpuinfo = libxl_list_vcpu(&ctx, domid, &nb_vcpu, &nrcpus))) {
                 fprintf(stderr, "libxl_list_vcpu failed.\n");
                 goto vcpulist_out;
             }
             for (; nb_vcpu > 0; --nb_vcpu, ++vcpuinfo) {
                 print_vcpuinfo(domid, vcpuinfo, physinfo.nr_cpus);
             }
+            libxl_free_vcpu_list(list);
         }
     }
   vcpulist_out:
#7 lore raw

PATCH 1/1 xl: don't use libxl allocator for nic_list

In reply to Gianni Tedesco
 tools/libxl/libxl.c       |  33 ++++++++++++++++++++-------------
 tools/libxl/libxl.h       |   1 +
 tools/libxl/libxl_utils.c |  15 +++++++++------
 tools/libxl/xl_cmdimpl.c  |   9 +++++----
 4 files changed, 35 insertions(+), 23 deletions(-)

This also fixes a bug with an erroneous call to libxl_free(). A destructor for the nic list is also implemented which is called from xl.

tools/libxl/libxl.c +20 -13
diff -r 4a877c3d722f -r 0c8c78fea130 tools/libxl/libxl.c
@@ -1729,10 +1729,21 @@ int libxl_device_nic_del(libxl_ctx *ctx,
     return libxl_device_del(ctx, &device, wait);
 }
 
+void libxl_free_nics_list(libxl_nicinfo *nics, unsigned int nb)
+{
+    unsigned int i;
+    for(i = 0; i < nb; i++) {
+        free(nics[i].backend);
+        free(nics[i].frontend);
+        free(nics[i].script);
+    }
+    free(nics);
+}
+
 libxl_nicinfo *libxl_list_nics(libxl_ctx *ctx, uint32_t domid, unsigned int *nb)
 {
     char *dompath, *nic_path_fe;
-    char **l;
+    char **l, **list;
     char *val, *tok;
     unsigned int nb_nics, i;
     libxl_nicinfo *res, *nics;
@@ -1741,22 +1752,21 @@ libxl_nicinfo *libxl_list_nics(libxl_ctx
     if (!dompath) {
         return NULL;
     }
-    l = libxl_xs_directory(ctx, XBT_NULL,
+    list = l = libxl_xs_directory(ctx, XBT_NULL,
                            libxl_sprintf(ctx, "%s/device/vif", dompath), &nb_nics);
     if (!l) {
         return NULL;
     }
-    res = libxl_calloc(ctx, nb_nics, sizeof (libxl_device_nic));
+    nics = res = calloc(nb_nics, sizeof (libxl_device_nic));
     if (!res) {
         libxl_free(ctx, l);
         return NULL;
     }
-    nics = res;
     for (*nb = nb_nics; nb_nics > 0; --nb_nics, ++l, ++nics) {
         nic_path_fe = libxl_sprintf(ctx, "%s/device/vif/%s", dompath, *l);
 
-        nics->backend = libxl_xs_read(ctx, XBT_NULL,
-                                      libxl_sprintf(ctx, "%s/backend", nic_path_fe));
+        nics->backend = xs_read(ctx->xsh, XBT_NULL,
+                                libxl_sprintf(ctx, "%s/backend", nic_path_fe), NULL);
         val = libxl_xs_read(ctx, XBT_NULL, libxl_sprintf(ctx, "%s/backend-id", nic_path_fe));
         nics->backend_id = val ? strtoul(val, NULL, 10) : -1;
 
@@ -1774,17 +1784,14 @@ libxl_nicinfo *libxl_list_nics(libxl_ctx
         nics->rref_tx = val ? strtol(val, NULL, 10) : -1;
         val = libxl_xs_read(ctx, XBT_NULL, libxl_sprintf(ctx, "%s/rx-ring-ref", nic_path_fe));
         nics->rref_rx = val ? strtol(val, NULL, 10) : -1;
-        nics->frontend = libxl_xs_read(ctx, XBT_NULL,
-                                       libxl_sprintf(ctx, "%s/frontend", nics->backend));
+        nics->frontend = xs_read(ctx->xsh, XBT_NULL,
+                                 libxl_sprintf(ctx, "%s/frontend", nics->backend), NULL);
         val = libxl_xs_read(ctx, XBT_NULL, libxl_sprintf(ctx, "%s/frontend-id", nics->backend));
         nics->frontend_id = val ? strtoul(val, NULL, 10) : -1;
-        nics->script = libxl_xs_read(ctx, XBT_NULL,
-                                     libxl_sprintf(ctx, "%s/script", nics->backend));
-
-        libxl_free(ctx, nic_path_fe);
+        nics->script = xs_read(ctx->xsh, XBT_NULL,
+                               libxl_sprintf(ctx, "%s/script", nics->backend), NULL);
     }
 
-    libxl_free(ctx, l);
     return res;
 }
 
tools/libxl/libxl.h +1 -0
diff -r 4a877c3d722f -r 0c8c78fea130 tools/libxl/libxl.h
@@ -509,6 +509,7 @@ typedef struct {
 int libxl_device_nic_add(libxl_ctx *ctx, uint32_t domid, libxl_device_nic *nic);
 int libxl_device_nic_del(libxl_ctx *ctx, libxl_device_nic *nic, int wait);
 libxl_nicinfo *libxl_list_nics(libxl_ctx *ctx, uint32_t domid, unsigned int *nb);
+void libxl_free_nics_list(libxl_nicinfo *nics, unsigned int nb);
 
 int libxl_device_console_add(libxl_ctx *ctx, uint32_t domid, libxl_device_console *console);
 
tools/libxl/libxl_utils.c +9 -6
diff -r 4a877c3d722f -r 0c8c78fea130 tools/libxl/libxl_utils.c
@@ -365,14 +365,14 @@ int libxl_pipe(libxl_ctx *ctx, int pipes
 int libxl_mac_to_device_nic(libxl_ctx *ctx, uint32_t domid,
                             const char *mac, libxl_device_nic *nic)
 {
-    libxl_nicinfo *nics;
-    unsigned int nb, i;
+    libxl_nicinfo *nics, *list;
+    unsigned int nb, i, j;
     uint8_t mac_n[6];
     uint8_t *a, *b;
     const char *tok;
     char *endptr;
 
-    nics = libxl_list_nics(ctx, domid, &nb);
+    list = nics = libxl_list_nics(ctx, domid, &nb);
     if (!nics) {
         return ERROR_FAIL;
     }
@@ -384,7 +384,7 @@ int libxl_mac_to_device_nic(libxl_ctx *c
         }
     }
     memset(nic, 0, sizeof (libxl_device_nic));
-    for (; nb; --nb, ++nics) {
+    for (j = 0; j < nb; ++j, ++nics) {
         for (i = 0, a = nics->mac, b = mac_n;
              (b < mac_n + 6) && (*a == *b); ++a, ++b)
             ;
@@ -394,12 +394,12 @@ int libxl_mac_to_device_nic(libxl_ctx *c
             nic->devid = nics->devid;
             memcpy(nic->mac, nics->mac, sizeof (nic->mac));
             nic->script = nics->script;
-            libxl_free(ctx, nics);
+            libxl_free_nics_list(list, nb);
             return 0;
         }
     }
 
-    libxl_free(ctx, nics);
+    libxl_free_nics_list(list, nb);
     return 0;
 }
 
@@ -419,6 +419,9 @@ int libxl_devid_to_device_nic(libxl_ctx 
     nic_path_be = libxl_xs_read(ctx, XBT_NULL,
                                 libxl_sprintf(ctx, "%s/backend", nic_path_fe));
     val = libxl_xs_read(ctx, XBT_NULL, libxl_sprintf(ctx, "%s/backend-id", nic_path_fe));
+    if ( NULL == val ) {
+        return ERROR_FAIL;
+    }
     nic->backend_domid = strtoul(val, NULL, 10);
     nic->devid = strtoul(devid, NULL, 10);
     libxl_free(ctx, val);
tools/libxl/xl_cmdimpl.c +5 -4
diff -r 4a877c3d722f -r 0c8c78fea130 tools/libxl/xl_cmdimpl.c
@@ -4031,8 +4031,8 @@ int main_networkattach(int argc, char **
 int main_networklist(int argc, char **argv)
 {
     int opt;
-    libxl_nicinfo *nics;
-    unsigned int nb;
+    libxl_nicinfo *nics, *list;
+    unsigned int nb, i;
 
     if (argc < 3) {
         help("network-list");
@@ -4057,10 +4057,10 @@ int main_networklist(int argc, char **ar
             fprintf(stderr, "%s is an invalid domain identifier\n", *argv);
             continue;
         }
-        if (!(nics = libxl_list_nics(&ctx, domid, &nb))) {
+        if (!(list = nics = libxl_list_nics(&ctx, domid, &nb))) {
             continue;
         }
-        for (; nb > 0; --nb, ++nics) {
+        for (i = 0; i < nb; ++i, ++nics) {
             /* Idx BE */
             printf("%-3d %-2d ", nics->devid, nics->backend_id);
             /* MAC */
@@ -4072,6 +4072,7 @@ int main_networklist(int argc, char **ar
                    nics->devid, nics->state, nics->evtch,
                    nics->rref_tx, nics->rref_rx, nics->backend);
         }
+        libxl_free_nics_list(list, nb);
     }
     return 0;
 }
#8 lore raw

PATCH 1/1 xl: Fix invalid return of libxl-internal pointers via libxl_domid_to_name()

In reply to Gianni Tedesco
 tools/libxl/libxl.c          |  12 +++++-----
 tools/libxl/libxl_internal.h |   3 +-
 tools/libxl/libxl_pci.c      |   2 +-
 tools/libxl/libxl_utils.c    |  11 ++++++++-
 tools/libxl/xl_cmdimpl.c     |  49 +++++++++++++++++++++++++++++--------------
 5 files changed, 50 insertions(+), 27 deletions(-)

This function has numerous in-and-out-of-library callers. In library callers now use _libxl_domid_to_name() which participates in garbage collection and out-of-library callers are fixed up to free() the domain name.

tools/libxl/libxl.c +6 -6
diff -r 0c8c78fea130 -r b4d2cf5a2a60 tools/libxl/libxl.c
@@ -1221,7 +1221,7 @@ static int libxl_create_stubdom(libxl_ct
 
     memset(&c_info, 0x00, sizeof(libxl_domain_create_info));
     c_info.hvm = 0;
-    c_info.name = libxl_sprintf(ctx, "%s-dm", libxl_domid_to_name(ctx, info->domid));
+    c_info.name = libxl_sprintf(ctx, "%s-dm", _libxl_domid_to_name(ctx, info->domid));
     for (i = 0; i < 16; i++)
         c_info.uuid[i] = info->uuid[i];
 
@@ -2028,7 +2028,7 @@ retry_transaction:
     flexarray_set(back, boffset++, "state");
     flexarray_set(back, boffset++, libxl_sprintf(ctx, "%d", 1));
     flexarray_set(back, boffset++, "domain");
-    flexarray_set(back, boffset++, libxl_domid_to_name(ctx, domid));
+    flexarray_set(back, boffset++, _libxl_domid_to_name(ctx, domid));
     flexarray_set(back, boffset++, "protocol");
     flexarray_set(back, boffset++, LIBXL_XENCONSOLE_PROTOCOL);
 
@@ -2085,7 +2085,7 @@ int libxl_device_vkb_add(libxl_ctx *ctx,
     flexarray_set(back, boffset++, "state");
     flexarray_set(back, boffset++, libxl_sprintf(ctx, "%d", 1));
     flexarray_set(back, boffset++, "domain");
-    flexarray_set(back, boffset++, libxl_domid_to_name(ctx, domid));
+    flexarray_set(back, boffset++, _libxl_domid_to_name(ctx, domid));
 
     flexarray_set(front, foffset++, "backend-id");
     flexarray_set(front, foffset++, libxl_sprintf(ctx, "%d", vkb->backend_domid));
@@ -2262,7 +2262,7 @@ static int libxl_build_xenpv_qemu_args(l
         uint32_t guest_domid;
         if (libxl_is_stubdom(ctx, vfb->domid, &guest_domid)) {
             char *filename;
-            char *name = libxl_sprintf(ctx, "qemu-dm-%s", libxl_domid_to_name(ctx, guest_domid));
+            char *name = libxl_sprintf(ctx, "qemu-dm-%s", _libxl_domid_to_name(ctx, guest_domid));
             libxl_create_logfile(ctx, name, &filename);
             info->serial = libxl_sprintf(ctx, "file:%s", filename);
             free(filename);
@@ -2280,7 +2280,7 @@ static int libxl_build_xenpv_qemu_args(l
         info->extra[j] = NULL;
     }
     info->domid = vfb->domid;
-    info->dom_name = libxl_domid_to_name(ctx, vfb->domid);
+    info->dom_name = _libxl_domid_to_name(ctx, vfb->domid);
     info->device_model = libxl_abs_path(ctx, "qemu-dm", libxl_libexec_path());
     info->type = XENPV;
     return 0;
@@ -2326,7 +2326,7 @@ int libxl_device_vfb_add(libxl_ctx *ctx,
     flexarray_set(back, boffset++, "state");
     flexarray_set(back, boffset++, libxl_sprintf(ctx, "%d", 1));
     flexarray_set(back, boffset++, "domain");
-    flexarray_set(back, boffset++, libxl_domid_to_name(ctx, domid));
+    flexarray_set(back, boffset++, _libxl_domid_to_name(ctx, domid));
     flexarray_set(back, boffset++, "vnc");
     flexarray_set(back, boffset++, libxl_sprintf(ctx, "%d", vfb->vnc));
     flexarray_set(back, boffset++, "vnclisten");
tools/libxl/libxl_internal.h +1 -2
diff -r 0c8c78fea130 -r b4d2cf5a2a60 tools/libxl/libxl_internal.h
@@ -226,8 +226,7 @@ char *libxl_abs_path(libxl_ctx *ctx, cha
 #define XL_LOG_WARNING XTL_WARN
 #define XL_LOG_ERROR   XTL_ERROR
 
-/* Error handling */
-int libxl_xc_error(int xc_err);
+char *_libxl_domid_to_name(libxl_ctx *ctx, uint32_t domid);
 
 #endif
 
tools/libxl/libxl_pci.c +1 -1
diff -r 0c8c78fea130 -r b4d2cf5a2a60 tools/libxl/libxl_pci.c
@@ -224,7 +224,7 @@ static int libxl_create_pci_backend(libx
     flexarray_set(back, boffset++, "state");
     flexarray_set(back, boffset++, libxl_sprintf(ctx, "%d", 1));
     flexarray_set(back, boffset++, "domain");
-    flexarray_set(back, boffset++, libxl_domid_to_name(ctx, domid));
+    flexarray_set(back, boffset++, _libxl_domid_to_name(ctx, domid));
     for (i = 0; i < num; i++) {
         flexarray_set(back, boffset++, libxl_sprintf(ctx, "key-%d", i));
         flexarray_set(back, boffset++, libxl_sprintf(ctx, PCI_BDF, pcidev->domain, pcidev->bus, pcidev->dev, pcidev->func));
tools/libxl/libxl_utils.c +9 -2
diff -r 0c8c78fea130 -r b4d2cf5a2a60 tools/libxl/libxl_utils.c
@@ -51,7 +51,14 @@ char *libxl_domid_to_name(libxl_ctx *ctx
 
     snprintf(path, sizeof(path), "/local/domain/%d/name", domid);
     s = xs_read(ctx->xsh, XBT_NULL, path, &len);
-    libxl_ptr_add(ctx, s);
+    return s;
+}
+
+char *_libxl_domid_to_name(libxl_ctx *ctx, uint32_t domid)
+{
+    char *s = libxl_domid_to_name(ctx, domid);
+    if ( s )
+        libxl_ptr_add(ctx, s);
     return s;
 }
 
@@ -68,7 +75,7 @@ int libxl_name_to_domid(libxl_ctx *ctx, 
         return ERROR_NOMEM;
 
     for (i = 0; i < nb_domains; i++) {
-        domname = libxl_domid_to_name(ctx, dominfo[i].domid);
+        domname = _libxl_domid_to_name(ctx, dominfo[i].domid);
         if (!domname)
             continue;
         if (strcmp(domname, name) == 0) {
tools/libxl/xl_cmdimpl.c +33 -16

150 lines not on this page · raw message

#9 lore raw

PATCH 1/1 xl: Fix invalid return of libxl-internal pointers via libxl_poolid_to_name()

In reply to Gianni Tedesco
 tools/libxl/libxl_internal.h |   1 +
 tools/libxl/libxl_utils.c    |  11 +++++++++--
 2 files changed, 10 insertions(+), 2 deletions(-)

This function has in-and-out-of-library callers. In library callers now use _libxl_poolid_to_name() which participates in garbage collection and out-of-library callers are fixed up to free() the domain name.

tools/libxl/libxl_internal.h +1 -0
diff -r b4d2cf5a2a60 -r 4c643fd7aaa2 tools/libxl/libxl_internal.h
@@ -227,6 +227,7 @@ char *libxl_abs_path(libxl_ctx *ctx, cha
 #define XL_LOG_ERROR   XTL_ERROR
 
 char *_libxl_domid_to_name(libxl_ctx *ctx, uint32_t domid);
+char *_libxl_poolid_to_name(libxl_ctx *ctx, uint32_t poolid);
 
 #endif
 
tools/libxl/libxl_utils.c +9 -2
diff -r b4d2cf5a2a60 -r 4c643fd7aaa2 tools/libxl/libxl_utils.c
@@ -98,7 +98,14 @@ char *libxl_poolid_to_name(libxl_ctx *ct
         return "Pool-0";
     snprintf(path, sizeof(path), "/local/pool/%d/name", poolid);
     s = xs_read(ctx->xsh, XBT_NULL, path, &len);
-    libxl_ptr_add(ctx, s);
+    return s;
+}
+
+char *_libxl_poolid_to_name(libxl_ctx *ctx, uint32_t poolid)
+{
+    char *s = libxl_poolid_to_name(ctx, poolid);
+    if ( s )
+        libxl_ptr_add(ctx, s);
     return s;
 }
 
@@ -114,7 +121,7 @@ int libxl_name_to_poolid(libxl_ctx *ctx,
         return ERROR_NOMEM;
 
     for (i = 0; i < nb_pools; i++) {
-        poolname = libxl_poolid_to_name(ctx, poolinfo[i].poolid);
+        poolname = _libxl_poolid_to_name(ctx, poolinfo[i].poolid);
         if (!poolname)
             continue;
         if (strcmp(poolname, name) == 0) {
#10 lore raw

PATCH 1/1 xl: Use gcc hidden visibility attribute

In reply to Gianni Tedesco
 tools/libxl/flexarray.c      |    3 +-
 tools/libxl/flexarray.h      |   12 ++--
 tools/libxl/libxl_internal.h |  102 +++++++++++++++++++++++-------------------
 3 files changed, 62 insertions(+), 55 deletions(-)

This kills off about 1,000 PLT entries speeding up link time and shaving about 1-2KB off of binary size. It also prevents users of the library erroneously calling libxl internal functions.

tools/libxl/flexarray.c +1 -2
diff -r 4c643fd7aaa2 -r e3677e1a76c9 tools/libxl/flexarray.c
@@ -13,8 +13,7 @@
  * GNU Lesser General Public License for more details.
  */
 
-#include <stdlib.h>
-#include "flexarray.h"
+#include "libxl_internal.h"
 
 flexarray_t *flexarray_make(int size, int autogrow)
 {
tools/libxl/flexarray.h +6 -6
diff -r 4c643fd7aaa2 -r e3677e1a76c9 tools/libxl/flexarray.h
@@ -22,12 +22,12 @@ typedef struct flexarray {
     void **data; /* array of pointer */
 } flexarray_t;
 
-flexarray_t *flexarray_make(int size, int autogrow);
-void flexarray_free(flexarray_t *array);
-int flexarray_grow(flexarray_t *array, int extents);
-int flexarray_set(flexarray_t *array, unsigned int index, void *ptr);
-int flexarray_get(flexarray_t *array, int index, void **ptr);
+_hidden flexarray_t *flexarray_make(int size, int autogrow);
+_hidden void flexarray_free(flexarray_t *array);
+_hidden int flexarray_grow(flexarray_t *array, int extents);
+_hidden int flexarray_set(flexarray_t *array, unsigned int index, void *ptr);
+_hidden int flexarray_get(flexarray_t *array, int index, void **ptr);
 
-void **flexarray_contents(flexarray_t *array);
+_hidden void **flexarray_contents(flexarray_t *array);
 
 #endif
tools/libxl/libxl_internal.h +55 -47

184 lines not on this page · raw message

#11 lore raw

Re: [PATCH 7 of 7] xl: Use gcc hidden visibility attribute

FYI, here is a hacky script to locate self-PLT entries in x86_64 binaries. It depends on binutils and elfutils and clobbers a file called './syms' in order to avoid having to think about deadlocks in rw popen.

8<-----------------------------------
#!/usr/bin/python

import os,sys

def check_file(fn):

    f_plt = os.popen("eu-readelf --relocs %s| grep 'X86_64_JUMP_SLOT 0x' | awk '{print $5}'"%fn)
    plt = {}.fromkeys(map(lambda x:x.rstrip("\r\n"), f_plt.readlines()))
    del f_plt

    f_ref = os.popen("objdump -d %s | egrep '\<callq\>.*@plt' | awk '{print $1 $9}'"%fn)

    tmp = map(lambda x:x.split(":"), f_ref.readlines())
    del f_ref

    refs = []
    for x in tmp:
        assert(len(x) == 2)
        (addr, sym) = x
        sym = sym.rstrip(">\r\n").lstrip("<")
        assert(len(sym) > 4 and sym[-4:] == '@plt')
        sym = sym[:-4]
        addr = int(addr, 16)
        refs.append((addr,sym))
    del tmp

    refs = filter(lambda x:plt.has_key(x[1]), refs)
    f_eu = os.popen("xargs eu-addr2line -e %s > syms"%fn, 'w')
    for (addr,sym) in refs:
        f_eu.write("0x%x\n"%addr)
        #print "0x%x %s"%(addr,sym)
    f_eu.close()

    f_eu = open("syms", 'r')
    os.unlink("syms")
    addrlines = map(lambda x:x.rstrip("\r\n"), f_eu.readlines())
    del f_eu

    assert(len(addrlines) == len(refs))

    for ((addr, sym), line) in zip(refs, addrlines):
        print "%s -> %s"%(line, sym)

    return True

if __name__ == '__main__':

    if len(sys.argv) != 2:
        print "Usage: %s <ELF-lib>"%sys.argv[0]
        raise SystemExit, 1
    if check_file(sys.argv[1]):
        raise SystemExit
    raise SystemExit, 1