[PATCH RFC V2 00/10] xl/libxl: JSON infrastructure and new "xl-json" format

[PATCH RFC V2 00/10] xl/libxl: JSON infrastructure and new "xl-json" format

[Wei Liu]

This series contains my original patches to add in JSON parser and a new patch
to add "xl-json" format. The last patch makes use of JSON parser.

For the JSON parser (taken from my previous email):

Libxl introduced JSON infrastructure to communicate with upstream QEMU. The
ability of exisiting JSON infrastructure is to convert libxl_FOO struct to
libxl JSON object and parse JSON object (string) returned by QEMU.

This series makes libxl able to convert a JSON string to libxl_FOO struct.
The conversion is done in two macro steps:
 1. convert JSON string to libxl JSON object (libxl__json_object)
 2. convert libxl JSON object to libxl_FOO struct

The first stage is done with existing infrastructure. This series focuses on
the second stage.

To break things down:
 * Patch 1 is only mechanical change
 * Patch 2, 3, 4 and 6 are changes to generic libxl JSON infrastructure
 * Patch 5 introduces parser functions for builtin types
 * Patch 7 and 8 are changes to code generator
 * Patch 9 contains changes to test code generator

Now we have the ability to do the following transformation:
  libxl_FOO struct -> libxl_FOO JSON object -> libxl JSON internal object ->
  libxl_FOO struct

This full cycle can be used to preserved runtime configurations of a domain.

Patch 10 makes use of the new parser and introduces "xl-json" format. Please
refer to commit log for details. It would be good if I can get some input on
this new format and it's usage. Currently it's restricted to internal use only,
that is, user is not allowed to supply a config file in JSON format.

It would be good if we can get this kind of infrastructure change through the
push gate before pursing next step ("pull runtime information for a domain").

Wei.

Changes in V2:
* address IanC's comments on JSON parser
* add "xl-json" format patch

Wei Liu (10):
  libxl IDL: rename json_fn to json_gen_fn
  libxl_json: introduce libx__object_from_json
  libxl_internal: make JSON_* types bit-field
  libxl_internal: introduce libxl__json_object_is_{null,number,double}
  libxl_json: introduce parser functions for builtin types
  libxl_json: allow basic JSON type objects generation
  libxl/gentypes.py: generate JSON object for keyed-union discriminator
  libxl IDL: generate code to parse libxl__json_object to libxl_FOO
    struct
  libxl/gentest.py: test JSON parser
  xl: introduce "xl-json" format

 docs/man/xl.cfg.pod.5                |    2 +-
 tools/libxl/gentest.py               |   27 +++-
 tools/libxl/gentypes.py              |  119 ++++++++++++++-
 tools/libxl/idl.py                   |   23 ++-
 tools/libxl/libxl.h                  |    1 +
 tools/libxl/libxl_cpuid.c            |   92 +++++++++---
 tools/libxl/libxl_internal.h         |   39 +++--
 tools/libxl/libxl_json.c             |  263 ++++++++++++++++++++++++++++++++--
 tools/libxl/libxl_json.h             |   27 +++-
 tools/libxl/libxl_nocpuid.c          |    7 +
 tools/libxl/libxl_types.idl          |   31 ++--
 tools/libxl/libxl_types_internal.idl |    4 +-
 tools/libxl/xl_cmdimpl.c             |  103 ++++++++++---
 13 files changed, 649 insertions(+), 89 deletions(-)

-- 
1.7.10.4

[PATCH RFC V2 01/10] libxl IDL: rename json_fn to json_gen_fn

[Wei Liu]

This json_fn is in fact used to generate string representation of a json
data structure. We will introduce another json function to parse json
data structure in later changeset, so rename json_fn to json_gen_fn to
clarify.

Signed-off-by: Wei Liu <wei.liu2@citrix.com>
Acked-by: Ian Campbell <ian.campbell@citrix.com>
---
 tools/libxl/gentest.py               |    4 ++--
 tools/libxl/gentypes.py              |   12 ++++++------
 tools/libxl/idl.py                   |   10 +++++-----
 tools/libxl/libxl_types.idl          |    4 ++--
 tools/libxl/libxl_types_internal.idl |    2 +-
 5 files changed, 16 insertions(+), 16 deletions(-)

diff --git a/tools/libxl/gentest.py b/tools/libxl/gentest.py
index 722b7f4..eb9a21b 100644
--- a/tools/libxl/gentest.py
+++ b/tools/libxl/gentest.py
@@ -52,7 +52,7 @@ def gen_rand_init(ty, v, indent = "    ", parent = None):
             s += "    break;\n"
         s += "}\n"
     elif isinstance(ty, idl.Struct) \
-     and (parent is None or ty.json_fn is None):
+     and (parent is None or ty.json_gen_fn is None):
         for f in [f for f in ty.fields if not f.const]:
             (nparent,fexpr) = ty.member(v, f, parent is None)
             s += gen_rand_init(f.type, fexpr, "", nparent)
@@ -243,7 +243,7 @@ int main(int argc, char **argv)
     f.write("    printf(\"Testing TYPE_to_json()\\n\");\n")
     f.write("    printf(\"----------------------\\n\");\n")
     f.write("    printf(\"\\n\");\n")
-    for ty in [t for t in types if t.json_fn is not None]:
+    for ty in [t for t in types if t.json_gen_fn is not None]:
         arg = ty.typename + "_val"
         f.write("    %s_rand_init(%s);\n" % (ty.typename, \
             ty.pass_arg(arg, isref=False, passby=idl.PASS_BY_REFERENCE)))
diff --git a/tools/libxl/gentypes.py b/tools/libxl/gentypes.py
index 917e2c2..61a2b3d 100644
--- a/tools/libxl/gentypes.py
+++ b/tools/libxl/gentypes.py
@@ -229,7 +229,7 @@ def libxl_C_type_gen_json(ty, v, indent = "    ", parent = None):
                 s += "        goto out;\n"
             s += "    break;\n"
         s += "}\n"
-    elif isinstance(ty, idl.Struct) and (parent is None or ty.json_fn is None):
+    elif isinstance(ty, idl.Struct) and (parent is None or ty.json_gen_fn is None):
         s += "s = yajl_gen_map_open(hand);\n"
         s += "if (s != yajl_gen_status_ok)\n"
         s += "    goto out;\n"
@@ -243,8 +243,8 @@ def libxl_C_type_gen_json(ty, v, indent = "    ", parent = None):
         s += "if (s != yajl_gen_status_ok)\n"
         s += "    goto out;\n"
     else:
-        if ty.json_fn is not None:
-            s += "s = %s(hand, %s);\n" % (ty.json_fn, ty.pass_arg(v, parent is None))
+        if ty.json_gen_fn is not None:
+            s += "s = %s(hand, %s);\n" % (ty.json_gen_fn, ty.pass_arg(v, parent is None))
             s += "if (s != yajl_gen_status_ok)\n"
             s += "    goto out;\n"
 
@@ -341,7 +341,7 @@ if __name__ == '__main__':
                 f.write("%svoid %s(%s, %s);\n" % (ty.hidden(), ty.init_fn + "_" + ku.keyvar.name,
                                                ty.make_arg("p"),
                                                ku.keyvar.type.make_arg(ku.keyvar.name)))
-        if ty.json_fn is not None:
+        if ty.json_gen_fn is not None:
             f.write("%schar *%s_to_json(libxl_ctx *ctx, %s);\n" % (ty.hidden(), ty.typename, ty.make_arg("p")))
         if isinstance(ty, idl.Enumeration):
             f.write("%sconst char *%s_to_string(%s);\n" % (ty.hidden(), ty.typename, ty.make_arg("p")))
@@ -369,7 +369,7 @@ if __name__ == '__main__':
 
 """ % (header_json_define, header_json_define, " ".join(sys.argv)))
 
-    for ty in [ty for ty in types if ty.json_fn is not None]:
+    for ty in [ty for ty in types if ty.json_gen_fn is not None]:
         f.write("%syajl_gen_status %s_gen_json(yajl_gen hand, %s);\n" % (ty.hidden(), ty.typename, ty.make_arg("p", passby=idl.PASS_BY_REFERENCE)))
 
     f.write("\n")
@@ -426,7 +426,7 @@ if __name__ == '__main__':
         f.write("}\n")
         f.write("\n")
 
-    for ty in [t for t in types if t.json_fn is not None]:
+    for ty in [t for t in types if t.json_gen_fn is not None]:
         f.write("yajl_gen_status %s_gen_json(yajl_gen hand, %s)\n" % (ty.typename, ty.make_arg("p", passby=idl.PASS_BY_REFERENCE)))
         f.write("{\n")
         f.write(libxl_C_type_gen_json(ty, "p"))
diff --git a/tools/libxl/idl.py b/tools/libxl/idl.py
index e4dc79b..92133a3 100644
--- a/tools/libxl/idl.py
+++ b/tools/libxl/idl.py
@@ -65,9 +65,9 @@ class Type(object):
         self.autogenerate_init_fn = kwargs.setdefault('autogenerate_init_fn', False)
 
         if self.typename is not None and not self.private:
-            self.json_fn = kwargs.setdefault('json_fn', self.typename + "_gen_json")
+            self.json_gen_fn = kwargs.setdefault('json_gen_fn', self.typename + "_gen_json")
         else:
-            self.json_fn = kwargs.setdefault('json_fn', None)
+            self.json_gen_fn = kwargs.setdefault('json_gen_fn', None)
 
         self.autogenerate_json = kwargs.setdefault('autogenerate_json', True)
 
@@ -118,7 +118,7 @@ class Number(Builtin):
         kwargs.setdefault('namespace', None)
         kwargs.setdefault('dispose_fn', None)
         kwargs.setdefault('signed', False)
-        kwargs.setdefault('json_fn', "yajl_gen_integer")
+        kwargs.setdefault('json_gen_fn', "yajl_gen_integer")
         self.signed = kwargs['signed']
         Builtin.__init__(self, ctype, **kwargs)
 
@@ -256,7 +256,7 @@ class KeyedUnion(Aggregate):
 
 void = Builtin("void *", namespace = None)
 bool = Builtin("bool", namespace = None,
-               json_fn = "yajl_gen_bool",
+               json_gen_fn = "yajl_gen_bool",
                autogenerate_json = False)
 
 size_t = Number("size_t", namespace = None)
@@ -269,7 +269,7 @@ uint32 = UInt(32)
 uint64 = UInt(64)
 
 string = Builtin("char *", namespace = None, dispose_fn = "free",
-                 json_fn = "libxl__string_gen_json",
+                 json_gen_fn = "libxl__string_gen_json",
                  autogenerate_json = False)
 
 class Array(Type):
diff --git a/tools/libxl/libxl_types.idl b/tools/libxl/libxl_types.idl
index 0f7bbf8..755c918 100644
--- a/tools/libxl/libxl_types.idl
+++ b/tools/libxl/libxl_types.idl
@@ -7,8 +7,8 @@ namespace("libxl_")
 
 libxl_defbool = Builtin("defbool", passby=PASS_BY_REFERENCE)
 
-libxl_domid = Builtin("domid", json_fn = "yajl_gen_integer", autogenerate_json = False)
-libxl_devid = Builtin("devid", json_fn = "yajl_gen_integer", autogenerate_json = False, signed = True, init_val="-1")
+libxl_domid = Builtin("domid", json_gen_fn = "yajl_gen_integer", autogenerate_json = False)
+libxl_devid = Builtin("devid", json_gen_fn = "yajl_gen_integer", autogenerate_json = False, signed = True, init_val="-1")
 libxl_uuid = Builtin("uuid", passby=PASS_BY_REFERENCE)
 libxl_mac = Builtin("mac", passby=PASS_BY_REFERENCE)
 libxl_bitmap = Builtin("bitmap", dispose_fn="libxl_bitmap_dispose", passby=PASS_BY_REFERENCE)
diff --git a/tools/libxl/libxl_types_internal.idl b/tools/libxl/libxl_types_internal.idl
index cb9444f..a964851 100644
--- a/tools/libxl/libxl_types_internal.idl
+++ b/tools/libxl/libxl_types_internal.idl
@@ -1,7 +1,7 @@
 namespace("libxl__")
 hidden(True)
 
-libxl_domid = Builtin("domid", namespace="libxl_", json_fn = "yajl_gen_integer")
+libxl_domid = Builtin("domid", namespace="libxl_", json_gen_fn = "yajl_gen_integer")
 
 libxl__qmp_message_type = Enumeration("qmp_message_type", [
     (1, "QMP"),
-- 
1.7.10.4

[PATCH RFC V2 02/10] libxl_json: introduce libx__object_from_json

[Wei Liu]

Given a JSON string, we need to convert it to libxl_FOO struct.

The approach is:
JSON string -> libxl__json_object -> libxl_FOO struct

With this approach we can make use of libxl's infrastructure to do the
first half (JSON string -> libxl__json_object).

Second half is done by auto-generated code by libxl's IDL
infrastructure. IDL patch(es) will come later.

Signed-off-by: Wei Liu <wei.liu2@citrix.com>
---
 tools/libxl/libxl_internal.h |    9 +++++++++
 tools/libxl/libxl_json.c     |   30 ++++++++++++++++++++++++++++++
 2 files changed, 39 insertions(+)

diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
index c2b73c4..9205c49 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -1496,6 +1496,15 @@ typedef yajl_gen_status (*libxl__gen_json_callback)(yajl_gen hand, void *);
 _hidden char *libxl__object_to_json(libxl_ctx *ctx, const char *type,
                                     libxl__gen_json_callback gen, void *p);
 
+typedef struct libxl__json_object libxl__json_object;
+typedef int (*libxl__json_parse_callback)(libxl_ctx *ctx,
+                                          libxl__json_object *o,
+                                          void *p);
+_hidden int libxl__object_from_json(libxl_ctx *ctx, const char *type,
+                                    libxl__json_parse_callback parse,
+                                    void *p,
+                                    const char *s);
+
   /* holds the CPUID response for a single CPUID leaf
    * input contains the value of the EAX and ECX register,
    * and each policy string contains a filter to apply to
diff --git a/tools/libxl/libxl_json.c b/tools/libxl/libxl_json.c
index 3ea56a4..361607f 100644
--- a/tools/libxl/libxl_json.c
+++ b/tools/libxl/libxl_json.c
@@ -794,6 +794,36 @@ out:
     return ret;
 }
 
+int libxl__object_from_json(libxl_ctx *ctx, const char *type,
+                            libxl__json_parse_callback parse,
+                            void *p, const char *s)
+{
+    GC_INIT(ctx);
+    libxl__json_object *o;
+    int rc;
+
+    o = libxl__json_parse(gc, s);
+    if (!o) {
+        LOG(ERROR,
+            "unable to generate libxl__json_object from JSON representation of %s.",
+            type);
+        rc = ERROR_FAIL;
+        goto out;
+    }
+
+    rc = parse(ctx, o, p);
+    if (rc) {
+        LOG(ERROR, "unable to convert libxl__json_object to %s.", type);
+        rc = ERROR_FAIL;
+        goto out;
+    }
+
+    rc = 0;
+out:
+    GC_FREE;
+    return rc;
+}
+
 /*
  * Local variables:
  * mode: C
-- 
1.7.10.4

Re: [PATCH RFC V2 02/10] libxl_json: introduce libx__object_from_json

[Ian Campbell]

On Thu, 2014-04-17 at 12:13 +0100, Wei Liu wrote:
> Given a JSON string, we need to convert it to libxl_FOO struct.
> 
> The approach is:
> JSON string -> libxl__json_object -> libxl_FOO struct
> 
> With this approach we can make use of libxl's infrastructure to do the
> first half (JSON string -> libxl__json_object).
> 
> Second half is done by auto-generated code by libxl's IDL
> infrastructure. IDL patch(es) will come later.
> 
> Signed-off-by: Wei Liu <wei.liu2@citrix.com>
> ---
>  tools/libxl/libxl_internal.h |    9 +++++++++
>  tools/libxl/libxl_json.c     |   30 ++++++++++++++++++++++++++++++
>  2 files changed, 39 insertions(+)
> 
> diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
> index c2b73c4..9205c49 100644
> --- a/tools/libxl/libxl_internal.h
> +++ b/tools/libxl/libxl_internal.h
> @@ -1496,6 +1496,15 @@ typedef yajl_gen_status (*libxl__gen_json_callback)(yajl_gen hand, void *);
>  _hidden char *libxl__object_to_json(libxl_ctx *ctx, const char *type,
>                                      libxl__gen_json_callback gen, void *p);
>  
> +typedef struct libxl__json_object libxl__json_object;

This typedef already exists I think via:
        typedef struct libxl__json_object { 
        	...
        } libxl__json_object;

Perhaps you just need to move these prototypes further down the file?

> +typedef int (*libxl__json_parse_callback)(libxl_ctx *ctx,
> +                                          libxl__json_object *o,
> +                                          void *p);
> +_hidden int libxl__object_from_json(libxl_ctx *ctx, const char *type,
> +                                    libxl__json_parse_callback parse,
> +                                    void *p,
> +                                    const char *s);
> +
>    /* holds the CPUID response for a single CPUID leaf
>     * input contains the value of the EAX and ECX register,
>     * and each policy string contains a filter to apply to
> diff --git a/tools/libxl/libxl_json.c b/tools/libxl/libxl_json.c
> index 3ea56a4..361607f 100644
> --- a/tools/libxl/libxl_json.c
> +++ b/tools/libxl/libxl_json.c
> @@ -794,6 +794,36 @@ out:
>      return ret;
>  }
>  
> +int libxl__object_from_json(libxl_ctx *ctx, const char *type,
> +                            libxl__json_parse_callback parse,
> +                            void *p, const char *s)

This looks ok to me.

[PATCH RFC V2 03/10] libxl_internal: make JSON_* types bit-field

[Wei Liu]

Libxl can generate number as type JSON_INTEGER, JSON_DOUBLE or
JSON_NUMBER, string as type JSON_STRING or JSON_NULL (if string is
null).

So make JSON_* type a bit-field and use it in libxl__json_map_get. This is
useful when parsing a libxl__json_object to libxl_FOO struct. We can
enforce type checking on libxl__json_object in an easy way.

Signed-off-by: Wei Liu <wei.liu2@citrix.com>
---
 tools/libxl/libxl_internal.h |   18 +++++++++---------
 tools/libxl/libxl_json.c     |    2 +-
 2 files changed, 10 insertions(+), 10 deletions(-)

diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
index 9205c49..a5b2669 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -1622,16 +1622,16 @@ _hidden yajl_gen_status libxl__yajl_gen_asciiz(yajl_gen hand, const char *str);
 _hidden yajl_gen_status libxl__yajl_gen_enum(yajl_gen hand, const char *str);
 
 typedef enum {
-    JSON_NULL,
-    JSON_BOOL,
-    JSON_INTEGER,
-    JSON_DOUBLE,
+    JSON_NULL    = (1 << 0),
+    JSON_BOOL    = (1 << 1),
+    JSON_INTEGER = (1 << 2),
+    JSON_DOUBLE  = (1 << 3),
     /* number is store in string, it's too big to be a long long or a double */
-    JSON_NUMBER,
-    JSON_STRING,
-    JSON_MAP,
-    JSON_ARRAY,
-    JSON_ANY
+    JSON_NUMBER  = (1 << 4),
+    JSON_STRING  = (1 << 5),
+    JSON_MAP     = (1 << 6),
+    JSON_ARRAY   = (1 << 7),
+    JSON_ANY     = 255 /* this is a mask of all values above, adjust as needed */
 } libxl__json_node_type;
 
 typedef struct libxl__json_object {
diff --git a/tools/libxl/libxl_json.c b/tools/libxl/libxl_json.c
index 361607f..6f1116f 100644
--- a/tools/libxl/libxl_json.c
+++ b/tools/libxl/libxl_json.c
@@ -363,7 +363,7 @@ const libxl__json_object *libxl__json_map_get(const char *key,
                 return NULL;
             if (strcmp(key, node->map_key) == 0) {
                 if (expected_type == JSON_ANY
-                    || (node->obj && node->obj->type == expected_type)) {
+                    || (node->obj && (node->obj->type & expected_type))) {
                     return node->obj;
                 } else {
                     return NULL;
-- 
1.7.10.4

Re: [PATCH RFC V2 03/10] libxl_internal: make JSON_* types bit-field

[Ian Campbell]

On Thu, 2014-04-17 at 12:13 +0100, Wei Liu wrote:
> Libxl can generate number as type JSON_INTEGER, JSON_DOUBLE or
> JSON_NUMBER, string as type JSON_STRING or JSON_NULL (if string is
> null).
> 
> So make JSON_* type a bit-field and use it in libxl__json_map_get. This is
> useful when parsing a libxl__json_object to libxl_FOO struct. We can
> enforce type checking on libxl__json_object in an easy way.
> 
> Signed-off-by: Wei Liu <wei.liu2@citrix.com>

Acked-by: Ian Campbell <ian.campbell@citrix.com>

Although, minor nit, "types a bit-field" in the subject would be better.

[PATCH RFC V2 04/10] libxl_internal: introduce libxl__json_object_is_{null, number, double}

[Wei Liu]

... which return true if json object is valid and of type
JSON_{NULL,NUMBER,DOUBLE}.

Signed-off-by: Wei Liu <wei.liu2@citrix.com>
Acked-by: Ian Campbell <ian.campbell@citrix.com>
---
 tools/libxl/libxl_internal.h |   12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
index a5b2669..f2cedd5 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -1656,6 +1656,10 @@ typedef struct {
 
 typedef struct libxl__yajl_ctx libxl__yajl_ctx;
 
+static inline bool libxl__json_object_is_null(const libxl__json_object *o)
+{
+    return o != NULL && o->type == JSON_NULL;
+}
 static inline bool libxl__json_object_is_bool(const libxl__json_object *o)
 {
     return o != NULL && o->type == JSON_BOOL;
@@ -1668,6 +1672,14 @@ static inline bool libxl__json_object_is_integer(const libxl__json_object *o)
 {
     return o != NULL && o->type == JSON_INTEGER;
 }
+static inline bool libxl__json_object_is_double(const libxl__json_object *o)
+{
+    return o != NULL && o->type == JSON_DOUBLE;
+}
+static inline bool libxl__json_object_is_number(const libxl__json_object *o)
+{
+    return o != NULL && o->type == JSON_NUMBER;
+}
 static inline bool libxl__json_object_is_map(const libxl__json_object *o)
 {
     return o != NULL && o->type == JSON_MAP;
-- 
1.7.10.4

[PATCH RFC V2 05/10] libxl_json: introduce parser functions for builtin types

[Wei Liu]

This changeset introduces following functions:
 * libxl_defbool_parse_json
 * libxl__bool_parse_json
 * libxl_uuid_parse_json
 * libxl_mac_parse_json
 * libxl_bitmap_parse_json
 * libxl_cpuid_policy_list_parse_json
 * libxl_string_list_parse_json
 * libxl_key_value_list_parse_json
 * libxl_hwcap_parse_json
 * libxl__integer_parse_json
 * libxl__string_parse_json

They will be used in later patch to convert the libxl__json_object
tree of a builtin type to libxl_FOO struct.

Also remove delcaration of libxl_domid_gen_json as libxl_domid uses
yajl_gen_integer to generate JSON object.

Signed-off-by: Wei Liu <wei.liu2@citrix.com>
---
 tools/libxl/libxl_cpuid.c   |   92 +++++++++++++++++----
 tools/libxl/libxl_json.c    |  191 +++++++++++++++++++++++++++++++++++++++++++
 tools/libxl/libxl_json.h    |   27 +++++-
 tools/libxl/libxl_nocpuid.c |    7 ++
 4 files changed, 300 insertions(+), 17 deletions(-)

diff --git a/tools/libxl/libxl_cpuid.c b/tools/libxl/libxl_cpuid.c
index dd21b78..2c725a5 100644
--- a/tools/libxl/libxl_cpuid.c
+++ b/tools/libxl/libxl_cpuid.c
@@ -336,29 +336,29 @@ void libxl_cpuid_set(libxl_ctx *ctx, uint32_t domid,
                      (const char**)(cpuid[i].policy), cpuid_res);
 }
 
+static const char *input_names[2] = { "leaf", "subleaf" };
+static const char *policy_names[4] = { "eax", "ebx", "ecx", "edx" };
+/*
+ * Aiming for:
+ * [
+ *     { 'leaf':    'val-eax',
+ *       'subleaf': 'val-ecx',
+ *       'eax':     'filter',
+ *       'ebx':     'filter',
+ *       'ecx':     'filter',
+ *       'edx':     'filter' },
+ *     { 'leaf':    'val-eax', ..., 'eax': 'filter', ... },
+ *     ... etc ...
+ * ]
+ */
+
 yajl_gen_status libxl_cpuid_policy_list_gen_json(yajl_gen hand,
                                 libxl_cpuid_policy_list *pcpuid)
 {
     libxl_cpuid_policy_list cpuid = *pcpuid;
     yajl_gen_status s;
-    const char *input_names[2] = { "leaf", "subleaf" };
-    const char *policy_names[4] = { "eax", "ebx", "ecx", "edx" };
     int i, j;
 
-    /*
-     * Aiming for:
-     * [
-     *     { 'leaf':    'val-eax',
-     *       'subleaf': 'val-ecx',
-     *       'eax':     'filter',
-     *       'ebx':     'filter',
-     *       'ecx':     'filter',
-     *       'edx':     'filter' },
-     *     { 'leaf':    'val-eax', ..., 'eax': 'filter', ... },
-     *     ... etc ...
-     * ]
-     */
-
     s = yajl_gen_array_open(hand);
     if (s != yajl_gen_status_ok) goto out;
 
@@ -396,6 +396,66 @@ out:
     return s;
 }
 
+int libxl_cpuid_policy_list_parse_json(libxl_ctx *ctx,
+                                       const libxl__json_object *o,
+                                       libxl_cpuid_policy_list *p)
+{
+    int i, size;
+    libxl_cpuid_policy_list l;
+
+    if (!libxl__json_object_is_array(o))
+        return -1;
+
+    if (!o->u.array->count)
+        return 0;
+
+    size = o->u.array->count;
+    /* need one extra slot as setinel */
+    l = *p = calloc(size + 1, sizeof(libxl_cpuid_policy));
+
+    if (!l)
+        return -1;
+
+    memset(l, 0, sizeof(libxl_cpuid_policy) * (size + 1));
+    l[size].input[0] = XEN_CPUID_INPUT_UNUSED;
+    l[size].input[1] = XEN_CPUID_INPUT_UNUSED;
+
+    for (i = 0; i < size; i++) {
+        const libxl__json_object *t;
+        int j;
+
+        if (flexarray_get(o->u.array, i, (void**)&t) != 0)
+            return -1;          /* dispose_fn will clean up memory
+                                 * allocation */
+
+        assert(libxl__json_object_is_map(t));
+
+        for (j = 0; j < 2; j++) {
+            const libxl__json_object *r;
+
+            r = libxl__json_map_get(input_names[j], t, JSON_INTEGER);
+            if (!r)
+                l[i].input[j] = XEN_CPUID_INPUT_UNUSED;
+            else
+                l[i].input[j] = r->u.i;
+        }
+
+        for (j = 0; j < 4; j++) {
+            const libxl__json_object *r;
+
+            r = libxl__json_map_get(policy_names[j], t, JSON_STRING);
+            if (!r)
+                l[i].policy[j] = NULL;
+            else
+                l[i].policy[j] = strdup(r->u.string);
+        }
+    }
+
+    assert(i == size);
+
+    return 0;
+}
+
 /*
  * Local variables:
  * mode: C
diff --git a/tools/libxl/libxl_json.c b/tools/libxl/libxl_json.c
index 6f1116f..e2d5dbe 100644
--- a/tools/libxl/libxl_json.c
+++ b/tools/libxl/libxl_json.c
@@ -100,6 +100,35 @@ yajl_gen_status libxl_defbool_gen_json(yajl_gen hand,
     return libxl__yajl_gen_asciiz(hand, libxl_defbool_to_string(*db));
 }
 
+int libxl_defbool_parse_json(libxl_ctx *ctx, const libxl__json_object *o,
+                             libxl_defbool *p)
+{
+    if (!libxl__json_object_is_string(o))
+        return -1;
+
+    if (!strncmp(o->u.string, "<default>", strlen("<default>")))
+        p->val = 0;
+    else if (!strncmp(o->u.string, "True", strlen("True")))
+        p->val = 1;
+    else if (!strncmp(o->u.string, "False", strlen("False")))
+        p->val = -1;
+    else
+        return -1;
+
+    return 0;
+}
+
+int libxl__bool_parse_json(libxl_ctx *ctx, const libxl__json_object *o,
+                           bool *p)
+{
+    if (!libxl__json_object_is_bool(o))
+        return -1;
+
+    *p = o->u.b;
+
+    return 0;
+}
+
 yajl_gen_status libxl_uuid_gen_json(yajl_gen hand,
                                     libxl_uuid *uuid)
 {
@@ -108,6 +137,15 @@ yajl_gen_status libxl_uuid_gen_json(yajl_gen hand,
     return yajl_gen_string(hand, (const unsigned char *)buf, LIBXL_UUID_FMTLEN);
 }
 
+int libxl_uuid_parse_json(libxl_ctx *ctx, const libxl__json_object *o,
+                          libxl_uuid *p)
+{
+    if (!libxl__json_object_is_string(o))
+        return -1;
+
+    return libxl_uuid_from_string(p, o->u.string);
+}
+
 yajl_gen_status libxl_bitmap_gen_json(yajl_gen hand,
                                       libxl_bitmap *bitmap)
 {
@@ -128,6 +166,34 @@ out:
     return s;
 }
 
+int libxl_bitmap_parse_json(libxl_ctx *ctx, const libxl__json_object *o,
+                            libxl_bitmap *p)
+{
+    int i;
+    int size;
+    const libxl__json_object *t;
+
+    if (!libxl__json_object_is_array(o))
+        return -1;
+
+    if (!o->u.array->count)
+        return 0;
+
+    t = libxl__json_array_get(o, o->u.array->count - 1);
+    size = t->u.i + 1;
+
+    libxl_bitmap_alloc(ctx, p, size);
+
+    for (i = 0; (t = libxl__json_array_get(o, i)); i++) {
+        if (!libxl__json_object_is_integer(t))
+            return -1;          /* dispose_fn will clean up memory
+                                 * allocation */
+        libxl_bitmap_set(p, t->u.i);
+    }
+
+    return 0;
+}
+
 yajl_gen_status libxl_key_value_list_gen_json(yajl_gen hand,
                                               libxl_key_value_list *pkvl)
 {
@@ -155,6 +221,43 @@ out:
     return s;
 }
 
+int libxl_key_value_list_parse_json(libxl_ctx *ctx, const libxl__json_object *o,
+                                    libxl_key_value_list *p)
+{
+    libxl__json_map_node *node = NULL;
+    flexarray_t *maps = NULL;
+    int i, size;
+    libxl_key_value_list kvl;
+
+    if (!libxl__json_object_is_map(o))
+        return -1;
+
+    maps = o->u.map;
+    size = maps->count * 2;
+    kvl = *p = calloc(size, sizeof(char *));
+    if (!kvl)
+        return -1;
+
+    memset(kvl, 0, sizeof(char *) * size);
+
+    for (i = 0; i < maps->count; i++) {
+        int idx = i * 2;
+        if (flexarray_get(maps, i, (void**)&node) != 0)
+            return -1;
+        assert(libxl__json_object_is_string(node->obj) ||
+               libxl__json_object_is_null(node->obj));
+        kvl[idx]   = strdup(node->map_key);
+        if (libxl__json_object_is_string(node->obj))
+            kvl[idx+1] = strdup(node->obj->u.string);
+        else
+            kvl[idx+1] = NULL;
+    }
+
+    assert(i == maps->count);
+
+    return 0;
+}
+
 yajl_gen_status libxl_string_list_gen_json(yajl_gen hand, libxl_string_list *pl)
 {
     libxl_string_list l = *pl;
@@ -176,6 +279,36 @@ out:
     return s;
 }
 
+int libxl_string_list_parse_json(libxl_ctx *ctx, const libxl__json_object *o,
+                                 libxl_string_list *p)
+{
+    const libxl__json_object *t;
+    libxl_string_list l;
+    flexarray_t *array = NULL;
+    int i, size;
+
+    if (!libxl__json_object_is_array(o))
+        return -1;
+
+    array = o->u.array;
+    size = array->count;
+    /* need one extra slot as sentinel */
+    l = *p = calloc(size + 1, sizeof(char *));
+
+    if (!l)
+        return -1;
+
+    memset(l, 0, sizeof(char *) * size);
+    l[size] = NULL;
+
+    for (i = 0; (t = libxl__json_array_get(o, i)); i++) {
+        assert(libxl__json_object_is_string(t));
+        l[i] = strdup(t->u.string);
+    }
+
+    return 0;
+}
+
 yajl_gen_status libxl_mac_gen_json(yajl_gen hand, libxl_mac *mac)
 {
     char buf[LIBXL_MAC_FMTLEN+1];
@@ -183,6 +316,15 @@ yajl_gen_status libxl_mac_gen_json(yajl_gen hand, libxl_mac *mac)
     return yajl_gen_string(hand, (const unsigned char *)buf, LIBXL_MAC_FMTLEN);
 }
 
+int libxl_mac_parse_json(libxl_ctx *ctx, const libxl__json_object *o,
+                         libxl_mac *p)
+{
+    if (!libxl__json_object_is_string(o))
+        return -1;
+
+    return libxl__parse_mac(o->u.string, *p);
+}
+
 yajl_gen_status libxl_hwcap_gen_json(yajl_gen hand,
                                      libxl_hwcap *p)
 {
@@ -201,6 +343,27 @@ out:
     return s;
 }
 
+int libxl_hwcap_parse_json(libxl_ctx *ctx, const libxl__json_object *o,
+                           libxl_hwcap *p)
+{
+    int i;
+
+    if (!libxl__json_object_is_array(o))
+        return -1;
+
+    for (i = 0; i<4; i++) {
+        const libxl__json_object *t;
+
+        t = libxl__json_array_get(o, i);
+        if (!t || !libxl__json_object_is_integer(t))
+            return -1;
+
+        (*p)[i] = t->u.i;
+    }
+
+    return 0;
+}
+
 yajl_gen_status libxl__string_gen_json(yajl_gen hand,
                                        const char *p)
 {
@@ -210,6 +373,23 @@ yajl_gen_status libxl__string_gen_json(yajl_gen hand,
         return yajl_gen_null(hand);
 }
 
+int libxl__string_parse_json(libxl_ctx *ctx, const libxl__json_object *o,
+                             char **p)
+{
+    if (!libxl__json_object_is_string(o) && !libxl__json_object_is_null(o))
+        return -1;
+
+    if (libxl__json_object_is_null(o)) {
+        *p = NULL;
+    } else {
+        *p = strdup(o->u.string);
+        if (!*p)
+            return -1;
+    }
+
+    return 0;
+}
+
 /*
  * libxl__json_object helper functions
  */
@@ -824,6 +1004,17 @@ out:
     return rc;
 }
 
+int libxl__integer_parse_json(libxl_ctx *ctx, const libxl__json_object *o,
+                              void *p)
+{
+    if (!libxl__json_object_is_integer(o))
+        return -1;
+
+    *((long long *)p) = o->u.i;
+
+    return 0;
+}
+
 /*
  * Local variables:
  * mode: C
diff --git a/tools/libxl/libxl_json.h b/tools/libxl/libxl_json.h
index a4dd8fc..d7bdeb5 100644
--- a/tools/libxl/libxl_json.h
+++ b/tools/libxl/libxl_json.h
@@ -22,17 +22,42 @@
 #  include <yajl/yajl_version.h>
 #endif
 
+typedef struct libxl__json_object libxl__json_object;
+
 yajl_gen_status libxl_defbool_gen_json(yajl_gen hand, libxl_defbool *p);
-yajl_gen_status libxl_domid_gen_json(yajl_gen hand, libxl_domid *p);
+int libxl_defbool_parse_json(libxl_ctx *ctx, const libxl__json_object *o,
+                             libxl_defbool *p);
+int libxl__bool_parse_json(libxl_ctx *ctx, const libxl__json_object *o,
+                           bool *p);
 yajl_gen_status libxl_uuid_gen_json(yajl_gen hand, libxl_uuid *p);
+int libxl_uuid_parse_json(libxl_ctx *ctx, const libxl__json_object *o,
+                          libxl_uuid *p);
 yajl_gen_status libxl_mac_gen_json(yajl_gen hand, libxl_mac *p);
+int libxl_mac_parse_json(libxl_ctx *ctx, const libxl__json_object *o,
+                         libxl_mac *p);
 yajl_gen_status libxl_bitmap_gen_json(yajl_gen hand, libxl_bitmap *p);
+int libxl_bitmap_parse_json(libxl_ctx *ctx, const libxl__json_object *o,
+                            libxl_bitmap *p);
 yajl_gen_status libxl_cpuid_policy_list_gen_json(yajl_gen hand,
                                                  libxl_cpuid_policy_list *p);
+int libxl_cpuid_policy_list_parse_json(libxl_ctx *ctx,
+                                       const libxl__json_object *o,
+                                       libxl_cpuid_policy_list *p);
 yajl_gen_status libxl_string_list_gen_json(yajl_gen hand, libxl_string_list *p);
+int libxl_string_list_parse_json(libxl_ctx *ctx, const libxl__json_object *o,
+                                 libxl_string_list *p);
 yajl_gen_status libxl_key_value_list_gen_json(yajl_gen hand,
                                               libxl_key_value_list *p);
+int libxl_key_value_list_parse_json(libxl_ctx *ctx,
+                                    const libxl__json_object *o,
+                                    libxl_key_value_list *p);
 yajl_gen_status libxl_hwcap_gen_json(yajl_gen hand, libxl_hwcap *p);
+int libxl_hwcap_parse_json(libxl_ctx *ctx, const libxl__json_object *o,
+                           libxl_hwcap *p);
+int libxl__integer_parse_json(libxl_ctx *ctx, const libxl__json_object *o,
+                              void *p);
+int libxl__string_parse_json(libxl_ctx *ctx, const libxl__json_object *o,
+                             char **p);
 
 #include <_libxl_types_json.h>
 
diff --git a/tools/libxl/libxl_nocpuid.c b/tools/libxl/libxl_nocpuid.c
index 5f7cb6a..7364dd5 100644
--- a/tools/libxl/libxl_nocpuid.c
+++ b/tools/libxl/libxl_nocpuid.c
@@ -44,6 +44,13 @@ yajl_gen_status libxl_cpuid_policy_list_gen_json(yajl_gen hand,
     return 0;
 }
 
+int libxl_cpuid_policy_list_parse_json(libxl_ctx *ctx,
+                                       const libxl__json_object *o,
+                                       libxl_cpuid_policy_list *p)
+{
+    return 0;
+}
+
 /*
  * Local variables:
  * mode: C
-- 
1.7.10.4

Re: [PATCH RFC V2 05/10] libxl_json: introduce parser functions for builtin types

[Ian Campbell]

Please could you CC Anthony too, I think he wrote all this
libxl__json_object stuff.

On Thu, 2014-04-17 at 12:13 +0100, Wei Liu wrote:
> @@ -396,6 +396,66 @@ out:
>      return s;
>  }
>  
> +int libxl_cpuid_policy_list_parse_json(libxl_ctx *ctx,
> +                                       const libxl__json_object *o,
> +                                       libxl_cpuid_policy_list *p)
> +{
> +    int i, size;
> +    libxl_cpuid_policy_list l;
> +
> +    if (!libxl__json_object_is_array(o))
> +        return -1;
> +
> +    if (!o->u.array->count)
> +        return 0;
> +
> +    size = o->u.array->count;
> +    /* need one extra slot as setinel */

"sentinel"

> +    l = *p = calloc(size + 1, sizeof(libxl_cpuid_policy));

This function should GC_INIT and use the provided gc for allocations,
shouldn't it?.

> +
> +    if (!l)
> +        return -1;
> +
> +    memset(l, 0, sizeof(libxl_cpuid_policy) * (size + 1));
> +    l[size].input[0] = XEN_CPUID_INPUT_UNUSED;
> +    l[size].input[1] = XEN_CPUID_INPUT_UNUSED;
> +
> +    for (i = 0; i < size; i++) {
> +        const libxl__json_object *t;
> +        int j;
> +
> +        if (flexarray_get(o->u.array, i, (void**)&t) != 0)
> +            return -1;          /* dispose_fn will clean up memory
> +                                 * allocation */

You haven't done a gc allocation, so it won't. Looking closer I think
this falls into case #1 of the comment in libxl.h, which means the
allocation should have come from the heap, but that means you need to
manually free it.

(I see this comment about dispose_fn a lot, so I won't repeat it)

> +        for (j = 0; j < 4; j++) {

This, and the code it is based on, really ought to use ARRAY_SIZE I
think.

> +            const libxl__json_object *r;
> +
> +            r = libxl__json_map_get(policy_names[j], t, JSON_STRING);
> +            if (!r)
> +                l[i].policy[j] = NULL;
> +            else
> +                l[i].policy[j] = strdup(r->u.string);
> +        }
> +    }
> +
> +    assert(i == size);

Given a lack of break's in the for loop this seems overly defensive to
me.

> +
> +    return 0;
> +}
> +
>  /*
>   * Local variables:
>   * mode: C
> diff --git a/tools/libxl/libxl_json.c b/tools/libxl/libxl_json.c
> index 6f1116f..e2d5dbe 100644
> --- a/tools/libxl/libxl_json.c
> +++ b/tools/libxl/libxl_json.c
> @@ -100,6 +100,35 @@ yajl_gen_status libxl_defbool_gen_json(yajl_gen hand,
>      return libxl__yajl_gen_asciiz(hand, libxl_defbool_to_string(*db));
>  }
>  
> +int libxl_defbool_parse_json(libxl_ctx *ctx, const libxl__json_object *o,
> +                             libxl_defbool *p)
> +{
> +    if (!libxl__json_object_is_string(o))
> +        return -1;
> +
> +    if (!strncmp(o->u.string, "<default>", strlen("<default>")))
> +        p->val = 0;
> +    else if (!strncmp(o->u.string, "True", strlen("True")))
> +        p->val = 1;
> +    else if (!strncmp(o->u.string, "False", strlen("False")))

Should have some internal #defines for these strings and use in the
generator too. Also you should use LIBXL__DEFBOOL_DEFAULT and friends.

> @@ -128,6 +166,34 @@ out:
>      return s;
>  }
>  
> +int libxl_bitmap_parse_json(libxl_ctx *ctx, const libxl__json_object *o,
> +                            libxl_bitmap *p)
> +{
> +    int i;
> +    int size;
> +    const libxl__json_object *t;
> +
> +    if (!libxl__json_object_is_array(o))
> +        return -1;
> +
> +    if (!o->u.array->count)
> +        return 0;
> +
> +    t = libxl__json_array_get(o, o->u.array->count - 1);
> +    size = t->u.i + 1;

What is this doing? It's retrieving the last item in the array, assuming
it is an integer and then inferring the length of the array from it?

Oh this is because a bitmask is represented as an array which contains a
list of the bit indexes which are true.

Is the json array guaranteed to be sorted? I guess it is if libxl
generated it, but can we rely on that?

I wonder if it is too late to change the json representation of a
bitmask that we are using. Thoughts?

> +
> +    libxl_bitmap_alloc(ctx, p, size);
> +
> +    for (i = 0; (t = libxl__json_array_get(o, i)); i++) {
> +        if (!libxl__json_object_is_integer(t))
> +            return -1;          /* dispose_fn will clean up memory
> +                                 * allocation */
> +        libxl_bitmap_set(p, t->u.i);
> +    }
> +
> +    return 0;
> +}
> +
>  yajl_gen_status libxl_key_value_list_gen_json(yajl_gen hand,
>                                                libxl_key_value_list *pkvl)
>  {
> @@ -155,6 +221,43 @@ out:
>      return s;
>  }
>  
> +int libxl_key_value_list_parse_json(libxl_ctx *ctx, const libxl__json_object *o,
> +                                    libxl_key_value_list *p)
> +{
> +    libxl__json_map_node *node = NULL;
> +    flexarray_t *maps = NULL;
> +    int i, size;
> +    libxl_key_value_list kvl;
> +
> +    if (!libxl__json_object_is_map(o))
> +        return -1;
> +
> +    maps = o->u.map;
> +    size = maps->count * 2;
> +    kvl = *p = calloc(size, sizeof(char *));
> +    if (!kvl)
> +        return -1;
> +
> +    memset(kvl, 0, sizeof(char *) * size);
> +
> +    for (i = 0; i < maps->count; i++) {
> +        int idx = i * 2;
> +        if (flexarray_get(maps, i, (void**)&node) != 0)
> +            return -1;
> +        assert(libxl__json_object_is_string(node->obj) ||
> +               libxl__json_object_is_null(node->obj));

I think we may need to be mindful of potentially malicious json code
which a toolstack is feeding to these parser functions, don't we?

IOW asserting and crashing the entire daemon would be unfortunate.

> +        kvl[idx]   = strdup(node->map_key);
> +        if (libxl__json_object_is_string(node->obj))
> +            kvl[idx+1] = strdup(node->obj->u.string);
> +        else
> +            kvl[idx+1] = NULL;
> +    }
> +
> +    assert(i == maps->count);

Too defensive again.

> +
> +    return 0;
> +}
> +
>  yajl_gen_status libxl_string_list_gen_json(yajl_gen hand, libxl_string_list *pl)
>  {
>      libxl_string_list l = *pl;
> @@ -201,6 +343,27 @@ out:
>      return s;
>  }
>  
> +int libxl_hwcap_parse_json(libxl_ctx *ctx, const libxl__json_object *o,
> +                           libxl_hwcap *p)
> +{
> +    int i;
> +
> +    if (!libxl__json_object_is_array(o))
> +        return -1;

I jut noticed the error handling here, but I'm sure it was the same for
all the preceding. libxl functions should return an ERROR_* code and not
-1 unless you have a good reason not to.

Ian.

Re: [PATCH RFC V2 05/10] libxl_json: introduce parser functions for builtin types

[Wei Liu]

On Tue, Apr 22, 2014 at 04:09:58PM +0100, Ian Campbell wrote:
> Please could you CC Anthony too, I think he wrote all this
> libxl__json_object stuff.
> 
> On Thu, 2014-04-17 at 12:13 +0100, Wei Liu wrote:
> > @@ -396,6 +396,66 @@ out:
> >      return s;
> >  }
> >  
> > +int libxl_cpuid_policy_list_parse_json(libxl_ctx *ctx,
> > +                                       const libxl__json_object *o,
> > +                                       libxl_cpuid_policy_list *p)
> > +{
> > +    int i, size;
> > +    libxl_cpuid_policy_list l;
> > +
> > +    if (!libxl__json_object_is_array(o))
> > +        return -1;
> > +
> > +    if (!o->u.array->count)
> > +        return 0;
> > +
> > +    size = o->u.array->count;
> > +    /* need one extra slot as setinel */
> 
> "sentinel"
> 
> > +    l = *p = calloc(size + 1, sizeof(libxl_cpuid_policy));
> 
> This function should GC_INIT and use the provided gc for allocations,
> shouldn't it?.
> 

I see. You're right here.

> > +
> > +    if (!l)
> > +        return -1;
> > +
> > +    memset(l, 0, sizeof(libxl_cpuid_policy) * (size + 1));
> > +    l[size].input[0] = XEN_CPUID_INPUT_UNUSED;
> > +    l[size].input[1] = XEN_CPUID_INPUT_UNUSED;
> > +
> > +    for (i = 0; i < size; i++) {
> > +        const libxl__json_object *t;
> > +        int j;
> > +
> > +        if (flexarray_get(o->u.array, i, (void**)&t) != 0)
> > +            return -1;          /* dispose_fn will clean up memory
> > +                                 * allocation */
> 
> You haven't done a gc allocation, so it won't. Looking closer I think
> this falls into case #1 of the comment in libxl.h, which means the
> allocation should have come from the heap, but that means you need to
> manually free it.
> 

The aformentioned "calloc" falls into #1 but not this one.

My comment is a bit confusing. The comment here actually refers to all
the malloc'ed memory in the loop, which will be freed by
libxl_cpuid_dispose.

In any case I will check other occurences of this.

> (I see this comment about dispose_fn a lot, so I won't repeat it)
> 
> > +        for (j = 0; j < 4; j++) {
> 
> This, and the code it is based on, really ought to use ARRAY_SIZE I
> think.
> 

Ack.

> > +            const libxl__json_object *r;
> > +
> > +            r = libxl__json_map_get(policy_names[j], t, JSON_STRING);
> > +            if (!r)
> > +                l[i].policy[j] = NULL;
> > +            else
> > +                l[i].policy[j] = strdup(r->u.string);
> > +        }
> > +    }
> > +
> > +    assert(i == size);
> 
> Given a lack of break's in the for loop this seems overly defensive to
> me.
> 

I will remove this.

> > +
> > +    return 0;
> > +}
> > +
> >  /*
> >   * Local variables:
> >   * mode: C
> > diff --git a/tools/libxl/libxl_json.c b/tools/libxl/libxl_json.c
> > index 6f1116f..e2d5dbe 100644
> > --- a/tools/libxl/libxl_json.c
> > +++ b/tools/libxl/libxl_json.c
> > @@ -100,6 +100,35 @@ yajl_gen_status libxl_defbool_gen_json(yajl_gen hand,
> >      return libxl__yajl_gen_asciiz(hand, libxl_defbool_to_string(*db));
> >  }
> >  
> > +int libxl_defbool_parse_json(libxl_ctx *ctx, const libxl__json_object *o,
> > +                             libxl_defbool *p)
> > +{
> > +    if (!libxl__json_object_is_string(o))
> > +        return -1;
> > +
> > +    if (!strncmp(o->u.string, "<default>", strlen("<default>")))
> > +        p->val = 0;
> > +    else if (!strncmp(o->u.string, "True", strlen("True")))
> > +        p->val = 1;
> > +    else if (!strncmp(o->u.string, "False", strlen("False")))
> 
> Should have some internal #defines for these strings and use in the
> generator too. Also you should use LIBXL__DEFBOOL_DEFAULT and friends.
> 

Ack.

> > @@ -128,6 +166,34 @@ out:
> >      return s;
> >  }
> >  
> > +int libxl_bitmap_parse_json(libxl_ctx *ctx, const libxl__json_object *o,
> > +                            libxl_bitmap *p)
> > +{
> > +    int i;
> > +    int size;
> > +    const libxl__json_object *t;
> > +
> > +    if (!libxl__json_object_is_array(o))
> > +        return -1;
> > +
> > +    if (!o->u.array->count)
> > +        return 0;
> > +
> > +    t = libxl__json_array_get(o, o->u.array->count - 1);
> > +    size = t->u.i + 1;
> 
> What is this doing? It's retrieving the last item in the array, assuming
> it is an integer and then inferring the length of the array from it?
> 

Yes.

> Oh this is because a bitmask is represented as an array which contains a
> list of the bit indexes which are true.
> 
> Is the json array guaranteed to be sorted? I guess it is if libxl
> generated it, but can we rely on that?
> 

Yes. I basically reverse-engineer the process to generate the array and
find out that it's sorted.

> I wonder if it is too late to change the json representation of a
> bitmask that we are using. Thoughts?
> 

Probably yes. It's a public function in libxl API so I wouldn't be
surprised if some high level tool depends on it.

> > +
> > +    libxl_bitmap_alloc(ctx, p, size);
> > +
> > +    for (i = 0; (t = libxl__json_array_get(o, i)); i++) {
> > +        if (!libxl__json_object_is_integer(t))
> > +            return -1;          /* dispose_fn will clean up memory
> > +                                 * allocation */
> > +        libxl_bitmap_set(p, t->u.i);
> > +    }
> > +
> > +    return 0;
> > +}
> > +
> >  yajl_gen_status libxl_key_value_list_gen_json(yajl_gen hand,
> >                                                libxl_key_value_list *pkvl)
> >  {
> > @@ -155,6 +221,43 @@ out:
> >      return s;
> >  }
> >  
> > +int libxl_key_value_list_parse_json(libxl_ctx *ctx, const libxl__json_object *o,
> > +                                    libxl_key_value_list *p)
> > +{
> > +    libxl__json_map_node *node = NULL;
> > +    flexarray_t *maps = NULL;
> > +    int i, size;
> > +    libxl_key_value_list kvl;
> > +
> > +    if (!libxl__json_object_is_map(o))
> > +        return -1;
> > +
> > +    maps = o->u.map;
> > +    size = maps->count * 2;
> > +    kvl = *p = calloc(size, sizeof(char *));
> > +    if (!kvl)
> > +        return -1;
> > +
> > +    memset(kvl, 0, sizeof(char *) * size);
> > +
> > +    for (i = 0; i < maps->count; i++) {
> > +        int idx = i * 2;
> > +        if (flexarray_get(maps, i, (void**)&node) != 0)
> > +            return -1;
> > +        assert(libxl__json_object_is_string(node->obj) ||
> > +               libxl__json_object_is_null(node->obj));
> 
> I think we may need to be mindful of potentially malicious json code
> which a toolstack is feeding to these parser functions, don't we?
> 
> IOW asserting and crashing the entire daemon would be unfortunate.
> 

I think "return -1" will be more appropriate in this case.

> > +        kvl[idx]   = strdup(node->map_key);
> > +        if (libxl__json_object_is_string(node->obj))
> > +            kvl[idx+1] = strdup(node->obj->u.string);
> > +        else
> > +            kvl[idx+1] = NULL;
> > +    }
> > +
> > +    assert(i == maps->count);
> 
> Too defensive again.
> 

Ack.

> > +
> > +    return 0;
> > +}
> > +
> >  yajl_gen_status libxl_string_list_gen_json(yajl_gen hand, libxl_string_list *pl)
> >  {
> >      libxl_string_list l = *pl;
> > @@ -201,6 +343,27 @@ out:
> >      return s;
> >  }
> >  
> > +int libxl_hwcap_parse_json(libxl_ctx *ctx, const libxl__json_object *o,
> > +                           libxl_hwcap *p)
> > +{
> > +    int i;
> > +
> > +    if (!libxl__json_object_is_array(o))
> > +        return -1;
> 
> I jut noticed the error handling here, but I'm sure it was the same for
> all the preceding. libxl functions should return an ERROR_* code and not
> -1 unless you have a good reason not to.
> 

OK. Will change to that.

Wei.

> Ian.

Re: [PATCH RFC V2 05/10] libxl_json: introduce parser functions for builtin types

[Ian Campbell]

On Wed, 2014-04-23 at 11:01 +0100, Wei Liu wrote:

> The aformentioned "calloc" falls into #1 but not this one.
> 
> My comment is a bit confusing. The comment here actually refers to all
> the malloc'ed memory in the loop, which will be freed by
> libxl_cpuid_dispose.

You mean when the caller eventually gets rid of the result, or the error
path here does it?

I don't think that needs an explicit comment, it's just the expected
behaviour of #1 type allocations.

> > I wonder if it is too late to change the json representation of a
> > bitmask that we are using. Thoughts?
> > 
> 
> Probably yes. It's a public function in libxl API so I wouldn't be
> surprised if some high level tool depends on it.

True, oh well.
> > 
> > I think we may need to be mindful of potentially malicious json code
> > which a toolstack is feeding to these parser functions, don't we?
> > 
> > IOW asserting and crashing the entire daemon would be unfortunate.
> > 
> 
> I think "return -1" will be more appropriate in this case.

return ERROR_* please.

> > > +
> > > +    return 0;
> > > +}
> > > +
> > >  yajl_gen_status libxl_string_list_gen_json(yajl_gen hand, libxl_string_list *pl)
> > >  {
> > >      libxl_string_list l = *pl;
> > > @@ -201,6 +343,27 @@ out:
> > >      return s;
> > >  }
> > >  
> > > +int libxl_hwcap_parse_json(libxl_ctx *ctx, const libxl__json_object *o,
> > > +                           libxl_hwcap *p)
> > > +{
> > > +    int i;
> > > +
> > > +    if (!libxl__json_object_is_array(o))
> > > +        return -1;
> > 
> > I jut noticed the error handling here, but I'm sure it was the same for
> > all the preceding. libxl functions should return an ERROR_* code and not
> > -1 unless you have a good reason not to.
> > 
> 
> OK. Will change to that.

Thanks.

Re: [PATCH RFC V2 05/10] libxl_json: introduce parser functions for builtin types

[Wei Liu]

On Wed, Apr 23, 2014 at 11:12:54AM +0100, Ian Campbell wrote:
> On Wed, 2014-04-23 at 11:01 +0100, Wei Liu wrote:
> 
> > The aformentioned "calloc" falls into #1 but not this one.
> > 
> > My comment is a bit confusing. The comment here actually refers to all
> > the malloc'ed memory in the loop, which will be freed by
> > libxl_cpuid_dispose.
> 
> You mean when the caller eventually gets rid of the result, or the error
> path here does it?
> 

The former. Caller will regardlessly call dispose_fn of a type, wouldn't
it?

> I don't think that needs an explicit comment, it's just the expected
> behaviour of #1 type allocations.
> 

OK.

Wei.

Re: [PATCH RFC V2 05/10] libxl_json: introduce parser functions for builtin types

[Ian Campbell]

On Wed, 2014-04-23 at 11:19 +0100, Wei Liu wrote:
> On Wed, Apr 23, 2014 at 11:12:54AM +0100, Ian Campbell wrote:
> > On Wed, 2014-04-23 at 11:01 +0100, Wei Liu wrote:
> > 
> > > The aformentioned "calloc" falls into #1 but not this one.
> > > 
> > > My comment is a bit confusing. The comment here actually refers to all
> > > the malloc'ed memory in the loop, which will be freed by
> > > libxl_cpuid_dispose.
> > 
> > You mean when the caller eventually gets rid of the result, or the error
> > path here does it?
> > 
> 
> The former. Caller will regardlessly call dispose_fn of a type, wouldn't
> it?

Hrm, I'm not sure what we expect the caller to do with the output of a
failed operation. I think I would expect that the failing operation
would clean up any partial result. Ian?

It might be worth having a poke around and seeing what other libxl
functions do under these circumstances.

Ian.

Re: [PATCH RFC V2 05/10] libxl_json: introduce parser functions for builtin types

[Wei Liu]

On Wed, Apr 23, 2014 at 11:31:19AM +0100, Ian Campbell wrote:
> On Wed, 2014-04-23 at 11:19 +0100, Wei Liu wrote:
> > On Wed, Apr 23, 2014 at 11:12:54AM +0100, Ian Campbell wrote:
> > > On Wed, 2014-04-23 at 11:01 +0100, Wei Liu wrote:
> > > 
> > > > The aformentioned "calloc" falls into #1 but not this one.
> > > > 
> > > > My comment is a bit confusing. The comment here actually refers to all
> > > > the malloc'ed memory in the loop, which will be freed by
> > > > libxl_cpuid_dispose.
> > > 
> > > You mean when the caller eventually gets rid of the result, or the error
> > > path here does it?
> > > 
> > 
> > The former. Caller will regardlessly call dispose_fn of a type, wouldn't
> > it?
> 
> Hrm, I'm not sure what we expect the caller to do with the output of a
> failed operation. I think I would expect that the failing operation
> would clean up any partial result. Ian?
> 
> It might be worth having a poke around and seeing what other libxl
> functions do under these circumstances.
> 

The way I see it is that in current libxl code, the caller , regardless
of failure or success, will always call TYPE_dispose to clean up.

Wei.

> Ian.

Re: [PATCH RFC V2 05/10] libxl_json: introduce parser functions for builtin types

[Wei Liu]

On Wed, Apr 23, 2014 at 11:42:42AM +0100, Wei Liu wrote:
> On Wed, Apr 23, 2014 at 11:31:19AM +0100, Ian Campbell wrote:
> > On Wed, 2014-04-23 at 11:19 +0100, Wei Liu wrote:
> > > On Wed, Apr 23, 2014 at 11:12:54AM +0100, Ian Campbell wrote:
> > > > On Wed, 2014-04-23 at 11:01 +0100, Wei Liu wrote:
> > > > 
> > > > > The aformentioned "calloc" falls into #1 but not this one.
> > > > > 
> > > > > My comment is a bit confusing. The comment here actually refers to all
> > > > > the malloc'ed memory in the loop, which will be freed by
> > > > > libxl_cpuid_dispose.
> > > > 
> > > > You mean when the caller eventually gets rid of the result, or the error
> > > > path here does it?
> > > > 
> > > 
> > > The former. Caller will regardlessly call dispose_fn of a type, wouldn't
> > > it?
> > 
> > Hrm, I'm not sure what we expect the caller to do with the output of a
> > failed operation. I think I would expect that the failing operation
> > would clean up any partial result. Ian?
> > 
> > It might be worth having a poke around and seeing what other libxl
> > functions do under these circumstances.
> > 
> 
> The way I see it is that in current libxl code, the caller , regardless

Sorry I meant "xl code".

Re: [PATCH RFC V2 05/10] libxl_json: introduce parser functions for builtin types

[Ian Campbell]

On Wed, 2014-04-23 at 12:14 +0100, Wei Liu wrote:
> On Wed, Apr 23, 2014 at 11:42:42AM +0100, Wei Liu wrote:
> > On Wed, Apr 23, 2014 at 11:31:19AM +0100, Ian Campbell wrote:
> > > On Wed, 2014-04-23 at 11:19 +0100, Wei Liu wrote:
> > > > On Wed, Apr 23, 2014 at 11:12:54AM +0100, Ian Campbell wrote:
> > > > > On Wed, 2014-04-23 at 11:01 +0100, Wei Liu wrote:
> > > > > 
> > > > > > The aformentioned "calloc" falls into #1 but not this one.
> > > > > > 
> > > > > > My comment is a bit confusing. The comment here actually refers to all
> > > > > > the malloc'ed memory in the loop, which will be freed by
> > > > > > libxl_cpuid_dispose.
> > > > > 
> > > > > You mean when the caller eventually gets rid of the result, or the error
> > > > > path here does it?
> > > > > 
> > > > 
> > > > The former. Caller will regardlessly call dispose_fn of a type, wouldn't
> > > > it?
> > > 
> > > Hrm, I'm not sure what we expect the caller to do with the output of a
> > > failed operation. I think I would expect that the failing operation
> > > would clean up any partial result. Ian?
> > > 
> > > It might be worth having a poke around and seeing what other libxl
> > > functions do under these circumstances.
> > > 
> > 
> > The way I see it is that in current libxl code, the caller , regardless
> 
> Sorry I meant "xl code".

It does look that way doesn't it. Lets go for consistency for sure!

Any chance you could add a comment to a relevant section of libxl.h to
describe this expectation (in the general case, not just for this
interface)?

Ian.

Re: [PATCH RFC V2 05/10] libxl_json: introduce parser functions for builtin types

[Wei Liu]

On Wed, Apr 23, 2014 at 12:41:46PM +0100, Ian Campbell wrote:
[...]
> > > 
> > > The way I see it is that in current libxl code, the caller , regardless
> > 
> > Sorry I meant "xl code".
> 
> It does look that way doesn't it. Lets go for consistency for sure!
> 
> Any chance you could add a comment to a relevant section of libxl.h to
> describe this expectation (in the general case, not just for this
> interface)?
> 

No problem.

Wei.

Re: [PATCH RFC V2 05/10] libxl_json: introduce parser functions for builtin types

[Ian Jackson]

Ian Campbell writes ("Re: [PATCH RFC V2 05/10] libxl_json: introduce parser functions for builtin types"):
> Hrm, I'm not sure what we expect the caller to do with the output of a
> failed operation. I think I would expect that the failing operation
> would clean up any partial result. Ian?

I agree.  I think that unless there's a compelling reason to do
otherwise, a failing operation should not try to pass ownership of
anything to the caller.  It should, however, explictly set the result
variable to 0 so that the caller can safely free it.

Ian.

Re: [PATCH RFC V2 05/10] libxl_json: introduce parser functions for builtin types

[Wei Liu]

On Wed, Apr 23, 2014 at 11:01:34AM +0100, Wei Liu wrote:
> On Tue, Apr 22, 2014 at 04:09:58PM +0100, Ian Campbell wrote:
> > Please could you CC Anthony too, I think he wrote all this
> > libxl__json_object stuff.
> > 
> > On Thu, 2014-04-17 at 12:13 +0100, Wei Liu wrote:
> > > @@ -396,6 +396,66 @@ out:
> > >      return s;
> > >  }
> > >  
> > > +int libxl_cpuid_policy_list_parse_json(libxl_ctx *ctx,
> > > +                                       const libxl__json_object *o,
> > > +                                       libxl_cpuid_policy_list *p)
> > > +{
> > > +    int i, size;
> > > +    libxl_cpuid_policy_list l;
> > > +
> > > +    if (!libxl__json_object_is_array(o))
> > > +        return -1;
> > > +
> > > +    if (!o->u.array->count)
> > > +        return 0;
> > > +
> > > +    size = o->u.array->count;
> > > +    /* need one extra slot as setinel */
> > 
> > "sentinel"
> > 
> > > +    l = *p = calloc(size + 1, sizeof(libxl_cpuid_policy));
> > 
> > This function should GC_INIT and use the provided gc for allocations,
> > shouldn't it?.
> > 
> 
> I see. You're right here.

I was wrong about this.

This (and other allocations alike) should use calloc instead of GC
allocation. We don't want GC to free the memory until dispose is called.

If you look at libxl_string_list_dispose you will see the allocation of
the list itself is freed at the end of the function.

libxl_cpuid_list forget to do that. It should be fixed.

Wei.

> 

Re: [PATCH RFC V2 05/10] libxl_json: introduce parser functions for builtin types

[Ian Jackson]

Wei Liu writes ("Re: [PATCH RFC V2 05/10] libxl_json: introduce parser functions for builtin types"):
> I was wrong about this.
> 
> This (and other allocations alike) should use calloc instead of GC
> allocation. We don't want GC to free the memory until dispose is called.

Instead of calloc, you should use libxl__zalloc(NOGC,...).  That has
the right error handling (and you don't need to check for errors
yourself).

Ian.

[PATCH RFC V2 06/10] libxl_json: allow basic JSON type objects generation

[Wei Liu]

The original logic is that basic JSON types (number, string and null)
must be an element of JSON map or array. This assumption doesn't hold
true anymore when we need to return basic JSON types.

Returning basic JSON types is required for parsing number, string and
null objects back into libxl__json_object.

Signed-off-by: Wei Liu <wei.liu2@citrix.com>
---
 tools/libxl/libxl_json.c |   40 ++++++++++++++++++++++++++++++++--------
 1 file changed, 32 insertions(+), 8 deletions(-)

diff --git a/tools/libxl/libxl_json.c b/tools/libxl/libxl_json.c
index e2d5dbe..6b2946d 100644
--- a/tools/libxl/libxl_json.c
+++ b/tools/libxl/libxl_json.c
@@ -629,8 +629,14 @@ static int json_callback_null(void *opaque)
 
     obj = libxl__json_object_alloc(ctx->gc, JSON_NULL);
 
-    if (libxl__json_object_append_to(ctx->gc, obj, ctx->current) == -1) {
-        return 0;
+    if (ctx->current) {
+        if (libxl__json_object_append_to(ctx->gc, obj, ctx->current) == -1) {
+            return 0;
+        }
+    }
+
+    if (ctx->head == NULL) {
+        ctx->head = obj;
     }
 
     return 1;
@@ -646,8 +652,14 @@ static int json_callback_boolean(void *opaque, int boolean)
     obj = libxl__json_object_alloc(ctx->gc, JSON_BOOL);
     obj->u.b = boolean;
 
-    if (libxl__json_object_append_to(ctx->gc, obj, ctx->current) == -1) {
-        return 0;
+    if (ctx->current) {
+        if (libxl__json_object_append_to(ctx->gc, obj, ctx->current) == -1) {
+            return 0;
+        }
+    }
+
+    if (ctx->head == NULL) {
+        ctx->head = obj;
     }
 
     return 1;
@@ -703,8 +715,14 @@ error:
     obj->u.string = t;
 
 out:
-    if (libxl__json_object_append_to(ctx->gc, obj, ctx->current) == -1) {
-        return 0;
+    if (ctx->current) {
+        if (libxl__json_object_append_to(ctx->gc, obj, ctx->current) == -1) {
+            return 0;
+        }
+    }
+
+    if (ctx->head == NULL) {
+        ctx->head = obj;
     }
 
     return 1;
@@ -727,8 +745,14 @@ static int json_callback_string(void *opaque, const unsigned char *str,
     obj = libxl__json_object_alloc(ctx->gc, JSON_STRING);
     obj->u.string = t;
 
-    if (libxl__json_object_append_to(ctx->gc, obj, ctx->current) == -1) {
-        return 0;
+    if (ctx->current) {
+        if (libxl__json_object_append_to(ctx->gc, obj, ctx->current) == -1) {
+            return 0;
+        }
+    }
+
+    if (ctx->head == NULL) {
+        ctx->head = obj;
     }
 
     return 1;
-- 
1.7.10.4

Re: [PATCH RFC V2 06/10] libxl_json: allow basic JSON type objects generation

[Ian Campbell]

On Thu, 2014-04-17 at 12:13 +0100, Wei Liu wrote:
> The original logic is that basic JSON types (number, string and null)
> must be an element of JSON map or array. This assumption doesn't hold
> true anymore when we need to return basic JSON types.
> 
> Returning basic JSON types is required for parsing number, string and
> null objects back into libxl__json_object.
> 
> Signed-off-by: Wei Liu <wei.liu2@citrix.com>
> ---
>  tools/libxl/libxl_json.c |   40 ++++++++++++++++++++++++++++++++--------
>  1 file changed, 32 insertions(+), 8 deletions(-)
> 
> diff --git a/tools/libxl/libxl_json.c b/tools/libxl/libxl_json.c
> index e2d5dbe..6b2946d 100644
> --- a/tools/libxl/libxl_json.c
> +++ b/tools/libxl/libxl_json.c
> @@ -629,8 +629,14 @@ static int json_callback_null(void *opaque)
>  
>      obj = libxl__json_object_alloc(ctx->gc, JSON_NULL);
>  
> -    if (libxl__json_object_append_to(ctx->gc, obj, ctx->current) == -1) {
> -        return 0;
> +    if (ctx->current) {
> +        if (libxl__json_object_append_to(ctx->gc, obj, ctx->current) == -1) {
> +            return 0;
> +        }
> +    }
> +
> +    if (ctx->head == NULL) {
> +        ctx->head = obj;

It seems that this pattern is now pretty much universal on all callers
of libxl__json_object_append_to. Perhaps much of this functionality
should be pushed down into the helper (which might now need to take ctx
and not ctx->current).


The existing callers seem to all set ctx->current too, just before the
ctx->head == NULL check. Why don't these versions need that too?

Ian

Re: [PATCH RFC V2 06/10] libxl_json: allow basic JSON type objects generation

[Wei Liu]

On Tue, Apr 22, 2014 at 04:22:20PM +0100, Ian Campbell wrote:
> On Thu, 2014-04-17 at 12:13 +0100, Wei Liu wrote:
> > The original logic is that basic JSON types (number, string and null)
> > must be an element of JSON map or array. This assumption doesn't hold
> > true anymore when we need to return basic JSON types.
> > 
> > Returning basic JSON types is required for parsing number, string and
> > null objects back into libxl__json_object.
> > 
> > Signed-off-by: Wei Liu <wei.liu2@citrix.com>
> > ---
> >  tools/libxl/libxl_json.c |   40 ++++++++++++++++++++++++++++++++--------
> >  1 file changed, 32 insertions(+), 8 deletions(-)
> > 
> > diff --git a/tools/libxl/libxl_json.c b/tools/libxl/libxl_json.c
> > index e2d5dbe..6b2946d 100644
> > --- a/tools/libxl/libxl_json.c
> > +++ b/tools/libxl/libxl_json.c
> > @@ -629,8 +629,14 @@ static int json_callback_null(void *opaque)
> >  
> >      obj = libxl__json_object_alloc(ctx->gc, JSON_NULL);
> >  
> > -    if (libxl__json_object_append_to(ctx->gc, obj, ctx->current) == -1) {
> > -        return 0;
> > +    if (ctx->current) {
> > +        if (libxl__json_object_append_to(ctx->gc, obj, ctx->current) == -1) {
> > +            return 0;
> > +        }
> > +    }
> > +
> > +    if (ctx->head == NULL) {
> > +        ctx->head = obj;
> 
> It seems that this pattern is now pretty much universal on all callers
> of libxl__json_object_append_to. Perhaps much of this functionality
> should be pushed down into the helper (which might now need to take ctx
> and not ctx->current).
> 

But not every new allocated object needs to be appended to some other
object, so pushing that functionality to libxl__json_object_append_to is
not correct.

> 
> The existing callers seem to all set ctx->current too, just before the
> ctx->head == NULL check. Why don't these versions need that too?
> 

Because "current" is used to reference to "the array or map that we're
currently in", so that we can call
libxl__json_object_append_to(current).

Not very useful to set "current" for basic types because no other
objects can be appended to them.

Wei.

> Ian

Re: [PATCH RFC V2 06/10] libxl_json: allow basic JSON type objects generation

[Ian Campbell]

On Wed, 2014-04-23 at 11:15 +0100, Wei Liu wrote:
> On Tue, Apr 22, 2014 at 04:22:20PM +0100, Ian Campbell wrote:
> > On Thu, 2014-04-17 at 12:13 +0100, Wei Liu wrote:
> > > The original logic is that basic JSON types (number, string and null)
> > > must be an element of JSON map or array. This assumption doesn't hold
> > > true anymore when we need to return basic JSON types.
> > > 
> > > Returning basic JSON types is required for parsing number, string and
> > > null objects back into libxl__json_object.
> > > 
> > > Signed-off-by: Wei Liu <wei.liu2@citrix.com>
> > > ---
> > >  tools/libxl/libxl_json.c |   40 ++++++++++++++++++++++++++++++++--------
> > >  1 file changed, 32 insertions(+), 8 deletions(-)
> > > 
> > > diff --git a/tools/libxl/libxl_json.c b/tools/libxl/libxl_json.c
> > > index e2d5dbe..6b2946d 100644
> > > --- a/tools/libxl/libxl_json.c
> > > +++ b/tools/libxl/libxl_json.c
> > > @@ -629,8 +629,14 @@ static int json_callback_null(void *opaque)
> > >  
> > >      obj = libxl__json_object_alloc(ctx->gc, JSON_NULL);
> > >  
> > > -    if (libxl__json_object_append_to(ctx->gc, obj, ctx->current) == -1) {
> > > -        return 0;
> > > +    if (ctx->current) {
> > > +        if (libxl__json_object_append_to(ctx->gc, obj, ctx->current) == -1) {
> > > +            return 0;
> > > +        }
> > > +    }
> > > +
> > > +    if (ctx->head == NULL) {
> > > +        ctx->head = obj;
> > 
> > It seems that this pattern is now pretty much universal on all callers
> > of libxl__json_object_append_to. Perhaps much of this functionality
> > should be pushed down into the helper (which might now need to take ctx
> > and not ctx->current).
> > 
> 
> But not every new allocated object needs to be appended to some other
> object, so pushing that functionality to libxl__json_object_append_to is
> not correct.

???

AFAICT every call of libxl__json_object_append_to after this patch now
has exactly the same boiler plate around it. The need append or not is a
property of the current context (in a map) not of the current object
being handled.

> > The existing callers seem to all set ctx->current too, just before the
> > ctx->head == NULL check. Why don't these versions need that too?
> > 
> 
> Because "current" is used to reference to "the array or map that we're
> currently in", so that we can call
> libxl__json_object_append_to(current).
> 
> Not very useful to set "current" for basic types because no other
> objects can be appended to them.

OK, I think this bit could be left in the relevant callers rather than
being made generic. Or it could be a parameter to object_append_to (e.g.
bool_t is_a_container, or pass the JSON_*).

Ian.

Re: [PATCH RFC V2 06/10] libxl_json: allow basic JSON type objects generation

[Wei Liu]

On Wed, Apr 23, 2014 at 11:30:03AM +0100, Ian Campbell wrote:
> On Wed, 2014-04-23 at 11:15 +0100, Wei Liu wrote:
> > On Tue, Apr 22, 2014 at 04:22:20PM +0100, Ian Campbell wrote:
> > > On Thu, 2014-04-17 at 12:13 +0100, Wei Liu wrote:
> > > > The original logic is that basic JSON types (number, string and null)
> > > > must be an element of JSON map or array. This assumption doesn't hold
> > > > true anymore when we need to return basic JSON types.
> > > > 
> > > > Returning basic JSON types is required for parsing number, string and
> > > > null objects back into libxl__json_object.
> > > > 
> > > > Signed-off-by: Wei Liu <wei.liu2@citrix.com>
> > > > ---
> > > >  tools/libxl/libxl_json.c |   40 ++++++++++++++++++++++++++++++++--------
> > > >  1 file changed, 32 insertions(+), 8 deletions(-)
> > > > 
> > > > diff --git a/tools/libxl/libxl_json.c b/tools/libxl/libxl_json.c
> > > > index e2d5dbe..6b2946d 100644
> > > > --- a/tools/libxl/libxl_json.c
> > > > +++ b/tools/libxl/libxl_json.c
> > > > @@ -629,8 +629,14 @@ static int json_callback_null(void *opaque)
> > > >  
> > > >      obj = libxl__json_object_alloc(ctx->gc, JSON_NULL);
> > > >  
> > > > -    if (libxl__json_object_append_to(ctx->gc, obj, ctx->current) == -1) {
> > > > -        return 0;
> > > > +    if (ctx->current) {
> > > > +        if (libxl__json_object_append_to(ctx->gc, obj, ctx->current) == -1) {
> > > > +            return 0;
> > > > +        }
> > > > +    }
> > > > +
> > > > +    if (ctx->head == NULL) {
> > > > +        ctx->head = obj;
> > > 
> > > It seems that this pattern is now pretty much universal on all callers
> > > of libxl__json_object_append_to. Perhaps much of this functionality
> > > should be pushed down into the helper (which might now need to take ctx
> > > and not ctx->current).
> > > 
> > 
> > But not every new allocated object needs to be appended to some other
> > object, so pushing that functionality to libxl__json_object_append_to is
> > not correct.
> 
> ???
> 
> AFAICT every call of libxl__json_object_append_to after this patch now
> has exactly the same boiler plate around it. The need append or not is a
> property of the current context (in a map) not of the current object
> being handled.
> 

But setting ctx->head doesn't really belong to append_to does it? The
semantic looks wrong to me.

Are you suggesting me change the semantic of libxl__json_object_append_to
to "append this object to current context" instead of "append this
object to the map or array pointed to by 'current'"?

Wei.

> > > The existing callers seem to all set ctx->current too, just before the
> > > ctx->head == NULL check. Why don't these versions need that too?
> > > 
> > 
> > Because "current" is used to reference to "the array or map that we're
> > currently in", so that we can call
> > libxl__json_object_append_to(current).
> > 
> > Not very useful to set "current" for basic types because no other
> > objects can be appended to them.
> 
> OK, I think this bit could be left in the relevant callers rather than
> being made generic. Or it could be a parameter to object_append_to (e.g.
> bool_t is_a_container, or pass the JSON_*).
> 
> Ian.

Re: [PATCH RFC V2 06/10] libxl_json: allow basic JSON type objects generation

[Ian Campbell]

On Wed, 2014-04-23 at 11:36 +0100, Wei Liu wrote:
> On Wed, Apr 23, 2014 at 11:30:03AM +0100, Ian Campbell wrote:
> > On Wed, 2014-04-23 at 11:15 +0100, Wei Liu wrote:
> > > On Tue, Apr 22, 2014 at 04:22:20PM +0100, Ian Campbell wrote:
> > > > On Thu, 2014-04-17 at 12:13 +0100, Wei Liu wrote:
> > > > > The original logic is that basic JSON types (number, string and null)
> > > > > must be an element of JSON map or array. This assumption doesn't hold
> > > > > true anymore when we need to return basic JSON types.
> > > > > 
> > > > > Returning basic JSON types is required for parsing number, string and
> > > > > null objects back into libxl__json_object.
> > > > > 
> > > > > Signed-off-by: Wei Liu <wei.liu2@citrix.com>
> > > > > ---
> > > > >  tools/libxl/libxl_json.c |   40 ++++++++++++++++++++++++++++++++--------
> > > > >  1 file changed, 32 insertions(+), 8 deletions(-)
> > > > > 
> > > > > diff --git a/tools/libxl/libxl_json.c b/tools/libxl/libxl_json.c
> > > > > index e2d5dbe..6b2946d 100644
> > > > > --- a/tools/libxl/libxl_json.c
> > > > > +++ b/tools/libxl/libxl_json.c
> > > > > @@ -629,8 +629,14 @@ static int json_callback_null(void *opaque)
> > > > >  
> > > > >      obj = libxl__json_object_alloc(ctx->gc, JSON_NULL);
> > > > >  
> > > > > -    if (libxl__json_object_append_to(ctx->gc, obj, ctx->current) == -1) {
> > > > > -        return 0;
> > > > > +    if (ctx->current) {
> > > > > +        if (libxl__json_object_append_to(ctx->gc, obj, ctx->current) == -1) {
> > > > > +            return 0;
> > > > > +        }
> > > > > +    }
> > > > > +
> > > > > +    if (ctx->head == NULL) {
> > > > > +        ctx->head = obj;
> > > > 
> > > > It seems that this pattern is now pretty much universal on all callers
> > > > of libxl__json_object_append_to. Perhaps much of this functionality
> > > > should be pushed down into the helper (which might now need to take ctx
> > > > and not ctx->current).
> > > > 
> > > 
> > > But not every new allocated object needs to be appended to some other
> > > object, so pushing that functionality to libxl__json_object_append_to is
> > > not correct.
> > 
> > ???
> > 
> > AFAICT every call of libxl__json_object_append_to after this patch now
> > has exactly the same boiler plate around it. The need append or not is a
> > property of the current context (in a map) not of the current object
> > being handled.
> > 
> 
> But setting ctx->head doesn't really belong to append_to does it? The
> semantic looks wrong to me.
> 
> Are you suggesting me change the semantic of libxl__json_object_append_to
> to "append this object to current context" instead of "append this
> object to the map or array pointed to by 'current'"?

I think that's what I'm suggesting, yes.

Ian.

[PATCH RFC V2 07/10] libxl/gentypes.py: generate JSON object for keyed-union discriminator

[Wei Liu]

Parser relies on the discriminator to go to correct branch.

Signed-off-by: Wei Liu <wei.liu2@citrix.com>
---
 tools/libxl/gentypes.py |    5 +++++
 1 file changed, 5 insertions(+)

diff --git a/tools/libxl/gentypes.py b/tools/libxl/gentypes.py
index 61a2b3d..8d7183a 100644
--- a/tools/libxl/gentypes.py
+++ b/tools/libxl/gentypes.py
@@ -229,6 +229,11 @@ def libxl_C_type_gen_json(ty, v, indent = "    ", parent = None):
                 s += "        goto out;\n"
             s += "    break;\n"
         s += "}\n"
+        s += "s = yajl_gen_string(hand, (const unsigned char *)\"%s\", sizeof(\"%s\")-1);\n" \
+             % (ty.keyvar.name, ty.keyvar.name)
+        s += "if (s != yajl_gen_status_ok)\n"
+        s += "    goto out;\n"
+        s += libxl_C_type_gen_json(ty.keyvar.type, (parent + ty.keyvar.name), indent, parent)
     elif isinstance(ty, idl.Struct) and (parent is None or ty.json_gen_fn is None):
         s += "s = yajl_gen_map_open(hand);\n"
         s += "if (s != yajl_gen_status_ok)\n"
-- 
1.7.10.4

Re: [PATCH RFC V2 07/10] libxl/gentypes.py: generate JSON object for keyed-union discriminator

[Ian Campbell]

On Thu, 2014-04-17 at 12:13 +0100, Wei Liu wrote:

When $subject save "generate JSON object" does it really mean "include
discriminator in the json output"? It's not really an object in this
context is it?

> Parser relies on the discriminator to go to correct branch.
> 
> Signed-off-by: Wei Liu <wei.liu2@citrix.com>
> ---
>  tools/libxl/gentypes.py |    5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/tools/libxl/gentypes.py b/tools/libxl/gentypes.py
> index 61a2b3d..8d7183a 100644
> --- a/tools/libxl/gentypes.py
> +++ b/tools/libxl/gentypes.py
> @@ -229,6 +229,11 @@ def libxl_C_type_gen_json(ty, v, indent = "    ", parent = None):
>                  s += "        goto out;\n"
>              s += "    break;\n"
>          s += "}\n"
> +        s += "s = yajl_gen_string(hand, (const unsigned char *)\"%s\", sizeof(\"%s\")-1);\n" \
> +             % (ty.keyvar.name, ty.keyvar.name)

> +        s += "if (s != yajl_gen_status_ok)\n"
> +        s += "    goto out;\n"
> +        s += libxl_C_type_gen_json(ty.keyvar.type, (parent + ty.keyvar.name), indent, parent)
>      elif isinstance(ty, idl.Struct) and (parent is None or ty.json_gen_fn is None):
>          s += "s = yajl_gen_map_open(hand);\n"
>          s += "if (s != yajl_gen_status_ok)\n"

Re: [PATCH RFC V2 07/10] libxl/gentypes.py: generate JSON object for keyed-union discriminator

[Wei Liu]

On Tue, Apr 22, 2014 at 04:27:44PM +0100, Ian Campbell wrote:
> On Thu, 2014-04-17 at 12:13 +0100, Wei Liu wrote:
> 
> When $subject save "generate JSON object" does it really mean "include
> discriminator in the json output"? It's not really an object in this
> context is it?
> 

The latter. I will use a better name.

Wei.

[PATCH RFC V2 08/10] libxl IDL: generate code to parse libxl__json_object to libxl_FOO struct

[Wei Liu]

libxl_FOO_parse_json functions are generated.

Note that these functions are used to parse libxl__json_object to
libxl__FOO struct. They don't consume JSON string.

Signed-off-by: Wei Liu <wei.liu2@citrix.com>
---
 tools/libxl/gentypes.py              |  102 ++++++++++++++++++++++++++++++++++
 tools/libxl/idl.py                   |   13 +++++
 tools/libxl/libxl_types.idl          |   31 ++++++-----
 tools/libxl/libxl_types_internal.idl |    4 +-
 4 files changed, 136 insertions(+), 14 deletions(-)

diff --git a/tools/libxl/gentypes.py b/tools/libxl/gentypes.py
index 8d7183a..d2671cb 100644
--- a/tools/libxl/gentypes.py
+++ b/tools/libxl/gentypes.py
@@ -270,6 +270,90 @@ def libxl_C_type_to_json(ty, v, indent = "    "):
         s = indent + s
     return s.replace("\n", "\n%s" % indent).rstrip(indent)
 
+def libxl_C_type_parse_json(ty, w, v, indent = "    ", parent = None):
+    s = ""
+    if parent is None:
+        s += "int rc = 0;\n"
+        s += "const libxl__json_object *x = o;\n"
+
+    if isinstance(ty, idl.Array):
+        if parent is None:
+            raise Exception("Array type must have a parent")
+        lenvar = parent + ty.lenvar.name
+        s += "{\n"
+        s += "    libxl__json_object *t;\n"
+        s += "    int i;\n"
+        s += "    assert(libxl__json_object_is_array(x));\n"
+        s += "    %s = x->u.array->count;\n" % lenvar
+        s += "    %s = calloc(%s, sizeof(*%s));\n" % (v, lenvar, v)
+        s += "    if (!%s) {\n" % v
+        s += "        rc = -1;\n"
+        s += "        goto out;\n"
+        s += "    }\n"
+        s += "    for (i=0; (t=libxl__json_array_get(x,i)); i++) {\n"
+        s += libxl_C_type_parse_json(ty.elem_type, "t", v+"[i]",
+                                     indent + "        ", parent)
+        s += "    }\n"
+        s += "    assert(i == %s);\n" % lenvar
+        s += "}\n"
+    elif isinstance(ty, idl.Enumeration):
+        s += "{\n"
+        s += "    const char *enum_str;\n"
+        s += "    assert(libxl__json_object_is_string(x));\n"
+        s += "    enum_str = libxl__json_object_get_string(x);\n"
+        s += "    rc = %s_from_string(enum_str, %s);\n" % (ty.typename, ty.pass_arg(v, parent is None, idl.PASS_BY_REFERENCE))
+        s += "    if (rc)\n"
+        s += "        goto out;\n"
+        s += "}\n"
+    elif isinstance(ty, idl.KeyedUnion):
+        if parent is None:
+            raise Exception("KeyedUnion type must have a parent")
+        s += "switch (%s) {\n" % (parent + ty.keyvar.name)
+        for f in ty.fields:
+            (nparent,fexpr) = ty.member(v, f, parent is None)
+            s += "case %s:\n" % f.enumname
+            if f.type is not None:
+                s += libxl_C_type_parse_json(f.type, w, fexpr, indent + "    ", nparent)
+            s += "    break;\n"
+        s += "}\n"
+    elif isinstance(ty, idl.Struct) and (parent is None or ty.json_parse_fn is None):
+        for f in [f for f in ty.fields if not f.const and not f.type.private]:
+            if isinstance(f.type, idl.KeyedUnion):
+                s += "x = libxl__json_map_get(\"%s\", %s, %s);\n" % \
+                     (f.type.keyvar.name, w, f.type.keyvar.type.json_parse_type)
+                s += "if (x) {\n"
+                (nparent,fexpr) = ty.member(v, f.type.keyvar, parent is None)
+                s += libxl_C_type_parse_json(f.type.keyvar.type, "x", fexpr, "    ", nparent)
+                s += "}\n"
+            s += "x = libxl__json_map_get(\"%s\", %s, %s);\n" % (f.name, w, f.type.json_parse_type)
+            s += "if (x) {\n"
+            (nparent,fexpr) = ty.member(v, f, parent is None)
+            s += libxl_C_type_parse_json(f.type, "x", fexpr, "    ", nparent)
+            s += "    x = x->parent;\n"
+            s += "}\n"
+    else:
+        if ty.json_parse_fn is not None:
+            s += "rc = %s(ctx, %s, &%s);\n" % (ty.json_parse_fn, w, v)
+            s += "if (rc)\n"
+            s += "    goto out;\n"
+
+    if parent is None:
+        s += "out:\n"
+        s += "return rc;\n"
+
+    if s != "":
+        s = indent +s
+    return s.replace("\n", "\n%s" % indent).rstrip(indent)
+
+def libxl_C_type_from_json(ty, v, w, indent = "    "):
+    s = ""
+    parse = "(libxl__json_parse_callback)&%s_parse_json" % ty.typename
+    s += "return libxl__object_from_json(ctx, \"%s\", %s, %s, %s);\n" % (ty.typename, parse, v, w)
+
+    if s != "":
+        s = indent + s
+    return s.replace("\n", "\n%s" % indent).rstrip(indent)
+
 def libxl_C_enum_to_string(ty, e, indent = "    "):
     s = ""
     s += "switch(%s) {\n" % e
@@ -348,6 +432,8 @@ if __name__ == '__main__':
                                                ku.keyvar.type.make_arg(ku.keyvar.name)))
         if ty.json_gen_fn is not None:
             f.write("%schar *%s_to_json(libxl_ctx *ctx, %s);\n" % (ty.hidden(), ty.typename, ty.make_arg("p")))
+        if ty.json_parse_fn is not None:
+            f.write("%sint %s_from_json(libxl_ctx *ctx, %s, const char *s);\n" % (ty.hidden(), ty.typename, ty.make_arg("p", passby=idl.PASS_BY_REFERENCE)))
         if isinstance(ty, idl.Enumeration):
             f.write("%sconst char *%s_to_string(%s);\n" % (ty.hidden(), ty.typename, ty.make_arg("p")))
             f.write("%sint %s_from_string(const char *s, %s);\n" % (ty.hidden(), ty.typename, ty.make_arg("e", passby=idl.PASS_BY_REFERENCE)))
@@ -377,6 +463,9 @@ if __name__ == '__main__':
     for ty in [ty for ty in types if ty.json_gen_fn is not None]:
         f.write("%syajl_gen_status %s_gen_json(yajl_gen hand, %s);\n" % (ty.hidden(), ty.typename, ty.make_arg("p", passby=idl.PASS_BY_REFERENCE)))
 
+    for ty in [ty for ty in types if ty.json_parse_fn is not None]:
+        f.write("%sint %s_parse_json(libxl_ctx *ctx, const libxl__json_object *o, %s);\n" % (ty.hidden(), ty.typename, ty.make_arg("p", passby=idl.PASS_BY_REFERENCE)))
+
     f.write("\n")
     f.write("""#endif /* %s */\n""" % header_json_define)
     f.close()
