9:56 a.m.
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_...
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
9:56 a.m.
New subject: [libvirt] [PATCH v2 1/4] qemu: command: escape commas in VM name
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
9:56 a.m.
New subject: [libvirt] [PATCH v2 2/4] qemu: command: escape commas in secret master path
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
9:56 a.m.
New subject: [libvirt] [PATCH v2 3/4] qemu: command: escape commas in chardev socket path
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
6:54 a.m.
New subject: [libvirt] [PATCH v2 3/4] qemu: command: escape commas in chardev socket path
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 \
7:11 a.m.
New subject: [libvirt] [PATCH v2 3/4] qemu: command: escape commas in chardev socket path
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
9:56 a.m.
New subject: [libvirt] [PATCH v2 4/4] qemu: command: Use -name guest= if available
-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
6:54 a.m.
New subject: [libvirt] [PATCH v2 4/4] qemu: command: Use -name guest= if available
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);
7:50 a.m.
New subject: [libvirt] [PATCH v2 4/4] qemu: command: Use -name guest= if available
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
6:54 a.m.
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_...
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
7:16 a.m.
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_...
>
> 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
3125
days inactive
3127
days old
10 comments
2 participants
participants (2)
-
Cole Robinson -
John Ferlan