Re: [Xen-devel] [PATCH 2/3] libxl_types.idl: use empty Struct for invalid domain type

[Wei Liu <wei.liu2@xxxxxxxxxx>]

On Thu, Apr 10, 2014 at 03:24:46PM +0100, Ian Campbell wrote:
> On Thu, 2014-04-10 at 14:00 +0100, Wei Liu wrote:
> > On Thu, Apr 10, 2014 at 01:45:15PM +0100, Ian Campbell wrote:
> > > On Thu, 2014-04-10 at 13:43 +0100, Wei Liu wrote:
> > > > Using None makes gentypes.py generates a C function which generates
> > > > nothing. Eventually the generated JSON string is malformed.
> > > 
> > > I'd prefer to fix the generator to not do this rather than muddying the
> > > input description. How hard would it be?
> > > 
> > 
> > It certainly yields a patch much larger than this one-liner. The way to
> > fix generator is when parsing keyed-union we need to 1) get hold of the
> > key, 2) get its "invalid" value, 3) skip code generation if key points
> > to "invalid".
> 
> Can you point me to the generate code which wrong? What I'm seeing in
> e.g. libxl_domain_build_info_gen_json is:
>     s = yajl_gen_map_open(hand);
>       ...

[ generate a map key called "u" ]

>     switch (p->type) {
>     case LIBXL_DOMAIN_TYPE_HVM:
>         s = yajl_gen_map_open(hand);
>               ...
>         s = yajl_gen_map_close(hand);
>         if (s != yajl_gen_status_ok)
>             goto out;
>         break;
>     case LIBXL_DOMAIN_TYPE_PV:
>         s = yajl_gen_map_open(hand);
>               ...
>         s = yajl_gen_map_close(hand);
>         if (s != yajl_gen_status_ok)
>             goto out;
>         break;
>     case LIBXL_DOMAIN_TYPE_INVALID:
>         break;

Here it doesn't generate value for map "u".
   "u"(end of output) -- malformed JSON

With this change now it generate an empty map as the value of "u".

   "u": { } -- correct JSON

>     }
>     s = yajl_gen_map_close(hand);
> 
> Does that produce invalid json somehow?
> 
> And if it did could we not detect the use of None with an explicit check

gentypes does that already. The result is not generating anything.

> in the generator somewhere rather than worrying about whether a given
> enum value is valid or invalid? Relying on INVALID would be wrong

In the above example we need to skip generation of "u" if we encounter
None or "INVALID". That's back referencing parent in recursive routines
which requires more complex changes.

> anyway. It is in theory legitimate for a valid value to have no extra
> data in the enum -- a bunch of the entries in libxl_event could be so
> for example.
> 
> > We don't have definition in IDL for "invalid" value yet, so we need to
> > add more code to get 2) working.
> > 
> > I only discovered this when I run testidl. There's no existing users of
> > the generation function in libxl, so I think this fix is quite safe.
> 
> At least libxl_domain_config_gen_json is used (by xl).

But when you create a domain the domain type will not be "INVALID".
That's why it doesn't fail I presume. I only discoverd this when running
testidl.

What's the possible downside you see with this change?

Wei.

> 
> Ian.

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