@@ -444,4 +533,17 @@ if __name__ == '__main__':
         f.write("}\n")
         f.write("\n")
 
+    for ty in [t for t in types if t.json_parse_fn is not None]:
+        f.write("int %s_parse_json(libxl_ctx *ctx, const libxl__json_object *%s, %s)\n" % (ty.typename,"o",ty.make_arg("p", passby=idl.PASS_BY_REFERENCE)))
+        f.write("{\n")
+        f.write(libxl_C_type_parse_json(ty, "o", "p"))
+        f.write("}\n")
+        f.write("\n")
+
+        f.write("int %s_from_json(libxl_ctx *ctx, %s, const char *s)\n" % (ty.typename, ty.make_arg("p", passby=idl.PASS_BY_REFERENCE)))
+        f.write("{\n")
+        f.write(libxl_C_type_from_json(ty, "p", "s"))
+        f.write("}\n")
+        f.write("\n")
+
     f.close()
diff --git a/tools/libxl/idl.py b/tools/libxl/idl.py
index 92133a3..ec1c429 100644
--- a/tools/libxl/idl.py
+++ b/tools/libxl/idl.py
@@ -66,8 +66,12 @@ class Type(object):
 
         if self.typename is not None and not self.private:
             self.json_gen_fn = kwargs.setdefault('json_gen_fn', self.typename + "_gen_json")
