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

[Wei Liu <wei.liu2@xxxxxxxxxx>]

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

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.

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