Re: [Xen-devel] [PATCH v3 18/19] libxlutil: nested list support

[Ian Jackson <Ian.Jackson@xxxxxxxxxxxxx>]

Wei Liu writes ("Re: [PATCH v3 18/19] libxlutil: nested list support"):
> On Tue, Jan 13, 2015 at 03:52:48PM +0000, Ian Jackson wrote:
> > This commit message is very brief.  For example, under the heading of
> > `Rework internal representation of setting' I would expect a clear
> > description of every formulaic change.
> 
> Originally the internal representation of setting is (string, string)
> pair, the first string being the name of the setting, second string
> being the value of the setting. Now the internal is changed to (string,
> ConfigValue) pair, where ConfigValue can refer to a string or a list of
> ConfigValue's. Internal functions to deal with setting are changed
> accordingly.
> 
> Does the above description makes things clearer?

Yes.  Something like that should be in the commit message.  It would
help to refer to the actual type names.  You could say (if true) for
example, "internal functions new refer to a ConfigSetting; the public
APIs still talk about ConfigValues" or some such.

> > Also, I think would be much easier to review if split up into 3 parts,
> > which from the description above ought to be doable without trouble.
> 
> OK. I can try to split this patch into three.

If it's difficult for some reason then do get back to me.

> > AFAICT from your changes, the API is not backward compatible.  ICBW,
> > but if I'm right that's not acceptable I'm afraid, even in libxlu.
> 
> The old APIs still have the same semantic as before, so any applications
> linked against those APIs still have the same results returned.

Oh yes.  Sorry, I had misread the patch and read your changes to
libxlu_internal.h as being in libxlutil.h.

> > > Previous APIs work as before.
> > 
> > That can't be right because you have to at least specify how they deal
> > with the additional config file syntax.
> 
> No, the old APIs don't deal with new syntax. If applications want to
> support new syntax, they need to use new API.

It's obvious that the new API can't return the new syntax.  The
question is what happens if you try.

> If old APIs try to get value from new syntax, it has no effect.

I don't think "has no effect" can be right.  What is the actual return
value from the function ?  Will it be treated as an error ?  (IMO it
should be, and this should be documented.)

Ian.

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