+            self.json_parse_type = kwargs.setdefault('json_parse_type', "JSON_ANY")
+            self.json_parse_fn = kwargs.setdefault('json_parse_fn', self.typename + "_parse_json")
         else:
             self.json_gen_fn = kwargs.setdefault('json_gen_fn', None)
+            self.json_parse_type = kwargs.setdefault('json_parse_type', None)
+            self.json_parse_fn = kwargs.setdefault('json_parse_fn', None)
 
         self.autogenerate_json = kwargs.setdefault('autogenerate_json', True)
 
@@ -119,6 +123,8 @@ class Number(Builtin):
         kwargs.setdefault('dispose_fn', None)
         kwargs.setdefault('signed', False)
         kwargs.setdefault('json_gen_fn', "yajl_gen_integer")
+        kwargs.setdefault('json_parse_type', "JSON_INTEGER")
+        kwargs.setdefault('json_parse_fn', "libxl__integer_parse_json")
         self.signed = kwargs['signed']
         Builtin.__init__(self, ctype, **kwargs)
 
@@ -142,6 +148,7 @@ class EnumerationValue(object):
 class Enumeration(Type):
     def __init__(self, typename, values, **kwargs):
         kwargs.setdefault('dispose_fn', None)
+        kwargs.setdefault('json_parse_type', "JSON_STRING")
         Type.__init__(self, typename, **kwargs)
 
         self.value_namespace = kwargs.setdefault('value_namespace',
@@ -171,6 +178,7 @@ class Field(object):
 class Aggregate(Type):
     """A type containing a collection of other types"""
     def __init__(self, kind, typename, fields, **kwargs):
+        kwargs.setdefault('json_parse_type', "JSON_MAP")
         Type.__init__(self, typename, **kwargs)
 
         if self.typename is not None:
@@ -257,6 +265,8 @@ class KeyedUnion(Aggregate):
 void = Builtin("void *", namespace = None)
 bool = Builtin("bool", namespace = None,
                json_gen_fn = "yajl_gen_bool",
+               json_parse_type = "JSON_BOOL",
+               json_parse_fn = "libxl__bool_parse_json",
                autogenerate_json = False)
 
 size_t = Number("size_t", namespace = None)
@@ -270,12 +280,15 @@ uint64 = UInt(64)
 
 string = Builtin("char *", namespace = None, dispose_fn = "free",
                  json_gen_fn = "libxl__string_gen_json",
+                 json_parse_type = "JSON_STRING | JSON_NULL",
+                 json_parse_fn = "libxl__string_parse_json",
                  autogenerate_json = False)
 
 class Array(Type):
     """An array of the same type"""
     def __init__(self, elem_type, lenvar_name, **kwargs):
         kwargs.setdefault('dispose_fn', 'free')
+        kwargs.setdefault('json_parse_type', 'JSON_ARRAY')
         Type.__init__(self, namespace=elem_type.namespace, typename=elem_type.rawname + " *", **kwargs)
 
         lv_kwargs = dict([(x.lstrip('lenvar_'),y) for (x,y) in kwargs.items() if x.startswith('lenvar_')])
diff --git a/tools/libxl/libxl_types.idl b/tools/libxl/libxl_types.idl
index 755c918..77601ff 100644
--- a/tools/libxl/libxl_types.idl
+++ b/tools/libxl/libxl_types.idl
@@ -5,18 +5,23 @@
 
 namespace("libxl_")
 
-libxl_defbool = Builtin("defbool", passby=PASS_BY_REFERENCE)
-
-libxl_domid = Builtin("domid", json_gen_fn = "yajl_gen_integer", autogenerate_json = False)
-libxl_devid = Builtin("devid", json_gen_fn = "yajl_gen_integer", autogenerate_json = False, signed = True, init_val="-1")
-libxl_uuid = Builtin("uuid", passby=PASS_BY_REFERENCE)
-libxl_mac = Builtin("mac", passby=PASS_BY_REFERENCE)
-libxl_bitmap = Builtin("bitmap", dispose_fn="libxl_bitmap_dispose", passby=PASS_BY_REFERENCE)
-libxl_cpuid_policy_list = Builtin("cpuid_policy_list", dispose_fn="libxl_cpuid_dispose", passby=PASS_BY_REFERENCE)
-
-libxl_string_list = Builtin("string_list", dispose_fn="libxl_string_list_dispose", passby=PASS_BY_REFERENCE)
-libxl_key_value_list = Builtin("key_value_list", dispose_fn="libxl_key_value_list_dispose", passby=PASS_BY_REFERENCE)
-libxl_hwcap = Builtin("hwcap", passby=PASS_BY_REFERENCE)
+libxl_defbool = Builtin("defbool", json_parse_type="JSON_STRING", passby=PASS_BY_REFERENCE)
+
+libxl_domid = Builtin("domid", json_gen_fn = "yajl_gen_integer", json_parse_fn = "libxl__integer_parse_json",
+                      json_parse_type = "JSON_INTEGER", autogenerate_json = False)
+libxl_devid = Builtin("devid", json_gen_fn = "yajl_gen_integer", json_parse_fn = "libxl__integer_parse_json",
+                      json_parse_type = "JSON_INTEGER", autogenerate_json = False, signed = True, init_val="-1")
+libxl_uuid = Builtin("uuid", json_parse_type="JSON_STRING", passby=PASS_BY_REFERENCE)
+libxl_mac = Builtin("mac", json_parse_type="JSON_STRING", passby=PASS_BY_REFERENCE)
+libxl_bitmap = Builtin("bitmap", json_parse_type="JSON_ARRAY", dispose_fn="libxl_bitmap_dispose", passby=PASS_BY_REFERENCE)
+libxl_cpuid_policy_list = Builtin("cpuid_policy_list", json_parse_type="JSON_ARRAY",
+                                  dispose_fn="libxl_cpuid_dispose", passby=PASS_BY_REFERENCE)
+
+libxl_string_list = Builtin("string_list", json_parse_type="JSON_ARRAY",
+                            dispose_fn="libxl_string_list_dispose", passby=PASS_BY_REFERENCE)
+libxl_key_value_list = Builtin("key_value_list", json_parse_type="JSON_MAP",
+                               dispose_fn="libxl_key_value_list_dispose", passby=PASS_BY_REFERENCE)
+libxl_hwcap = Builtin("hwcap", json_parse_type="JSON_ARRAY", passby=PASS_BY_REFERENCE)
 
 #
 # Specific integer types
@@ -575,7 +580,7 @@ libxl_event_type = Enumeration("event_type", [
 
 libxl_ev_user = UInt(64)
 
-libxl_ev_link = Builtin("ev_link", passby=PASS_BY_REFERENCE, private=True)
+libxl_ev_link = Builtin("ev_link", json_parse_type="JSON_STRING", passby=PASS_BY_REFERENCE, private=True)
 
 libxl_event = Struct("event",[
     ("link",     libxl_ev_link),
diff --git a/tools/libxl/libxl_types_internal.idl b/tools/libxl/libxl_types_internal.idl
index a964851..125dbac 100644
--- a/tools/libxl/libxl_types_internal.idl
+++ b/tools/libxl/libxl_types_internal.idl
@@ -1,7 +1,9 @@
 namespace("libxl__")
 hidden(True)
 
-libxl_domid = Builtin("domid", namespace="libxl_", json_gen_fn = "yajl_gen_integer")
+libxl_domid = Builtin("domid", namespace="libxl_", json_gen_fn = "yajl_gen_integer",
+		      json_parse_fn = "libxl__integer_parse_json", json_parse_type = "JSON_INTEGER",
+		      autogenerate_json = False)
 
 libxl__qmp_message_type = Enumeration("qmp_message_type", [
     (1, "QMP"),
-- 
1.7.10.4

Re: [PATCH RFC V2 08/10] libxl IDL: generate code to parse libxl__json_object to libxl_FOO struct

[Ian Campbell]

On Thu, 2014-04-17 at 12:13 +0100, Wei Liu wrote:
> libxl_FOO_parse_json functions are generated.
> 
> Note that these functions are used to parse libxl__json_object to
> libxl__FOO struct. They don't consume JSON string.
> 
> Signed-off-by: Wei Liu <wei.liu2@citrix.com>

The existing code is a mess of overly long lines, so I'm not going to
ask you to change that.

I've mostly reviewed this by looking at the generated code.

> +def libxl_C_type_parse_json(ty, w, v, indent = "    ", parent = None):
> +    s = ""
> +    if parent is None:
> +        s += "int rc = 0;\n"
> +        s += "const libxl__json_object *x = o;\n"
> +        s += "{\n"
> +        s += "    libxl__json_object *t;\n"
> +        s += "    int i;\n"
> +        s += "    assert(libxl__json_object_is_array(x));\n"

Unless something in the libxl layer has already ensured this is true
then I think this code needs to be more accepting of malformed input.
i.e. should generate an error.

> 
> +                (nparent,fexpr) = ty.member(v, f.type.keyvar, parent is None)
> +                s += libxl_C_type_parse_json(f.type.keyvar.type, "x", fexpr, "    ", nparent)
> +                s += "}\n"
> +            s += "x = libxl__json_map_get(\"%s\", %s, %s);\n" % (f.name, w, f.type.json_parse_type)
> +            s += "if (x) {\n"
> +            (nparent,fexpr) = ty.member(v, f, parent is None)
> +            s += libxl_C_type_parse_json(f.type, "x", fexpr, "    ", nparent)
> +            s += "    x = x->parent;\n"

What is this x->parent for?

Perhaps it could be avoided by constructing a unique temporary variable
name for each member?

> +            s += "}\n"

> @@ -444,4 +533,17 @@ if __name__ == '__main__':
>          f.write("}\n")
>          f.write("\n")
>  
> +    for ty in [t for t in types if t.json_parse_fn is not None]:
> +        f.write("int %s_parse_json(libxl_ctx *ctx, const libxl__json_object *%s, %s)\n" % (ty.typename,"o",ty.make_arg("p", passby=idl.PASS_BY_REFERENCE)))
> +        f.write("{\n")

Are we assuming/requiring the caller to have called the init function
for the type? Did you consider having this function do it?

> +        f.write(libxl_C_type_parse_json(ty, "o", "p"))
> +        f.write("}\n")
> +        f.write("\n")
> +
> +        f.write("int %s_from_json(libxl_ctx *ctx, %s, const char *s)\n" % (ty.typename, ty.make_arg("p", passby=idl.PASS_BY_REFERENCE)))
> +        f.write("{\n")


> +        f.write(libxl_C_type_from_json(ty, "p", "s"))
> +        f.write("}\n")
> +        f.write("\n")
> +
>      f.close()
> diff --git a/tools/libxl/libxl_types.idl b/tools/libxl/libxl_types.idl
> index 755c918..77601ff 100644
> --- a/tools/libxl/libxl_types.idl
> +++ b/tools/libxl/libxl_types.idl
> @@ -5,18 +5,23 @@
>  
>  namespace("libxl_")
>  
> -libxl_defbool = Builtin("defbool", passby=PASS_BY_REFERENCE)
> -
> -libxl_domid = Builtin("domid", json_gen_fn = "yajl_gen_integer", autogenerate_json = False)
> -libxl_devid = Builtin("devid", json_gen_fn = "yajl_gen_integer", autogenerate_json = False, signed = True, init_val="-1")
> -libxl_uuid = Builtin("uuid", passby=PASS_BY_REFERENCE)
> -libxl_mac = Builtin("mac", passby=PASS_BY_REFERENCE)
> -libxl_bitmap = Builtin("bitmap", dispose_fn="libxl_bitmap_dispose", passby=PASS_BY_REFERENCE)
> -libxl_cpuid_policy_list = Builtin("cpuid_policy_list", dispose_fn="libxl_cpuid_dispose", passby=PASS_BY_REFERENCE)
> -
> -libxl_string_list = Builtin("string_list", dispose_fn="libxl_string_list_dispose", passby=PASS_BY_REFERENCE)
> -libxl_key_value_list = Builtin("key_value_list", dispose_fn="libxl_key_value_list_dispose", passby=PASS_BY_REFERENCE)
> -libxl_hwcap = Builtin("hwcap", passby=PASS_BY_REFERENCE)
> +libxl_defbool = Builtin("defbool", json_parse_type="JSON_STRING", passby=PASS_BY_REFERENCE)

Please update idl.txt for any new properties.

Looks like you forgot this when you renamed json_fn too.

Ian.

Re: [PATCH RFC V2 08/10] libxl IDL: generate code to parse libxl__json_object to libxl_FOO struct

[Wei Liu]

On Tue, Apr 22, 2014 at 04:46:13PM +0100, Ian Campbell wrote:
> On Thu, 2014-04-17 at 12:13 +0100, Wei Liu wrote:
> > libxl_FOO_parse_json functions are generated.
> > 
> > Note that these functions are used to parse libxl__json_object to
> > libxl__FOO struct. They don't consume JSON string.
> > 
> > Signed-off-by: Wei Liu <wei.liu2@citrix.com>
> 
> The existing code is a mess of overly long lines, so I'm not going to
> ask you to change that.
> 
> I've mostly reviewed this by looking at the generated code.
> 
> > +def libxl_C_type_parse_json(ty, w, v, indent = "    ", parent = None):
> > +    s = ""
> > +    if parent is None:
> > +        s += "int rc = 0;\n"
> > +        s += "const libxl__json_object *x = o;\n"
> > +        s += "{\n"
> > +        s += "    libxl__json_object *t;\n"
> > +        s += "    int i;\n"
> > +        s += "    assert(libxl__json_object_is_array(x));\n"
> 
> Unless something in the libxl layer has already ensured this is true
> then I think this code needs to be more accepting of malformed input.
> i.e. should generate an error.
> 

Ack.

> > 
> > +                (nparent,fexpr) = ty.member(v, f.type.keyvar, parent is None)
> > +                s += libxl_C_type_parse_json(f.type.keyvar.type, "x", fexpr, "    ", nparent)
> > +                s += "}\n"
> > +            s += "x = libxl__json_map_get(\"%s\", %s, %s);\n" % (f.name, w, f.type.json_parse_type)
> > +            s += "if (x) {\n"
> > +            (nparent,fexpr) = ty.member(v, f, parent is None)
> > +            s += libxl_C_type_parse_json(f.type, "x", fexpr, "    ", nparent)
> > +            s += "    x = x->parent;\n"
> 
> What is this x->parent for?
> 

To make sure when we finishes processing this member, X points back to
its parent again, so that next call to libxl__json_map_get(x) is using
the right "x".

> Perhaps it could be avoided by constructing a unique temporary variable
> name for each member?
> 

I don't think that's necessary. We are essentially trasvering a tree of
objects.

> > +            s += "}\n"
> 
> > @@ -444,4 +533,17 @@ if __name__ == '__main__':
> >          f.write("}\n")
> >          f.write("\n")
> >  
> > +    for ty in [t for t in types if t.json_parse_fn is not None]:
> > +        f.write("int %s_parse_json(libxl_ctx *ctx, const libxl__json_object *%s, %s)\n" % (ty.typename,"o",ty.make_arg("p", passby=idl.PASS_BY_REFERENCE)))
> > +        f.write("{\n")
> 
> Are we assuming/requiring the caller to have called the init function
> for the type? Did you consider having this function do it?
> 

Yes, I'm assuming so.

But I can also make this function do it if you wish.

> > +        f.write(libxl_C_type_parse_json(ty, "o", "p"))
> > +        f.write("}\n")
> > +        f.write("\n")
> > +
> > +        f.write("int %s_from_json(libxl_ctx *ctx, %s, const char *s)\n" % (ty.typename, ty.make_arg("p", passby=idl.PASS_BY_REFERENCE)))
> > +        f.write("{\n")
> 
> 
> > +        f.write(libxl_C_type_from_json(ty, "p", "s"))
> > +        f.write("}\n")
> > +        f.write("\n")
> > +
> >      f.close()
> > diff --git a/tools/libxl/libxl_types.idl b/tools/libxl/libxl_types.idl
> > index 755c918..77601ff 100644
> > --- a/tools/libxl/libxl_types.idl
> > +++ b/tools/libxl/libxl_types.idl
> > @@ -5,18 +5,23 @@
> >  
> >  namespace("libxl_")
> >  
> > -libxl_defbool = Builtin("defbool", passby=PASS_BY_REFERENCE)
> > -
> > -libxl_domid = Builtin("domid", json_gen_fn = "yajl_gen_integer", autogenerate_json = False)
> > -libxl_devid = Builtin("devid", json_gen_fn = "yajl_gen_integer", autogenerate_json = False, signed = True, init_val="-1")
> > -libxl_uuid = Builtin("uuid", passby=PASS_BY_REFERENCE)
> > -libxl_mac = Builtin("mac", passby=PASS_BY_REFERENCE)
> > -libxl_bitmap = Builtin("bitmap", dispose_fn="libxl_bitmap_dispose", passby=PASS_BY_REFERENCE)
> > -libxl_cpuid_policy_list = Builtin("cpuid_policy_list", dispose_fn="libxl_cpuid_dispose", passby=PASS_BY_REFERENCE)
> > -
> > -libxl_string_list = Builtin("string_list", dispose_fn="libxl_string_list_dispose", passby=PASS_BY_REFERENCE)
> > -libxl_key_value_list = Builtin("key_value_list", dispose_fn="libxl_key_value_list_dispose", passby=PASS_BY_REFERENCE)
> > -libxl_hwcap = Builtin("hwcap", passby=PASS_BY_REFERENCE)
> > +libxl_defbool = Builtin("defbool", json_parse_type="JSON_STRING", passby=PASS_BY_REFERENCE)
> 
> Please update idl.txt for any new properties.
> 
> Looks like you forgot this when you renamed json_fn too.
> 

Already fix in my latest version.

Wei.

> Ian.

Re: [PATCH RFC V2 08/10] libxl IDL: generate code to parse libxl__json_object to libxl_FOO struct

[Ian Campbell]

On Tue, 2014-04-22 at 16:54 +0100, Wei Liu wrote:
> > > 
> > > +                (nparent,fexpr) = ty.member(v, f.type.keyvar, parent is None)
> > > +                s += libxl_C_type_parse_json(f.type.keyvar.type, "x", fexpr, "    ", nparent)
> > > +                s += "}\n"
> > > +            s += "x = libxl__json_map_get(\"%s\", %s, %s);\n" % (f.name, w, f.type.json_parse_type)
> > > +            s += "if (x) {\n"
> > > +            (nparent,fexpr) = ty.member(v, f, parent is None)
> > > +            s += libxl_C_type_parse_json(f.type, "x", fexpr, "    ", nparent)
> > > +            s += "    x = x->parent;\n"
> > 
> > What is this x->parent for?
> > 
> 
> To make sure when we finishes processing this member, X points back to
> its parent again, so that next call to libxl__json_map_get(x) is using
> the right "x".

Can we create temporary fixed scope "x"s for each field? It's ok if that
shadows the existing "x" (for the parent) I think, or am I missing
something?

> > > +            s += "}\n"
> > 
> > > @@ -444,4 +533,17 @@ if __name__ == '__main__':
> > >          f.write("}\n")
> > >          f.write("\n")
> > >  
> > > +    for ty in [t for t in types if t.json_parse_fn is not None]:
> > > +        f.write("int %s_parse_json(libxl_ctx *ctx, const libxl__json_object *%s, %s)\n" % (ty.typename,"o",ty.make_arg("p", passby=idl.PASS_BY_REFERENCE)))
> > > +        f.write("{\n")
> > 
> > Are we assuming/requiring the caller to have called the init function
> > for the type? Did you consider having this function do it?
> > 
> 
> Yes, I'm assuming so.
> 
> But I can also make this function do it if you wish.

I think that would be best, this function can be thought of as "init
from this json", so it should probably do the whole job, which includes
setting all the appropriate defaults if the json omits fields etc.

Ian.

Re: [PATCH RFC V2 08/10] libxl IDL: generate code to parse libxl__json_object to libxl_FOO struct

[Wei Liu]

On Tue, Apr 22, 2014 at 05:01:43PM +0100, Ian Campbell wrote:
> On Tue, 2014-04-22 at 16:54 +0100, Wei Liu wrote:
> > > > 
> > > > +                (nparent,fexpr) = ty.member(v, f.type.keyvar, parent is None)
> > > > +                s += libxl_C_type_parse_json(f.type.keyvar.type, "x", fexpr, "    ", nparent)
> > > > +                s += "}\n"
> > > > +            s += "x = libxl__json_map_get(\"%s\", %s, %s);\n" % (f.name, w, f.type.json_parse_type)
> > > > +            s += "if (x) {\n"
> > > > +            (nparent,fexpr) = ty.member(v, f, parent is None)
> > > > +            s += libxl_C_type_parse_json(f.type, "x", fexpr, "    ", nparent)
> > > > +            s += "    x = x->parent;\n"
> > > 
> > > What is this x->parent for?
> > > 
> > 
> > To make sure when we finishes processing this member, X points back to
> > its parent again, so that next call to libxl__json_map_get(x) is using
> > the right "x".
> 
> Can we create temporary fixed scope "x"s for each field? It's ok if that
> shadows the existing "x" (for the parent) I think, or am I missing
> something?
> 

libxl has -Wshadow on.

> > > > +            s += "}\n"
> > > 
> > > > @@ -444,4 +533,17 @@ if __name__ == '__main__':
> > > >          f.write("}\n")
> > > >          f.write("\n")
> > > >  
> > > > +    for ty in [t for t in types if t.json_parse_fn is not None]:
> > > > +        f.write("int %s_parse_json(libxl_ctx *ctx, const libxl__json_object *%s, %s)\n" % (ty.typename,"o",ty.make_arg("p", passby=idl.PASS_BY_REFERENCE)))
> > > > +        f.write("{\n")
> > > 
> > > Are we assuming/requiring the caller to have called the init function
> > > for the type? Did you consider having this function do it?
> > > 
> > 
> > Yes, I'm assuming so.
> > 
> > But I can also make this function do it if you wish.
> 
> I think that would be best, this function can be thought of as "init
> from this json", so it should probably do the whole job, which includes
> setting all the appropriate defaults if the json omits fields etc.
> 

Ack.

Wei.

> Ian.

[PATCH RFC V2 09/10] libxl/gentest.py: test JSON parser

[Wei Liu]

The test is done in following steps:

1. initialise libxl_FOO struct
2. generate JSON string A for libxl_FOO struct FOO1
3. convert JSON string A to libxl_FOO struct FOO2
4. generate JSON string B for libxl_FOO struct FOO2
5. compare A and B

If A and B are identical then we are good.

Signed-off-by: Wei Liu <wei.liu2@citrix.com>
---
 tools/libxl/gentest.py |   23 ++++++++++++++++++++---
 1 file changed, 20 insertions(+), 3 deletions(-)

diff --git a/tools/libxl/gentest.py b/tools/libxl/gentest.py
index eb9a21b..b92d092 100644
--- a/tools/libxl/gentest.py
+++ b/tools/libxl/gentest.py
@@ -225,10 +225,11 @@ int main(int argc, char **argv)
 """)
 
     for ty in types:
-        f.write("    %s %s_val;\n" % (ty.typename, ty.typename))
+        f.write("    %s %s_val, %s_val_new;\n" % \
+                (ty.typename, ty.typename, ty.typename))
     f.write("""
     int rc;
-    char *s;
+    char *s, *new_s;
     xentoollog_logger_stdiostream *logger;
     libxl_ctx *ctx;
 
@@ -240,20 +241,36 @@ int main(int argc, char **argv)
         exit(1);
     }
 """)
-    f.write("    printf(\"Testing TYPE_to_json()\\n\");\n")
+    f.write("    printf(\"Testing TYPE_to/from_json()\\n\");\n")
     f.write("    printf(\"----------------------\\n\");\n")
     f.write("    printf(\"\\n\");\n")
     for ty in [t for t in types if t.json_gen_fn is not None]:
         arg = ty.typename + "_val"
         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)))
         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)
         f.write("    if (s == NULL) abort();\n")
+        f.write("    rc = %s_from_json(ctx, &%s_val_new, s);\n" % \
+                (ty.typename, ty.typename))
+        f.write("    if (rc) abort();\n")
+        f.write("    new_s = %s_to_json(ctx, %s_new);\n" % \
+                (ty.typename, ty.pass_arg(arg, isref=False)))
+        f.write("    if (new_s == NULL) abort();\n")
+        f.write("    if (strcmp(s, new_s)) {\n")
+        f.write("        printf(\"Huh? Regenerated string different from original string.\\n\");\n")
+        f.write("        printf(\"Regenerated string: %s\\n\", new_s);\n")
+        f.write("        abort();\n")
+        f.write("    }\n")
         f.write("    free(s);\n")
+        f.write("    free(new_s);\n")
         if ty.dispose_fn is not None:
             f.write("    %s(&%s_val);\n" % (ty.dispose_fn, ty.typename))
+            f.write("    %s(&%s_val_new);\n" % (ty.dispose_fn, ty.typename))
         f.write("\n")
 
     f.write("    printf(\"Testing Enumerations\\n\");\n")
-- 
1.7.10.4

Re: [PATCH RFC V2 09/10] libxl/gentest.py: test JSON parser

[Ian Campbell]

On Thu, 2014-04-17 at 12:13 +0100, Wei Liu wrote:
> @@ -240,20 +241,36 @@ int main(int argc, char **argv)
>          exit(1);
>      }
>  """)
> -    f.write("    printf(\"Testing TYPE_to_json()\\n\");\n")
> +    f.write("    printf(\"Testing TYPE_to/from_json()\\n\");\n")
>      f.write("    printf(\"----------------------\\n\");\n")
>      f.write("    printf(\"\\n\");\n")
>      for ty in [t for t in types if t.json_gen_fn is not None]:
>          arg = ty.typename + "_val"
>          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)))
>          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)
>          f.write("    if (s == NULL) abort();\n")
> +        f.write("    rc = %s_from_json(ctx, &%s_val_new, s);\n" % \
> +                (ty.typename, ty.typename))
> +        f.write("    if (rc) abort();\n")
> +        f.write("    new_s = %s_to_json(ctx, %s_new);\n" % \
> +                (ty.typename, ty.pass_arg(arg, isref=False)))
> +        f.write("    if (new_s == NULL) abort();\n")
> +        f.write("    if (strcmp(s, new_s)) {\n")
> +        f.write("        printf(\"Huh? Regenerated string different from original string.\\n\");\n")
> +        f.write("        printf(\"Regenerated string: %s\\n\", new_s);\n")

This could use some hint as to which struct went wrong. Perhaps
including the original string too for comparison?

Re: [PATCH RFC V2 09/10] libxl/gentest.py: test JSON parser

[Wei Liu]

On Tue, Apr 22, 2014 at 04:47:22PM +0100, Ian Campbell wrote:
> On Thu, 2014-04-17 at 12:13 +0100, Wei Liu wrote:
> > @@ -240,20 +241,36 @@ int main(int argc, char **argv)
> >          exit(1);
> >      }
> >  """)
> > -    f.write("    printf(\"Testing TYPE_to_json()\\n\");\n")
> > +    f.write("    printf(\"Testing TYPE_to/from_json()\\n\");\n")
> >      f.write("    printf(\"----------------------\\n\");\n")
> >      f.write("    printf(\"\\n\");\n")
> >      for ty in [t for t in types if t.json_gen_fn is not None]:
> >          arg = ty.typename + "_val"
> >          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)))
> >          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)
> >          f.write("    if (s == NULL) abort();\n")
> > +        f.write("    rc = %s_from_json(ctx, &%s_val_new, s);\n" % \
> > +                (ty.typename, ty.typename))
> > +        f.write("    if (rc) abort();\n")
> > +        f.write("    new_s = %s_to_json(ctx, %s_new);\n" % \
> > +                (ty.typename, ty.pass_arg(arg, isref=False)))
> > +        f.write("    if (new_s == NULL) abort();\n")
> > +        f.write("    if (strcmp(s, new_s)) {\n")
> > +        f.write("        printf(\"Huh? Regenerated string different from original string.\\n\");\n")
> > +        f.write("        printf(\"Regenerated string: %s\\n\", new_s);\n")
> 
> This could use some hint as to which struct went wrong. Perhaps
> including the original string too for comparison?
> 
> 

It's already printed a few line above. :-)

Wei.

Re: [PATCH RFC V2 09/10] libxl/gentest.py: test JSON parser

[Ian Campbell]

On Tue, 2014-04-22 at 16:49 +0100, Wei Liu wrote:
> On Tue, Apr 22, 2014 at 04:47:22PM +0100, Ian Campbell wrote:
> > On Thu, 2014-04-17 at 12:13 +0100, Wei Liu wrote:
> > > @@ -240,20 +241,36 @@ int main(int argc, char **argv)
> > >          exit(1);
> > >      }
> > >  """)
> > > -    f.write("    printf(\"Testing TYPE_to_json()\\n\");\n")
> > > +    f.write("    printf(\"Testing TYPE_to/from_json()\\n\");\n")
> > >      f.write("    printf(\"----------------------\\n\");\n")
> > >      f.write("    printf(\"\\n\");\n")
> > >      for ty in [t for t in types if t.json_gen_fn is not None]:
> > >          arg = ty.typename + "_val"
> > >          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)))
> > >          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)
> > >          f.write("    if (s == NULL) abort();\n")
> > > +        f.write("    rc = %s_from_json(ctx, &%s_val_new, s);\n" % \
> > > +                (ty.typename, ty.typename))
> > > +        f.write("    if (rc) abort();\n")
> > > +        f.write("    new_s = %s_to_json(ctx, %s_new);\n" % \
> > > +                (ty.typename, ty.pass_arg(arg, isref=False)))
> > > +        f.write("    if (new_s == NULL) abort();\n")
> > > +        f.write("    if (strcmp(s, new_s)) {\n")
> > > +        f.write("        printf(\"Huh? Regenerated string different from original string.\\n\");\n")
> > > +        f.write("        printf(\"Regenerated string: %s\\n\", new_s);\n")
> > 
> > This could use some hint as to which struct went wrong. Perhaps
> > including the original string too for comparison?
> > 
> > 
> 
> It's already printed a few line above. :-)

