Re: [Xen-devel] [PATCH LIBVIRT v2] libxl: Support cmdline= in xl config files

[Ian Campbell <ian.campbell@xxxxxxxxxx>]

On Wed, 2016-01-20 at 11:16 -0700, Jim Fehlig wrote:
> Ian Campbell wrote:
> > ... and consolidate the cmdline/extra/root parsing to facilitate doing
> > so.
> > 
> > The logic is the same as xl's parse_cmdline from the current xen.git
> > master
> > branch (e6f0e099d2c17de47fd86e817b1998db903cab61).
> > 
> > In order to introduce a use of VIR_WARN for logging I had to add
> > virerror.h and VIR_LOG_INIT.
> > 
> > Signed-off-by: Ian Campbell <ian.campbell@xxxxxxxxxx>
> > ---
> > NB: I was unsure if I was supposed to change VIR_FROM_NONE into
> > VIR_FROM_XEN, so I didn't (but will on advice).
> 
> It seems some of the VIR_FROM_ needs cleanup throughout the various 
> Xen-related
> files. We already have VIR_FROM_SEXPR and VIR_FROM_XENXM.
> src/xenconfig/xen_sxpr.c should use the former, src/xenconfig/xen_xm.c the
> latter. And given the pattern, I think we should introduce VIR_FROM_XENXL to
> cover xl.cfg related code. I can take care of this.

I've acked you patches about this.

> > +static int xenParseCmdline(virConfPtr conf, char **r_cmdline)
> > +{
> > +ÂÂÂÂchar *cmdline;
> 
> One too many initializers removed :-).
> 
> > +ÂÂÂÂconst char *root, *extra, *buf;
> > +
> > +ÂÂÂÂif (xenConfigGetString(conf, "cmdline", &buf, NULL) < 0)
> > +ÂÂÂÂÂÂÂÂreturn -1;
> > +
> > +ÂÂÂÂif (xenConfigGetString(conf, "root", &root, NULL) < 0)
> > +ÂÂÂÂÂÂÂÂreturn -1;
> > +
> > +ÂÂÂÂif (xenConfigGetString(conf, "extra", &extra, NULL) < 0)
> > +ÂÂÂÂÂÂÂÂreturn -1;
> > +
> > +ÂÂÂÂif (buf) {
> > +ÂÂÂÂÂÂÂÂif (VIR_STRDUP(cmdline, buf) < 0)
> > +ÂÂÂÂÂÂÂÂÂÂÂÂreturn -1;
> > +ÂÂÂÂÂÂÂÂif (root || extra)
> > +ÂÂÂÂÂÂÂÂÂÂÂÂVIR_WARN("ignoring root= and extra= in favour of
> > cmdline=");
> > +ÂÂÂÂ} else {
> > +ÂÂÂÂÂÂÂÂif (root && extra) {
> > +ÂÂÂÂÂÂÂÂÂÂÂÂif (virAsprintf(&cmdline, "root=%s %s", root, extra) < 0)
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂreturn -1;
> > +ÂÂÂÂÂÂÂÂ} else if (root) {
> > +ÂÂÂÂÂÂÂÂÂÂÂÂif (virAsprintf(&cmdline, "root=%s", root) < 0)
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂreturn -1;
> > +ÂÂÂÂÂÂÂÂ} else if (extra) {
> > +ÂÂÂÂÂÂÂÂÂÂÂÂif (VIR_STRDUP(cmdline, extra) < 0)
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂreturn -1;
> > +ÂÂÂÂÂÂÂÂ}
> > +ÂÂÂÂ}
> > +
> > +ÂÂÂÂ*r_cmdline = cmdline;
> 
> If none of cmdline, extra, or root are set in the config, def->os.cmdline gets
> set to garbage. xlconfigtest explodes when running 'make check'.

I even thought about this quite carefully but missed this case :-/

Would you prefer and = NULL on the decl or an else cmdline = NULL at the
end of that chain?

> Sorry for not mentioning it in my previous review, but we should add a
> test for cmdline, root, and extra.

Ack, will do so for v3.

Ian.

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