| Message ID | 1403081925.25771.62.camel@kazak.uk.xensource.com |
|---|---|
| State | Accepted |
| Commit | ac63014cfd75cdc4b9c9533899c593044aed8e1a |
| Headers | show |
Ian Campbell wrote: > On Tue, 2014-06-17 at 16:24 +0100, Ian Campbell wrote: > >> + if (xenXMConfigGetString(conf, "extra", &extra, NULL) < 0) >> > > This was subtly broken. The default needs to be "". > > -----8<------ > > >From 539412a6deac8b928c82945d692ef20a49535d65 Mon Sep 17 00:00:00 2001 > From: Ian Campbell <ian.campbell@citrix.com> > Date: Tue, 17 Jun 2014 15:48:48 +0100 > Subject: [PATCH] xen: handle root= in xen-xm configuration files. > > In addition to extra= xm supported a root= option which was supposed > to be incorporated into the final command line. Handle that for "virsh > domxml-from-native xen-xm". Tested with the libxl backend. > > Signed-off-by: Ian Campbell <ian.campbell@citrix.com> > --- > v2: extra should default to "". > --- > .gnulib | 2 +- > > ---> WTF. I stripped this out of the patch shown below... > > src/xenxs/xen_xm.c | 14 +++++++++++++- > 2 files changed, 14 insertions(+), 2 deletions(-) > ACK and pushed; thanks! Regards, Jim > diff --git a/src/xenxs/xen_xm.c b/src/xenxs/xen_xm.c > index b2db97d..745041b 100644 > --- a/src/xenxs/xen_xm.c > +++ b/src/xenxs/xen_xm.c > @@ -339,6 +339,8 @@ xenParseXM(virConfPtr conf, int xendConfigVersion, > def->os.nBootDevs++; > } > } else { > + const char *extra, *root; > + > if (xenXMConfigCopyStringOpt(conf, "bootloader", &def->os.bootloader) < 0) > goto cleanup; > if (xenXMConfigCopyStringOpt(conf, "bootargs", &def->os.bootloaderArgs) < 0) > @@ -348,8 +350,18 @@ xenParseXM(virConfPtr conf, int xendConfigVersion, > goto cleanup; > if (xenXMConfigCopyStringOpt(conf, "ramdisk", &def->os.initrd) < 0) > goto cleanup; > - if (xenXMConfigCopyStringOpt(conf, "extra", &def->os.cmdline) < 0) > + if (xenXMConfigGetString(conf, "extra", &extra, "") < 0) > + goto cleanup; > + if (xenXMConfigGetString(conf, "root", &root, NULL) < 0) > goto cleanup; > + > + if (root) { > + if (virAsprintf(&def->os.cmdline, "root=%s %s", root, extra) < 0) > + goto cleanup; > + } else { > + if (VIR_STRDUP(def->os.cmdline, extra) < 0) > + goto cleanup; > + } > } > > if (xenXMConfigGetULongLong(conf, "memory", &def->mem.cur_balloon, >
On 06/18/2014 11:46 AM, Jim Fehlig wrote: >> >> In addition to extra= xm supported a root= option which was supposed >> to be incorporated into the final command line. Handle that for "virsh >> domxml-from-native xen-xm". Tested with the libxl backend. >> >> Signed-off-by: Ian Campbell <ian.campbell@citrix.com> >> --- >> v2: extra should default to "". >> --- >> .gnulib | 2 +- >> >> ---> WTF. I stripped this out of the patch shown below... >> >> src/xenxs/xen_xm.c | 14 +++++++++++++- >> 2 files changed, 14 insertions(+), 2 deletions(-) >> > > ACK and pushed; thanks! I'm now seeing test failures on Fedora 20, non-xen setup: TEST: xmconfigtest 1) Xen XM-2-XML Parse paravirt-old-pvfb ... OK 2) Xen XM-2-XML Format paravirt-old-pvfb ... Offset 324 Expect [<] Actual [ <cmdline></cmdline> <] ... FAILED 3) Xen XM-2-XML Parse paravirt-old-pvfb-vncdisplay ... OK 4) Xen XM-2-XML Format paravirt-old-pvfb-vncdisplay ... Offset 324 Expect [<] Actual [ <cmdline></cmdline> <] ... FAILED 5) Xen XM-2-XML Parse paravirt-new-pvfb ... OK and several others. Looks like something in the code is outputting an empty <cmdline> element when it is not needed.
Eric Blake wrote: > On 06/18/2014 11:46 AM, Jim Fehlig wrote: > > >>> In addition to extra= xm supported a root= option which was supposed >>> to be incorporated into the final command line. Handle that for "virsh >>> domxml-from-native xen-xm". Tested with the libxl backend. >>> >>> Signed-off-by: Ian Campbell <ian.campbell@citrix.com> >>> --- >>> v2: extra should default to "". >>> --- >>> .gnulib | 2 +- >>> >>> ---> WTF. I stripped this out of the patch shown below... >>> >>> src/xenxs/xen_xm.c | 14 +++++++++++++- >>> 2 files changed, 14 insertions(+), 2 deletions(-) >>> >>> >> ACK and pushed; thanks! >> > > I'm now seeing test failures on Fedora 20, non-xen setup: > > TEST: xmconfigtest > I pushed a fix for this https://www.redhat.com/archives/libvir-list/2014-June/msg00857.html Regards, Jim
Ian Campbell wrote: > On Tue, 2014-06-17 at 16:24 +0100, Ian Campbell wrote: > >> + if (xenXMConfigGetString(conf, "extra", &extra, NULL) < 0) >> > > This was subtly broken. The default needs to be "". > Turns out, it wasn't :). Prior to this patch, def->os.cmdline was set to NULL if 'extra' did not exist or was empty. I pushed a fix to restore the behavior, as your original patch did https://www.redhat.com/archives/libvir-list/2014-June/msg00857.html Regards, Jim
On Thu, 2014-06-19 at 00:15 -0600, Jim Fehlig wrote: > Ian Campbell wrote: > > On Tue, 2014-06-17 at 16:24 +0100, Ian Campbell wrote: > > > >> + if (xenXMConfigGetString(conf, "extra", &extra, NULL) < 0) > >> > > > > This was subtly broken. The default needs to be "". > > > > Turns out, it wasn't :). Prior to this patch, def->os.cmdline was set > to NULL if 'extra' did not exist or was empty. I pushed a fix to > restore the behavior, as your original patch did > > https://www.redhat.com/archives/libvir-list/2014-June/msg00857.html Sorry about that and thanks for the fix. I see VIR_STRDUP actually does handle NULL input correctly, which was what I was worried about. Ian.
On 06/24/2014 03:20 AM, Ian Campbell wrote: > > I see VIR_STRDUP actually does handle NULL input correctly, which was > what I was worried about. Yes, it is by design that VIR_STRDUP(NULL) works, and gives a different return value (0) than when dup'ing a string (positive) or on failure (negative).
diff --git a/src/xenxs/xen_xm.c b/src/xenxs/xen_xm.c index b2db97d..745041b 100644 --- a/src/xenxs/xen_xm.c +++ b/src/xenxs/xen_xm.c @@ -339,6 +339,8 @@ xenParseXM(virConfPtr conf, int xendConfigVersion, def->os.nBootDevs++; } } else { + const char *extra, *root; + if (xenXMConfigCopyStringOpt(conf, "bootloader", &def->os.bootloader) < 0) goto cleanup; if (xenXMConfigCopyStringOpt(conf, "bootargs", &def->os.bootloaderArgs) < 0) @@ -348,8 +350,18 @@ xenParseXM(virConfPtr conf, int xendConfigVersion, goto cleanup; if (xenXMConfigCopyStringOpt(conf, "ramdisk", &def->os.initrd) < 0) goto cleanup; - if (xenXMConfigCopyStringOpt(conf, "extra", &def->os.cmdline) < 0) + if (xenXMConfigGetString(conf, "extra", &extra, "") < 0) + goto cleanup; + if (xenXMConfigGetString(conf, "root", &root, NULL) < 0) goto cleanup; + + if (root) { + if (virAsprintf(&def->os.cmdline, "root=%s %s", root, extra) < 0) + goto cleanup; + } else { + if (VIR_STRDUP(def->os.cmdline, extra) < 0) + goto cleanup; + } } if (xenXMConfigGetULongLong(conf, "memory", &def->mem.cur_balloon,