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

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

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@xxxxxxxxxx>

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.


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel