[PATCH v2 0/7] libxl: make _dispose idempotent and NULL-tolerant

[PATCH v2 0/7] libxl: make _dispose idempotent and NULL-tolerant

[Wei Liu]

The first two patches are bug fixes.

Other patches are used to make _dispose idempotent and NULL-tolerant.

Wei.

Wei Liu (7):
  libxl: fix off-by-one error in JSON parser
  gentest: make testidl valgrind clean
  libxl: make some _dipose functions idempotent and tolerate NULL
  gentypes: zero out structure in _dispose function
  gentypes: make dispose function tolerate NULL
  testidl: call _init and _dispose several times
  libxl: update libxl.h to say _dispose is idempotent

 tools/libxl/gentest.py    | 22 ++++++++++++++++------
 tools/libxl/gentypes.py   |  4 ++--
 tools/libxl/libxl.c       | 11 +++++++++--
 tools/libxl/libxl.h       |  3 +--
 tools/libxl/libxl_cpuid.c |  5 ++++-
 tools/libxl/libxl_json.c  |  2 +-
 tools/libxl/libxl_utils.c |  5 +++++
 7 files changed, 38 insertions(+), 14 deletions(-)

-- 
1.9.1

[PATCH v2 1/7] libxl: fix off-by-one error in JSON parser

[Wei Liu]

We need a sentinel slot in the generated libxl_key_value_list.

Signed-off-by: Wei Liu <wei.liu2@citrix.com>
Cc: Ian Campbell <ian.campbell@citrix.com>
Cc: Ian Jackson <ian.jackson@eu.citrix.com>
---
This should be backported to 4.5.
---
 tools/libxl/libxl_json.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/libxl/libxl_json.c b/tools/libxl/libxl_json.c
