|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH 1 of 6] libxl: IDL: autogenerate functions to produce JSON from libxl data structures
On Thu, 2011-10-06 at 15:55 +0100, Ian Jackson wrote:
> Ian Campbell writes ("[Xen-devel] [PATCH 1 of 6] libxl: IDL: autogenerate
> functi> libxl: IDL: autogenerate functions to produce JSON from libxl data
> structures.
> ...
> > Also update testidl to generate a random version of each IDL datastructure
> > and
> > convert it to JSON. Unfortunately this requires a libxl_ctx and therefore
> > the
> > test must be run on a Xen system now.
>
> Perhaps we should have a new version of libxl_ctx_alloc which doesn't
> attempt to connect to xenstore etc., for these kind of purposes. This
> might turn out to be useful for xl's command line handling too.
>
> Not essential for this patch, though.
I considered adding a new flag to enable this behaviour, but then I
realised that libxl_ctx_alloc doesn't have a flag parameter... Perhaps
we should add one?
>
> > + elif isinstance(ty, libxltypes.Struct) and (parent is None or
> > ty.json_fn is None):
>
> Do we care about these >80-column lines ?
I suppose we should. The IDL stuff isn't totally 80-column clean right
now but it is close so I may as well clean it up.
>
> > f.write("""
> > #include <stdio.h>
> > +#include <stdlib.h>
> > +#include <string.h>
> > +
> > #include \"libxl.h\"
> > +#include \"libxl_utils.h\"
>
> AIU Python's quoting rules, these \'s are unnecessary. And indeed
> later we have:
Will fix.
>
> > + /* A random selection from libxl_cpuid_parse_config */
> > + {"maxleaf", 32},
>
> > +static void rand_bytes(uint8_t *p, size_t sz)
> > +{
> > + int i;
> > + for (i=0; i<sz; i++)
> > + p[i] = rand() % 256;
> > + //p[i] = i;
>
> This line is leftover cruft ?
Yes.
>
> > diff -r fb42038b1f5c -r 030e844fcceb tools/libxl/gentypes.py
> > --- a/tools/libxl/gentypes.py Thu Sep 29 16:57:52 2011 +0100
> > +++ b/tools/libxl/gentypes.py Thu Sep 29 17:02:14 2011 +0100
> > @@ -29,7 +29,6 @@ def libxl_C_instance_of(ty, instancename
>
> I haven't reviewed this in detail but I have instead looked at the
> output:
>
> > yajl_gen_status libxl_uuid_gen_json(yajl_gen hand,
> > libxl_uuid *p);
>
> Are we confident that these functions will never need to take a
> libxl_ctx ?
My thinking was that these should be consistent with the interface
provided by the equivalent yajl functions (yajl_gen_integer etc).
Partly because otherwise I need to track which types need a ctx and
which don't.
I can't think of any reason a ctx would be needed, any allocations made
would need to be of the type which could be returned to a caller so they
won't be using the gc.
>
> > yajl_gen_status libxl_disk_format_gen_json(yajl_gen hand, libxl_disk_format
> > *p)
> > {
> > yajl_gen_status s;
> > {
> > const char *se = libxl_disk_format_to_string(*p);
> > if (se)
> > s = yajl_gen_string(hand, (const unsigned char *)se,
> > strlen(se));
> > else
> > s = yajl_gen_null(hand);
> > if (s != yajl_gen_status_ok)
> > goto out;
> > }
> > out:
> > return s;
> > }
>
> There are quite a few functions which all look like this. Perhaps the
> bulk of this function should be a helper function, so you end up with
> something like this:
>
> yajl_gen_status libxl_disk_format_gen_json(yajl_gen hand, libxl_disk_format
> *p)
> {
> return libxl__yajl_gen_enum(hand, libxl_disk_format_to_string(*p));
> }
>
> That might reduce the size of the compiled code quite a bit. IME
> autogenerated code can get very big if one isn't careful.
Yes, I'll try this.
> > diff -r fb42038b1f5c -r 030e844fcceb tools/libxl/idl.txt
> > --- a/tools/libxl/idl.txt Thu Sep 29 16:57:52 2011 +0100
> > +++ b/tools/libxl/idl.txt Thu Sep 29 17:02:14 2011 +0100
> ...
> > +yajl_gen_status libxl_cpuid_policy_list_gen_json(yajl_gen hand,
> > + libxl_cpuid_policy_list *pcpuid)
> > +{
>
> This rather ad-hoc treatment of cpuid policies isn't ideal I think.
> Did we want to try to recast them as some more general aggregate ?
We have a smallish number of "builtin" types which really ought to be
moved into the IDL, but which are generally complex and need more
thought (other than cpuid we have cpumaps, topology info, hwcaps etc).
The cpuid case is interesting because the libxl_cpuid_policy(_list)
types are actually opaque typedefs of libxl__cpuid_policy and so are not
part of the public IDL. We do now have the internal IDL which could be
used and libxl_cpuid_policy_list_gen_json would become a wrapper of
sorts for libxl__cpuid_policy_gen_json. Even that is more complex than
it sounds since the actual type is currently not using the richer
semantic names like "leaf" and "subleaf" or "e*x" but rather has
input[2] and policy[4].
I'm not especially keen to tackle all that right now though.
> > + 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-edx',
> > + * 'ebx': 'filter',
> > + * 'ecx': 'filter',
> > + * 'edx': 'filter' }, ],
> > + * { 'leaf': 'val-eax', ..., 'eax': 'filter', ... },
> > + * ... etc ...
> > + * }
> > + */
>
> Mismatched brackets, or confused indenting, in the comment.
Will fix. Once I figure out what I meant...
> > diff -r fb42038b1f5c -r 030e844fcceb tools/libxl/libxl_json.h
> > --- /dev/null Thu Jan 01 00:00:00 1970 +0000
> > +++ b/tools/libxl/libxl_json.h Thu Sep 29 17:02:14 2011 +0100
> ...
> > +yajl_gen_status libxl_uuid_gen_json(yajl_gen hand,
> > + libxl_uuid *p);
>
> Shouldn't these declarations be produced automatically by the IDL
> compiler ? After all it's going to generate calls to these functions.
It's somewhat consistent with how we deal with these "builtin" types in
other places (and simpler in gentypes.py) to declare these by hand. I'll
see if I can make it work though.
Thanks for the review.
Ian.
>
> Ian.
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |