Re: [Xen-devel] [PATCH V4 19/24] xl: introduce and use "xl-json" format

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

On Tue, 2014-05-06 at 16:17 +0100, Wei Liu wrote:
> > > diff --git a/docs/man/xl.pod.1 b/docs/man/xl.pod.1
> > > index 30bd4bf..f1a1b9b 100644
> > > --- a/docs/man/xl.pod.1
> > > +++ b/docs/man/xl.pod.1
> > > @@ -146,6 +146,11 @@ useful for determining issues with crashing domains 
> > > and just as a
> > >  general convenience since you often want to watch the
> > >  domain boot.
> > >  
> > > +=item B<-j>
> > > +
> > > +The provided I<configfile> is in JSON format. Cannot be used with 
> > > "key=value"
> > > +at the same time.
> > 
> > Do I understand correctly that the intention here is to introduce and
> > support JSON format configuration files generally, rather than only as
> > part of the migration protocol?
> > 
> 
> Yes. You understanding is correct.
> 
> > I don't necessarily object to this but we should we wary of adding new
> > things to support just because they appear to be easy to do as a side
> > effect of some other work, there is a maintenance burden associated with
> > adding this feature, which might potentially be substantial in the long
> > run.
> > 
> > Do we think there is much call for this feature?
> > 
> > (I'm not vetoing this, I just want to make sure we've thought it
> > through...)
> > 
> 
> IanJ seemed to fond of this so I added it here. But we can wait, I
> think. Next version will be much shorter without this. :-)

It certainly would be. IanJ -- thoughts?

> > (having read through the patch, I'm not sure how "free" this is -- the
> > use of config_in_json seems to have got its tendrils into a lot of
> > places)
> > 
> > > diff --git a/tools/libxl/libxl.h b/tools/libxl/libxl.h
> > > index c6a9a0d..970eba2 100644
> > > --- a/tools/libxl/libxl.h
> > > +++ b/tools/libxl/libxl.h
> > > @@ -1081,6 +1081,7 @@ void libxl_cpuid_set(libxl_ctx *ctx, uint32_t domid,
> > >   *
> > >   *  userid        Data contents
> > >   *  "xl"          domain config file in xl format, Unix line endings
> > > + *  "xl-json"     domain config in JSON format generated by xl
> > 
> > "generated by libxl" I think. Could even call it "libxl-json".
> > 
> 
> In my original design this file is actually a JSON representation of xl
> config file, but it's not the case anymore. So I think calling it
> libxl-json is sufficient.
> 
> On the flip side this file is produced by xl (for now) and only used by
> xl only, so I think "xl-json" is also approriate.
> 
> Not sure which one is better.

Me neither. I suppose the question is would any other toolstack which
wanted to save a libxl_domain_config in this way want to use the same
userid or a different one?

By using a common one could we perhaps fix the issue of not being able
to poke at e.g. libvirt domain using xl? (specifically things like xl
list -v on a system with a mix of domains does odd things IIRC).

Perhaps storing (and all the updating) this thing is something which
libxl rather than xl should take care of? With a function
libxl_domain_fetch_config(ctx, domid, &libxl_domain_config)? 

> > > +
> > > +    nic_update_default(d_config);
> > > +}
> > > +
> > >  static void parse_config_data(const char *config_source,
> > >                                const char *config_data,
> > >                                int config_len,
> > > @@ -930,6 +1000,8 @@ static void parse_config_data(const char 
> > > *config_source,
> > >          b_info->rtc_timeoffset = l;
> > >  
> > >      if (dom_info && !xlu_cfg_get_long(config, "vncviewer", &l, 0)) {
> > > +        fprintf(stderr, "WARNING: \"vncviewer\" option found. It might 
> > > be removed in future release. "
> > > +            "Use \"-V\" option of \"xl create\" to automatically spawn 
> > > vncviewer.\n");
> > 
> > We should either decide this is deprecated or not "might be removed in
> > the future" is not a helpful thing to say I don't think. It either will
> > or it won't be removed. Are we feeling brave enough to just remove it?
> > 
> 
> I think it's quite safe to remove it. This functional regression is in
> no way critical and can be easily worked around.

Lets do that then, in a separate patch with an obvious title so it isn't
going in "under the radar".

Ian.


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