index ceb014a..98335b0 100644
--- a/tools/libxl/libxl_json.c
+++ b/tools/libxl/libxl_json.c
@@ -247,7 +247,7 @@ int libxl__key_value_list_parse_json(libxl__gc *gc, const libxl__json_object *o,
 
     maps = libxl__json_object_get_map(o);
     size = maps->count * 2;
-    kvl = *p = libxl__calloc(NOGC, size, sizeof(char *));
+    kvl = *p = libxl__calloc(NOGC, size+1, sizeof(char *));
 
     for (i = 0; i < maps->count; i++) {
         int idx = i * 2;
-- 
1.9.1

[PATCH v2 2/7] gentest: make testidl valgrind clean

[Wei Liu]

Free the JSON string after use to avoid memory leak. With this change
testidl is valgrind clean.

Signed-off-by: Wei Liu <wei.liu2@citrix.com>
Cc: Ian Campbell <ian.campbell@citrix.com>
Cc: Ian Jackson <ian.jackson@eu.citrix.com>
---
 tools/libxl/gentest.py | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/tools/libxl/gentest.py b/tools/libxl/gentest.py
index 95323d1..849bfc7 100644
--- a/tools/libxl/gentest.py
+++ b/tools/libxl/gentest.py
@@ -229,7 +229,7 @@ int main(int argc, char **argv)
                 (ty.typename, ty.typename, ty.typename))
     f.write("""
     int rc;
-    char *s, *new_s;
+    char *s, *new_s, *json_string;
     xentoollog_logger_stdiostream *logger;
     libxl_ctx *ctx;
 
@@ -323,9 +323,13 @@ int main(int argc, char **argv)
 
         f.write("    printf(\"%s -- to JSON:\\n\");\n" % (ty.typename))
         for v in ty.values:
+            f.write("    json_string = %s_to_json(ctx, %s);\n" % \
+                    (ty.typename, v.name))
             f.write("    printf(\"\\t%s = %%d = %%s\", " \
-                    "%s, %s_to_json(ctx, %s));\n" %\
-                    (v.valuename, v.name, ty.typename, v.name))
+                    "%s, json_string);\n" %\
+                    (v.valuename, v.name))
+            f.write("    free(json_string);\n");
+            f.write("    json_string = NULL;\n");
         f.write("\n")
 
         f.write("    printf(\"%s -- from string:\\n\");\n" % (ty.typename))
-- 
1.9.1

[PATCH v2 3/7] libxl: make some _dipose functions idempotent and tolerate NULL

[Wei Liu]

These functions are not generated, so we need to do it by hand.

Functions list:
 libxl_bitmap_dispose
 libxl_string_list_dispose
 libxl_key_value_list_dipose
 libxl_cpuid_dispose

Signed-off-by: Wei Liu <wei.liu2@citrix.com>
Cc: Ian Campbell <ian.campbell@citrix.com>
Cc: Ian Jackson <ian.jackson@eu.citrix.com>
---
 tools/libxl/libxl.c       | 11 +++++++++--
 tools/libxl/libxl_cpuid.c |  5 ++++-
 tools/libxl/libxl_utils.c |  5 +++++
 3 files changed, 18 insertions(+), 3 deletions(-)

diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
index b9a1941..486daf8 100644
--- a/tools/libxl/libxl.c
+++ b/tools/libxl/libxl.c
@@ -214,9 +214,12 @@ void libxl_string_list_dispose(libxl_string_list *psl)
     if (!sl)
         return;
 
-    for (i = 0; sl[i] != NULL; i++)
+    for (i = 0; sl[i] != NULL; i++) {
         free(sl[i]);
+        sl[i] = NULL;
+    }
     free(sl);
+    *psl = NULL;
 }
 
 void libxl_string_list_copy(libxl_ctx *ctx,
@@ -276,10 +279,14 @@ void libxl_key_value_list_dispose(libxl_key_value_list *pkvl)
 
     for (i = 0; kvl[i] != NULL; i += 2) {
         free(kvl[i]);
-        if (kvl[i + 1])
+        kvl[i] = NULL;
+        if (kvl[i + 1]) {
             free(kvl[i + 1]);
+            kvl[i+1] = NULL;
+        }
     }
     free(kvl);
+    *pkvl = NULL;
 }
 
 void libxl_key_value_list_copy(libxl_ctx *ctx,
diff --git a/tools/libxl/libxl_cpuid.c b/tools/libxl/libxl_cpuid.c
index 7cfa6b7..b0bdc9d 100644
--- a/tools/libxl/libxl_cpuid.c
+++ b/tools/libxl/libxl_cpuid.c
@@ -28,10 +28,13 @@ void libxl_cpuid_dispose(libxl_cpuid_policy_list *p_cpuid_list)
         return;
     for (i = 0; cpuid_list[i].input[0] != XEN_CPUID_INPUT_UNUSED; i++) {
         for (j = 0; j < 4; j++)
-            if (cpuid_list[i].policy[j] != NULL)
+            if (cpuid_list[i].policy[j] != NULL) {
                 free(cpuid_list[i].policy[j]);
+                cpuid_list[i].policy[j] = NULL;
+            }
     }
     free(cpuid_list);
+    *p_cpuid_list = NULL;
     return;
 }
 
diff --git a/tools/libxl/libxl_utils.c b/tools/libxl/libxl_utils.c
index 7095b58..9053b27 100644
--- a/tools/libxl/libxl_utils.c
+++ b/tools/libxl/libxl_utils.c
@@ -604,7 +604,12 @@ void libxl_bitmap_init(libxl_bitmap *map)
 
 void libxl_bitmap_dispose(libxl_bitmap *map)
 {
+    if (!map)
+        return;
+
     free(map->map);
+    map->map = NULL;
+    map->size = 0;
 }
 
 void libxl_bitmap_copy(libxl_ctx *ctx, libxl_bitmap *dptr,
-- 
1.9.1

[PATCH v2 4/7] gentypes: zero out structure in _dispose function

[Wei Liu]

Original the structure was memset to a poison value. That prevented
_dispose to be made idempotent. We should stop doing so.

Memseting the structure to 0 makes all pointers in structure become
NULL, which can be handled by free().

Signed-off-by: Wei Liu <wei.liu2@citrix.com>
Cc: Ian Campbell <ian.campbell@citrix.com>
Cc: Ian Jackson <ian.jackson@eu.citrix.com>
---
 tools/libxl/gentypes.py | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/tools/libxl/gentypes.py b/tools/libxl/gentypes.py
index d9e14fd..afd4eea 100644
--- a/tools/libxl/gentypes.py
+++ b/tools/libxl/gentypes.py
@@ -636,7 +636,6 @@ if __name__ == '__main__':
 
 #include "libxl_internal.h"
 
-#define LIBXL_DTOR_POISON 0xa5
 
 """ % " ".join(sys.argv))
 
@@ -644,7 +643,7 @@ if __name__ == '__main__':
         f.write("void %s(%s)\n" % (ty.dispose_fn, ty.make_arg("p")))
         f.write("{\n")
         f.write(libxl_C_type_dispose(ty, "p"))
-        f.write("    memset(p, LIBXL_DTOR_POISON, sizeof(*p));\n")
+        f.write("    memset(p, 0, sizeof(*p));\n")
         f.write("}\n")
         f.write("\n")
 
-- 
1.9.1

[PATCH v2 5/7] gentypes: make dispose function tolerate NULL

[Wei Liu]

Signed-off-by: Wei Liu <wei.liu2@citrix.com>
Cc: Ian Campbell <ian.campbell@citrix.com>
Cc: Ian Jackson <ian.jackson@eu.citrix.com>
---
 tools/libxl/gentypes.py | 1 +
 1 file changed, 1 insertion(+)

diff --git a/tools/libxl/gentypes.py b/tools/libxl/gentypes.py
index afd4eea..00816c0 100644
--- a/tools/libxl/gentypes.py
+++ b/tools/libxl/gentypes.py
@@ -642,6 +642,7 @@ if __name__ == '__main__':
     for ty in [t for t in types if t.dispose_fn is not None and t.autogenerate_dispose_fn]:
         f.write("void %s(%s)\n" % (ty.dispose_fn, ty.make_arg("p")))
         f.write("{\n")
+        f.write("    if (!p) return;\n")
         f.write(libxl_C_type_dispose(ty, "p"))
         f.write("    memset(p, 0, sizeof(*p));\n")
         f.write("}\n")
-- 
1.9.1

[PATCH v2 6/7] testidl: call _init and _dispose several times

[Wei Liu]

Call _init and _dispose between 1 to 10 times on a type to test if _init
and _dispose are idempotent.

Signed-off-by: Wei Liu <wei.liu2@citrix.com>
Cc: Ian Campbell <ian.campbell@citrix.com>
Cc: Ian Jackson <ian.jackson@eu.citrix.com>
---
 tools/libxl/gentest.py | 12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/tools/libxl/gentest.py b/tools/libxl/gentest.py
index 849bfc7..7621a1e 100644
--- a/tools/libxl/gentest.py
+++ b/tools/libxl/gentest.py
@@ -249,8 +249,11 @@ int main(int argc, char **argv)
         f.write("    %s_rand_init(%s);\n" % (ty.typename, \
             ty.pass_arg(arg, isref=False, passby=idl.PASS_BY_REFERENCE)))
         if not isinstance(ty, idl.Enumeration):
-            f.write("    %s_init(%s_new);\n" % (ty.typename, \
-                ty.pass_arg(arg, isref=False, passby=idl.PASS_BY_REFERENCE)))
+            iters = random.randrange(1,10)
+            while iters > 0:
+                f.write("    %s_init(%s_new);\n" % (ty.typename, \
+                    ty.pass_arg(arg, isref=False, passby=idl.PASS_BY_REFERENCE)))
+                iters -= 1
         f.write("    s = %s_to_json(ctx, %s);\n" % \
                 (ty.typename, ty.pass_arg(arg, isref=False)))
         f.write("    printf(\"%%s: %%s\\n\", \"%s\", s);\n" % ty.typename)
@@ -269,8 +272,11 @@ int main(int argc, char **argv)
         f.write("    free(s);\n")
         f.write("    free(new_s);\n")
         if ty.dispose_fn is not None:
+            iters = random.randrange(1,10)
             f.write("    %s(&%s_val);\n" % (ty.dispose_fn, ty.typename))
-            f.write("    %s(&%s_val_new);\n" % (ty.dispose_fn, ty.typename))
+            while iters > 0:
+                f.write("    %s(&%s_val_new);\n" % (ty.dispose_fn, ty.typename))
+                iters -= 1
         f.write("\n")
 
     f.write("    printf(\"Testing TYPE_copy()\\n\");\n")
-- 
1.9.1

[PATCH v2 7/7] libxl: update libxl.h to say _dispose is idempotent

[Wei Liu]

Signed-off-by: Wei Liu <wei.liu2@citrix.com>
Cc: Ian Campbell <ian.campbell@citrix.com>
Cc: Ian Jackson <ian.jackson@eu.citrix.com>
---
 tools/libxl/libxl.h | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/tools/libxl/libxl.h b/tools/libxl/libxl.h
index c219f59..b394ee8 100644
--- a/tools/libxl/libxl.h
+++ b/tools/libxl/libxl.h
@@ -308,8 +308,7 @@
  * once afterwards, to clean up, regardless of whether operations on
  * this object succeeded or failed.  See the xl code for examples.
  *
- * "init" is idempotent.  We intend that "dispose" will become
- * idempotent, but this is not currently the case.
+ * "init" and "dispose" are idempotent.
  *
  * void libxl_<type>_init(<type> *p):
  *
-- 
1.9.1

Re: [PATCH v2 3/7] libxl: make some _dipose functions idempotent and tolerate NULL

[Ian Campbell]

On Wed, 2015-02-25 at 14:56 +0000, Wei Liu wrote:
>          free(sl[i]);
> +        sl[i] = NULL;

I wonder if a helper which takes a void**p and does "free(*p); *p =
NULL" would be at all useful?

(NB, I intend to apply this patch regardless and am in the process of
doing so)

Re: [PATCH v2 0/7] libxl: make _dispose idempotent and NULL-tolerant

[Ian Campbell]

On Wed, 2015-02-25 at 14:55 +0000, Wei Liu wrote:
> The first two patches are bug fixes.
> 
> Other patches are used to make _dispose idempotent and NULL-tolerant.
> 
> Wei.
> 
> Wei Liu (7):

All acked + applied, with one typo fixed.

>   libxl: fix off-by-one error in JSON parser
>   gentest: make testidl valgrind clean
>   libxl: make some _dipose functions idempotent and tolerate NULL

"dispose".

>   gentypes: zero out structure in _dispose function
>   gentypes: make dispose function tolerate NULL
>   testidl: call _init and _dispose several times
>   libxl: update libxl.h to say _dispose is idempotent
> 
>  tools/libxl/gentest.py    | 22 ++++++++++++++++------
>  tools/libxl/gentypes.py   |  4 ++--
>  tools/libxl/libxl.c       | 11 +++++++++--
>  tools/libxl/libxl.h       |  3 +--
>  tools/libxl/libxl_cpuid.c |  5 ++++-
>  tools/libxl/libxl_json.c  |  2 +-
>  tools/libxl/libxl_utils.c |  5 +++++
>  7 files changed, 38 insertions(+), 14 deletions(-)
>