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

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

On Thu, 2014-04-10 at 15:41 +0100, Wei Liu wrote:
> 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.

So make it generate something? That's just a map_open/map_close isn't
it?

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

Which would be needed to push generating map "u" down into the switch?
OK to avoiding that then.

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

It's a pointless empty struct specified in the idl which I'd rather
avoid.

Ian.


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