[libvirt] [PATCH V2] Do not drop kernel cmdline for xen pv domains

Kernel cmdline args can be passed to xen pv domains even when a bootloader is specified. The current config-to-sxpr mapping ignores cmdline when bootloader is present. Since the xend sub-driver is used with many xen toolstack versions, this patch takes conservative approach of adding an else block to existing !def->os.bootloader, and only appends sxpr if def->os.cmdline is non-NULL. V2: Fix existing testcase broken by this patch and add new testcases --- src/xenxs/xen_sxpr.c | 6 ++++ .../sexpr2xml-pv-bootloader-cmdline.sexpr | 5 +++ .../sexpr2xml-pv-bootloader-cmdline.xml | 27 ++++++++++++++++++++ tests/sexpr2xmltest.c | 1 + .../xml2sexpr-disk-block-shareable.sexpr | 1 + .../xml2sexpr-pv-bootloader-cmdline.sexpr | 5 +++ .../xml2sexpr-pv-bootloader-cmdline.xml | 22 ++++++++++++++++ tests/xml2sexprtest.c | 1 + 8 files changed, 68 insertions(+), 0 deletions(-) create mode 100644 tests/sexpr2xmldata/sexpr2xml-pv-bootloader-cmdline.sexpr create mode 100644 tests/sexpr2xmldata/sexpr2xml-pv-bootloader-cmdline.xml create mode 100644 tests/xml2sexprdata/xml2sexpr-pv-bootloader-cmdline.sexpr create mode 100644 tests/xml2sexprdata/xml2sexpr-pv-bootloader-cmdline.xml diff --git a/src/xenxs/xen_sxpr.c b/src/xenxs/xen_sxpr.c index 13ca015..bd770bc 100644 --- a/src/xenxs/xen_sxpr.c +++ b/src/xenxs/xen_sxpr.c @@ -2261,6 +2261,12 @@ xenFormatSxpr(virConnectPtr conn, } virBufferAddLit(&buf, "))"); + } else { + /* PV domains accept kernel cmdline args */ + if (def->os.cmdline) { + virBufferEscapeSexpr(&buf, "(image (linux (args '%s')))", + def->os.cmdline); + } } for (i = 0 ; i < def->ndisks ; i++) diff --git a/tests/sexpr2xmldata/sexpr2xml-pv-bootloader-cmdline.sexpr b/tests/sexpr2xmldata/sexpr2xml-pv-bootloader-cmdline.sexpr new file mode 100644 index 0000000..dde37fc --- /dev/null +++ b/tests/sexpr2xmldata/sexpr2xml-pv-bootloader-cmdline.sexpr @@ -0,0 +1,5 @@ +(domain (domid 6)(name 'pvtest')(memory 420)(maxmem 420)(vcpus 2)\ +(uuid '596a5d2171f48fb2e068e2386a5c413e')(bootloader '/usr/bin/pygrub')\ +(bootloader_args '-q')(image (linux (args ' xenfb.video=8,1280,1024 ')))\ +(on_poweroff 'destroy')(on_reboot 'destroy')(on_crash 'destroy')\ +(device (vbd (dev 'xvda')(uname 'file:/root/some.img')(mode 'w')))) diff --git a/tests/sexpr2xmldata/sexpr2xml-pv-bootloader-cmdline.xml b/tests/sexpr2xmldata/sexpr2xml-pv-bootloader-cmdline.xml new file mode 100644 index 0000000..7bc99d8 --- /dev/null +++ b/tests/sexpr2xmldata/sexpr2xml-pv-bootloader-cmdline.xml @@ -0,0 +1,27 @@ +<domain type='xen' id='6'> + <name>pvtest</name> + <uuid>596a5d21-71f4-8fb2-e068-e2386a5c413e</uuid> + <memory>430080</memory> + <currentMemory>430080</currentMemory> + <vcpu>2</vcpu> + <bootloader>/usr/bin/pygrub</bootloader> + <bootloader_args>-q</bootloader_args> + <os> + <type>linux</type> + <cmdline> xenfb.video=8,1280,1024 </cmdline> + </os> + <clock offset='utc'/> + <on_poweroff>destroy</on_poweroff> + <on_reboot>destroy</on_reboot> + <on_crash>destroy</on_crash> + <devices> + <disk type='file' device='disk'> + <driver name='file'/> + <source file='/root/some.img'/> + <target dev='xvda' bus='xen'/> + </disk> + <console type='pty'> + <target type='xen' port='0'/> + </console> + </devices> +</domain> diff --git a/tests/sexpr2xmltest.c b/tests/sexpr2xmltest.c index f393935..26a987d 100644 --- a/tests/sexpr2xmltest.c +++ b/tests/sexpr2xmltest.c @@ -134,6 +134,7 @@ mymain(void) DO_TEST("pv-vfb-type-crash", "pv-vfb-type-crash", 3); DO_TEST("fv-autoport", "fv-autoport", 3); DO_TEST("pv-bootloader", "pv-bootloader", 1); + DO_TEST("pv-bootloader-cmdline", "pv-bootloader-cmdline", 1); DO_TEST("pv-vcpus", "pv-vcpus", 1); DO_TEST("disk-file", "disk-file", 2); diff --git a/tests/xml2sexprdata/xml2sexpr-disk-block-shareable.sexpr b/tests/xml2sexprdata/xml2sexpr-disk-block-shareable.sexpr index 8c0b1cd..b8387e5 100644 --- a/tests/xml2sexprdata/xml2sexpr-disk-block-shareable.sexpr +++ b/tests/xml2sexprdata/xml2sexpr-disk-block-shareable.sexpr @@ -1,6 +1,7 @@ (vm (name 'pvtest')(memory 384)(maxmem 512)(vcpus 1)\ (uuid '49a0c6ff-c066-5392-6498-3632d093c2e7')(bootloader '/usr/bin/pygrub')\ (on_poweroff 'destroy')(on_reboot 'restart')(on_crash 'restart')\ +(image (linux (args 'ro root=/dev/VolGroup00/LogVol00')))\ (device (tap (dev 'xvda')(uname 'tap:aio:/var/lib/xen/images/rhel5pv.img')\ (mode 'w!')))(device (vif (mac '00:16:3e:23:9e:eb')(bridge 'xenbr0')\ (script 'vif-bridge'))))\ diff --git a/tests/xml2sexprdata/xml2sexpr-pv-bootloader-cmdline.sexpr b/tests/xml2sexprdata/xml2sexpr-pv-bootloader-cmdline.sexpr new file mode 100644 index 0000000..c3ed727 --- /dev/null +++ b/tests/xml2sexprdata/xml2sexpr-pv-bootloader-cmdline.sexpr @@ -0,0 +1,5 @@ +(vm (name 'pvtest')(memory 420)(maxmem 420)(vcpus 2)\ +(uuid '596a5d21-71f4-8fb2-e068-e2386a5c413e')(bootloader '/usr/bin/pygrub')\ +(bootloader_args '-q')(on_poweroff 'destroy')(on_reboot 'destroy')\ +(on_crash 'destroy')(image (linux (args ' xenfb.video=8,1280,1024 ')))\ +(device (vbd (dev 'xvda')(uname 'file:/root/some.img')(mode 'w'))))\ diff --git a/tests/xml2sexprdata/xml2sexpr-pv-bootloader-cmdline.xml b/tests/xml2sexprdata/xml2sexpr-pv-bootloader-cmdline.xml new file mode 100644 index 0000000..983b7ce --- /dev/null +++ b/tests/xml2sexprdata/xml2sexpr-pv-bootloader-cmdline.xml @@ -0,0 +1,22 @@ +<domain type='xen' id='15'> + <name>pvtest</name> + <uuid>596a5d2171f48fb2e068e2386a5c413e</uuid> + <bootloader>/usr/bin/pygrub</bootloader> + <bootloader_args>-q</bootloader_args> + <os> + <type>linux</type> + <cmdline> xenfb.video=8,1280,1024 </cmdline> + </os> + <memory>430080</memory> + <vcpu>2</vcpu> + <on_poweroff>destroy</on_poweroff> + <on_reboot>destroy</on_reboot> + <on_crash>destroy</on_crash> + <devices> + <disk type='file' device='disk'> + <source file='/root/some.img'/> + <target dev='xvda'/> + </disk> + <console tty='/dev/pts/4'/> + </devices> +</domain> diff --git a/tests/xml2sexprtest.c b/tests/xml2sexprtest.c index d8cdcbb..e068e69 100644 --- a/tests/xml2sexprtest.c +++ b/tests/xml2sexprtest.c @@ -116,6 +116,7 @@ mymain(void) DO_TEST("pv-vfb-new", "pv-vfb-new", "pvtest", 3); DO_TEST("pv-vfb-new-auto", "pv-vfb-new-auto", "pvtest", 3); DO_TEST("pv-bootloader", "pv-bootloader", "pvtest", 1); + DO_TEST("pv-bootloader-cmdline", "pv-bootloader-cmdline", "pvtest", 1); DO_TEST("pv-vcpus", "pv-vcpus", "pvtest", 1); DO_TEST("disk-file", "disk-file", "pvtest", 2); -- 1.7.5.4

