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

[Jim Fehlig <jfehlig@suse.com>]

Ian Campbell wrote:
> 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@citrix.com>
>>> ---
>>> 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.

Thanks. I'll push those trivial patches shortly.

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

'char *cmdline = NULL;' is fine.

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

Thanks!

Regards,
Jim