Ah, I missed it in the noise. Great.

[PATCH RFC V2 10/10] xl: introduce "xl-json" format

[Wei Liu]

Originally xl verbatimly copies the domain config file in its user data
store. This patch adds the functionality to transform text domain config
file to JSON object and save that object in user data store.

What this patch does:
1. add a mandatory flag to save protocol to indicate whether the saved
   config is a JSON object
2. register a new private data type "xl-json" in libxl.h
3. modify xl to save / load "xl-json" file where necessary

After this change xl supports both "xl" format and "xl-json" format.
The user-supplied config file is still restricted to normal text config
file format ("xl"), as xl has more sanity checks when parsing text
config file. "xl-json" format is only used internally. The saved config
file is always in "xl-json" format.

Tests done so far (xl.{new,old} denotes xl with{,out} "xl_json"
support):
1. xl.new create then hexdump saved file: domain config saved in JSON format
2. xl.new create then xl.old restore: failed on mandatory flag check
3. xl.new create then xl.new restore: succeeded
4. xl.old create then xl.new restore: succeeded
5. xl.new create then local migrate, receiving end xl.new: succeeded
6. xl.old create then local migrate, receiving end xl.new: succeeded

The only drawback is that when restoring a domain, xl cannot
automatically spawn a vncviewer anymore. That's because that option is
part of domain_create info not a domain configuration thus it's not
saved in the JSON config.