Jim Fehlig wrote:
Kernel cmdline args can be passed to xen pv domains even when a bootloader is specified. The current config-to-sxpr mapping ignores cmdline when bootloader is present.
Since the xend sub-driver is used with many xen toolstack versions, this patch takes conservative approach of adding an else block to existing !def->os.bootloader, and only appends sxpr if def->os.cmdline is non-NULL.
V2: Fix existing testcase broken by this patch and add new testcases
Hmm, now domainschematest is failing
--- src/xenxs/xen_sxpr.c | 6 ++++ .../sexpr2xml-pv-bootloader-cmdline.sexpr | 5 +++ .../sexpr2xml-pv-bootloader-cmdline.xml | 27 ++++++++++++++++++++ tests/sexpr2xmltest.c | 1 + .../xml2sexpr-disk-block-shareable.sexpr | 1 + .../xml2sexpr-pv-bootloader-cmdline.sexpr | 5 +++ .../xml2sexpr-pv-bootloader-cmdline.xml | 22 ++++++++++++++++ tests/xml2sexprtest.c | 1 + 8 files changed, 68 insertions(+), 0 deletions(-) create mode 100644 tests/sexpr2xmldata/sexpr2xml-pv-bootloader-cmdline.sexpr create mode 100644 tests/sexpr2xmldata/sexpr2xml-pv-bootloader-cmdline.xml create mode 100644 tests/xml2sexprdata/xml2sexpr-pv-bootloader-cmdline.sexpr create mode 100644 tests/xml2sexprdata/xml2sexpr-pv-bootloader-cmdline.xml
on these two xml files. If I'm reading domain.rng correctly, kernel must be specified?? Can it be optional like initrd and cmdline? Regards, Jim
diff --git a/src/xenxs/xen_sxpr.c b/src/xenxs/xen_sxpr.c index 13ca015..bd770bc 100644 --- a/src/xenxs/xen_sxpr.c +++ b/src/xenxs/xen_sxpr.c @@ -2261,6 +2261,12 @@ xenFormatSxpr(virConnectPtr conn, }
virBufferAddLit(&buf, "))"); + } else { + /* PV domains accept kernel cmdline args */ + if (def->os.cmdline) { + virBufferEscapeSexpr(&buf, "(image (linux (args '%s')))", + def->os.cmdline); + } }
for (i = 0 ; i < def->ndisks ; i++) diff --git a/tests/sexpr2xmldata/sexpr2xml-pv-bootloader-cmdline.sexpr b/tests/sexpr2xmldata/sexpr2xml-pv-bootloader-cmdline.sexpr new file mode 100644 index 0000000..dde37fc --- /dev/null +++ b/tests/sexpr2xmldata/sexpr2xml-pv-bootloader-cmdline.sexpr @@ -0,0 +1,5 @@ +(domain (domid 6)(name 'pvtest')(memory 420)(maxmem 420)(vcpus 2)\ +(uuid '596a5d2171f48fb2e068e2386a5c413e')(bootloader '/usr/bin/pygrub')\ +(bootloader_args '-q')(image (linux (args ' xenfb.video=8,1280,1024 ')))\ +(on_poweroff 'destroy')(on_reboot 'destroy')(on_crash 'destroy')\ +(device (vbd (dev 'xvda')(uname 'file:/root/some.img')(mode 'w')))) diff --git a/tests/sexpr2xmldata/sexpr2xml-pv-bootloader-cmdline.xml b/tests/sexpr2xmldata/sexpr2xml-pv-bootloader-cmdline.xml new file mode 100644 index 0000000..7bc99d8 --- /dev/null +++ b/tests/sexpr2xmldata/sexpr2xml-pv-bootloader-cmdline.xml @@ -0,0 +1,27 @@ +<domain type='xen' id='6'> + <name>pvtest</name> + <uuid>596a5d21-71f4-8fb2-e068-e2386a5c413e</uuid> + <memory>430080</memory> + <currentMemory>430080</currentMemory> + <vcpu>2</vcpu> + <bootloader>/usr/bin/pygrub</bootloader> + <bootloader_args>-q</bootloader_args> + <os> + <type>linux</type> + <cmdline> xenfb.video=8,1280,1024 </cmdline> + </os> + <clock offset='utc'/> + <on_poweroff>destroy</on_poweroff> + <on_reboot>destroy</on_reboot> + <on_crash>destroy</on_crash> + <devices> + <disk type='file' device='disk'> + <driver name='file'/> + <source file='/root/some.img'/> + <target dev='xvda' bus='xen'/> + </disk> + <console type='pty'> + <target type='xen' port='0'/> + </console> + </devices> +</domain> diff --git a/tests/sexpr2xmltest.c b/tests/sexpr2xmltest.c index f393935..26a987d 100644 --- a/tests/sexpr2xmltest.c +++ b/tests/sexpr2xmltest.c @@ -134,6 +134,7 @@ mymain(void) DO_TEST("pv-vfb-type-crash", "pv-vfb-type-crash", 3); DO_TEST("fv-autoport", "fv-autoport", 3); DO_TEST("pv-bootloader", "pv-bootloader", 1); + DO_TEST("pv-bootloader-cmdline", "pv-bootloader-cmdline", 1); DO_TEST("pv-vcpus", "pv-vcpus", 1);
DO_TEST("disk-file", "disk-file", 2); diff --git a/tests/xml2sexprdata/xml2sexpr-disk-block-shareable.sexpr b/tests/xml2sexprdata/xml2sexpr-disk-block-shareable.sexpr index 8c0b1cd..b8387e5 100644 --- a/tests/xml2sexprdata/xml2sexpr-disk-block-shareable.sexpr +++ b/tests/xml2sexprdata/xml2sexpr-disk-block-shareable.sexpr @@ -1,6 +1,7 @@ (vm (name 'pvtest')(memory 384)(maxmem 512)(vcpus 1)\ (uuid '49a0c6ff-c066-5392-6498-3632d093c2e7')(bootloader '/usr/bin/pygrub')\ (on_poweroff 'destroy')(on_reboot 'restart')(on_crash 'restart')\ +(image (linux (args 'ro root=/dev/VolGroup00/LogVol00')))\ (device (tap (dev 'xvda')(uname 'tap:aio:/var/lib/xen/images/rhel5pv.img')\ (mode 'w!')))(device (vif (mac '00:16:3e:23:9e:eb')(bridge 'xenbr0')\ (script 'vif-bridge'))))\ diff --git a/tests/xml2sexprdata/xml2sexpr-pv-bootloader-cmdline.sexpr b/tests/xml2sexprdata/xml2sexpr-pv-bootloader-cmdline.sexpr new file mode 100644 index 0000000..c3ed727 --- /dev/null +++ b/tests/xml2sexprdata/xml2sexpr-pv-bootloader-cmdline.sexpr @@ -0,0 +1,5 @@ +(vm (name 'pvtest')(memory 420)(maxmem 420)(vcpus 2)\ +(uuid '596a5d21-71f4-8fb2-e068-e2386a5c413e')(bootloader '/usr/bin/pygrub')\ +(bootloader_args '-q')(on_poweroff 'destroy')(on_reboot 'destroy')\ +(on_crash 'destroy')(image (linux (args ' xenfb.video=8,1280,1024 ')))\ +(device (vbd (dev 'xvda')(uname 'file:/root/some.img')(mode 'w'))))\ diff --git a/tests/xml2sexprdata/xml2sexpr-pv-bootloader-cmdline.xml b/tests/xml2sexprdata/xml2sexpr-pv-bootloader-cmdline.xml new file mode 100644 index 0000000..983b7ce --- /dev/null +++ b/tests/xml2sexprdata/xml2sexpr-pv-bootloader-cmdline.xml @@ -0,0 +1,22 @@ +<domain type='xen' id='15'> + <name>pvtest</name> + <uuid>596a5d2171f48fb2e068e2386a5c413e</uuid> + <bootloader>/usr/bin/pygrub</bootloader> + <bootloader_args>-q</bootloader_args> + <os> + <type>linux</type> + <cmdline> xenfb.video=8,1280,1024 </cmdline> + </os> + <memory>430080</memory> + <vcpu>2</vcpu> + <on_poweroff>destroy</on_poweroff> + <on_reboot>destroy</on_reboot> + <on_crash>destroy</on_crash> + <devices> + <disk type='file' device='disk'> + <source file='/root/some.img'/> + <target dev='xvda'/> + </disk> + <console tty='/dev/pts/4'/> + </devices> +</domain> diff --git a/tests/xml2sexprtest.c b/tests/xml2sexprtest.c index d8cdcbb..e068e69 100644 --- a/tests/xml2sexprtest.c +++ b/tests/xml2sexprtest.c @@ -116,6 +116,7 @@ mymain(void) DO_TEST("pv-vfb-new", "pv-vfb-new", "pvtest", 3); DO_TEST("pv-vfb-new-auto", "pv-vfb-new-auto", "pvtest", 3); DO_TEST("pv-bootloader", "pv-bootloader", "pvtest", 1); + DO_TEST("pv-bootloader-cmdline", "pv-bootloader-cmdline", "pvtest", 1); DO_TEST("pv-vcpus", "pv-vcpus", "pvtest", 1);
DO_TEST("disk-file", "disk-file", "pvtest", 2);

