[libvirt] [PATCH v2 0/4] qemu: handle ',' and '=' in VM name
This series adds qemu cli comma escaping to several places that are dependent on the VM name, to enable names with embedded commas. Patch 4 makes use of qemu -name guest=X value to allow names with '=' in them. https://bugzilla.redhat.com/show_bug.cgi?id=639926 https://bugzilla.redhat.com/show_bug.cgi?id=1276485 Note: There's likely other places that are VM name dependent that need comma escaping too, but this hits the mandatory ones. I've listed some more on the BiteSizedTasks page: http://wiki.libvirt.org/page/BiteSizedTasks#qemu:_Use_comma_escaping_for_mor... v2: Rebase to master Cole Robinson (4): qemu: command: escape commas in VM name qemu: command: escape commas in secret master path qemu: command: escape commas in chardev socket path qemu: command: Use -name guest= if available src/qemu/qemu_capabilities.c | 2 ++ src/qemu/qemu_capabilities.h | 1 + src/qemu/qemu_command.c | 22 ++++++++++++-------- tests/qemucapabilitiesdata/caps_2.1.1-1.caps | 1 + tests/qemucapabilitiesdata/caps_2.4.0-1.caps | 1 + tests/qemucapabilitiesdata/caps_2.5.0-1.caps | 1 + tests/qemucapabilitiesdata/caps_2.6.0-1.caps | 1 + .../qemuxml2argvdata/qemuxml2argv-name-escape.args | 24 ++++++++++++++++++++++ .../qemuxml2argvdata/qemuxml2argv-name-escape.xml | 18 ++++++++++++++++ tests/qemuxml2argvtest.c | 2 ++ 10 files changed, 65 insertions(+), 8 deletions(-) create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-name-escape.args create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-name-escape.xml -- 2.7.4
This isn't sufficient on its own, since the VM name is used for things like monitor paths, which we don't escape yet --- src/qemu/qemu_command.c | 2 +- .../qemuxml2argvdata/qemuxml2argv-name-escape.args | 24 ++++++++++++++++++++++ .../qemuxml2argvdata/qemuxml2argv-name-escape.xml | 18 ++++++++++++++++ tests/qemuxml2argvtest.c | 2 ++ 4 files changed, 45 insertions(+), 1 deletion(-) create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-name-escape.args create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-name-escape.xml diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 08c6c61..b13df49 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -6788,7 +6788,7 @@ qemuBuildNameCommandLine(virCommandPtr cmd, virCommandAddArg(cmd, "-name"); - virBufferAsprintf(&buf, "%s", def->name); + virBufferEscape(&buf, ',', ",", "%s", def->name); if (cfg->setProcessName && virQEMUCapsGet(qemuCaps, QEMU_CAPS_NAME_PROCESS)) diff --git a/tests/qemuxml2argvdata/qemuxml2argv-name-escape.args b/tests/qemuxml2argvdata/qemuxml2argv-name-escape.args new file mode 100644 index 0000000..94a3133 --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-name-escape.args @@ -0,0 +1,24 @@ +LC_ALL=C \ +PATH=/bin \ +HOME=/home/test \ +USER=test \ +LOGNAME=test \ +QEMU_AUDIO_DRV=none \ +/usr/bin/qemu \ +-name foo,,bar,debug-threads=on \ +-S \ +-object secret,id=masterKey0,format=raw,file=/tmp/lib/domain--1-foo,\ +bar/master-key.aes \ +-M pc \ +-m 214 \ +-smp 1 \ +-uuid c7a5fdbd-edaf-9455-926a-d65c16db1809 \ +-nographic \ +-nodefaults \ +-chardev socket,id=charmonitor,path=/tmp/lib/domain--1-foo,bar/monitor.sock,\ +server,nowait \ +-mon chardev=charmonitor,id=monitor,mode=readline \ +-no-acpi \ +-boot c \ +-usb \ +-device virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0x3 diff --git a/tests/qemuxml2argvdata/qemuxml2argv-name-escape.xml b/tests/qemuxml2argvdata/qemuxml2argv-name-escape.xml new file mode 100644 index 0000000..3a8c3cd --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-name-escape.xml @@ -0,0 +1,18 @@ +<domain type='qemu'> + <name>foo,bar</name> + <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid> + <memory unit='KiB'>219100</memory> + <currentMemory unit='KiB'>219100</currentMemory> + <vcpu placement='static'>1</vcpu> + <os> + <type arch='i686' machine='pc'>hvm</type> + <boot dev='hd'/> + </os> + <clock offset='utc'/> + <on_poweroff>destroy</on_poweroff> + <on_reboot>restart</on_reboot> + <on_crash>destroy</on_crash> + <devices> + <emulator>/usr/bin/qemu</emulator> + </devices> +</domain> diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c index e41444d..d8834cb 100644 --- a/tests/qemuxml2argvtest.c +++ b/tests/qemuxml2argvtest.c @@ -1893,6 +1893,8 @@ mymain(void) VIR_DOMAIN_DEF_PARSE_SKIP_OSTYPE_CHECKS, NONE); + DO_TEST("name-escape", QEMU_CAPS_NAME_DEBUG_THREADS, + QEMU_CAPS_OBJECT_SECRET, QEMU_CAPS_CHARDEV); DO_TEST("debug-threads", QEMU_CAPS_NAME_DEBUG_THREADS); DO_TEST("master-key", QEMU_CAPS_OBJECT_SECRET); -- 2.7.4
Need to convert the local function to virBuffer usage, so we can use virBufferEscape --- src/qemu/qemu_command.c | 7 +++++-- tests/qemuxml2argvdata/qemuxml2argv-name-escape.args | 2 +- 2 files changed, 6 insertions(+), 3 deletions(-) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index b13df49..d54d0d1 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -180,6 +180,7 @@ qemuBuildMasterKeyCommandLine(virCommandPtr cmd, int ret = -1; char *alias = NULL; char *path = NULL; + virBuffer buf = VIR_BUFFER_INITIALIZER; /* If the -object secret does not exist, then just return. This just * means the domain won't be able to use a secret master key and is @@ -202,12 +203,14 @@ qemuBuildMasterKeyCommandLine(virCommandPtr cmd, goto cleanup; virCommandAddArg(cmd, "-object"); - virCommandAddArgFormat(cmd, "secret,id=%s,format=raw,file=%s", - alias, path); + virBufferAsprintf(&buf, "secret,id=%s,format=raw,file=", alias); + virBufferEscape(&buf, ',', ",", "%s", path); + virCommandAddArgBuffer(cmd, &buf); ret = 0; cleanup: + virBufferFreeAndReset(&buf); VIR_FREE(alias); VIR_FREE(path); return ret; diff --git a/tests/qemuxml2argvdata/qemuxml2argv-name-escape.args b/tests/qemuxml2argvdata/qemuxml2argv-name-escape.args index 94a3133..a5e35b8 100644 --- a/tests/qemuxml2argvdata/qemuxml2argv-name-escape.args +++ b/tests/qemuxml2argvdata/qemuxml2argv-name-escape.args @@ -7,7 +7,7 @@ QEMU_AUDIO_DRV=none \ /usr/bin/qemu \ -name foo,,bar,debug-threads=on \ -S \ --object secret,id=masterKey0,format=raw,file=/tmp/lib/domain--1-foo,\ +-object secret,id=masterKey0,format=raw,file=/tmp/lib/domain--1-foo,,\ bar/master-key.aes \ -M pc \ -m 214 \ -- 2.7.4
After this, a default virt-manager VM will startup with a comma in the VM name: https://bugzilla.redhat.com/show_bug.cgi?id=639926 --- src/qemu/qemu_command.c | 9 ++++----- tests/qemuxml2argvdata/qemuxml2argv-name-escape.args | 2 +- 2 files changed, 5 insertions(+), 6 deletions(-) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index d54d0d1..c2f55b5 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -4833,11 +4833,10 @@ qemuBuildChrChardevStr(virLogManagerPtr logManager, break; case VIR_DOMAIN_CHR_TYPE_UNIX: - virBufferAsprintf(&buf, - "socket,id=char%s,path=%s%s", - alias, - dev->data.nix.path, - dev->data.nix.listen ? ",server,nowait" : ""); + virBufferAsprintf(&buf, "socket,id=char%s,path=", alias); + virBufferEscape(&buf, ',', ",", "%s", dev->data.nix.path); + if (dev->data.nix.listen) + virBufferAddLit(&buf, ",server,nowait"); break; case VIR_DOMAIN_CHR_TYPE_SPICEVMC: diff --git a/tests/qemuxml2argvdata/qemuxml2argv-name-escape.args b/tests/qemuxml2argvdata/qemuxml2argv-name-escape.args index a5e35b8..772d94f 100644 --- a/tests/qemuxml2argvdata/qemuxml2argv-name-escape.args +++ b/tests/qemuxml2argvdata/qemuxml2argv-name-escape.args @@ -15,7 +15,7 @@ bar/master-key.aes \ -uuid c7a5fdbd-edaf-9455-926a-d65c16db1809 \ -nographic \ -nodefaults \ --chardev socket,id=charmonitor,path=/tmp/lib/domain--1-foo,bar/monitor.sock,\ +-chardev socket,id=charmonitor,path=/tmp/lib/domain--1-foo,,bar/monitor.sock,\ server,nowait \ -mon chardev=charmonitor,id=monitor,mode=readline \ -no-acpi \ -- 2.7.4
On 05/04/2016 10:56 AM, Cole Robinson wrote:
After this, a default virt-manager VM will startup with a comma in the VM name:
https://bugzilla.redhat.com/show_bug.cgi?id=639926 --- src/qemu/qemu_command.c | 9 ++++----- tests/qemuxml2argvdata/qemuxml2argv-name-escape.args | 2 +- 2 files changed, 5 insertions(+), 6 deletions(-)
FWIW: w/r/t my comment in your v1 about vmagent... It's the "guest_agent" I was thinking about. In any case, I see it uses the chardev path generation code, so I think that's covered by your patch 3.
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index d54d0d1..c2f55b5 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -4833,11 +4833,10 @@ qemuBuildChrChardevStr(virLogManagerPtr logManager, break;
case VIR_DOMAIN_CHR_TYPE_UNIX: - virBufferAsprintf(&buf, - "socket,id=char%s,path=%s%s", - alias, - dev->data.nix.path, - dev->data.nix.listen ? ",server,nowait" : ""); + virBufferAsprintf(&buf, "socket,id=char%s,path=", alias); + virBufferEscape(&buf, ',', ",", "%s", dev->data.nix.path);
Unless you know/see that the data.nix.path is built using domainChannelTargetDir during qemuBuildChannelsCommandLine it may not be "intuitively obvious" why doing the escape here is necessary... John
+ if (dev->data.nix.listen) + virBufferAddLit(&buf, ",server,nowait"); break;
case VIR_DOMAIN_CHR_TYPE_SPICEVMC: diff --git a/tests/qemuxml2argvdata/qemuxml2argv-name-escape.args b/tests/qemuxml2argvdata/qemuxml2argv-name-escape.args index a5e35b8..772d94f 100644 --- a/tests/qemuxml2argvdata/qemuxml2argv-name-escape.args +++ b/tests/qemuxml2argvdata/qemuxml2argv-name-escape.args @@ -15,7 +15,7 @@ bar/master-key.aes \ -uuid c7a5fdbd-edaf-9455-926a-d65c16db1809 \ -nographic \ -nodefaults \ --chardev socket,id=charmonitor,path=/tmp/lib/domain--1-foo,bar/monitor.sock,\ +-chardev socket,id=charmonitor,path=/tmp/lib/domain--1-foo,,bar/monitor.sock,\ server,nowait \ -mon chardev=charmonitor,id=monitor,mode=readline \ -no-acpi \
On 05/06/2016 07:54 AM, John Ferlan wrote:
On 05/04/2016 10:56 AM, Cole Robinson wrote:
After this, a default virt-manager VM will startup with a comma in the VM name:
https://bugzilla.redhat.com/show_bug.cgi?id=639926 --- src/qemu/qemu_command.c | 9 ++++----- tests/qemuxml2argvdata/qemuxml2argv-name-escape.args | 2 +- 2 files changed, 5 insertions(+), 6 deletions(-)
FWIW: w/r/t my comment in your v1 about vmagent... It's the "guest_agent" I was thinking about. In any case, I see it uses the chardev path generation code, so I think that's covered by your patch 3.
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index d54d0d1..c2f55b5 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -4833,11 +4833,10 @@ qemuBuildChrChardevStr(virLogManagerPtr logManager, break;
case VIR_DOMAIN_CHR_TYPE_UNIX: - virBufferAsprintf(&buf, - "socket,id=char%s,path=%s%s", - alias, - dev->data.nix.path, - dev->data.nix.listen ? ",server,nowait" : ""); + virBufferAsprintf(&buf, "socket,id=char%s,path=", alias); + virBufferEscape(&buf, ',', ",", "%s", dev->data.nix.path);
Unless you know/see that the data.nix.path is built using domainChannelTargetDir during qemuBuildChannelsCommandLine it may not be "intuitively obvious" why doing the escape here is necessary...
It's not specific to the autocreated socket path based on domainChannelTargetDir, the user could have passed in a manual path with a comma in it The rule is that any time a user specified string is passed to the qemu command line, we should be escaping commas. <name> happens to trickle out into several other places, but doing this for the unix path is still beneficial regardless of the <name> focus of this series - Cole
-name guest= is the explicit parameter for passing a VM name. Using it is required to allow a VM with an '=' in the name https://bugzilla.redhat.com/show_bug.cgi?id=1276485 --- src/qemu/qemu_capabilities.c | 2 ++ src/qemu/qemu_capabilities.h | 1 + src/qemu/qemu_command.c | 4 ++++ tests/qemucapabilitiesdata/caps_2.1.1-1.caps | 1 + tests/qemucapabilitiesdata/caps_2.4.0-1.caps | 1 + tests/qemucapabilitiesdata/caps_2.5.0-1.caps | 1 + tests/qemucapabilitiesdata/caps_2.6.0-1.caps | 1 + tests/qemuxml2argvdata/qemuxml2argv-name-escape.args | 8 ++++---- tests/qemuxml2argvdata/qemuxml2argv-name-escape.xml | 2 +- tests/qemuxml2argvtest.c | 2 +- 10 files changed, 17 insertions(+), 6 deletions(-) diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c index 65c3d69..da47759 100644 --- a/src/qemu/qemu_capabilities.c +++ b/src/qemu/qemu_capabilities.c @@ -328,6 +328,7 @@ VIR_ENUM_IMPL(virQEMUCaps, QEMU_CAPS_LAST, "device-tray-moved-event", "nec-usb-xhci-ports", "virtio-scsi-pci.iothread", + "name-guest", ); @@ -2666,6 +2667,7 @@ static struct virQEMUCapsCommandLineProps virQEMUCapsCommandLine[] = { { "spice", "gl", QEMU_CAPS_SPICE_GL }, { "chardev", "logfile", QEMU_CAPS_CHARDEV_LOGFILE }, { "name", "debug-threads", QEMU_CAPS_NAME_DEBUG_THREADS }, + { "name", "guest", QEMU_CAPS_NAME_GUEST }, }; static int diff --git a/src/qemu/qemu_capabilities.h b/src/qemu/qemu_capabilities.h index e7d0a60..9145f2d 100644 --- a/src/qemu/qemu_capabilities.h +++ b/src/qemu/qemu_capabilities.h @@ -359,6 +359,7 @@ typedef enum { QEMU_CAPS_DEVICE_TRAY_MOVED, /* DEVICE_TRAY_MOVED event */ QEMU_CAPS_NEC_USB_XHCI_PORTS, /* -device nec-usb-xhci.p3 ports setting */ QEMU_CAPS_VIRTIO_SCSI_IOTHREAD, /* virtio-scsi-{pci,ccw}.iothread */ + QEMU_CAPS_NAME_GUEST, /* -name guest= */ QEMU_CAPS_LAST /* this must always be the last item */ } virQEMUCapsFlags; diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index c2f55b5..570566c 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -6790,6 +6790,10 @@ qemuBuildNameCommandLine(virCommandPtr cmd, virCommandAddArg(cmd, "-name"); + /* The 'guest' option let's us handle a name with '=' embedded in it */ + if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_NAME_GUEST)) + virBufferAddLit(&buf, "guest="); + virBufferEscape(&buf, ',', ",", "%s", def->name); if (cfg->setProcessName && diff --git a/tests/qemucapabilitiesdata/caps_2.1.1-1.caps b/tests/qemucapabilitiesdata/caps_2.1.1-1.caps index 729000f..29214bb 100644 --- a/tests/qemucapabilitiesdata/caps_2.1.1-1.caps +++ b/tests/qemucapabilitiesdata/caps_2.1.1-1.caps @@ -164,4 +164,5 @@ <flag name='debug-threads'/> <flag name='device-tray-moved-event'/> <flag name='nec-usb-xhci-ports'/> + <flag name='name-guest'/> </qemuCaps> diff --git a/tests/qemucapabilitiesdata/caps_2.4.0-1.caps b/tests/qemucapabilitiesdata/caps_2.4.0-1.caps index 8009b8f..7b1de29 100644 --- a/tests/qemucapabilitiesdata/caps_2.4.0-1.caps +++ b/tests/qemucapabilitiesdata/caps_2.4.0-1.caps @@ -177,4 +177,5 @@ <flag name='device-tray-moved-event'/> <flag name='nec-usb-xhci-ports'/> <flag name='virtio-scsi-pci.iothread'/> + <flag name='name-guest'/> </qemuCaps> diff --git a/tests/qemucapabilitiesdata/caps_2.5.0-1.caps b/tests/qemucapabilitiesdata/caps_2.5.0-1.caps index e139455..19192da 100644 --- a/tests/qemucapabilitiesdata/caps_2.5.0-1.caps +++ b/tests/qemucapabilitiesdata/caps_2.5.0-1.caps @@ -178,4 +178,5 @@ <flag name='device-tray-moved-event'/> <flag name='nec-usb-xhci-ports'/> <flag name='virtio-scsi-pci.iothread'/> + <flag name='name-guest'/> </qemuCaps> diff --git a/tests/qemucapabilitiesdata/caps_2.6.0-1.caps b/tests/qemucapabilitiesdata/caps_2.6.0-1.caps index ec5bfcc..59f637f 100644 --- a/tests/qemucapabilitiesdata/caps_2.6.0-1.caps +++ b/tests/qemucapabilitiesdata/caps_2.6.0-1.caps @@ -183,4 +183,5 @@ <flag name='device-tray-moved-event'/> <flag name='nec-usb-xhci-ports'/> <flag name='virtio-scsi-pci.iothread'/> + <flag name='name-guest'/> </qemuCaps> diff --git a/tests/qemuxml2argvdata/qemuxml2argv-name-escape.args b/tests/qemuxml2argvdata/qemuxml2argv-name-escape.args index 772d94f..7ecdca5 100644 --- a/tests/qemuxml2argvdata/qemuxml2argv-name-escape.args +++ b/tests/qemuxml2argvdata/qemuxml2argv-name-escape.args @@ -5,17 +5,17 @@ USER=test \ LOGNAME=test \ QEMU_AUDIO_DRV=none \ /usr/bin/qemu \ --name foo,,bar,debug-threads=on \ +-name guest=foo=1,,bar=2,debug-threads=on \ -S \ --object secret,id=masterKey0,format=raw,file=/tmp/lib/domain--1-foo,,\ -bar/master-key.aes \ +-object secret,id=masterKey0,format=raw,file=/tmp/lib/domain--1-foo=1,,\ +bar=2/master-key.aes \ -M pc \ -m 214 \ -smp 1 \ -uuid c7a5fdbd-edaf-9455-926a-d65c16db1809 \ -nographic \ -nodefaults \ --chardev socket,id=charmonitor,path=/tmp/lib/domain--1-foo,,bar/monitor.sock,\ +-chardev socket,id=charmonitor,path=/tmp/lib/domain--1-foo=1,,bar=2/monitor.sock,\ server,nowait \ -mon chardev=charmonitor,id=monitor,mode=readline \ -no-acpi \ diff --git a/tests/qemuxml2argvdata/qemuxml2argv-name-escape.xml b/tests/qemuxml2argvdata/qemuxml2argv-name-escape.xml index 3a8c3cd..0057062 100644 --- a/tests/qemuxml2argvdata/qemuxml2argv-name-escape.xml +++ b/tests/qemuxml2argvdata/qemuxml2argv-name-escape.xml @@ -1,5 +1,5 @@ <domain type='qemu'> - <name>foo,bar</name> + <name>foo=1,bar=2</name> <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid> <memory unit='KiB'>219100</memory> <currentMemory unit='KiB'>219100</currentMemory> diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c index d8834cb..d7b06cf 100644 --- a/tests/qemuxml2argvtest.c +++ b/tests/qemuxml2argvtest.c @@ -1894,7 +1894,7 @@ mymain(void) NONE); DO_TEST("name-escape", QEMU_CAPS_NAME_DEBUG_THREADS, - QEMU_CAPS_OBJECT_SECRET, QEMU_CAPS_CHARDEV); + QEMU_CAPS_OBJECT_SECRET, QEMU_CAPS_CHARDEV, QEMU_CAPS_NAME_GUEST); DO_TEST("debug-threads", QEMU_CAPS_NAME_DEBUG_THREADS); DO_TEST("master-key", QEMU_CAPS_OBJECT_SECRET); -- 2.7.4
On 05/04/2016 10:56 AM, Cole Robinson wrote:
-name guest= is the explicit parameter for passing a VM name. Using it is required to allow a VM with an '=' in the name
https://bugzilla.redhat.com/show_bug.cgi?id=1276485 --- src/qemu/qemu_capabilities.c | 2 ++ src/qemu/qemu_capabilities.h | 1 + src/qemu/qemu_command.c | 4 ++++ tests/qemucapabilitiesdata/caps_2.1.1-1.caps | 1 + tests/qemucapabilitiesdata/caps_2.4.0-1.caps | 1 + tests/qemucapabilitiesdata/caps_2.5.0-1.caps | 1 + tests/qemucapabilitiesdata/caps_2.6.0-1.caps | 1 + tests/qemuxml2argvdata/qemuxml2argv-name-escape.args | 8 ++++---- tests/qemuxml2argvdata/qemuxml2argv-name-escape.xml | 2 +- tests/qemuxml2argvtest.c | 2 +- 10 files changed, 17 insertions(+), 6 deletions(-)
You're overloading the name-escape test with the "guest=" option which I'm never quite sure how others feel about. I would think you'd want a separate test that just adds the "guest=". I also see this biting us at some point in the future in some test when 'guest=' becomes the default and all the test outputs have to change to add that. I'm sure it seems you're chasing a moving target with changes to the *.caps files... Recent changes in that space will cause more adjustments here <sigh>.
diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c index 65c3d69..da47759 100644 --- a/src/qemu/qemu_capabilities.c +++ b/src/qemu/qemu_capabilities.c @@ -328,6 +328,7 @@ VIR_ENUM_IMPL(virQEMUCaps, QEMU_CAPS_LAST, "device-tray-moved-event", "nec-usb-xhci-ports", "virtio-scsi-pci.iothread", + "name-guest", );
@@ -2666,6 +2667,7 @@ static struct virQEMUCapsCommandLineProps virQEMUCapsCommandLine[] = { { "spice", "gl", QEMU_CAPS_SPICE_GL }, { "chardev", "logfile", QEMU_CAPS_CHARDEV_LOGFILE }, { "name", "debug-threads", QEMU_CAPS_NAME_DEBUG_THREADS }, + { "name", "guest", QEMU_CAPS_NAME_GUEST }, };
static int diff --git a/src/qemu/qemu_capabilities.h b/src/qemu/qemu_capabilities.h index e7d0a60..9145f2d 100644 --- a/src/qemu/qemu_capabilities.h +++ b/src/qemu/qemu_capabilities.h @@ -359,6 +359,7 @@ typedef enum { QEMU_CAPS_DEVICE_TRAY_MOVED, /* DEVICE_TRAY_MOVED event */ QEMU_CAPS_NEC_USB_XHCI_PORTS, /* -device nec-usb-xhci.p3 ports setting */ QEMU_CAPS_VIRTIO_SCSI_IOTHREAD, /* virtio-scsi-{pci,ccw}.iothread */ + QEMU_CAPS_NAME_GUEST, /* -name guest= */
QEMU_CAPS_LAST /* this must always be the last item */ } virQEMUCapsFlags; diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index c2f55b5..570566c 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -6790,6 +6790,10 @@ qemuBuildNameCommandLine(virCommandPtr cmd,
virCommandAddArg(cmd, "-name");
+ /* The 'guest' option let's us handle a name with '=' embedded in it */ + if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_NAME_GUEST)) + virBufferAddLit(&buf, "guest="); +
So, any domain that has qemu 2.1 or later will use the "-name guest=$name..." naming scheme rather than just "-name $name..." (regardless of whether there's a '=' in $name or not). I don't necessarily mind using the guest=$name syntax; however, it could be quite a surprise to anything that searches on/for "-name $name" rather than "-name guest=$name" in/for the running qemu John
virBufferEscape(&buf, ',', ",", "%s", def->name);
if (cfg->setProcessName && diff --git a/tests/qemucapabilitiesdata/caps_2.1.1-1.caps b/tests/qemucapabilitiesdata/caps_2.1.1-1.caps index 729000f..29214bb 100644 --- a/tests/qemucapabilitiesdata/caps_2.1.1-1.caps +++ b/tests/qemucapabilitiesdata/caps_2.1.1-1.caps @@ -164,4 +164,5 @@ <flag name='debug-threads'/> <flag name='device-tray-moved-event'/> <flag name='nec-usb-xhci-ports'/> + <flag name='name-guest'/> </qemuCaps> diff --git a/tests/qemucapabilitiesdata/caps_2.4.0-1.caps b/tests/qemucapabilitiesdata/caps_2.4.0-1.caps index 8009b8f..7b1de29 100644 --- a/tests/qemucapabilitiesdata/caps_2.4.0-1.caps +++ b/tests/qemucapabilitiesdata/caps_2.4.0-1.caps @@ -177,4 +177,5 @@ <flag name='device-tray-moved-event'/> <flag name='nec-usb-xhci-ports'/> <flag name='virtio-scsi-pci.iothread'/> + <flag name='name-guest'/> </qemuCaps> diff --git a/tests/qemucapabilitiesdata/caps_2.5.0-1.caps b/tests/qemucapabilitiesdata/caps_2.5.0-1.caps index e139455..19192da 100644 --- a/tests/qemucapabilitiesdata/caps_2.5.0-1.caps +++ b/tests/qemucapabilitiesdata/caps_2.5.0-1.caps @@ -178,4 +178,5 @@ <flag name='device-tray-moved-event'/> <flag name='nec-usb-xhci-ports'/> <flag name='virtio-scsi-pci.iothread'/> + <flag name='name-guest'/> </qemuCaps> diff --git a/tests/qemucapabilitiesdata/caps_2.6.0-1.caps b/tests/qemucapabilitiesdata/caps_2.6.0-1.caps index ec5bfcc..59f637f 100644 --- a/tests/qemucapabilitiesdata/caps_2.6.0-1.caps +++ b/tests/qemucapabilitiesdata/caps_2.6.0-1.caps @@ -183,4 +183,5 @@ <flag name='device-tray-moved-event'/> <flag name='nec-usb-xhci-ports'/> <flag name='virtio-scsi-pci.iothread'/> + <flag name='name-guest'/> </qemuCaps> diff --git a/tests/qemuxml2argvdata/qemuxml2argv-name-escape.args b/tests/qemuxml2argvdata/qemuxml2argv-name-escape.args index 772d94f..7ecdca5 100644 --- a/tests/qemuxml2argvdata/qemuxml2argv-name-escape.args +++ b/tests/qemuxml2argvdata/qemuxml2argv-name-escape.args @@ -5,17 +5,17 @@ USER=test \ LOGNAME=test \ QEMU_AUDIO_DRV=none \ /usr/bin/qemu \ --name foo,,bar,debug-threads=on \ +-name guest=foo=1,,bar=2,debug-threads=on \ -S \ --object secret,id=masterKey0,format=raw,file=/tmp/lib/domain--1-foo,,\ -bar/master-key.aes \ +-object secret,id=masterKey0,format=raw,file=/tmp/lib/domain--1-foo=1,,\ +bar=2/master-key.aes \ -M pc \ -m 214 \ -smp 1 \ -uuid c7a5fdbd-edaf-9455-926a-d65c16db1809 \ -nographic \ -nodefaults \ --chardev socket,id=charmonitor,path=/tmp/lib/domain--1-foo,,bar/monitor.sock,\ +-chardev socket,id=charmonitor,path=/tmp/lib/domain--1-foo=1,,bar=2/monitor.sock,\ server,nowait \ -mon chardev=charmonitor,id=monitor,mode=readline \ -no-acpi \ diff --git a/tests/qemuxml2argvdata/qemuxml2argv-name-escape.xml b/tests/qemuxml2argvdata/qemuxml2argv-name-escape.xml index 3a8c3cd..0057062 100644 --- a/tests/qemuxml2argvdata/qemuxml2argv-name-escape.xml +++ b/tests/qemuxml2argvdata/qemuxml2argv-name-escape.xml @@ -1,5 +1,5 @@ <domain type='qemu'> - <name>foo,bar</name> + <name>foo=1,bar=2</name> <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid> <memory unit='KiB'>219100</memory> <currentMemory unit='KiB'>219100</currentMemory> diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c index d8834cb..d7b06cf 100644 --- a/tests/qemuxml2argvtest.c +++ b/tests/qemuxml2argvtest.c @@ -1894,7 +1894,7 @@ mymain(void) NONE);
DO_TEST("name-escape", QEMU_CAPS_NAME_DEBUG_THREADS, - QEMU_CAPS_OBJECT_SECRET, QEMU_CAPS_CHARDEV); + QEMU_CAPS_OBJECT_SECRET, QEMU_CAPS_CHARDEV, QEMU_CAPS_NAME_GUEST); DO_TEST("debug-threads", QEMU_CAPS_NAME_DEBUG_THREADS);
DO_TEST("master-key", QEMU_CAPS_OBJECT_SECRET);
On 05/06/2016 07:54 AM, John Ferlan wrote:
On 05/04/2016 10:56 AM, Cole Robinson wrote:
-name guest= is the explicit parameter for passing a VM name. Using it is required to allow a VM with an '=' in the name
https://bugzilla.redhat.com/show_bug.cgi?id=1276485 --- src/qemu/qemu_capabilities.c | 2 ++ src/qemu/qemu_capabilities.h | 1 + src/qemu/qemu_command.c | 4 ++++ tests/qemucapabilitiesdata/caps_2.1.1-1.caps | 1 + tests/qemucapabilitiesdata/caps_2.4.0-1.caps | 1 + tests/qemucapabilitiesdata/caps_2.5.0-1.caps | 1 + tests/qemucapabilitiesdata/caps_2.6.0-1.caps | 1 + tests/qemuxml2argvdata/qemuxml2argv-name-escape.args | 8 ++++---- tests/qemuxml2argvdata/qemuxml2argv-name-escape.xml | 2 +- tests/qemuxml2argvtest.c | 2 +- 10 files changed, 17 insertions(+), 6 deletions(-)
You're overloading the name-escape test with the "guest=" option which I'm never quite sure how others feel about. I would think you'd want a separate test that just adds the "guest=".
That's generally the pattern, to add a new XML file for every qemu addition, but that's overkill IMO
I also see this biting us at some point in the future in some test when 'guest=' becomes the default and all the test outputs have to change to add that.
That's not really any different than the unconditional -name debug-threads=on option we add for qemu 2.5 and later, or secrets initializing, etc - Cole
On 05/04/2016 10:56 AM, Cole Robinson wrote:
This series adds qemu cli comma escaping to several places that are dependent on the VM name, to enable names with embedded commas.
Patch 4 makes use of qemu -name guest=X value to allow names with '=' in them.
https://bugzilla.redhat.com/show_bug.cgi?id=639926 https://bugzilla.redhat.com/show_bug.cgi?id=1276485
Note: There's likely other places that are VM name dependent that need comma escaping too, but this hits the mandatory ones. I've listed some more on the BiteSizedTasks page:
http://wiki.libvirt.org/page/BiteSizedTasks#qemu:_Use_comma_escaping_for_mor...
v2: Rebase to master
Cole Robinson (4): qemu: command: escape commas in VM name qemu: command: escape commas in secret master path qemu: command: escape commas in chardev socket path qemu: command: Use -name guest= if available
src/qemu/qemu_capabilities.c | 2 ++ src/qemu/qemu_capabilities.h | 1 + src/qemu/qemu_command.c | 22 ++++++++++++-------- tests/qemucapabilitiesdata/caps_2.1.1-1.caps | 1 + tests/qemucapabilitiesdata/caps_2.4.0-1.caps | 1 + tests/qemucapabilitiesdata/caps_2.5.0-1.caps | 1 + tests/qemucapabilitiesdata/caps_2.6.0-1.caps | 1 + .../qemuxml2argvdata/qemuxml2argv-name-escape.args | 24 ++++++++++++++++++++++ .../qemuxml2argvdata/qemuxml2argv-name-escape.xml | 18 ++++++++++++++++ tests/qemuxml2argvtest.c | 2 ++ 10 files changed, 65 insertions(+), 8 deletions(-) create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-name-escape.args create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-name-escape.xml
ewww again ;-) While I cannot imagine a reason for someone (other than testing perhaps) to want a "," or an "=" in a vm name, I suppose since it's possible to handle by qemu, then we should 'handle' it. Sorry I dropped the ball after my initial pass - I guess I was wondering if there were any other (similar) feelings! After reading the bz, I see why you've gone with double commas, although it wasn't apparent without that read... So maybe each commit that adds that double comma escape could explain why the double comma works (or even at points in the code where it occurs?). Searching qemu_command.c for domainChannelTargetDir and it's seems things are covered (good)... Searching for domainLibDir and I think there's one usage not covered - qemuBuildGraphicsVNCCommandLine uses it to build "$path_with_domainLibDir/vnc.sock". Of course finding these "one off" type issues makes me think at some point in the future one of those will be used for "something" and the double comma will be missed. So should we generate a local "escaped" names for domainLibDir and domainChannelTargetDir that would include the double comma and that would be passed to the corresponding users? I think as long as you cover the VNC case and add a few more words why the ",," can be used on the command line, then the series seems reasonable to me, although you may want to wait a bit to push to see if anyone else has angst over the two changes, especially now having "guest=$name" be the default for anyone using qemu 2.1 or later. John
On 05/06/2016 07:54 AM, John Ferlan wrote:
On 05/04/2016 10:56 AM, Cole Robinson wrote:
This series adds qemu cli comma escaping to several places that are dependent on the VM name, to enable names with embedded commas.
Patch 4 makes use of qemu -name guest=X value to allow names with '=' in them.
https://bugzilla.redhat.com/show_bug.cgi?id=639926 https://bugzilla.redhat.com/show_bug.cgi?id=1276485
Note: There's likely other places that are VM name dependent that need comma escaping too, but this hits the mandatory ones. I've listed some more on the BiteSizedTasks page:
http://wiki.libvirt.org/page/BiteSizedTasks#qemu:_Use_comma_escaping_for_mor...
v2: Rebase to master
Cole Robinson (4): qemu: command: escape commas in VM name qemu: command: escape commas in secret master path qemu: command: escape commas in chardev socket path qemu: command: Use -name guest= if available
src/qemu/qemu_capabilities.c | 2 ++ src/qemu/qemu_capabilities.h | 1 + src/qemu/qemu_command.c | 22 ++++++++++++-------- tests/qemucapabilitiesdata/caps_2.1.1-1.caps | 1 + tests/qemucapabilitiesdata/caps_2.4.0-1.caps | 1 + tests/qemucapabilitiesdata/caps_2.5.0-1.caps | 1 + tests/qemucapabilitiesdata/caps_2.6.0-1.caps | 1 + .../qemuxml2argvdata/qemuxml2argv-name-escape.args | 24 ++++++++++++++++++++++ .../qemuxml2argvdata/qemuxml2argv-name-escape.xml | 18 ++++++++++++++++ tests/qemuxml2argvtest.c | 2 ++ 10 files changed, 65 insertions(+), 8 deletions(-) create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-name-escape.args create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-name-escape.xml
ewww again ;-)
While I cannot imagine a reason for someone (other than testing perhaps) to want a "," or an "=" in a vm name, I suppose since it's possible to handle by qemu, then we should 'handle' it.
Sorry I dropped the ball after my initial pass - I guess I was wondering if there were any other (similar) feelings!
No worries
After reading the bz, I see why you've gone with double commas, although it wasn't apparent without that read... So maybe each commit that adds that double comma escape could explain why the double comma works (or even at points in the code where it occurs?).
Yeah the double comma is the qemu convention. I'll add an explicit function like qemuBufEscapeCommas or something to make this a bit more obvious
Searching qemu_command.c for domainChannelTargetDir and it's seems things are covered (good)... Searching for domainLibDir and I think there's one usage not covered - qemuBuildGraphicsVNCCommandLine uses it to build "$path_with_domainLibDir/vnc.sock".
Okay, I'll add that.
Of course finding these "one off" type issues makes me think at some point in the future one of those will be used for "something" and the double comma will be missed. So should we generate a local "escaped" names for domainLibDir and domainChannelTargetDir that would include the double comma and that would be passed to the corresponding users?
TBH I don't really think this is important enough to try and come up with a safe generalized solution at this point. I think it will largely work itself out if I add the explicit escaping function, and there's enough usages in qemu_command.c that they will get cargoculted around when new code is added.
I think as long as you cover the VNC case and add a few more words why the ",," can be used on the command line, then the series seems reasonable to me, although you may want to wait a bit to push to see if anyone else has angst over the two changes, especially now having "guest=$name" be the default for anyone using qemu 2.1 or later.
I'll send a v3 anyways so we can see if anyone cares - Cole
participants (2)
-
Cole Robinson -
John Ferlan