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

[Ian Jackson <Ian.Jackson@xxxxxxxxxxxxx>]

Wei Liu writes ("Re: [PATCH V4 19/24] xl: introduce and use "xl-json" format"):
> On Tue, May 06, 2014 at 03:26:42PM +0100, Ian Campbell wrote:
> > 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 think this is an essential feature.  The official interface to libxl
is the IDL, and the xl config file format is a very indirect (but more
convenient for humans) way of specifying that.  We should certainly
provide a way to use xl with a programmatically-generated config file.

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

OTOH it might profitably be in a separate patch.

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

Perhaps the code could be simplified by changing the types of
"config_file" et al to some struct containing the char* and the
format indicator.

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

I don't think it matters very much.

> > You can use the replace_string() helper for all these I think.
> > 
> > Except -- what about any explicitly configured script (ie. vif =
> > ['script=blargle']), does this not replace it with the default? Should
> > this be if !nic->script ?
> 
> Haven't thought about this. This is actually just code motion. I think
> you just discovered a bug. One more patch added to queue!

I think these defaults belong in libxl, actually.  IMO there shouldn't
be any validation or modification of the input json structure before
it is passed to libxl.

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

Yes.  Probably it would be best to remove it in a pre-patch to make
this patch smaller.

Ian.

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