On 07/07/2011 05:37 PM, Jim Fehlig wrote:
Jim Fehlig wrote:
Kernel cmdline args can be passed to xen pv domains even when a bootloader is specified. The current config-to-sxpr mapping ignores cmdline when bootloader is present.
Since the xend sub-driver is used with many xen toolstack versions, this patch takes conservative approach of adding an else block to existing !def->os.bootloader, and only appends sxpr if def->os.cmdline is non-NULL.
V2: Fix existing testcase broken by this patch and add new testcases
Hmm, now domainschematest is failing
on these two xml files. If I'm reading domain.rng correctly, kernel must be specified?? Can it be optional like initrd and cmdline?
Reading domain_conf.c agrees with that interpretation. I would be fine with you squashing this in: diff --git i/docs/schemas/domain.rng w/docs/schemas/domain.rng index c01801e..b659da9 100644 --- i/docs/schemas/domain.rng +++ w/docs/schemas/domain.rng @@ -553,9 +553,11 @@ </define> <define name="osbootkernel"> <interleave> - <element name="kernel"> - <ref name="absFilePath"/> - </element> + <optional> + <element name="kernel"> + <ref name="absFilePath"/> + </element> + </optional> <optional> <element name="initrd"> <ref name="absFilePath"/> ACK, once you fix that and address Matthias' comment about spacing in the .xml file:
+ <cmdline> xenfb.video=8,1280,1024 </cmdline>
-- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org

Eric Blake wrote:
On 07/07/2011 05:37 PM, Jim Fehlig wrote:
Jim Fehlig wrote:
Kernel cmdline args can be passed to xen pv domains even when a bootloader is specified. The current config-to-sxpr mapping ignores cmdline when bootloader is present.
Since the xend sub-driver is used with many xen toolstack versions, this patch takes conservative approach of adding an else block to existing !def->os.bootloader, and only appends sxpr if def->os.cmdline is non-NULL.
V2: Fix existing testcase broken by this patch and add new testcases
Hmm, now domainschematest is failing
on these two xml files. If I'm reading domain.rng correctly, kernel must be specified?? Can it be optional like initrd and cmdline?
Reading domain_conf.c agrees with that interpretation. I would be fine with you squashing this in:
diff --git i/docs/schemas/domain.rng w/docs/schemas/domain.rng index c01801e..b659da9 100644 --- i/docs/schemas/domain.rng +++ w/docs/schemas/domain.rng @@ -553,9 +553,11 @@ </define> <define name="osbootkernel"> <interleave> - <element name="kernel"> - <ref name="absFilePath"/> - </element> + <optional> + <element name="kernel"> + <ref name="absFilePath"/> + </element> + </optional> <optional> <element name="initrd"> <ref name="absFilePath"/>
ACK, once you fix that and address Matthias' comment about spacing in the .xml file:
Squashed in your change to domain.rng, removed spaces noted by Matthias, and pushed. Regards, Jim