Signed-off-by: Wei Liu <wei.liu2@citrix.com>
---
 docs/man/xl.cfg.pod.5    |    2 +-
 tools/libxl/libxl.h      |    1 +
 tools/libxl/xl_cmdimpl.c |  103 +++++++++++++++++++++++++++++++++++-----------
 3 files changed, 82 insertions(+), 24 deletions(-)

diff --git a/docs/man/xl.cfg.pod.5 b/docs/man/xl.cfg.pod.5
index a6663b9..a20f25f 100644
--- a/docs/man/xl.cfg.pod.5
+++ b/docs/man/xl.cfg.pod.5
@@ -1106,7 +1106,7 @@ other VNC-related settings.  The default is to enable this.
 
 =item B<vncviewer=BOOLEAN>
 
-Automatically spawn a vncviewer when creating/restoring a guest.
+Automatically spawn a vncviewer when creating a guest.
 
 =item B<vnclisten="ADDRESS[:DISPLAYNUM]">
 
diff --git a/tools/libxl/libxl.h b/tools/libxl/libxl.h
index b2c3015..01a89a8 100644
--- a/tools/libxl/libxl.h
+++ b/tools/libxl/libxl.h
@@ -1046,6 +1046,7 @@ void libxl_cpuid_set(libxl_ctx *ctx, uint32_t domid,
  *
  *  userid        Data contents
  *  "xl"          domain config file in xl format, Unix line endings
+ *  "xl-json"     domain config file in JSON format generated by xl
  *  "libvirt-xml" domain config file in libvirt XML format.  See
  *                http://libvirt.org/formatdomain.html
  *
diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
index 0b38b32..56aeaff 100644
--- a/tools/libxl/xl_cmdimpl.c
+++ b/tools/libxl/xl_cmdimpl.c
@@ -110,6 +110,8 @@ static const char migrate_report[]=
    *            from target to source
    */
 
+#define XL_MANDATORY_FLAG_JSON (1U << 0) /* config data is in JSON format  */
+#define XL_MANDATORY_FLAG_ALL  (XL_MANDATORY_FLAG_JSON)
 struct save_file_header {
     char magic[32]; /* savefileheader_magic */
     /* All uint32_ts are in domain's byte order. */
@@ -725,6 +727,18 @@ static void parse_top_level_sdl_options(XLU_Config *config,
     xlu_cfg_replace_string (config, "xauthority", &sdl->xauthority, 0);
 }
 
+static void parse_config_data_json(const char *config_data,
+                                   libxl_domain_config *d_config)
+{
+    int ret;
+
+    ret = libxl_domain_config_from_json(ctx, d_config, config_data);
+    if (ret) {
+        fprintf(stderr, "Failed to parse config\n");
+        exit(1);
+    }
+}
+
 static void parse_config_data(const char *config_source,
                               const char *config_data,
                               int config_len,
@@ -1769,12 +1783,13 @@ skip_vfb:
 }
 
 static void reload_domain_config(uint32_t domid,
-                                 uint8_t **config_data, int *config_len)
+                                 uint8_t **config_data, int *config_len,
+                                 bool *config_in_json)
 {
     uint8_t *t_data;
     int ret, t_len;
 
-    ret = libxl_userdata_retrieve(ctx, domid, "xl", &t_data, &t_len);
+    ret = libxl_userdata_retrieve(ctx, domid, "xl-json", &t_data, &t_len);
     if (ret) {
         LOG("failed to retrieve guest configuration (rc=%d). "
             "reusing old configuration", ret);
@@ -1784,6 +1799,7 @@ static void reload_domain_config(uint32_t domid,
     free(*config_data);
     *config_data = t_data;
     *config_len = t_len;
+    *config_in_json = true;
 }
 
 /* Returns 1 if domain should be restarted,
@@ -1792,6 +1808,7 @@ static void reload_domain_config(uint32_t domid,
 static int handle_domain_death(uint32_t *r_domid,
                                libxl_event *event,
                                uint8_t **config_data, int *config_len,
+                               bool *config_in_json,
                                libxl_domain_config *d_config)
 
 {
@@ -1849,12 +1866,14 @@ static int handle_domain_death(uint32_t *r_domid,
         break;
 
     case LIBXL_ACTION_ON_SHUTDOWN_RESTART_RENAME:
-        reload_domain_config(*r_domid, config_data, config_len);
+        reload_domain_config(*r_domid, config_data, config_len,
+                             config_in_json);
         restart = 2;
         break;
 
     case LIBXL_ACTION_ON_SHUTDOWN_RESTART:
-        reload_domain_config(*r_domid, config_data, config_len);
+        reload_domain_config(*r_domid, config_data, config_len,
+                             config_in_json);
 
         restart = 1;
         /* fall-through */
@@ -2040,6 +2059,7 @@ static uint32_t create_domain(struct domain_create *dom_info)
     uint32_t domid = INVALID_DOMID;
 
     libxl_domain_config d_config;
+    char *d_config_json = NULL;
 
     int debug = dom_info->debug;
     int daemonize = dom_info->daemonize;
@@ -2060,6 +2080,7 @@ static uint32_t create_domain(struct domain_create *dom_info)
     libxl_evgen_disk_eject **diskws = NULL; /* one per disk */
     void *config_data = 0;
     int config_len = 0;
+    bool config_in_json = false;
     int restore_fd = -1;
     const libxl_asyncprogress_how *autoconnect_console_how;
     struct save_file_header hdr;
@@ -2106,7 +2127,7 @@ static uint32_t create_domain(struct domain_create *dom_info)
                 restore_source, hdr.mandatory_flags, hdr.optional_flags,
                 hdr.optional_data_len);
 
-        badflags = hdr.mandatory_flags & ~( 0 /* none understood yet */ );
+        badflags = hdr.mandatory_flags & ~XL_MANDATORY_FLAG_ALL;
         if (badflags) {
             fprintf(stderr, "Savefile has mandatory flag(s) 0x%"PRIx32" "
                     "which are not supported; need newer xl\n",
@@ -2134,7 +2155,10 @@ static uint32_t create_domain(struct domain_create *dom_info)
         optdata_here = optdata_begin;
 
         if (OPTDATA_LEFT) {
-            fprintf(stderr, " Savefile contains xl domain config\n");
+            config_in_json =
+                !!(hdr.mandatory_flags & XL_MANDATORY_FLAG_JSON);
+            fprintf(stderr, " Savefile contains xl domain config%s\n",
+                    config_in_json ? " in JSON format" : "");
             WITH_OPTDATA(4, {
                 memcpy(u32buf.b, optdata_here, 4);
                 config_len = u32buf.u32;
@@ -2174,6 +2198,8 @@ static uint32_t create_domain(struct domain_create *dom_info)
                 extra_config);
         }
         config_source=config_file;
+        /* User supplied file is just a text config file */
+        config_in_json = false;
     } else {
         if (!config_data) {
             fprintf(stderr, "Config file not specified and"
@@ -2186,7 +2212,11 @@ static uint32_t create_domain(struct domain_create *dom_info)
     if (!dom_info->quiet)
         printf("Parsing config from %s\n", config_source);
 
-    parse_config_data(config_source, config_data, config_len, &d_config, dom_info);
+    if (config_in_json)
+        parse_config_data_json(config_data, &d_config);
+    else
+        parse_config_data(config_source, config_data, config_len,
+                          &d_config, dom_info);
 
     if (migrate_fd >= 0) {
         if (d_config.c_info.name) {
@@ -2213,6 +2243,10 @@ static uint32_t create_domain(struct domain_create *dom_info)
     if (dom_info->dryrun)
         goto out;
 
+    d_config_json = libxl_domain_config_to_json(ctx, &d_config);
+    if (!d_config_json)
+        goto out;
+
 start:
     assert(domid == INVALID_DOMID);
 
@@ -2281,8 +2315,9 @@ start:
         free(vcpu_to_pcpu); vcpu_to_pcpu = NULL;
     }
 
-    ret = libxl_userdata_store(ctx, domid, "xl",
-                                    config_data, config_len);
+    ret = libxl_userdata_store(ctx, domid, "xl-json",
+                               (const uint8_t *)d_config_json,
+                               strlen(d_config_json));
     if (ret) {
         perror("cannot save config file");
         ret = ERROR_FAIL;
@@ -2347,7 +2382,7 @@ start:
                 event->u.domain_shutdown.shutdown_reason);
             switch (handle_domain_death(&domid, event,
                                         (uint8_t **)&config_data, &config_len,
-                                        &d_config)) {
+                                        &config_in_json, &d_config)) {
             case 2:
                 if (!preserve_domain(&domid, event, &d_config)) {
                     /* If we fail then exit leaving the old domain in place. */
@@ -2387,8 +2422,11 @@ start:
                 /* Reparse the configuration in case it has changed */
                 libxl_domain_config_dispose(&d_config);
                 libxl_domain_config_init(&d_config);
-                parse_config_data(config_source, config_data, config_len,
-                                  &d_config, dom_info);
+                if (config_in_json)
+                    parse_config_data_json(config_data, &d_config);
+                else
+                    parse_config_data(config_source, config_data, config_len,
+                                      &d_config, dom_info);
 
                 /*
                  * XXX FIXME: If this sleep is not there then domain
@@ -2444,6 +2482,8 @@ out:
 
     free(config_data);
 
+    free(d_config_json);
+
     console_child_report(child_console);
 
     if (deathw)
@@ -3194,12 +3234,14 @@ static void list_domains_details(const libxl_dominfo *info, int nb_domain)
         /* no detailed info available on dom0 */
         if (info[i].domid == 0)
             continue;
-        rc = libxl_userdata_retrieve(ctx, info[i].domid, "xl", &data, &len);
+        rc = libxl_userdata_retrieve(ctx, info[i].domid, "xl-json",
+                                     &data, &len);
         if (rc)
             continue;
         CHK_SYSCALL(asprintf(&config_source, "<domid %d data>", info[i].domid));
         libxl_domain_config_init(&d_config);
-        parse_config_data(config_source, (char *)data, len, &d_config, NULL);
+        /* Saved config file is in JSON format */
+        parse_config_data_json((const char *)data, &d_config);
         if (default_output_format == OUTPUT_FORMAT_JSON)
             s = printf_info_one_json(hand, info[i].domid, &d_config);
         else
@@ -3407,7 +3449,7 @@ static void save_domain_core_begin(uint32_t domid,
                                       &config_v, config_len_r);
         *config_data_r = config_v;
     } else {
-        rc = libxl_userdata_retrieve(ctx, domid, "xl",
+        rc = libxl_userdata_retrieve(ctx, domid, "xl-json",
                                      config_data_r, config_len_r);
     }
     if (rc) {
@@ -3417,7 +3459,8 @@ static void save_domain_core_begin(uint32_t domid,
 }
 
 static void save_domain_core_writeconfig(int fd, const char *source,
-                                  const uint8_t *config_data, int config_len)
+                                         const uint8_t *config_data, int config_len,
+                                         bool config_in_json)
 {
     struct save_file_header hdr;
     uint8_t *optdata_begin;
@@ -3441,6 +3484,8 @@ static void save_domain_core_writeconfig(int fd, const char *source,
     u32buf.u32 = config_len;
     ADD_OPTDATA(u32buf.b,    4);
     ADD_OPTDATA(config_data, config_len);
+    if (config_in_json)
+        hdr.mandatory_flags |= XL_MANDATORY_FLAG_JSON;
 
     /* that's the optional data */
 
@@ -3464,6 +3509,10 @@ static int save_domain(uint32_t domid, const char *filename, int checkpoint,
     int fd;
     uint8_t *config_data;
     int config_len;
+    /* If user doesn't provide override_config_file, we use saved
+     * config file which is in JSON format.
+     */
+    bool config_in_json = (override_config_file == NULL);
 
     save_domain_core_begin(domid, override_config_file,
                            &config_data, &config_len);
@@ -3478,7 +3527,8 @@ static int save_domain(uint32_t domid, const char *filename, int checkpoint,
         exit(2);
     }
 
-    save_domain_core_writeconfig(fd, filename, config_data, config_len);
+    save_domain_core_writeconfig(fd, filename, config_data, config_len,
+                                 config_in_json);
 
     int rc = libxl_domain_suspend(ctx, domid, fd, 0, NULL);
     close(fd);
@@ -3619,7 +3669,7 @@ static void migration_child_report(int recv_fd) {
 
 static void migrate_do_preamble(int send_fd, int recv_fd, pid_t child,
                                 uint8_t *config_data, int config_len,
-                                const char *rune)
+                                bool config_in_json, const char *rune)
 {
     int rc = 0;
 
@@ -3638,7 +3688,7 @@ static void migrate_do_preamble(int send_fd, int recv_fd, pid_t child,
     }
 
     save_domain_core_writeconfig(send_fd, "migration stream",
-                                 config_data, config_len);
+                                 config_data, config_len, config_in_json);
 
 }
 
@@ -3652,6 +3702,7 @@ static void migrate_domain(uint32_t domid, const char *rune, int debug,
     char rc_buf;
     uint8_t *config_data;
     int config_len, flags = LIBXL_SUSPEND_LIVE;
+    bool config_in_json = (override_config_file == NULL);
 
     save_domain_core_begin(domid, override_config_file,
                            &config_data, &config_len);
@@ -3665,7 +3716,7 @@ static void migrate_domain(uint32_t domid, const char *rune, int debug,
     child = create_migration_child(rune, &send_fd, &recv_fd);
 
     migrate_do_preamble(send_fd, recv_fd, child, config_data, config_len,
-                        rune);
+                        config_in_json, rune);
 
     xtl_stdiostream_adjust_flags(logger, XTL_STDIOSTREAM_HIDE_PROGRESS, 0);
 
@@ -4507,19 +4558,24 @@ int main_config_update(int argc, char **argv)
 
     libxl_domain_config_init(&d_config);
 
+    /* User supplied config is just text config file */
     parse_config_data(filename, config_data, config_len, &d_config, NULL);
 
     if (debug || dryrun_only)
         printf_info(default_output_format, -1, &d_config);
 
     if (!dryrun_only) {
+        char *d_config_json = NULL;
         fprintf(stderr, "setting dom%d configuration\n", domid);
-        rc = libxl_userdata_store(ctx, domid, "xl",
-                                   config_data, config_len);
+        d_config_json = libxl_domain_config_to_json(ctx, &d_config);
+        rc = libxl_userdata_store(ctx, domid, "xl-json",
+                                  (const uint8_t*)d_config_json,
+                                  strlen(d_config_json));
         if (rc) {
             fprintf(stderr, "failed to update configuration\n");
             exit(1);
         }
+        free(d_config_json);
     }
 
     libxl_domain_config_dispose(&d_config);
@@ -7333,7 +7389,8 @@ int main_remus(int argc, char **argv)
 
         child = create_migration_child(rune, &send_fd, &recv_fd);
 
-        migrate_do_preamble(send_fd, recv_fd, child, config_data, config_len,
+        migrate_do_preamble(send_fd, recv_fd, child,
+                            config_data, config_len, true /* config_in_json */,
                             rune);
 
         if (ssh_command[0])
-- 
1.7.10.4

Re: [PATCH RFC V2 10/10] xl: introduce "xl-json" format

[Wei Liu]

On Thu, Apr 17, 2014 at 12:13:11PM +0100, Wei Liu wrote:
> Originally xl verbatimly copies the domain config file in its user data
> store. This patch adds the functionality to transform text domain config
> file to JSON object and save that object in user data store.
> 
> What this patch does:
> 1. add a mandatory flag to save protocol to indicate whether the saved
>    config is a JSON object
> 2. register a new private data type "xl-json" in libxl.h
> 3. modify xl to save / load "xl-json" file where necessary
> 
> After this change xl supports both "xl" format and "xl-json" format.
> The user-supplied config file is still restricted to normal text config
> file format ("xl"), as xl has more sanity checks when parsing text
> config file. "xl-json" format is only used internally. The saved config
> file is always in "xl-json" format.
> 
> Tests done so far (xl.{new,old} denotes xl with{,out} "xl_json"
> support):
> 1. xl.new create then hexdump saved file: domain config saved in JSON format
> 2. xl.new create then xl.old restore: failed on mandatory flag check
> 3. xl.new create then xl.new restore: succeeded
> 4. xl.old create then xl.new restore: succeeded
> 5. xl.new create then local migrate, receiving end xl.new: succeeded
> 6. xl.old create then local migrate, receiving end xl.new: succeeded
> 
> The only drawback is that when restoring a domain, xl cannot
> automatically spawn a vncviewer anymore. That's because that option is
> part of domain_create info not a domain configuration thus it's not
> saved in the JSON config.
> 

On a second thought I might as well create abstraction of a config file
in IDL. This might be more flexible in the long run. Then the burden
will be whenever an option that doesn't belong to domain_config is
added, IDL will also need to be taken care of.

Wei.

Re: [PATCH RFC V2 10/10] xl: introduce "xl-json" format

[Ian Campbell]

On Sat, 2014-04-19 at 09:35 +0100, Wei Liu wrote:
> On Thu, Apr 17, 2014 at 12:13:11PM +0100, Wei Liu wrote:
> > Originally xl verbatimly copies the domain config file in its user data
> > store. This patch adds the functionality to transform text domain config
> > file to JSON object and save that object in user data store.
> > 
> > What this patch does:
> > 1. add a mandatory flag to save protocol to indicate whether the saved
> >    config is a JSON object
> > 2. register a new private data type "xl-json" in libxl.h
> > 3. modify xl to save / load "xl-json" file where necessary
> > 
> > After this change xl supports both "xl" format and "xl-json" format.
> > The user-supplied config file is still restricted to normal text config
> > file format ("xl"), as xl has more sanity checks when parsing text
> > config file. "xl-json" format is only used internally. The saved config
> > file is always in "xl-json" format.
> > 
> > Tests done so far (xl.{new,old} denotes xl with{,out} "xl_json"
> > support):
> > 1. xl.new create then hexdump saved file: domain config saved in JSON format
> > 2. xl.new create then xl.old restore: failed on mandatory flag check
> > 3. xl.new create then xl.new restore: succeeded
> > 4. xl.old create then xl.new restore: succeeded
> > 5. xl.new create then local migrate, receiving end xl.new: succeeded
> > 6. xl.old create then local migrate, receiving end xl.new: succeeded
> > 
> > The only drawback is that when restoring a domain, xl cannot
> > automatically spawn a vncviewer anymore. That's because that option is
> > part of domain_create info not a domain configuration thus it's not
> > saved in the JSON config.
> > 
> 
> On a second thought I might as well create abstraction of a config file
> in IDL. This might be more flexible in the long run. Then the burden
> will be whenever an option that doesn't belong to domain_config is
> added, IDL will also need to be taken care of.

Is the issue here that the vnc autospawning is controlled via the xl
command line (-A) rather than something in the configuration file? Or is
there a config file way to do this too?

If it's because it is on the command line then we should be propagating
that to the xl migrate-restore perhaps.

We could also have xl transmit some state of its own outside of the
domain configuration, either by using the IDL to do it in JSON or some
other mechanism.

I'm not sure about adding things to the libxl interface which libxl
itself doesn't deal with just as a way to conveniently carry the
information around.

Ian.

Re: [PATCH RFC V2 10/10] xl: introduce "xl-json" format

[Wei Liu]

On Tue, Apr 22, 2014 at 11:15:10AM +0100, Ian Campbell wrote:
> On Sat, 2014-04-19 at 09:35 +0100, Wei Liu wrote:
> > On Thu, Apr 17, 2014 at 12:13:11PM +0100, Wei Liu wrote:
> > > Originally xl verbatimly copies the domain config file in its user data
> > > store. This patch adds the functionality to transform text domain config
> > > file to JSON object and save that object in user data store.
> > > 
> > > What this patch does:
> > > 1. add a mandatory flag to save protocol to indicate whether the saved
> > >    config is a JSON object
> > > 2. register a new private data type "xl-json" in libxl.h
> > > 3. modify xl to save / load "xl-json" file where necessary
> > > 
> > > After this change xl supports both "xl" format and "xl-json" format.
> > > The user-supplied config file is still restricted to normal text config
> > > file format ("xl"), as xl has more sanity checks when parsing text
> > > config file. "xl-json" format is only used internally. The saved config
> > > file is always in "xl-json" format.
> > > 
> > > Tests done so far (xl.{new,old} denotes xl with{,out} "xl_json"
> > > support):
> > > 1. xl.new create then hexdump saved file: domain config saved in JSON format
> > > 2. xl.new create then xl.old restore: failed on mandatory flag check
> > > 3. xl.new create then xl.new restore: succeeded
> > > 4. xl.old create then xl.new restore: succeeded
> > > 5. xl.new create then local migrate, receiving end xl.new: succeeded
> > > 6. xl.old create then local migrate, receiving end xl.new: succeeded
> > > 
> > > The only drawback is that when restoring a domain, xl cannot
> > > automatically spawn a vncviewer anymore. That's because that option is
> > > part of domain_create info not a domain configuration thus it's not
> > > saved in the JSON config.
> > > 
> > 
> > On a second thought I might as well create abstraction of a config file
> > in IDL. This might be more flexible in the long run. Then the burden
> > will be whenever an option that doesn't belong to domain_config is
> > added, IDL will also need to be taken care of.
> 
> Is the issue here that the vnc autospawning is controlled via the xl
> command line (-A) rather than something in the configuration file? Or is
> there a config file way to do this too?
> 

It can be controlled by both. There's a "vncviewer" option in config
file but command line option will override that if set.

If you look at parse_config_data you will see "vncviewer" is parsed and
assign to domain_create_info which is only used for domain creation.

> If it's because it is on the command line then we should be propagating
> that to the xl migrate-restore perhaps.
> 

auto-spawning a vncviewer on remote host? How is it useful for a user?
I don't have any prejudgement here so what I actually propose is just
providing a libxl abstraction of domain config file.

+libxl_domain_config_file = Struct("domain_config_file", [
+    ("vncviewer", bool),
+    ("domain_config", libxl_domain_config)
+    ])

> We could also have xl transmit some state of its own outside of the
> domain configuration, either by using the IDL to do it in JSON or some
> other mechanism.
> 
> I'm not sure about adding things to the libxl interface which libxl
> itself doesn't deal with just as a way to conveniently carry the
> information around.
> 

Libxl does use that. It needs those bits to rebuild domain.

Wei.

> Ian.

Re: [PATCH RFC V2 10/10] xl: introduce "xl-json" format

[Ian Campbell]

On Tue, 2014-04-22 at 11:25 +0100, Wei Liu wrote:
> On Tue, Apr 22, 2014 at 11:15:10AM +0100, Ian Campbell wrote:
> > On Sat, 2014-04-19 at 09:35 +0100, Wei Liu wrote:
> > > On Thu, Apr 17, 2014 at 12:13:11PM +0100, Wei Liu wrote:
> > > > Originally xl verbatimly copies the domain config file in its user data
> > > > store. This patch adds the functionality to transform text domain config
> > > > file to JSON object and save that object in user data store.
> > > > 
> > > > What this patch does:
> > > > 1. add a mandatory flag to save protocol to indicate whether the saved
> > > >    config is a JSON object
> > > > 2. register a new private data type "xl-json" in libxl.h
> > > > 3. modify xl to save / load "xl-json" file where necessary
> > > > 
> > > > After this change xl supports both "xl" format and "xl-json" format.
> > > > The user-supplied config file is still restricted to normal text config
> > > > file format ("xl"), as xl has more sanity checks when parsing text
> > > > config file. "xl-json" format is only used internally. The saved config
> > > > file is always in "xl-json" format.
> > > > 
> > > > Tests done so far (xl.{new,old} denotes xl with{,out} "xl_json"
> > > > support):
> > > > 1. xl.new create then hexdump saved file: domain config saved in JSON format
> > > > 2. xl.new create then xl.old restore: failed on mandatory flag check
> > > > 3. xl.new create then xl.new restore: succeeded
> > > > 4. xl.old create then xl.new restore: succeeded
> > > > 5. xl.new create then local migrate, receiving end xl.new: succeeded
> > > > 6. xl.old create then local migrate, receiving end xl.new: succeeded
> > > > 
> > > > The only drawback is that when restoring a domain, xl cannot
> > > > automatically spawn a vncviewer anymore. That's because that option is
> > > > part of domain_create info not a domain configuration thus it's not
> > > > saved in the JSON config.
> > > > 
> > > 
> > > On a second thought I might as well create abstraction of a config file
> > > in IDL. This might be more flexible in the long run. Then the burden
> > > will be whenever an option that doesn't belong to domain_config is
> > > added, IDL will also need to be taken care of.
> > 
> > Is the issue here that the vnc autospawning is controlled via the xl
> > command line (-A) rather than something in the configuration file? Or is
> > there a config file way to do this too?
> > 
> 
> It can be controlled by both. There's a "vncviewer" option in config
> file but command line option will override that if set.
> 
> If you look at parse_config_data you will see "vncviewer" is parsed and
> assign to domain_create_info which is only used for domain creation.

Oops, for some reason I searched for \"vnc (with the slash) when
escaping wasn't needed or correct so I missed it.

> > If it's because it is on the command line then we should be propagating
> > that to the xl migrate-restore perhaps.
> > 
> 
> auto-spawning a vncviewer on remote host? How is it useful for a user?
> I don't have any prejudgement here so what I actually propose is just
> providing a libxl abstraction of domain config file.

What would consume this abstraction? Why does it have to be part of
libxl?

> +libxl_domain_config_file = Struct("domain_config_file", [
> +    ("vncviewer", bool),
> +    ("domain_config", libxl_domain_config)
> +    ])
> 
> > We could also have xl transmit some state of its own outside of the
> > domain configuration, either by using the IDL to do it in JSON or some
> > other mechanism.
> > 
> > I'm not sure about adding things to the libxl interface which libxl
> > itself doesn't deal with just as a way to conveniently carry the
> > information around.
> > 
> 
> Libxl does use that. It needs those bits to rebuild domain.

libxl can autospawn a vncviewer? I thought that was an xl feature. If it
is actually a libxl feature then fine, but I think it makes most sense
as an xl feature.

Ian.

Re: [PATCH RFC V2 10/10] xl: introduce "xl-json" format

[Ian Jackson]

Ian Campbell writes ("Re: [PATCH RFC V2 10/10] xl: introduce "xl-json" format"):
> libxl can autospawn a vncviewer? I thought that was an xl feature. If it
> is actually a libxl feature then fine, but I think it makes most sense
> as an xl feature.

Yes, libxl can.  I think this is mad.

Ian.