[libvirt] [PATCH 0/4] qemu: handle ',' and '=' in VM names
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. There's likely other places that are VM name dependent that need escaping too, but this hits the mandatory ones. I'm going to through the remaining list on the BiteSizedTasks page 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.3
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 ef4a62d..8324639 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -6729,7 +6729,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 8842b2f..13b07c7 100644 --- a/tests/qemuxml2argvtest.c +++ b/tests/qemuxml2argvtest.c @@ -1875,6 +1875,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.3
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 8324639..6defa56 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -181,6 +181,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 @@ -203,12 +204,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.3
--- 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 6defa56..262e8d9 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -4772,11 +4772,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.3
-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 de54788..57c972d 100644 --- a/src/qemu/qemu_capabilities.c +++ b/src/qemu/qemu_capabilities.c @@ -325,6 +325,7 @@ VIR_ENUM_IMPL(virQEMUCaps, QEMU_CAPS_LAST, "pxb", "pxb-pcie", /* 220 */ + "name-guest", ); @@ -2649,6 +2650,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 83b8be3..309017f 100644 --- a/src/qemu/qemu_capabilities.h +++ b/src/qemu/qemu_capabilities.h @@ -356,6 +356,7 @@ typedef enum { /* 220 */ QEMU_CAPS_DEVICE_PXB_PCIE, /* -device pxb-pcie */ + QEMU_CAPS_NAME_GUEST, /* Is -name guest= available */ 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 262e8d9..8f64ef2 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -6731,6 +6731,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 8175743..1cb4116 100644 --- a/tests/qemucapabilitiesdata/caps_2.1.1-1.caps +++ b/tests/qemucapabilitiesdata/caps_2.1.1-1.caps @@ -162,4 +162,5 @@ <flag name='qxl.vram64_size_mb'/> <flag name='qxl-vga.vram64_size_mb'/> <flag name='debug-threads'/> + <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 2cdf961..f2b8c62 100644 --- a/tests/qemucapabilitiesdata/caps_2.4.0-1.caps +++ b/tests/qemucapabilitiesdata/caps_2.4.0-1.caps @@ -174,4 +174,5 @@ <flag name='qxl-vga.vram64_size_mb'/> <flag name='debug-threads'/> <flag name='pxb'/> + <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 ecaa165..046a25c 100644 --- a/tests/qemucapabilitiesdata/caps_2.5.0-1.caps +++ b/tests/qemucapabilitiesdata/caps_2.5.0-1.caps @@ -175,4 +175,5 @@ <flag name='qxl-vga.vram64_size_mb'/> <flag name='debug-threads'/> <flag name='pxb'/> + <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 e243f8a..24c92c9 100644 --- a/tests/qemucapabilitiesdata/caps_2.6.0-1.caps +++ b/tests/qemucapabilitiesdata/caps_2.6.0-1.caps @@ -180,4 +180,5 @@ <flag name='secret'/> <flag name='pxb'/> <flag name='pxb-pcie'/> + <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 13b07c7..d32e104 100644 --- a/tests/qemuxml2argvtest.c +++ b/tests/qemuxml2argvtest.c @@ -1876,7 +1876,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.3
On 04/22/2016 06:46 PM, 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.
There's likely other places that are VM name dependent that need escaping too, but this hits the mandatory ones. I'm going to through the remaining list on the BiteSizedTasks page
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
Is "ewww" the proper word here ;-) What about the vmagent path? Since the 'path' that the masterKey uses is essentially the priv->libDir path, would fixing that path to have escaped chars work? (just typing without researching, thinking)... Similarly channelTargetDir. IOW: qemuDomainSetPrivatePaths What about places in qemu_process which use the [obj->]def->name to build Path's (stateDir). Does this work with domain name rename? migrations? If the target host doesn't support "guest=". I guess I'm also somewhat surprised that there'd be no issues w/ cgroups and systemd interactions. Just some quick Saturday morning thoughts with only 1 cup of coffee... John
On 04/23/2016 08:47 AM, John Ferlan wrote:
On 04/22/2016 06:46 PM, 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.
There's likely other places that are VM name dependent that need escaping too, but this hits the mandatory ones. I'm going to through the remaining list on the BiteSizedTasks page
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
Is "ewww" the proper word here ;-)
Maybe, seems to be popular on the list recently ;)
What about the vmagent path?
? 'git grep -i vmagent' doesn't give me anything. I guess you mean qemu-ga path... that's handled by the chardev socket= escaping, which is what most <channel> devices will be using.
Since the 'path' that the masterKey uses is essentially the priv->libDir path, would fixing that path to have escaped chars work? (just typing without researching, thinking)... Similarly channelTargetDir. IOW: qemuDomainSetPrivatePaths
Escaping these paths is a qemu command line thing only. Editing libDir directly means that non-cli uses of libDir in the code will be trying to access the wrong path (one with a literal double comma in it, vs the actual single comma)
What about places in qemu_process which use the [obj->]def->name to build Path's (stateDir).
Those don't matter, they can have embedded comma/equals just fine. Those characters are only a problem on the qemu command line which uses it as a delimiter, so it needs special handling. For regular FS paths those characters are totally legit
Does this work with domain name rename? migrations? If the target host doesn't support "guest=".
If the target host doesn't support guest= it may fail, but only if the name has an embedded equals sign. I think that's fine.
I guess I'm also somewhat surprised that there'd be no issues w/ cgroups and systemd interactions.
Just some quick Saturday morning thoughts with only 1 cup of coffee...
See above, I think maybe you are confusing this 'escaping' with general shell escaping? Or I myself need more coffee, still working on cup #1 :) - Cole
participants (2)
-
Cole Robinson -
John Ferlan