[PATCH 1/5] libxl, gc: fix memory leak in libxl__strndup()

[PATCH 1/5] libxl, gc: fix memory leak in libxl__strndup()

[Lai Jiangshan]

Signed-off-by: Lai Jiangshan <laijs@cn.fujitsu.com>
---
 tools/libxl/libxl_internal.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/tools/libxl/libxl_internal.c b/tools/libxl/libxl_internal.c
index cf17658..16b82b9 100644
--- a/tools/libxl/libxl_internal.c
+++ b/tools/libxl/libxl_internal.c
@@ -155,6 +155,8 @@ char *libxl__strndup(libxl__gc *gc, const char *c, size_t n)
 
     if (!s) libxl__alloc_failed(CTX, __func__, n, 1);
 
+    libxl__ptr_add(gc, s);
+
     return s;
 }
 
-- 
1.7.1

[PATCH 2/5] libxl: simplify libxl__dirname()

[Lai Jiangshan]

Signed-off-by: Lai Jiangshan <laijs@cn.fujitsu.com>
---
 tools/libxl/libxl_internal.c |    8 +++-----
 1 files changed, 3 insertions(+), 5 deletions(-)

diff --git a/tools/libxl/libxl_internal.c b/tools/libxl/libxl_internal.c
index 16b82b9..1db48b6 100644
--- a/tools/libxl/libxl_internal.c
+++ b/tools/libxl/libxl_internal.c
@@ -162,14 +162,12 @@ char *libxl__strndup(libxl__gc *gc, const char *c, size_t n)
 
 char *libxl__dirname(libxl__gc *gc, const char *s)
 {
-    char *c;
-    char *ptr = libxl__strdup(gc, s);
+    char *c = strrchr(s, '/');
 
-    c = strrchr(ptr, '/');
     if (!c)
         return NULL;
-    *c = '\0';
-    return ptr;
+
+    return libxl__strndup(gc, s, c - s);
 }
 
 void libxl__logv(libxl_ctx *ctx, xentoollog_level msglevel, int errnoval,
-- 
1.7.1

Re: [PATCH 2/5] libxl: simplify libxl__dirname()

[Ian Jackson]

Lai Jiangshan writes ("[PATCH 2/5] libxl: simplify libxl__dirname()"):
> Signed-off-by: Lai Jiangshan <laijs@cn.fujitsu.com>

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

[PATCH 3/5] libxl: add @count to libxl_gc

[Lai Jiangshan]

Signed-off-by: Lai Jiangshan <laijs@cn.fujitsu.com>
---
 tools/libxl/libxl_internal.c |   22 ++++++++--------------
 tools/libxl/libxl_internal.h |    2 ++
 2 files changed, 10 insertions(+), 14 deletions(-)

diff --git a/tools/libxl/libxl_internal.c b/tools/libxl/libxl_internal.c
index 1db48b6..a79d841 100644
--- a/tools/libxl/libxl_internal.c
+++ b/tools/libxl/libxl_internal.c
@@ -32,8 +32,6 @@ void libxl__alloc_failed(libxl_ctx *ctx, const char *func,
 
 void libxl__ptr_add(libxl__gc *gc, void *ptr)
 {
-    int i;
-
     if (!libxl__gc_is_real(gc))
         return;
 
@@ -41,22 +39,17 @@ void libxl__ptr_add(libxl__gc *gc, void *ptr)
         return;
 
     /* fast case: we have space in the array for storing the pointer */
-    for (i = 0; i < gc->alloc_maxsize; i++) {
-        if (!gc->alloc_ptrs[i]) {
-            gc->alloc_ptrs[i] = ptr;
-            return;
-        }
-    }
+    if (gc->count < gc->alloc_maxsize)
+        gc->alloc_ptrs[gc->count++] = ptr;
+
     int new_maxsize = gc->alloc_maxsize * 2 + 25;
     assert(new_maxsize < INT_MAX / sizeof(void*) / 2);
     gc->alloc_ptrs = realloc(gc->alloc_ptrs, new_maxsize * sizeof(void *));
     if (!gc->alloc_ptrs)
         libxl__alloc_failed(CTX, __func__, new_maxsize, sizeof(void*));
 
-    gc->alloc_ptrs[gc->alloc_maxsize++] = ptr;
-
-    while (gc->alloc_maxsize < new_maxsize)
-        gc->alloc_ptrs[gc->alloc_maxsize++] = 0;
+    gc->alloc_ptrs[gc->count++] = ptr;
+    gc->alloc_maxsize = new_maxsize;
 
     return;
 }
@@ -68,7 +61,7 @@ void libxl__free_all(libxl__gc *gc)
 
     assert(libxl__gc_is_real(gc));
 
-    for (i = 0; i < gc->alloc_maxsize; i++) {
+    for (i = 0; i < gc->count; i++) {
         ptr = gc->alloc_ptrs[i];
         gc->alloc_ptrs[i] = NULL;
         free(ptr);
@@ -76,6 +69,7 @@ void libxl__free_all(libxl__gc *gc)
     free(gc->alloc_ptrs);
     gc->alloc_ptrs = 0;
     gc->alloc_maxsize = 0;
+    gc->count = 0;
 }
 
 void *libxl__zalloc(libxl__gc *gc, int bytes)
@@ -107,7 +101,7 @@ void *libxl__realloc(libxl__gc *gc, void *ptr, size_t new_size)
     if (ptr == NULL) {
         libxl__ptr_add(gc, new_ptr);
     } else if (new_ptr != ptr && libxl__gc_is_real(gc)) {
-        for (i = 0; i < gc->alloc_maxsize; i++) {
+        for (i = 0; i < gc->count; i++) {
             if (gc->alloc_ptrs[i] == ptr) {
                 gc->alloc_ptrs[i] = new_ptr;
                 break;
diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
index 1bd23ff..d2100d8 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -294,6 +294,7 @@ struct libxl__poller {
 
 struct libxl__gc {
     /* mini-GC */
+    int count;
     int alloc_maxsize; /* -1 means this is the dummy non-gc gc */
     void **alloc_ptrs;
     libxl_ctx *owner;
@@ -438,6 +439,7 @@ struct libxl__ao {
 };
 
 #define LIBXL_INIT_GC(gc,ctx) do{               \
+        (gc).count = 0;                         \
         (gc).alloc_maxsize = 0;                 \
         (gc).alloc_ptrs = 0;                    \
         (gc).owner = (ctx);                     \
-- 
1.7.1

Re: [PATCH 3/5] libxl: add @count to libxl_gc

[Ian Jackson]

Lai Jiangshan writes ("[PATCH 3/5] libxl: add @count to libxl_gc"):
> Signed-off-by: Lai Jiangshan <laijs@cn.fujitsu.com>

I'm afraid this needs a lot more rationale.

Ian.

[PATCH 4/5] xc_save: avoid to alloc local constant string

[Lai Jiangshan]

cmd_str will only cleanup by other place,
it don't need to be allocated.

Signed-off-by: Lai Jiangshan <laijs@cn.fujitsu.com>
---
 tools/xcutils/xc_save.c |    4 +---
 1 files changed, 1 insertions(+), 3 deletions(-)

diff --git a/tools/xcutils/xc_save.c b/tools/xcutils/xc_save.c
index e34bd2c..1304433 100644
--- a/tools/xcutils/xc_save.c
+++ b/tools/xcutils/xc_save.c
@@ -128,8 +128,7 @@ static int switch_qemu_logdirty(int domid, unsigned int enable, void *data)
     if (!xs_watch(xs, path, "qemu-logdirty-ret"))
         errx(1, "can't set watch in store (%s)\n", path);
 
-    if (!(cmd_str = strdup( enable == 0 ? "disable" : "enable")))
-        errx(1, "can't get logdirty cmd path in store");
+    cmd_str = enable == 0 ? "disable" : "enable";
 
     /* Tell qemu that we want it to start logging dirty page to Xen */
     strcpy(p, "cmd");
@@ -157,7 +156,6 @@ static int switch_qemu_logdirty(int domid, unsigned int enable, void *data)
         goto read_again;
 
     free(path);
-    free(cmd_str);
     free(ret_str);
 
     return 0;
-- 
1.7.1

Re: [PATCH 4/5] xc_save: avoid to alloc local constant string

[Ian Jackson]

Lai Jiangshan writes ("[PATCH 4/5] xc_save: avoid to alloc local constant string"):
> cmd_str will only cleanup by other place,
> it don't need to be allocated.
> 
> Signed-off-by: Lai Jiangshan <laijs@cn.fujitsu.com>

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

[PATCH 5/5] xc_save: simplify switch_qemu_logdirty() by use stack strings

[Lai Jiangshan]

Signed-off-by: Lai Jiangshan <laijs@cn.fujitsu.com>
---
 tools/xcutils/xc_save.c |   35 +++++++++++++----------------------
 1 files changed, 13 insertions(+), 22 deletions(-)

diff --git a/tools/xcutils/xc_save.c b/tools/xcutils/xc_save.c
index 1304433..521bbae 100644
--- a/tools/xcutils/xc_save.c
+++ b/tools/xcutils/xc_save.c
@@ -101,40 +101,33 @@ static int suspend(void* data)
  * the new active buffer.  xc_save can then process and clear the old
  * active buffer. */
 
-
 static int switch_qemu_logdirty(int domid, unsigned int enable, void *data)
 {
     struct xs_handle *xs;
-    char *path, *p, *ret_str, *cmd_str, **watch;
+    char *ret_str, *cmd_str, **watch;
     unsigned int len;
     struct timeval tv;
     fd_set fdset;
 
+#define PATH_FMT	"/local/domain/0/device-model/%d/logdirty/%s"
+#define PATH_LEN	(sizeof(PATH_FMT) + 10 + sizeof("cmd"))
+    char path_cmd[PATH_LEN], path_ret[PATH_LEN];
+
+    snprintf(path_cmd, PATH_LEN, PATH_FMT, domid, "cmd");
+    snprintf(path_ret, PATH_LEN, PATH_FMT, domid, "ret");
+
     if ((xs = xs_daemon_open()) == NULL)
         errx(1, "Couldn't contact xenstore");
-    if (!(path = strdup("/local/domain/0/device-model/")))
-        errx(1, "can't get domain path in store");
-    if (!(path = realloc(path, strlen(path) 
-                         + 10 
-                         + strlen("/logdirty/cmd") + 1)))
-        errx(1, "no memory for constructing xenstore path");
-    snprintf(path + strlen(path), 11, "%i", domid);
-    strcat(path, "/logdirty/");
-    p = path + strlen(path);
-
 
     /* Watch for qemu's return value */
-    strcpy(p, "ret");
-    if (!xs_watch(xs, path, "qemu-logdirty-ret"))
-        errx(1, "can't set watch in store (%s)\n", path);
+    if (!xs_watch(xs, path_ret, "qemu-logdirty-ret"))
+        errx(1, "can't set watch in store (%s)\n", path_ret);
 
     cmd_str = enable == 0 ? "disable" : "enable";
 
     /* Tell qemu that we want it to start logging dirty page to Xen */
-    strcpy(p, "cmd");
-    if (!xs_write(xs, XBT_NULL, path, cmd_str, strlen(cmd_str)))
-        errx(1, "can't write  to store path (%s)\n",
-             path);
+    if (!xs_write(xs, XBT_NULL, path_cmd, cmd_str, strlen(cmd_str)))
+        errx(1, "can't write  to store path (%s)\n", path_cmd);
 
     /* Wait a while for qemu to signal that it has service logdirty command */
  read_again:
@@ -149,13 +142,11 @@ static int switch_qemu_logdirty(int domid, unsigned int enable, void *data)
     watch = xs_read_watch(xs, &len);
     free(watch);
 
-    strcpy(p, "ret");
-    ret_str = xs_read(xs, XBT_NULL, path, &len);
+    ret_str = xs_read(xs, XBT_NULL, path_ret, &len);
     if (ret_str == NULL || strcmp(ret_str, cmd_str))
         /* Watch fired but value is not yet right */
         goto read_again;
 
-    free(path);
     free(ret_str);
 
     return 0;
-- 
1.7.1

Re: [PATCH 5/5] xc_save: simplify switch_qemu_logdirty() by use stack strings

[Ian Jackson]

Lai Jiangshan writes ("[PATCH 5/5] xc_save: simplify switch_qemu_logdirty() by use stack strings"):
> Signed-off-by: Lai Jiangshan <laijs@cn.fujitsu.com>

Thanks for this submission.  This is quite fiddly to review.  Can you
explain what the concrete benefit of this is ?

Thanks,
Ian.

Re: [PATCH 5/5] xc_save: simplify switch_qemu_logdirty() by use stack strings

[Lai Jiangshan]

On 03/14/2014 12:54 AM, Ian Jackson wrote:
> Lai Jiangshan writes ("[PATCH 5/5] xc_save: simplify switch_qemu_logdirty() by use stack strings"):
>> Signed-off-by: Lai Jiangshan <laijs@cn.fujitsu.com>
> 
> Thanks for this submission.  This is quite fiddly to review.  Can you
> explain what the concrete benefit of this is ?
> 
Hi, Ian Jackson.

no concrete benefit, but help for reviewing:

simplify by "allocating" strings from stack.
reduce memory-allocation and reduce error-handling-branch.

path_cmd and path_ret strings are separated.

Thanks,
Lai

Re: [PATCH 1/5] libxl, gc: fix memory leak in libxl__strndup()

[Ian Jackson]

Lai Jiangshan writes ("[PATCH 1/5] libxl,gc: fix memory leak in libxl__strndup()"):
> Signed-off-by: Lai Jiangshan <laijs@cn.fujitsu.com>

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