Re: [Xen-devel] [PATCH 1 of 6] libxl: IDL: autogenerate functions to produce JSON from libxl data structures

[Ian Jackson <Ian.Jackson@xxxxxxxxxxxxx>]

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.

> +    elif isinstance(ty, libxltypes.Struct) and (parent is None or ty.json_fn 
> is None):

Do we care about these >80-column lines ?

>      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:

> +      /* 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 ?

> 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 ?

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

> 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 ?

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

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

Ian.

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel