[libvirt] [PATCH v2 0/3] fix hot(un)plug of chardev devices with TLS encryption
Pavel Hrdina (3): qemu_alias: introduce qemuAliasChardevFromDevAlias helper qemu_command: create prefixed alias to separate variable qemu: always generate the same alias for tls-creds-x509 object src/qemu/qemu_alias.c | 16 +++++++++ src/qemu/qemu_alias.h | 3 ++ src/qemu/qemu_command.c | 41 ++++++++++++---------- src/qemu/qemu_hotplug.c | 23 +++++++----- ...xml2argv-serial-tcp-tlsx509-chardev-verify.args | 4 +-- .../qemuxml2argv-serial-tcp-tlsx509-chardev.args | 4 +-- 6 files changed, 60 insertions(+), 31 deletions(-) -- 2.10.1
Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- src/qemu/qemu_alias.c | 16 ++++++++++++++++ src/qemu/qemu_alias.h | 3 +++ src/qemu/qemu_command.c | 2 +- src/qemu/qemu_hotplug.c | 14 +++++++------- 4 files changed, 27 insertions(+), 8 deletions(-) diff --git a/src/qemu/qemu_alias.c b/src/qemu/qemu_alias.c index cc83fec..9737158 100644 --- a/src/qemu/qemu_alias.c +++ b/src/qemu/qemu_alias.c @@ -606,3 +606,19 @@ qemuAliasTLSObjFromChardevAlias(const char *chardev_alias) return ret; } + + +/* qemuAliasChardevFromDevAlias: + * @devAlias: pointer do device alias + * + * Generate and return a string to be used as chardev alias. + */ +char * +qemuAliasChardevFromDevAlias(const char *devAlias) +{ + char *ret; + + ignore_value(virAsprintf(&ret, "char%s", devAlias)); + + return ret; +} diff --git a/src/qemu/qemu_alias.h b/src/qemu/qemu_alias.h index 11d9fde..d298a4d 100644 --- a/src/qemu/qemu_alias.h +++ b/src/qemu/qemu_alias.h @@ -83,4 +83,7 @@ char *qemuDomainGetSecretAESAlias(const char *srcalias, char *qemuAliasTLSObjFromChardevAlias(const char *chardev_alias) ATTRIBUTE_NONNULL(1); +char *qemuAliasChardevFromDevAlias(const char *devAlias) + ATTRIBUTE_NONNULL(1); + #endif /* __QEMU_ALIAS_H__*/ diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 21fd85c..f5ed490 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -5484,7 +5484,7 @@ qemuBuildRNGBackendProps(virDomainRNGDefPtr rng, *type = "rng-egd"; - if (virAsprintf(&charBackendAlias, "char%s", rng->info.alias) < 0) + if (!(charBackendAlias = qemuAliasChardevFromDevAlias(rng->info.alias))) goto cleanup; if (virJSONValueObjectCreate(props, "s:chardev", charBackendAlias, diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index c2ba935..af87581 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -1058,7 +1058,7 @@ qemuDomainAttachNetDevice(virQEMUDriverPtr driver, goto cleanup; } - if (virAsprintf(&charDevAlias, "char%s", net->info.alias) < 0) + if (!(charDevAlias = qemuAliasChardevFromDevAlias(net->info.alias))) goto cleanup; break; @@ -1487,7 +1487,7 @@ int qemuDomainAttachRedirdevDevice(virQEMUDriverPtr driver, if (qemuAssignDeviceRedirdevAlias(def, redirdev, -1) < 0) goto cleanup; - if (virAsprintf(&charAlias, "char%s", redirdev->info.alias) < 0) + if (!(charAlias = qemuAliasChardevFromDevAlias(redirdev->info.alias))) goto cleanup; if (!(devstr = qemuBuildRedirdevDevStr(def, redirdev, priv->qemuCaps))) @@ -1723,7 +1723,7 @@ int qemuDomainAttachChrDevice(virQEMUDriverPtr driver, if (qemuBuildChrDeviceStr(&devstr, vmdef, chr, priv->qemuCaps) < 0) goto cleanup; - if (virAsprintf(&charAlias, "char%s", chr->info.alias) < 0) + if (!(charAlias = qemuAliasChardevFromDevAlias(chr->info.alias))) goto cleanup; if (qemuDomainChrPreInsert(vmdef, chr) < 0) @@ -1858,7 +1858,7 @@ qemuDomainAttachRNGDevice(virQEMUDriverPtr driver, if (virAsprintf(&objAlias, "obj%s", rng->info.alias) < 0) goto cleanup; - if (virAsprintf(&charAlias, "char%s", rng->info.alias) < 0) + if (!(charAlias = qemuAliasChardevFromDevAlias(rng->info.alias))) goto cleanup; qemuDomainObjEnterMonitor(driver, vm); @@ -3370,7 +3370,7 @@ qemuDomainRemoveNetDevice(virQEMUDriverPtr driver, net->info.alias, vm, vm->def->name); if (virAsprintf(&hostnet_name, "host%s", net->info.alias) < 0 || - virAsprintf(&charDevAlias, "char%s", net->info.alias) < 0) + !(charDevAlias = qemuAliasChardevFromDevAlias(net->info.alias))) goto cleanup; @@ -3477,7 +3477,7 @@ qemuDomainRemoveChrDevice(virQEMUDriverPtr driver, VIR_DEBUG("Removing character device %s from domain %p %s", chr->info.alias, vm, vm->def->name); - if (virAsprintf(&charAlias, "char%s", chr->info.alias) < 0) + if (!(charAlias = qemuAliasChardevFromDevAlias(chr->info.alias))) goto cleanup; qemuDomainObjEnterMonitor(driver, vm); @@ -3522,7 +3522,7 @@ qemuDomainRemoveRNGDevice(virQEMUDriverPtr driver, if (virAsprintf(&objAlias, "obj%s", rng->info.alias) < 0) goto cleanup; - if (virAsprintf(&charAlias, "char%s", rng->info.alias) < 0) + if (!(charAlias = qemuAliasChardevFromDevAlias(rng->info.alias))) goto cleanup; qemuDomainObjEnterMonitor(driver, vm); -- 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 f5ed490..848937c 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 (!(charAlias = qemuAliasChardevFromDevAlias(alias))) + 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
There was inconsistency between alias used to create tls-creds-x509 object and alias used to link that object to chardev while hotpluging. Hotplug ends with this error: error: Failed to detach device from channel-tcp.xml error: internal error: unable to execute QEMU command 'chardev-add': No TLS credentials with id 'objcharchannel3_tls0' 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 848937c..8282162 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 af87581..2cb2267 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", @@ -4399,9 +4400,12 @@ int qemuDomainDetachChrDevice(virQEMUDriverPtr driver, sa_assert(tmpChr->info.alias); + if (!(charAlias = qemuAliasChardevFromDevAlias(tmpChr->info.alias))) + goto cleanup; + 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 11:04 AM, Pavel Hrdina wrote:
Pavel Hrdina (3): qemu_alias: introduce qemuAliasChardevFromDevAlias helper qemu_command: create prefixed alias to separate variable qemu: always generate the same alias for tls-creds-x509 object
src/qemu/qemu_alias.c | 16 +++++++++ src/qemu/qemu_alias.h | 3 ++ src/qemu/qemu_command.c | 41 ++++++++++++---------- src/qemu/qemu_hotplug.c | 23 +++++++----- ...xml2argv-serial-tcp-tlsx509-chardev-verify.args | 4 +-- .../qemuxml2argv-serial-tcp-tlsx509-chardev.args | 4 +-- 6 files changed, 60 insertions(+), 31 deletions(-)
ACK series John BTW: I did check the sa_assert won't be necessary any more...
On Tue, Oct 18, 2016 at 03:02:10PM -0400, John Ferlan wrote:
On 10/18/2016 11:04 AM, Pavel Hrdina wrote:
Pavel Hrdina (3): qemu_alias: introduce qemuAliasChardevFromDevAlias helper qemu_command: create prefixed alias to separate variable qemu: always generate the same alias for tls-creds-x509 object
src/qemu/qemu_alias.c | 16 +++++++++ src/qemu/qemu_alias.h | 3 ++ src/qemu/qemu_command.c | 41 ++++++++++++---------- src/qemu/qemu_hotplug.c | 23 +++++++----- ...xml2argv-serial-tcp-tlsx509-chardev-verify.args | 4 +-- .../qemuxml2argv-serial-tcp-tlsx509-chardev.args | 4 +-- 6 files changed, 60 insertions(+), 31 deletions(-)
ACK series
Thanks Pavel
participants (2)
-
John Ferlan -
Pavel Hrdina