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

[Wei Liu <wei.liu2@xxxxxxxxxx>]

On Wed, May 07, 2014 at 11:11:09AM +0100, Ian Campbell wrote:
> 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?
> 

I am not sure if any other toolstack will use the same format. libvirt
has its own entry called "libvirt-xml". 

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

Not really, at least not without making libvirt use the same stored
data, or not without xl being able to consume libvirt-xml.

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

As I stated in 00 I wasn't sure about this either. It's a design
decision which needs more input.

One thing that makes me feel weird is that having libxl to manage domain
state -- that means a library is managing domain state? My gut feeling
is that it should be a "tool" not a "library" that manages domain
states.

Wei.

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