[libvirt] [PATCH 0/2] fix hot(un)plug of chardev devices with TLS encryption
Pavel Hrdina (2): qemu_command: create prefixed alias to separate variable qemu: always generate the same alias for tls-creds-x509 object src/qemu/qemu_command.c | 39 ++++++++++++---------- src/qemu/qemu_hotplug.c | 9 +++-- ...xml2argv-serial-tcp-tlsx509-chardev-verify.args | 4 +-- .../qemuxml2argv-serial-tcp-tlsx509-chardev.args | 4 +-- 4 files changed, 33 insertions(+), 23 deletions(-) -- 2.10.1
Instead of typing the prefix every time we want to append parameters to qemu command line use a variable that contains prefixed alias. Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- src/qemu/qemu_command.c | 35 ++++++++++++++++++++--------------- 1 file changed, 20 insertions(+), 15 deletions(-) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 21fd85c..74f65c0 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -4861,28 +4861,32 @@ qemuBuildChrChardevStr(virLogManagerPtr logManager, { virBuffer buf = VIR_BUFFER_INITIALIZER; bool telnet; + char *charAlias = NULL; + + if (virAsprintf(&charAlias, "char%s", alias) < 0) + goto error; switch (dev->type) { case VIR_DOMAIN_CHR_TYPE_NULL: - virBufferAsprintf(&buf, "null,id=char%s", alias); + virBufferAsprintf(&buf, "null,id=%s", charAlias); break; case VIR_DOMAIN_CHR_TYPE_VC: - virBufferAsprintf(&buf, "vc,id=char%s", alias); + virBufferAsprintf(&buf, "vc,id=%s", charAlias); break; case VIR_DOMAIN_CHR_TYPE_PTY: - virBufferAsprintf(&buf, "pty,id=char%s", alias); + virBufferAsprintf(&buf, "pty,id=%s", charAlias); break; case VIR_DOMAIN_CHR_TYPE_DEV: - virBufferAsprintf(&buf, "%s,id=char%s,path=%s", + virBufferAsprintf(&buf, "%s,id=%s,path=%s", STRPREFIX(alias, "parallel") ? "parport" : "tty", - alias, dev->data.file.path); + charAlias, dev->data.file.path); break; case VIR_DOMAIN_CHR_TYPE_FILE: - virBufferAsprintf(&buf, "file,id=char%s", alias); + virBufferAsprintf(&buf, "file,id=%s", charAlias); if (dev->data.file.append != VIR_TRISTATE_SWITCH_ABSENT && !virQEMUCapsGet(qemuCaps, QEMU_CAPS_CHARDEV_FILE_APPEND)) { @@ -4898,12 +4902,12 @@ qemuBuildChrChardevStr(virLogManagerPtr logManager, break; case VIR_DOMAIN_CHR_TYPE_PIPE: - virBufferAsprintf(&buf, "pipe,id=char%s,path=%s", alias, + virBufferAsprintf(&buf, "pipe,id=%s,path=%s", charAlias, dev->data.file.path); break; case VIR_DOMAIN_CHR_TYPE_STDIO: - virBufferAsprintf(&buf, "stdio,id=char%s", alias); + virBufferAsprintf(&buf, "stdio,id=%s", charAlias); break; case VIR_DOMAIN_CHR_TYPE_UDP: { @@ -4919,9 +4923,9 @@ qemuBuildChrChardevStr(virLogManagerPtr logManager, bindService = "0"; virBufferAsprintf(&buf, - "udp,id=char%s,host=%s,port=%s,localaddr=%s," + "udp,id=%s,host=%s,port=%s,localaddr=%s," "localport=%s", - alias, + charAlias, connectHost, dev->data.udp.connectService, bindHost, bindService); @@ -4930,8 +4934,8 @@ qemuBuildChrChardevStr(virLogManagerPtr logManager, case VIR_DOMAIN_CHR_TYPE_TCP: telnet = dev->data.tcp.protocol == VIR_DOMAIN_CHR_TCP_PROTOCOL_TELNET; virBufferAsprintf(&buf, - "socket,id=char%s,host=%s,port=%s%s", - alias, + "socket,id=%s,host=%s,port=%s%s", + charAlias, dev->data.tcp.host, dev->data.tcp.service, telnet ? ",telnet" : ""); @@ -4956,7 +4960,7 @@ qemuBuildChrChardevStr(virLogManagerPtr logManager, break; case VIR_DOMAIN_CHR_TYPE_UNIX: - virBufferAsprintf(&buf, "socket,id=char%s,path=", alias); + virBufferAsprintf(&buf, "socket,id=%s,path=", charAlias); virQEMUBuildBufferEscapeComma(&buf, dev->data.nix.path); if (dev->data.nix.listen) virBufferAdd(&buf, nowait ? ",server,nowait" : ",server", -1); @@ -4968,7 +4972,7 @@ qemuBuildChrChardevStr(virLogManagerPtr logManager, _("spicevmc not supported in this QEMU binary")); goto error; } - virBufferAsprintf(&buf, "spicevmc,id=char%s,name=%s", alias, + virBufferAsprintf(&buf, "spicevmc,id=%s,name=%s", charAlias, virDomainChrSpicevmcTypeToString(dev->data.spicevmc)); break; @@ -4978,7 +4982,7 @@ qemuBuildChrChardevStr(virLogManagerPtr logManager, _("spiceport not supported in this QEMU binary")); goto error; } - virBufferAsprintf(&buf, "spiceport,id=char%s,name=%s", alias, + virBufferAsprintf(&buf, "spiceport,id=%s,name=%s", charAlias, dev->data.spiceport.channel); break; @@ -5007,6 +5011,7 @@ qemuBuildChrChardevStr(virLogManagerPtr logManager, return virBufferContentAndReset(&buf); error: + VIR_FREE(charAlias); virBufferFreeAndReset(&buf); return NULL; } -- 2.10.1
On 10/18/2016 09:58 AM, Pavel Hrdina wrote:
Instead of typing the prefix every time we want to append parameters to qemu command line use a variable that contains prefixed alias.
Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- src/qemu/qemu_command.c | 35 ++++++++++++++++++++--------------- 1 file changed, 20 insertions(+), 15 deletions(-)
Why not create a qemu_alias.c helper that then can also be used in your followup patch? John
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 21fd85c..74f65c0 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -4861,28 +4861,32 @@ qemuBuildChrChardevStr(virLogManagerPtr logManager, { virBuffer buf = VIR_BUFFER_INITIALIZER; bool telnet; + char *charAlias = NULL; + + if (virAsprintf(&charAlias, "char%s", alias) < 0) + goto error;
switch (dev->type) { case VIR_DOMAIN_CHR_TYPE_NULL: - virBufferAsprintf(&buf, "null,id=char%s", alias); + virBufferAsprintf(&buf, "null,id=%s", charAlias); break;
case VIR_DOMAIN_CHR_TYPE_VC: - virBufferAsprintf(&buf, "vc,id=char%s", alias); + virBufferAsprintf(&buf, "vc,id=%s", charAlias); break;
case VIR_DOMAIN_CHR_TYPE_PTY: - virBufferAsprintf(&buf, "pty,id=char%s", alias); + virBufferAsprintf(&buf, "pty,id=%s", charAlias); break;
case VIR_DOMAIN_CHR_TYPE_DEV: - virBufferAsprintf(&buf, "%s,id=char%s,path=%s", + virBufferAsprintf(&buf, "%s,id=%s,path=%s", STRPREFIX(alias, "parallel") ? "parport" : "tty", - alias, dev->data.file.path); + charAlias, dev->data.file.path); break;
case VIR_DOMAIN_CHR_TYPE_FILE: - virBufferAsprintf(&buf, "file,id=char%s", alias); + virBufferAsprintf(&buf, "file,id=%s", charAlias);
if (dev->data.file.append != VIR_TRISTATE_SWITCH_ABSENT && !virQEMUCapsGet(qemuCaps, QEMU_CAPS_CHARDEV_FILE_APPEND)) { @@ -4898,12 +4902,12 @@ qemuBuildChrChardevStr(virLogManagerPtr logManager, break;
case VIR_DOMAIN_CHR_TYPE_PIPE: - virBufferAsprintf(&buf, "pipe,id=char%s,path=%s", alias, + virBufferAsprintf(&buf, "pipe,id=%s,path=%s", charAlias, dev->data.file.path); break;
case VIR_DOMAIN_CHR_TYPE_STDIO: - virBufferAsprintf(&buf, "stdio,id=char%s", alias); + virBufferAsprintf(&buf, "stdio,id=%s", charAlias); break;
case VIR_DOMAIN_CHR_TYPE_UDP: { @@ -4919,9 +4923,9 @@ qemuBuildChrChardevStr(virLogManagerPtr logManager, bindService = "0";
virBufferAsprintf(&buf, - "udp,id=char%s,host=%s,port=%s,localaddr=%s," + "udp,id=%s,host=%s,port=%s,localaddr=%s," "localport=%s", - alias, + charAlias, connectHost, dev->data.udp.connectService, bindHost, bindService); @@ -4930,8 +4934,8 @@ qemuBuildChrChardevStr(virLogManagerPtr logManager, case VIR_DOMAIN_CHR_TYPE_TCP: telnet = dev->data.tcp.protocol == VIR_DOMAIN_CHR_TCP_PROTOCOL_TELNET; virBufferAsprintf(&buf, - "socket,id=char%s,host=%s,port=%s%s", - alias, + "socket,id=%s,host=%s,port=%s%s", + charAlias, dev->data.tcp.host, dev->data.tcp.service, telnet ? ",telnet" : ""); @@ -4956,7 +4960,7 @@ qemuBuildChrChardevStr(virLogManagerPtr logManager, break;
case VIR_DOMAIN_CHR_TYPE_UNIX: - virBufferAsprintf(&buf, "socket,id=char%s,path=", alias); + virBufferAsprintf(&buf, "socket,id=%s,path=", charAlias); virQEMUBuildBufferEscapeComma(&buf, dev->data.nix.path); if (dev->data.nix.listen) virBufferAdd(&buf, nowait ? ",server,nowait" : ",server", -1); @@ -4968,7 +4972,7 @@ qemuBuildChrChardevStr(virLogManagerPtr logManager, _("spicevmc not supported in this QEMU binary")); goto error; } - virBufferAsprintf(&buf, "spicevmc,id=char%s,name=%s", alias, + virBufferAsprintf(&buf, "spicevmc,id=%s,name=%s", charAlias, virDomainChrSpicevmcTypeToString(dev->data.spicevmc)); break;
@@ -4978,7 +4982,7 @@ qemuBuildChrChardevStr(virLogManagerPtr logManager, _("spiceport not supported in this QEMU binary")); goto error; } - virBufferAsprintf(&buf, "spiceport,id=char%s,name=%s", alias, + virBufferAsprintf(&buf, "spiceport,id=%s,name=%s", charAlias, dev->data.spiceport.channel); break;
@@ -5007,6 +5011,7 @@ qemuBuildChrChardevStr(virLogManagerPtr logManager, return virBufferContentAndReset(&buf);
error: + VIR_FREE(charAlias); virBufferFreeAndReset(&buf); return NULL; }
On Tue, Oct 18, 2016 at 10:22:50AM -0400, John Ferlan wrote:
On 10/18/2016 09:58 AM, Pavel Hrdina wrote:
Instead of typing the prefix every time we want to append parameters to qemu command line use a variable that contains prefixed alias.
Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- src/qemu/qemu_command.c | 35 ++++++++++++++++++++--------------- 1 file changed, 20 insertions(+), 15 deletions(-)
Why not create a qemu_alias.c helper that then can also be used in your followup patch?
That's a good point, I did not realize that we have qemu_alias.c. I'll send v2, thanks. Pavel
There was inconsistency between alias used to create tls-creds-x509 object and alias used to link that object to chardev while hotpluging. In XML we have for example alias "serial0", but on qemu command line we generate "charserial0". The issue was that code, that creates QMP command to hotplug chardev devices uses only the second alias "charserial0" and that alias is also used to link the tls-creds-x509 object. This patch unifies the aliases for tls-creds-x509 to be always generated from "charserial0". Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- src/qemu/qemu_command.c | 4 ++-- src/qemu/qemu_hotplug.c | 9 +++++++-- .../qemuxml2argv-serial-tcp-tlsx509-chardev-verify.args | 4 ++-- .../qemuxml2argv-serial-tcp-tlsx509-chardev.args | 4 ++-- 4 files changed, 13 insertions(+), 8 deletions(-) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 74f65c0..8d87e69 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -4949,10 +4949,10 @@ qemuBuildChrChardevStr(virLogManagerPtr logManager, if (qemuBuildTLSx509CommandLine(cmd, cfg->chardevTLSx509certdir, dev->data.tcp.listen, cfg->chardevTLSx509verify, - alias, qemuCaps) < 0) + charAlias, qemuCaps) < 0) goto error; - if (!(objalias = qemuAliasTLSObjFromChardevAlias(alias))) + if (!(objalias = qemuAliasTLSObjFromChardevAlias(charAlias))) goto error; virBufferAsprintf(&buf, ",tls-creds=%s", objalias); VIR_FREE(objalias); diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index c2ba935..e39bd8b 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -1738,7 +1738,7 @@ int qemuDomainAttachChrDevice(virQEMUDriverPtr driver, &tlsProps) < 0) goto cleanup; - if (!(tlsAlias = qemuAliasTLSObjFromChardevAlias(chr->info.alias))) + if (!(tlsAlias = qemuAliasTLSObjFromChardevAlias(charAlias))) goto cleanup; dev->data.tcp.tlscreds = true; } @@ -4387,6 +4387,7 @@ int qemuDomainDetachChrDevice(virQEMUDriverPtr driver, virDomainChrDefPtr tmpChr; char *objAlias = NULL; char *devstr = NULL; + char *charAlias = NULL; if (!(tmpChr = virDomainChrFind(vmdef, chr))) { virReportError(VIR_ERR_OPERATION_INVALID, "%s", @@ -4397,11 +4398,14 @@ int qemuDomainDetachChrDevice(virQEMUDriverPtr driver, if (!tmpChr->info.alias && qemuAssignDeviceChrAlias(vmdef, tmpChr, -1) < 0) goto cleanup; + if (virAsprintf(&charAlias, "char%s", tmpChr->info.alias) < 0) + goto cleanup; + sa_assert(tmpChr->info.alias); if (tmpChr->source.type == VIR_DOMAIN_CHR_TYPE_TCP && cfg->chardevTLS && - !(objAlias = qemuAliasTLSObjFromChardevAlias(tmpChr->info.alias))) + !(objAlias = qemuAliasTLSObjFromChardevAlias(charAlias))) goto cleanup; if (qemuBuildChrDeviceStr(&devstr, vmdef, chr, priv->qemuCaps) < 0) @@ -4427,6 +4431,7 @@ int qemuDomainDetachChrDevice(virQEMUDriverPtr driver, cleanup: qemuDomainResetDeviceRemoval(vm); VIR_FREE(devstr); + VIR_FREE(charAlias); virObjectUnref(cfg); return ret; diff --git a/tests/qemuxml2argvdata/qemuxml2argv-serial-tcp-tlsx509-chardev-verify.args b/tests/qemuxml2argvdata/qemuxml2argv-serial-tcp-tlsx509-chardev-verify.args index f521e33..003d11d 100644 --- a/tests/qemuxml2argvdata/qemuxml2argv-serial-tcp-tlsx509-chardev-verify.args +++ b/tests/qemuxml2argvdata/qemuxml2argv-serial-tcp-tlsx509-chardev-verify.args @@ -25,9 +25,9 @@ server,nowait \ -chardev udp,id=charserial0,host=127.0.0.1,port=2222,localaddr=127.0.0.1,\ localport=1111 \ -device isa-serial,chardev=charserial0,id=serial0 \ --object tls-creds-x509,id=objserial1_tls0,dir=/etc/pki/libvirt-chardev,\ +-object tls-creds-x509,id=objcharserial1_tls0,dir=/etc/pki/libvirt-chardev,\ endpoint=client,verify-peer=yes \ -chardev socket,id=charserial1,host=127.0.0.1,port=5555,\ -tls-creds=objserial1_tls0 \ +tls-creds=objcharserial1_tls0 \ -device isa-serial,chardev=charserial1,id=serial1 \ -device virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0x3 diff --git a/tests/qemuxml2argvdata/qemuxml2argv-serial-tcp-tlsx509-chardev.args b/tests/qemuxml2argvdata/qemuxml2argv-serial-tcp-tlsx509-chardev.args index 4c8c23e..b456cce 100644 --- a/tests/qemuxml2argvdata/qemuxml2argv-serial-tcp-tlsx509-chardev.args +++ b/tests/qemuxml2argvdata/qemuxml2argv-serial-tcp-tlsx509-chardev.args @@ -25,9 +25,9 @@ server,nowait \ -chardev udp,id=charserial0,host=127.0.0.1,port=2222,localaddr=127.0.0.1,\ localport=1111 \ -device isa-serial,chardev=charserial0,id=serial0 \ --object tls-creds-x509,id=objserial1_tls0,dir=/etc/pki/libvirt-chardev,\ +-object tls-creds-x509,id=objcharserial1_tls0,dir=/etc/pki/libvirt-chardev,\ endpoint=client,verify-peer=no \ -chardev socket,id=charserial1,host=127.0.0.1,port=5555,\ -tls-creds=objserial1_tls0 \ +tls-creds=objcharserial1_tls0 \ -device isa-serial,chardev=charserial1,id=serial1 \ -device virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0x3 -- 2.10.1
On 10/18/2016 09:58 AM, Pavel Hrdina wrote:
There was inconsistency between alias used to create tls-creds-x509 object and alias used to link that object to chardev while hotpluging.
In XML we have for example alias "serial0", but on qemu command line we generate "charserial0".
The issue was that code, that creates QMP command to hotplug chardev devices uses only the second alias "charserial0" and that alias is also used to link the tls-creds-x509 object.
Which two objects used the same ID in hotplug? tlsProps would use obj%s_tls0 and this changes it to be essentially objchar%s_tls0 Essentially I'm trying to figure out from the commit message prior to this patch what was created incorrectly. John
This patch unifies the aliases for tls-creds-x509 to be always generated from "charserial0".
Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- src/qemu/qemu_command.c | 4 ++-- src/qemu/qemu_hotplug.c | 9 +++++++-- .../qemuxml2argv-serial-tcp-tlsx509-chardev-verify.args | 4 ++-- .../qemuxml2argv-serial-tcp-tlsx509-chardev.args | 4 ++-- 4 files changed, 13 insertions(+), 8 deletions(-)
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 74f65c0..8d87e69 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -4949,10 +4949,10 @@ qemuBuildChrChardevStr(virLogManagerPtr logManager, if (qemuBuildTLSx509CommandLine(cmd, cfg->chardevTLSx509certdir, dev->data.tcp.listen, cfg->chardevTLSx509verify, - alias, qemuCaps) < 0) + charAlias, qemuCaps) < 0) goto error;
- if (!(objalias = qemuAliasTLSObjFromChardevAlias(alias))) + if (!(objalias = qemuAliasTLSObjFromChardevAlias(charAlias))) goto error; virBufferAsprintf(&buf, ",tls-creds=%s", objalias); VIR_FREE(objalias); diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index c2ba935..e39bd8b 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -1738,7 +1738,7 @@ int qemuDomainAttachChrDevice(virQEMUDriverPtr driver, &tlsProps) < 0) goto cleanup;
- if (!(tlsAlias = qemuAliasTLSObjFromChardevAlias(chr->info.alias))) + if (!(tlsAlias = qemuAliasTLSObjFromChardevAlias(charAlias))) goto cleanup; dev->data.tcp.tlscreds = true; } @@ -4387,6 +4387,7 @@ int qemuDomainDetachChrDevice(virQEMUDriverPtr driver, virDomainChrDefPtr tmpChr; char *objAlias = NULL; char *devstr = NULL; + char *charAlias = NULL;
if (!(tmpChr = virDomainChrFind(vmdef, chr))) { virReportError(VIR_ERR_OPERATION_INVALID, "%s", @@ -4397,11 +4398,14 @@ int qemuDomainDetachChrDevice(virQEMUDriverPtr driver, if (!tmpChr->info.alias && qemuAssignDeviceChrAlias(vmdef, tmpChr, -1) < 0) goto cleanup;
+ if (virAsprintf(&charAlias, "char%s", tmpChr->info.alias) < 0) + goto cleanup; +
This would seemingly need to go after the subsequent line... Although I think the subsequent line gets removed if use a qemu_alias.c helper.
sa_assert(tmpChr->info.alias);
if (tmpChr->source.type == VIR_DOMAIN_CHR_TYPE_TCP && cfg->chardevTLS && - !(objAlias = qemuAliasTLSObjFromChardevAlias(tmpChr->info.alias))) + !(objAlias = qemuAliasTLSObjFromChardevAlias(charAlias))) goto cleanup;
if (qemuBuildChrDeviceStr(&devstr, vmdef, chr, priv->qemuCaps) < 0) @@ -4427,6 +4431,7 @@ int qemuDomainDetachChrDevice(virQEMUDriverPtr driver, cleanup: qemuDomainResetDeviceRemoval(vm); VIR_FREE(devstr); + VIR_FREE(charAlias); virObjectUnref(cfg); return ret;
diff --git a/tests/qemuxml2argvdata/qemuxml2argv-serial-tcp-tlsx509-chardev-verify.args b/tests/qemuxml2argvdata/qemuxml2argv-serial-tcp-tlsx509-chardev-verify.args index f521e33..003d11d 100644 --- a/tests/qemuxml2argvdata/qemuxml2argv-serial-tcp-tlsx509-chardev-verify.args +++ b/tests/qemuxml2argvdata/qemuxml2argv-serial-tcp-tlsx509-chardev-verify.args @@ -25,9 +25,9 @@ server,nowait \ -chardev udp,id=charserial0,host=127.0.0.1,port=2222,localaddr=127.0.0.1,\ localport=1111 \ -device isa-serial,chardev=charserial0,id=serial0 \ --object tls-creds-x509,id=objserial1_tls0,dir=/etc/pki/libvirt-chardev,\ +-object tls-creds-x509,id=objcharserial1_tls0,dir=/etc/pki/libvirt-chardev,\ endpoint=client,verify-peer=yes \ -chardev socket,id=charserial1,host=127.0.0.1,port=5555,\ -tls-creds=objserial1_tls0 \ +tls-creds=objcharserial1_tls0 \ -device isa-serial,chardev=charserial1,id=serial1 \ -device virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0x3 diff --git a/tests/qemuxml2argvdata/qemuxml2argv-serial-tcp-tlsx509-chardev.args b/tests/qemuxml2argvdata/qemuxml2argv-serial-tcp-tlsx509-chardev.args index 4c8c23e..b456cce 100644 --- a/tests/qemuxml2argvdata/qemuxml2argv-serial-tcp-tlsx509-chardev.args +++ b/tests/qemuxml2argvdata/qemuxml2argv-serial-tcp-tlsx509-chardev.args @@ -25,9 +25,9 @@ server,nowait \ -chardev udp,id=charserial0,host=127.0.0.1,port=2222,localaddr=127.0.0.1,\ localport=1111 \ -device isa-serial,chardev=charserial0,id=serial0 \ --object tls-creds-x509,id=objserial1_tls0,dir=/etc/pki/libvirt-chardev,\ +-object tls-creds-x509,id=objcharserial1_tls0,dir=/etc/pki/libvirt-chardev,\ endpoint=client,verify-peer=no \ -chardev socket,id=charserial1,host=127.0.0.1,port=5555,\ -tls-creds=objserial1_tls0 \ +tls-creds=objcharserial1_tls0 \ -device isa-serial,chardev=charserial1,id=serial1 \ -device virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0x3
On Tue, Oct 18, 2016 at 10:37:37AM -0400, John Ferlan wrote:
On 10/18/2016 09:58 AM, Pavel Hrdina wrote:
There was inconsistency between alias used to create tls-creds-x509 object and alias used to link that object to chardev while hotpluging.
In XML we have for example alias "serial0", but on qemu command line we generate "charserial0".
The issue was that code, that creates QMP command to hotplug chardev devices uses only the second alias "charserial0" and that alias is also used to link the tls-creds-x509 object.
Which two objects used the same ID in hotplug?
tlsProps would use obj%s_tls0
and this changes it to be essentially objchar%s_tls0
Essentially I'm trying to figure out from the commit message prior to this patch what was created incorrectly.
The issue is that while hotpluging chardev device those QMP command are created as I've described: { "execute":"object-add", "arguments": { "qom-type":"tls-creds-x509", "id":"objchannel3_tls0", ^^^^^^^^^^^^^^^^ "props": { "dir":"/etc/pki/libvirt-chardev", "endpoint":"server", "verify-peer":false } }, "id":"libvirt-29" } { "execute":"chardev-add", "arguments": { "id":"charchannel3", "backend": { "type":"socket", "data": { "addr": { "type":"inet", "data": { "host":"localhost", "port":"3344" } }, "wait":false, "telnet":false, "server":true, "tls-creds":"objcharchannel3_tls0" ^^^^^^^^^^^^^^^^^^^^ } } }, "id":"libvirt-30" } And as you can see the alias used to create tls-creds-x509 object is different than the one used while attaching chardev. That's because function qemuMonitorJSONAttachCharDevCommand() takes as first argument "charchannel3" instead of "channel3". So the logical solution is to use "charchannel3" as base while creating "obj%s_tls0" alias.
This patch unifies the aliases for tls-creds-x509 to be always generated from "charserial0".
Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- src/qemu/qemu_command.c | 4 ++-- src/qemu/qemu_hotplug.c | 9 +++++++-- .../qemuxml2argv-serial-tcp-tlsx509-chardev-verify.args | 4 ++-- .../qemuxml2argv-serial-tcp-tlsx509-chardev.args | 4 ++-- 4 files changed, 13 insertions(+), 8 deletions(-)
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 74f65c0..8d87e69 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -4949,10 +4949,10 @@ qemuBuildChrChardevStr(virLogManagerPtr logManager, if (qemuBuildTLSx509CommandLine(cmd, cfg->chardevTLSx509certdir, dev->data.tcp.listen, cfg->chardevTLSx509verify, - alias, qemuCaps) < 0) + charAlias, qemuCaps) < 0) goto error;
- if (!(objalias = qemuAliasTLSObjFromChardevAlias(alias))) + if (!(objalias = qemuAliasTLSObjFromChardevAlias(charAlias))) goto error; virBufferAsprintf(&buf, ",tls-creds=%s", objalias); VIR_FREE(objalias); diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index c2ba935..e39bd8b 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -1738,7 +1738,7 @@ int qemuDomainAttachChrDevice(virQEMUDriverPtr driver, &tlsProps) < 0) goto cleanup;
- if (!(tlsAlias = qemuAliasTLSObjFromChardevAlias(chr->info.alias))) + if (!(tlsAlias = qemuAliasTLSObjFromChardevAlias(charAlias))) goto cleanup; dev->data.tcp.tlscreds = true; } @@ -4387,6 +4387,7 @@ int qemuDomainDetachChrDevice(virQEMUDriverPtr driver, virDomainChrDefPtr tmpChr; char *objAlias = NULL; char *devstr = NULL; + char *charAlias = NULL;
if (!(tmpChr = virDomainChrFind(vmdef, chr))) { virReportError(VIR_ERR_OPERATION_INVALID, "%s", @@ -4397,11 +4398,14 @@ int qemuDomainDetachChrDevice(virQEMUDriverPtr driver, if (!tmpChr->info.alias && qemuAssignDeviceChrAlias(vmdef, tmpChr, -1) < 0) goto cleanup;
+ if (virAsprintf(&charAlias, "char%s", tmpChr->info.alias) < 0) + goto cleanup; +
This would seemingly need to go after the subsequent line... Although I think the subsequent line gets removed if use a qemu_alias.c helper.
The helper would be called here, so I'll move it after that assert. Thanks Pavel
sa_assert(tmpChr->info.alias);
if (tmpChr->source.type == VIR_DOMAIN_CHR_TYPE_TCP && cfg->chardevTLS && - !(objAlias = qemuAliasTLSObjFromChardevAlias(tmpChr->info.alias))) + !(objAlias = qemuAliasTLSObjFromChardevAlias(charAlias))) goto cleanup;
if (qemuBuildChrDeviceStr(&devstr, vmdef, chr, priv->qemuCaps) < 0) @@ -4427,6 +4431,7 @@ int qemuDomainDetachChrDevice(virQEMUDriverPtr driver, cleanup: qemuDomainResetDeviceRemoval(vm); VIR_FREE(devstr); + VIR_FREE(charAlias); virObjectUnref(cfg); return ret;
diff --git a/tests/qemuxml2argvdata/qemuxml2argv-serial-tcp-tlsx509-chardev-verify.args b/tests/qemuxml2argvdata/qemuxml2argv-serial-tcp-tlsx509-chardev-verify.args index f521e33..003d11d 100644 --- a/tests/qemuxml2argvdata/qemuxml2argv-serial-tcp-tlsx509-chardev-verify.args +++ b/tests/qemuxml2argvdata/qemuxml2argv-serial-tcp-tlsx509-chardev-verify.args @@ -25,9 +25,9 @@ server,nowait \ -chardev udp,id=charserial0,host=127.0.0.1,port=2222,localaddr=127.0.0.1,\ localport=1111 \ -device isa-serial,chardev=charserial0,id=serial0 \ --object tls-creds-x509,id=objserial1_tls0,dir=/etc/pki/libvirt-chardev,\ +-object tls-creds-x509,id=objcharserial1_tls0,dir=/etc/pki/libvirt-chardev,\ endpoint=client,verify-peer=yes \ -chardev socket,id=charserial1,host=127.0.0.1,port=5555,\ -tls-creds=objserial1_tls0 \ +tls-creds=objcharserial1_tls0 \ -device isa-serial,chardev=charserial1,id=serial1 \ -device virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0x3 diff --git a/tests/qemuxml2argvdata/qemuxml2argv-serial-tcp-tlsx509-chardev.args b/tests/qemuxml2argvdata/qemuxml2argv-serial-tcp-tlsx509-chardev.args index 4c8c23e..b456cce 100644 --- a/tests/qemuxml2argvdata/qemuxml2argv-serial-tcp-tlsx509-chardev.args +++ b/tests/qemuxml2argvdata/qemuxml2argv-serial-tcp-tlsx509-chardev.args @@ -25,9 +25,9 @@ server,nowait \ -chardev udp,id=charserial0,host=127.0.0.1,port=2222,localaddr=127.0.0.1,\ localport=1111 \ -device isa-serial,chardev=charserial0,id=serial0 \ --object tls-creds-x509,id=objserial1_tls0,dir=/etc/pki/libvirt-chardev,\ +-object tls-creds-x509,id=objcharserial1_tls0,dir=/etc/pki/libvirt-chardev,\ endpoint=client,verify-peer=no \ -chardev socket,id=charserial1,host=127.0.0.1,port=5555,\ -tls-creds=objserial1_tls0 \ +tls-creds=objcharserial1_tls0 \ -device isa-serial,chardev=charserial1,id=serial1 \ -device virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0x3
On 10/18/2016 10:57 AM, Pavel Hrdina wrote:
On Tue, Oct 18, 2016 at 10:37:37AM -0400, John Ferlan wrote:
On 10/18/2016 09:58 AM, Pavel Hrdina wrote:
There was inconsistency between alias used to create tls-creds-x509 object and alias used to link that object to chardev while hotpluging.
In XML we have for example alias "serial0", but on qemu command line we generate "charserial0".
The issue was that code, that creates QMP command to hotplug chardev devices uses only the second alias "charserial0" and that alias is also used to link the tls-creds-x509 object.
Which two objects used the same ID in hotplug?
tlsProps would use obj%s_tls0
and this changes it to be essentially objchar%s_tls0
Essentially I'm trying to figure out from the commit message prior to this patch what was created incorrectly.
The issue is that while hotpluging chardev device those QMP command are created as I've described:
{ "execute":"object-add", "arguments": { "qom-type":"tls-creds-x509", "id":"objchannel3_tls0", ^^^^^^^^^^^^^^^^ "props": { "dir":"/etc/pki/libvirt-chardev", "endpoint":"server", "verify-peer":false } }, "id":"libvirt-29" }
{ "execute":"chardev-add", "arguments": { "id":"charchannel3", "backend": { "type":"socket", "data": { "addr": { "type":"inet", "data": { "host":"localhost", "port":"3344" } }, "wait":false, "telnet":false, "server":true, "tls-creds":"objcharchannel3_tls0" ^^^^^^^^^^^^^^^^^^^^ } } }, "id":"libvirt-30" }
And as you can see the alias used to create tls-creds-x509 object is different than the one used while attaching chardev. That's because function qemuMonitorJSONAttachCharDevCommand() takes as first argument "charchannel3" instead of "channel3". So the logical solution is to use "charchannel3" as base while creating "obj%s_tls0" alias.
Ah... OK I see... I wasn't visualizing the chardev-add from the commit message. tks - John
This patch unifies the aliases for tls-creds-x509 to be always generated from "charserial0".
Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- src/qemu/qemu_command.c | 4 ++-- src/qemu/qemu_hotplug.c | 9 +++++++-- .../qemuxml2argv-serial-tcp-tlsx509-chardev-verify.args | 4 ++-- .../qemuxml2argv-serial-tcp-tlsx509-chardev.args | 4 ++-- 4 files changed, 13 insertions(+), 8 deletions(-)
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 74f65c0..8d87e69 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -4949,10 +4949,10 @@ qemuBuildChrChardevStr(virLogManagerPtr logManager, if (qemuBuildTLSx509CommandLine(cmd, cfg->chardevTLSx509certdir, dev->data.tcp.listen, cfg->chardevTLSx509verify, - alias, qemuCaps) < 0) + charAlias, qemuCaps) < 0) goto error;
- if (!(objalias = qemuAliasTLSObjFromChardevAlias(alias))) + if (!(objalias = qemuAliasTLSObjFromChardevAlias(charAlias))) goto error; virBufferAsprintf(&buf, ",tls-creds=%s", objalias); VIR_FREE(objalias); diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index c2ba935..e39bd8b 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -1738,7 +1738,7 @@ int qemuDomainAttachChrDevice(virQEMUDriverPtr driver, &tlsProps) < 0) goto cleanup;
- if (!(tlsAlias = qemuAliasTLSObjFromChardevAlias(chr->info.alias))) + if (!(tlsAlias = qemuAliasTLSObjFromChardevAlias(charAlias))) goto cleanup; dev->data.tcp.tlscreds = true; } @@ -4387,6 +4387,7 @@ int qemuDomainDetachChrDevice(virQEMUDriverPtr driver, virDomainChrDefPtr tmpChr; char *objAlias = NULL; char *devstr = NULL; + char *charAlias = NULL;
if (!(tmpChr = virDomainChrFind(vmdef, chr))) { virReportError(VIR_ERR_OPERATION_INVALID, "%s", @@ -4397,11 +4398,14 @@ int qemuDomainDetachChrDevice(virQEMUDriverPtr driver, if (!tmpChr->info.alias && qemuAssignDeviceChrAlias(vmdef, tmpChr, -1) < 0) goto cleanup;
+ if (virAsprintf(&charAlias, "char%s", tmpChr->info.alias) < 0) + goto cleanup; +
This would seemingly need to go after the subsequent line... Although I think the subsequent line gets removed if use a qemu_alias.c helper.
The helper would be called here, so I'll move it after that assert.
Thanks
Pavel
sa_assert(tmpChr->info.alias);
if (tmpChr->source.type == VIR_DOMAIN_CHR_TYPE_TCP && cfg->chardevTLS && - !(objAlias = qemuAliasTLSObjFromChardevAlias(tmpChr->info.alias))) + !(objAlias = qemuAliasTLSObjFromChardevAlias(charAlias))) goto cleanup;
if (qemuBuildChrDeviceStr(&devstr, vmdef, chr, priv->qemuCaps) < 0) @@ -4427,6 +4431,7 @@ int qemuDomainDetachChrDevice(virQEMUDriverPtr driver, cleanup: qemuDomainResetDeviceRemoval(vm); VIR_FREE(devstr); + VIR_FREE(charAlias); virObjectUnref(cfg); return ret;
diff --git a/tests/qemuxml2argvdata/qemuxml2argv-serial-tcp-tlsx509-chardev-verify.args b/tests/qemuxml2argvdata/qemuxml2argv-serial-tcp-tlsx509-chardev-verify.args index f521e33..003d11d 100644 --- a/tests/qemuxml2argvdata/qemuxml2argv-serial-tcp-tlsx509-chardev-verify.args +++ b/tests/qemuxml2argvdata/qemuxml2argv-serial-tcp-tlsx509-chardev-verify.args @@ -25,9 +25,9 @@ server,nowait \ -chardev udp,id=charserial0,host=127.0.0.1,port=2222,localaddr=127.0.0.1,\ localport=1111 \ -device isa-serial,chardev=charserial0,id=serial0 \ --object tls-creds-x509,id=objserial1_tls0,dir=/etc/pki/libvirt-chardev,\ +-object tls-creds-x509,id=objcharserial1_tls0,dir=/etc/pki/libvirt-chardev,\ endpoint=client,verify-peer=yes \ -chardev socket,id=charserial1,host=127.0.0.1,port=5555,\ -tls-creds=objserial1_tls0 \ +tls-creds=objcharserial1_tls0 \ -device isa-serial,chardev=charserial1,id=serial1 \ -device virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0x3 diff --git a/tests/qemuxml2argvdata/qemuxml2argv-serial-tcp-tlsx509-chardev.args b/tests/qemuxml2argvdata/qemuxml2argv-serial-tcp-tlsx509-chardev.args index 4c8c23e..b456cce 100644 --- a/tests/qemuxml2argvdata/qemuxml2argv-serial-tcp-tlsx509-chardev.args +++ b/tests/qemuxml2argvdata/qemuxml2argv-serial-tcp-tlsx509-chardev.args @@ -25,9 +25,9 @@ server,nowait \ -chardev udp,id=charserial0,host=127.0.0.1,port=2222,localaddr=127.0.0.1,\ localport=1111 \ -device isa-serial,chardev=charserial0,id=serial0 \ --object tls-creds-x509,id=objserial1_tls0,dir=/etc/pki/libvirt-chardev,\ +-object tls-creds-x509,id=objcharserial1_tls0,dir=/etc/pki/libvirt-chardev,\ endpoint=client,verify-peer=no \ -chardev socket,id=charserial1,host=127.0.0.1,port=5555,\ -tls-creds=objserial1_tls0 \ +tls-creds=objcharserial1_tls0 \ -device isa-serial,chardev=charserial1,id=serial1 \ -device virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0x3
participants (2)
-
John Ferlan -
Pavel Hrdina