Re: [libvirt] [PATCH LIBVIRT v2] libxl: Support cmdline= in xl config files

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(a)citrix.com&gt; > --- > 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.

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

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