2011/7/8 Jim Fehlig <jfehlig@novell.com>:
Kernel cmdline args can be passed to xen pv domains even when a bootloader is specified. The current config-to-sxpr mapping ignores cmdline when bootloader is present.
Since the xend sub-driver is used with many xen toolstack versions, this patch takes conservative approach of adding an else block to existing !def->os.bootloader, and only appends sxpr if def->os.cmdline is non-NULL.
V2: Fix existing testcase broken by this patch and add new testcases ---
+++ b/tests/sexpr2xmldata/sexpr2xml-pv-bootloader-cmdline.xml @@ -0,0 +1,27 @@ +<domain type='xen' id='6'> + <name>pvtest</name> + <uuid>596a5d21-71f4-8fb2-e068-e2386a5c413e</uuid> + <memory>430080</memory> + <currentMemory>430080</currentMemory> + <vcpu>2</vcpu> + <bootloader>/usr/bin/pygrub</bootloader> + <bootloader_args>-q</bootloader_args> + <os> + <type>linux</type> + <cmdline> xenfb.video=8,1280,1024 </cmdline>
Are the leading and trailing spaces essential? If not I'd suggest to remove them. -- Matthias Bolte http://photron.blogspot.com
participants (3)
-
Eric Blake
-
Jim Fehlig
-
Matthias Bolte