| Message ID | 1403018646-13890-1-git-send-email-ian.campbell@citrix.com |
|---|---|
| State | New |
| Headers | show |
On 06/17/2014 09:24 AM, Ian Campbell 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> > --- > .gnulib | 2 +- > src/xenxs/xen_xm.c | 14 +++++++++++++- > 2 files changed, 14 insertions(+), 2 deletions(-) > > diff --git a/.gnulib b/.gnulib > index d55899f..e8e0eb6 160000 > --- a/.gnulib > +++ b/.gnulib > @@ -1 +1 @@ > -Subproject commit d55899fd2c5794ac85ecb14d5e2f646a89e4b4dd > +Subproject commit e8e0eb6bfb728685ec8d5afd924e41b18e9d928d Was the submodule bump intended? > diff --git a/src/xenxs/xen_xm.c b/src/xenxs/xen_xm.c > index b2db97d..2cd6d4c 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, NULL) < 0) > + goto cleanup; > + if (xenXMConfigGetString(conf, "root", &root, NULL) < 0) > goto cleanup; What's the difference between xenXMConfigCopyStringOpt and xenXMConfigGetString? Once I understand that, then this patch (minus the .gnulib bump) seems okay. > + > + 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, >
Eric Blake wrote: > On 06/17/2014 09:24 AM, Ian Campbell 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> >> --- >> .gnulib | 2 +- >> src/xenxs/xen_xm.c | 14 +++++++++++++- >> 2 files changed, 14 insertions(+), 2 deletions(-) >> >> diff --git a/.gnulib b/.gnulib >> index d55899f..e8e0eb6 160000 >> --- a/.gnulib >> +++ b/.gnulib >> @@ -1 +1 @@ >> -Subproject commit d55899fd2c5794ac85ecb14d5e2f646a89e4b4dd >> +Subproject commit e8e0eb6bfb728685ec8d5afd924e41b18e9d928d >> > > Was the submodule bump intended? > > >> diff --git a/src/xenxs/xen_xm.c b/src/xenxs/xen_xm.c >> index b2db97d..2cd6d4c 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, NULL) < 0) >> + goto cleanup; >> + if (xenXMConfigGetString(conf, "root", &root, NULL) < 0) >> goto cleanup; >> > > What's the difference between xenXMConfigCopyStringOpt and > xenXMConfigGetString? The first thing both do is set the pointer in the third param to NULL. If the conf setting specified by the second param does not exist or is empty, both will return success. The only difference is xenXMConfigGetString will set the pointer in the third param to the value provided in the fourth param. Oh, and xenXMConfigGetString doesn't alloc any memory, so no need to free 'extra' and 'root'. > Once I understand that, then this patch (minus > the .gnulib bump) seems okay. > Nod, I think it is fine after removing the gnulib hunk. I tested with the old Xen driver too. BTW, if <cmdline> contains root=, I noticed that domxml-to-native will put it in extra= instead of creating a root= entry. E.g. <cmdline>root=/dev/bla foo=bar</cmdline> converts to extra="root=/dev/bla foo=bar", which is still valid config so perhaps not such a big deal. Regards, Jim > >> + >> + 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 Tue, 2014-06-17 at 15:36 -0600, Jim Fehlig wrote: > Eric Blake wrote: > > On 06/17/2014 09:24 AM, Ian Campbell 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> > >> --- > >> .gnulib | 2 +- > >> src/xenxs/xen_xm.c | 14 +++++++++++++- > >> 2 files changed, 14 insertions(+), 2 deletions(-) > >> > >> diff --git a/.gnulib b/.gnulib > >> index d55899f..e8e0eb6 160000 > >> --- a/.gnulib > >> +++ b/.gnulib > >> @@ -1 +1 @@ > >> -Subproject commit d55899fd2c5794ac85ecb14d5e2f646a89e4b4dd > >> +Subproject commit e8e0eb6bfb728685ec8d5afd924e41b18e9d928d > >> > > > > Was the submodule bump intended? No, sorry, I've no idea how that happened (/me curses git submodules yet again). > > Once I understand that, then this patch (minus > > the .gnulib bump) seems okay. NB I just sent out a v2 -- extra should default to "" not NULL for this to work as intended. > BTW, if <cmdline> contains root=, I noticed that domxml-to-native will > put it in extra= instead of creating a root= entry. E.g. > <cmdline>root=/dev/bla foo=bar</cmdline> converts to > extra="root=/dev/bla foo=bar", which is still valid config so perhaps > not such a big deal. I think this is fine. Personally I consider the root= stuff to be a weird wart, in that it effectively exposes details of the Linux command line syntax in the xm/xl cfg file. It's far better IMHO to ignore it and write root=foo in the actual command line. Ian.
diff --git a/.gnulib b/.gnulib index d55899f..e8e0eb6 160000 --- a/.gnulib +++ b/.gnulib @@ -1 +1 @@ -Subproject commit d55899fd2c5794ac85ecb14d5e2f646a89e4b4dd +Subproject commit e8e0eb6bfb728685ec8d5afd924e41b18e9d928d diff --git a/src/xenxs/xen_xm.c b/src/xenxs/xen_xm.c index b2db97d..2cd6d4c 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, NULL) < 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,
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> --- .gnulib | 2 +- src/xenxs/xen_xm.c | 14 +++++++++++++- 2 files changed, 14 insertions(+), 2 deletions(-)