[libvirt] [PATCH v5 0/5] Chardev hotplug

I've pushed all ACKed patches from previous rounds. So now just the rest. Michal Privoznik (5): qemuBuildChrDeviceCommandLine: Don't leak devstr domain_conf: Auto fill chardev port qemu: Implement chardev hotplug on config level qemu: Implement chardev hotplug on live level qemuhotplugtest: Introduce test for chardev hotplug src/conf/domain_conf.c | 52 +++++- src/conf/domain_conf.h | 3 + src/libvirt_private.syms | 1 + src/qemu/qemu_command.c | 39 ++++- src/qemu/qemu_driver.c | 42 ++++- src/qemu/qemu_hotplug.c | 175 +++++++++++++++++++ src/qemu/qemu_hotplug.h | 13 ++ tests/qemuhotplugtest.c | 194 +++++++++++++++++---- .../qemuhotplug-console-virtio.xml | 5 + .../qemuxml2argv-console-compat-2.xml | 122 +++++++++++++ 10 files changed, 597 insertions(+), 49 deletions(-) create mode 100644 tests/qemuhotplugtestdata/qemuhotplug-console-virtio.xml create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-console-compat-2.xml -- 1.8.1.5

It's caller's responsibility to free return value of qemuBuildChrDeviceStr(). --- src/qemu/qemu_command.c | 1 + 1 file changed, 1 insertion(+) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 0e517f2..d6ef9cd 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -6608,6 +6608,7 @@ qemuBuildChrDeviceCommandLine(virCommandPtr cmd, return -1; virCommandAddArgList(cmd, "-device", devstr, NULL); + VIR_FREE(devstr); return 0; } -- 1.8.1.5

On Fri, Jul 12, 2013 at 07:23:58PM +0200, Michal Privoznik wrote:
It's caller's responsibility to free return value of qemuBuildChrDeviceStr(). --- src/qemu/qemu_command.c | 1 + 1 file changed, 1 insertion(+)
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 0e517f2..d6ef9cd 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -6608,6 +6608,7 @@ qemuBuildChrDeviceCommandLine(virCommandPtr cmd, return -1;
virCommandAddArgList(cmd, "-device", devstr, NULL); + VIR_FREE(devstr); return 0; }
ACK, When was this leak introduced ? Or rather, does this need to go in any -maint branch. Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|

On 07/15/2013 08:11 AM, Daniel P. Berrange wrote:
On Fri, Jul 12, 2013 at 07:23:58PM +0200, Michal Privoznik wrote:
It's caller's responsibility to free return value of qemuBuildChrDeviceStr(). --- src/qemu/qemu_command.c | 1 + 1 file changed, 1 insertion(+)
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 0e517f2..d6ef9cd 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -6608,6 +6608,7 @@ qemuBuildChrDeviceCommandLine(virCommandPtr cmd, return -1;
virCommandAddArgList(cmd, "-device", devstr, NULL); + VIR_FREE(devstr); return 0; }
ACK, When was this leak introduced ? Or rather, does this need to go in any -maint branch.
Introduced in the v4 of the series - https://www.redhat.com/archives/libvir-list/2013-July/msg00656.html I had run valgrind and found/noted here: https://www.redhat.com/archives/libvir-list/2013-July/msg00823.html John

On 15.07.2013 15:18, John Ferlan wrote:
On 07/15/2013 08:11 AM, Daniel P. Berrange wrote:
On Fri, Jul 12, 2013 at 07:23:58PM +0200, Michal Privoznik wrote:
It's caller's responsibility to free return value of qemuBuildChrDeviceStr(). --- src/qemu/qemu_command.c | 1 + 1 file changed, 1 insertion(+)
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 0e517f2..d6ef9cd 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -6608,6 +6608,7 @@ qemuBuildChrDeviceCommandLine(virCommandPtr cmd, return -1;
virCommandAddArgList(cmd, "-device", devstr, NULL); + VIR_FREE(devstr); return 0; }
ACK, When was this leak introduced ? Or rather, does this need to go in any -maint branch.
Introduced in the v4 of the series -
https://www.redhat.com/archives/libvir-list/2013-July/msg00656.html
I had run valgrind and found/noted here:
https://www.redhat.com/archives/libvir-list/2013-July/msg00823.html
John
I've pushed this one now. Michal

Now that we have callbacks, we should auto fill in omitted pieces of information. It's important for chardev hotplug to fill in the correct /{serial,parallel,console,channel}/target/@port if no value has been provided by user. --- src/conf/domain_conf.c | 46 +++++++++++++++++++++++++++++++++++++++++----- 1 file changed, 41 insertions(+), 5 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 5f0366e..80ea18b 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -2780,13 +2780,49 @@ virDomainDefPostParseInternal(virDomainDefPtr def, static int virDomainDeviceDefPostParseInternal(virDomainDeviceDefPtr dev, - virDomainDefPtr def ATTRIBUTE_UNUSED, + virDomainDefPtr def, virCapsPtr caps ATTRIBUTE_UNUSED) { - if (dev->type == VIR_DOMAIN_DEVICE_CHR && - dev->data.chr->deviceType == VIR_DOMAIN_CHR_DEVICE_TYPE_CONSOLE && - dev->data.chr->targetType == VIR_DOMAIN_CHR_CONSOLE_TARGET_TYPE_NONE) - dev->data.chr->targetType = VIR_DOMAIN_CHR_CONSOLE_TARGET_TYPE_SERIAL; + if (dev->type == VIR_DOMAIN_DEVICE_CHR) { + virDomainChrDefPtr chr = dev->data.chr; + virDomainChrDefPtr **arrPtr; + size_t i, *cnt; + + virDomainChrGetDomainPtrs(def, chr, &arrPtr, &cnt); + + if (chr->deviceType == VIR_DOMAIN_CHR_DEVICE_TYPE_CONSOLE && + chr->targetType == VIR_DOMAIN_CHR_CONSOLE_TARGET_TYPE_NONE) + chr->targetType = VIR_DOMAIN_CHR_CONSOLE_TARGET_TYPE_SERIAL; + + if (chr->target.port == -1 && + (chr->deviceType == VIR_DOMAIN_CHR_DEVICE_TYPE_PARALLEL || + chr->deviceType == VIR_DOMAIN_CHR_DEVICE_TYPE_SERIAL || + chr->deviceType == VIR_DOMAIN_CHR_DEVICE_TYPE_CONSOLE)) { + int maxport = -1; + + for (i = 0; i < *cnt; i++) { + if ((*arrPtr)[i]->target.port > maxport) + maxport = (*arrPtr)[i]->target.port; + } + + chr->target.port = maxport + 1; + } + + if (chr->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_VIRTIO_SERIAL && + chr->info.addr.vioserial.port == 0) { + int maxport = 0; + + for (i = 0; i < *cnt; i++) { + virDomainChrDefPtr thischr = (*arrPtr)[i]; + if (thischr->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_VIRTIO_SERIAL && + thischr->info.addr.vioserial.controller == chr->info.addr.vioserial.controller && + thischr->info.addr.vioserial.bus == chr->info.addr.vioserial.bus && + (int)thischr->info.addr.vioserial.port > maxport) + maxport = thischr->info.addr.vioserial.port; + } + chr->info.addr.vioserial.port = maxport + 1; + } + } return 0; } -- 1.8.1.5

On Fri, Jul 12, 2013 at 07:23:59PM +0200, Michal Privoznik wrote:
Now that we have callbacks, we should auto fill in omitted pieces of information. It's important for chardev hotplug to fill in the correct /{serial,parallel,console,channel}/target/@port if no value has been provided by user. --- src/conf/domain_conf.c | 46 +++++++++++++++++++++++++++++++++++++++++----- 1 file changed, 41 insertions(+), 5 deletions(-)
ACK Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|

There are two levels on which a device may be hotplugged: config and live. The config level requires just an insert or remove from internal domain definition structure, which is exactly what this patch does. There is currently no implementation for a chardev update action, as there's not much to be updated. But more importantly, the only thing that can be updated is path or socket address by which chardevs are distinguished. So the update action is currently not supported. --- src/conf/domain_conf.c | 6 ++-- src/conf/domain_conf.h | 3 ++ src/libvirt_private.syms | 1 + src/qemu/qemu_driver.c | 16 +++++++++++ src/qemu/qemu_hotplug.c | 72 ++++++++++++++++++++++++++++++++++++++++++++++++ src/qemu/qemu_hotplug.h | 7 +++++ 6 files changed, 103 insertions(+), 2 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 80ea18b..1d3ba77 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -10092,7 +10092,7 @@ virDomainLeaseRemove(virDomainDefPtr def, return virDomainLeaseRemoveAt(def, idx); } -static bool +bool virDomainChrEquals(virDomainChrDefPtr src, virDomainChrDefPtr tgt) { @@ -18029,9 +18029,11 @@ virDomainDeviceDefCopy(virDomainDeviceDefPtr src, case VIR_DOMAIN_DEVICE_RNG: rc = virDomainRNGDefFormat(&buf, src->data.rng, flags); break; + case VIR_DOMAIN_DEVICE_CHR: + rc = virDomainChrDefFormat(&buf, src->data.chr, flags); + break; case VIR_DOMAIN_DEVICE_NONE: case VIR_DOMAIN_DEVICE_SMARTCARD: - case VIR_DOMAIN_DEVICE_CHR: case VIR_DOMAIN_DEVICE_MEMBALLOON: case VIR_DOMAIN_DEVICE_NVRAM: case VIR_DOMAIN_DEVICE_LAST: diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index ef72d24..c26f4e2 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -2399,6 +2399,9 @@ virDomainChrGetDomainPtrs(virDomainDefPtr vmdef, virDomainChrDefPtr virDomainChrFind(virDomainDefPtr def, virDomainChrDefPtr target); +bool +virDomainChrEquals(virDomainChrDefPtr src, + virDomainChrDefPtr tgt); int virDomainChrInsert(virDomainDefPtr vmdef, virDomainChrDefPtr chr); diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 0ab7632..31eae1c 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -114,6 +114,7 @@ virDomainChrConsoleTargetTypeToString; virDomainChrDefForeach; virDomainChrDefFree; virDomainChrDefNew; +virDomainChrEquals; virDomainChrFind; virDomainChrGetDomainPtrs; virDomainChrInsert; diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 495867a..852db8b 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -6686,6 +6686,12 @@ qemuDomainAttachDeviceConfig(virQEMUCapsPtr qemuCaps, return -1; break; + case VIR_DOMAIN_DEVICE_CHR: + if (qemuDomainChrInsert(vmdef, dev->data.chr) < 0) + return -1; + dev->data.chr = NULL; + break; + default: virReportError(VIR_ERR_OPERATION_UNSUPPORTED, _("persistent attach of device '%s' is not supported"), @@ -6705,6 +6711,7 @@ qemuDomainDetachDeviceConfig(virDomainDefPtr vmdef, virDomainHostdevDefPtr hostdev, det_hostdev; virDomainLeaseDefPtr lease, det_lease; virDomainControllerDefPtr cont, det_cont; + virDomainChrDefPtr chr; int idx; char mac[VIR_MAC_STRING_BUFLEN]; @@ -6772,6 +6779,15 @@ qemuDomainDetachDeviceConfig(virDomainDefPtr vmdef, break; + case VIR_DOMAIN_DEVICE_CHR: + if (!(chr = qemuDomainChrRemove(vmdef, dev->data.chr))) + return -1; + + virDomainChrDefFree(chr); + virDomainChrDefFree(dev->data.chr); + dev->data.chr = NULL; + break; + default: virReportError(VIR_ERR_OPERATION_UNSUPPORTED, _("persistent detach of device '%s' is not supported"), diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index e05b4b3..94f29e5 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -1162,6 +1162,78 @@ error: } +int +qemuDomainChrInsert(virDomainDefPtr vmdef, + virDomainChrDefPtr chr) +{ + if (chr->deviceType == VIR_DOMAIN_CHR_DEVICE_TYPE_CONSOLE && + chr->targetType == VIR_DOMAIN_CHR_CONSOLE_TARGET_TYPE_SERIAL) { + virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s", + _("attaching serial console is not supported")); + return -1; + } + + if (virDomainChrFind(vmdef, chr)) { + virReportError(VIR_ERR_OPERATION_INVALID, "%s", + _("chardev already exists")); + return -1; + } + + if (virDomainChrInsert(vmdef, chr) < 0) + return -1; + + /* Due to some crazy backcompat stuff, the first serial device is an alias + * to the first console too. If this is the case, the definition must be + * duplicated as first console device. */ + if (vmdef->nserials == 1 && vmdef->nconsoles == 0) { + if ((!vmdef->consoles && VIR_ALLOC(vmdef->consoles) < 0) || + VIR_ALLOC(vmdef->consoles[0]) < 0) { + virDomainChrRemove(vmdef, chr); + return -1; + } + vmdef->nconsoles = 1; + + /* Create an console alias for the serial port */ + vmdef->consoles[0]->deviceType = VIR_DOMAIN_CHR_DEVICE_TYPE_CONSOLE; + vmdef->consoles[0]->targetType = VIR_DOMAIN_CHR_CONSOLE_TARGET_TYPE_SERIAL; + } + + return 0; +} + +virDomainChrDefPtr +qemuDomainChrRemove(virDomainDefPtr vmdef, + virDomainChrDefPtr chr) +{ + virDomainChrDefPtr ret; + bool removeCompat; + + if (chr->deviceType == VIR_DOMAIN_CHR_DEVICE_TYPE_CONSOLE && + chr->targetType == VIR_DOMAIN_CHR_CONSOLE_TARGET_TYPE_SERIAL) { + virReportError(VIR_ERR_OPERATION_INVALID, "%s", + _("detaching serial console is not supported")); + return NULL; + } + + /* Due to some crazy backcompat stuff, the first serial device is an alias + * to the first console too. If this is the case, the definition must be + * duplicated as first console device. */ + removeCompat = vmdef->nserials && vmdef->nconsoles && + vmdef->consoles[0]->deviceType == VIR_DOMAIN_CHR_DEVICE_TYPE_CONSOLE && + vmdef->consoles[0]->targetType == VIR_DOMAIN_CHR_CONSOLE_TARGET_TYPE_SERIAL && + virDomainChrEquals(vmdef->serials[0], chr); + + if (!(ret = virDomainChrRemove(vmdef, chr))) { + virReportError(VIR_ERR_INVALID_ARG, "%s", + _("device not present in domain configuration")); + return NULL; + } + + if (removeCompat) + VIR_DELETE_ELEMENT(vmdef->consoles, 0, vmdef->nconsoles); + + return ret; +} int qemuDomainAttachHostUsbDevice(virQEMUDriverPtr driver, virDomainObjPtr vm, virDomainHostdevDefPtr hostdev) diff --git a/src/qemu/qemu_hotplug.h b/src/qemu/qemu_hotplug.h index da20eb1..c947d56 100644 --- a/src/qemu/qemu_hotplug.h +++ b/src/qemu/qemu_hotplug.h @@ -106,4 +106,11 @@ int qemuDomainDetachLease(virQEMUDriverPtr driver, virDomainLeaseDefPtr lease); +int +qemuDomainChrInsert(virDomainDefPtr vmdef, + virDomainChrDefPtr chr); +virDomainChrDefPtr +qemuDomainChrRemove(virDomainDefPtr vmdef, + virDomainChrDefPtr chr); + #endif /* __QEMU_HOTPLUG_H__ */ -- 1.8.1.5

On Fri, Jul 12, 2013 at 07:24:00PM +0200, Michal Privoznik wrote:
There are two levels on which a device may be hotplugged: config and live. The config level requires just an insert or remove from internal domain definition structure, which is exactly what this patch does. There is currently no implementation for a chardev update action, as there's not much to be updated. But more importantly, the only thing that can be updated is path or socket address by which chardevs are distinguished. So the update action is currently not supported. --- src/conf/domain_conf.c | 6 ++-- src/conf/domain_conf.h | 3 ++ src/libvirt_private.syms | 1 + src/qemu/qemu_driver.c | 16 +++++++++++ src/qemu/qemu_hotplug.c | 72 ++++++++++++++++++++++++++++++++++++++++++++++++ src/qemu/qemu_hotplug.h | 7 +++++ 6 files changed, 103 insertions(+), 2 deletions(-)
ACK Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|

Since previous patches has prepared everything for us, we may now implement live hotplug of a character device. --- src/qemu/qemu_command.c | 38 +++++++++++++++++- src/qemu/qemu_driver.c | 26 ++++++++++-- src/qemu/qemu_hotplug.c | 103 ++++++++++++++++++++++++++++++++++++++++++++++++ src/qemu/qemu_hotplug.h | 6 +++ 4 files changed, 168 insertions(+), 5 deletions(-) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index d6ef9cd..735f300 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -863,8 +863,41 @@ qemuAssignDeviceControllerAlias(virDomainControllerDefPtr controller) return virAsprintf(&controller->info.alias, "%s%d", prefix, controller->idx); } +static ssize_t +qemuGetNextChrDevIndex(virDomainDefPtr def, + virDomainChrDefPtr chr, + const char *prefix) +{ + virDomainChrDefPtr **arrPtr; + size_t *cntPtr; + size_t i; + ssize_t idx = 0; + const char *prefix2 = NULL; + + if (chr->deviceType == VIR_DOMAIN_CHR_DEVICE_TYPE_CONSOLE) + prefix2 = "serial"; + + virDomainChrGetDomainPtrs(def, chr, &arrPtr, &cntPtr); + + for (i = 0; i < *cntPtr; i++) { + ssize_t thisidx; + if (((thisidx = qemuDomainDeviceAliasIndex(&(*arrPtr)[i]->info, prefix)) < 0) && + (prefix2 && + (thisidx = qemuDomainDeviceAliasIndex(&(*arrPtr)[i]->info, prefix2)) < 0)) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Unable to determine device index for character device")); + return -1; + } + if (thisidx >= idx) + idx = thisidx + 1; + } + + return idx; +} + + int -qemuAssignDeviceChrAlias(virDomainDefPtr def ATTRIBUTE_UNUSED, +qemuAssignDeviceChrAlias(virDomainDefPtr def, virDomainChrDefPtr chr, ssize_t idx) { @@ -891,6 +924,9 @@ qemuAssignDeviceChrAlias(virDomainDefPtr def ATTRIBUTE_UNUSED, return -1; } + if (idx == -1 && (idx = qemuGetNextChrDevIndex(def, chr, prefix)) < 0) + return -1; + return virAsprintf(&chr->info.alias, "%s%zd", prefix, idx); } diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 852db8b..b4a668a 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -6390,6 +6390,13 @@ qemuDomainAttachDeviceLive(virDomainObjPtr vm, dev->data.redirdev = NULL; break; + case VIR_DOMAIN_DEVICE_CHR: + ret = qemuDomainAttachChrDevice(driver, vm, + dev->data.chr); + if (!ret) + dev->data.chr = NULL; + break; + default: virReportError(VIR_ERR_OPERATION_UNSUPPORTED, _("live attach of device '%s' is not supported"), @@ -6477,6 +6484,9 @@ qemuDomainDetachDeviceLive(virDomainObjPtr vm, case VIR_DOMAIN_DEVICE_HOSTDEV: ret = qemuDomainDetachHostDevice(driver, vm, dev); break; + case VIR_DOMAIN_DEVICE_CHR: + ret = qemuDomainDetachChrDevice(driver, vm, dev->data.chr); + break; default: virReportError(VIR_ERR_OPERATION_UNSUPPORTED, _("live detach of device '%s' is not supported"), @@ -6886,7 +6896,7 @@ static int qemuDomainAttachDeviceFlags(virDomainPtr dom, const char *xml, virDomainDefPtr vmdef = NULL; virDomainDeviceDefPtr dev = NULL, dev_copy = NULL; int ret = -1; - unsigned int affect; + unsigned int affect, parse_flags = 0; virQEMUCapsPtr qemuCaps = NULL; qemuDomainObjPrivatePtr priv; virQEMUDriverConfigPtr cfg = NULL; @@ -6934,9 +6944,13 @@ static int qemuDomainAttachDeviceFlags(virDomainPtr dom, const char *xml, goto endjob; } + if ((flags & VIR_DOMAIN_AFFECT_CONFIG) && + !(flags & VIR_DOMAIN_AFFECT_LIVE)) + parse_flags |= VIR_DOMAIN_XML_INACTIVE; + dev = dev_copy = virDomainDeviceDefParse(xml, vm->def, caps, driver->xmlopt, - VIR_DOMAIN_XML_INACTIVE); + parse_flags); if (dev == NULL) goto endjob; @@ -7164,7 +7178,7 @@ static int qemuDomainDetachDeviceFlags(virDomainPtr dom, const char *xml, virDomainDefPtr vmdef = NULL; virDomainDeviceDefPtr dev = NULL, dev_copy = NULL; int ret = -1; - unsigned int affect; + unsigned int affect, parse_flags = 0; virQEMUCapsPtr qemuCaps = NULL; qemuDomainObjPrivatePtr priv; virQEMUDriverConfigPtr cfg = NULL; @@ -7212,9 +7226,13 @@ static int qemuDomainDetachDeviceFlags(virDomainPtr dom, const char *xml, goto endjob; } + if ((flags & VIR_DOMAIN_AFFECT_CONFIG) && + !(flags & VIR_DOMAIN_AFFECT_LIVE)) + parse_flags |= VIR_DOMAIN_XML_INACTIVE; + dev = dev_copy = virDomainDeviceDefParse(xml, vm->def, caps, driver->xmlopt, - VIR_DOMAIN_XML_INACTIVE); + parse_flags); if (dev == NULL) goto endjob; diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index 94f29e5..966e11c 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -1234,6 +1234,62 @@ qemuDomainChrRemove(virDomainDefPtr vmdef, return ret; } + +int qemuDomainAttachChrDevice(virQEMUDriverPtr driver, + virDomainObjPtr vm, + virDomainChrDefPtr chr) +{ + int ret = -1; + qemuDomainObjPrivatePtr priv = vm->privateData; + virDomainDefPtr vmdef = vm->def; + char *devstr = NULL; + char *charAlias = NULL; + bool remove = false; + + if (!virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_DEVICE)) { + virReportError(VIR_ERR_OPERATION_INVALID, "%s", + _("qemu does not support -device")); + return ret; + } + + if (qemuAssignDeviceChrAlias(vmdef, chr, -1) < 0) + return ret; + + if (qemuBuildChrDeviceStr(&devstr, vm->def, chr, priv->qemuCaps) < 0) + return ret; + + if (virAsprintf(&charAlias, "char%s", chr->info.alias) < 0) { + virReportOOMError(); + goto cleanup; + } + + if (qemuDomainChrInsert(vmdef, chr) < 0) + goto cleanup; + remove = true; + + qemuDomainObjEnterMonitor(driver, vm); + if (qemuMonitorAttachCharDev(priv->mon, charAlias, &chr->source) < 0) { + qemuDomainObjExitMonitor(driver, vm); + goto cleanup; + } + + if (devstr && qemuMonitorAddDevice(priv->mon, devstr) < 0) { + /* detach associated chardev on error */ + qemuMonitorDetachCharDev(priv->mon, charAlias); + qemuDomainObjExitMonitor(driver, vm); + goto cleanup; + } + qemuDomainObjExitMonitor(driver, vm); + + ret = 0; +cleanup: + if (ret < 0 && remove) + qemuDomainChrRemove(vmdef, chr); + VIR_FREE(charAlias); + VIR_FREE(devstr); + return ret; +} + int qemuDomainAttachHostUsbDevice(virQEMUDriverPtr driver, virDomainObjPtr vm, virDomainHostdevDefPtr hostdev) @@ -3068,3 +3124,50 @@ int qemuDomainDetachLease(virQEMUDriverPtr driver, virDomainLeaseDefFree(det_lease); return 0; } + +int qemuDomainDetachChrDevice(virQEMUDriverPtr driver, + virDomainObjPtr vm, + virDomainChrDefPtr chr) +{ + int ret = -1; + qemuDomainObjPrivatePtr priv = vm->privateData; + virDomainDefPtr vmdef = vm->def; + virDomainChrDefPtr tmpChr; + char *charAlias = NULL; + char *devstr = NULL; + + if (!(tmpChr = virDomainChrFind(vmdef, chr))) { + virReportError(VIR_ERR_OPERATION_INVALID, "%s", + _("device not present in domain configuration")); + return ret; + } + + if (qemuBuildChrDeviceStr(&devstr, vm->def, chr, priv->qemuCaps) < 0) + return ret; + + if (virAsprintf(&charAlias, "char%s", tmpChr->info.alias) < 0) { + virReportOOMError(); + return ret; + } + + qemuDomainObjEnterMonitor(driver, vm); + if (devstr && qemuMonitorDelDevice(priv->mon, tmpChr->info.alias) < 0) { + qemuDomainObjExitMonitor(driver, vm); + goto cleanup; + } + + if (qemuMonitorDetachCharDev(priv->mon, charAlias) < 0) { + qemuDomainObjExitMonitor(driver, vm); + goto cleanup; + } + qemuDomainObjExitMonitor(driver, vm); + + qemuDomainChrRemove(vmdef, tmpChr); + virDomainChrDefFree(tmpChr); + ret = 0; + +cleanup: + VIR_FREE(devstr); + VIR_FREE(charAlias); + return ret; +} diff --git a/src/qemu/qemu_hotplug.h b/src/qemu/qemu_hotplug.h index c947d56..3f7e77e 100644 --- a/src/qemu/qemu_hotplug.h +++ b/src/qemu/qemu_hotplug.h @@ -104,6 +104,12 @@ int qemuDomainAttachLease(virQEMUDriverPtr driver, int qemuDomainDetachLease(virQEMUDriverPtr driver, virDomainObjPtr vm, virDomainLeaseDefPtr lease); +int qemuDomainAttachChrDevice(virQEMUDriverPtr driver, + virDomainObjPtr vm, + virDomainChrDefPtr chr); +int qemuDomainDetachChrDevice(virQEMUDriverPtr driver, + virDomainObjPtr vm, + virDomainChrDefPtr chr); int -- 1.8.1.5

On Fri, Jul 12, 2013 at 07:24:01PM +0200, Michal Privoznik wrote:
Since previous patches has prepared everything for us, we may now implement live hotplug of a character device. --- src/qemu/qemu_command.c | 38 +++++++++++++++++- src/qemu/qemu_driver.c | 26 ++++++++++-- src/qemu/qemu_hotplug.c | 103 ++++++++++++++++++++++++++++++++++++++++++++++++ src/qemu/qemu_hotplug.h | 6 +++ 4 files changed, 168 insertions(+), 5 deletions(-)
ACK Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|

The test is currently testing just device update function. However, chardev hotplug is implemented just for device attach and detach. This fact means, the test needs to be rewritten (the majority of the code is still shared). Moreover, we are now able to pass VM among multiple test runs. So for instance, while we add a device in the first run, we can remove it in the second run. --- tests/qemuhotplugtest.c | 194 +++++++++++++++++---- .../qemuhotplug-console-virtio.xml | 5 + .../qemuxml2argv-console-compat-2.xml | 122 +++++++++++++ 3 files changed, 284 insertions(+), 37 deletions(-) create mode 100644 tests/qemuhotplugtestdata/qemuhotplug-console-virtio.xml create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-console-compat-2.xml diff --git a/tests/qemuhotplugtest.c b/tests/qemuhotplugtest.c index 15d70d2..d4971c2 100644 --- a/tests/qemuhotplugtest.c +++ b/tests/qemuhotplugtest.c @@ -33,11 +33,20 @@ static virQEMUDriver driver; +enum { + ATTACH, + DETACH, + UPDATE +}; + struct qemuHotplugTestData { const char *domain_filename; const char *device_filename; bool fail; const char *const *mon; + int action; + bool keep; + virDomainObjPtr vm; }; static int @@ -46,6 +55,7 @@ qemuHotplugCreateObjects(virDomainXMLOptionPtr xmlopt, const char *filename) { int ret = -1; + qemuDomainObjPrivatePtr priv = NULL; if (!(*vm = virDomainObjNew(xmlopt))) goto cleanup; @@ -57,12 +67,85 @@ qemuHotplugCreateObjects(virDomainXMLOptionPtr xmlopt, 0))) goto cleanup; + priv = (*vm)->privateData; + + if (!(priv->qemuCaps = virQEMUCapsNew())) + goto cleanup; + + /* for attach & detach qemu must support -device */ + virQEMUCapsSet(priv->qemuCaps, QEMU_CAPS_DEVICE); + ret = 0; cleanup: return ret; } static int +testQemuHotplugAttach(virDomainObjPtr vm, + virDomainDeviceDefPtr dev) +{ + int ret = -1; + + switch (dev->type) { + case VIR_DOMAIN_DEVICE_CHR: + ret = qemuDomainAttachChrDevice(&driver, vm, dev->data.chr); + break; + default: + if (virTestGetVerbose()) + fprintf(stderr, "device type '%s' cannot be attached", + virDomainDeviceTypeToString(dev->type)); + break; + } + + return ret; +} + +static int +testQemuHotplugDetach(virDomainObjPtr vm, + virDomainDeviceDefPtr dev) +{ + int ret = -1; + + switch (dev->type) { + case VIR_DOMAIN_DEVICE_CHR: + ret = qemuDomainDetachChrDevice(&driver, vm, dev->data.chr); + break; + default: + if (virTestGetVerbose()) + fprintf(stderr, "device type '%s' cannot be attached", + virDomainDeviceTypeToString(dev->type)); + break; + } + + return ret; +} + +static int +testQemuHotplugUpdate(virDomainObjPtr vm, + virDomainDeviceDefPtr dev) +{ + int ret = -1; + + /* XXX Ideally, we would call qemuDomainUpdateDeviceLive here. But that + * would require us to provide virConnectPtr and virDomainPtr (they're used + * in case of updating a disk device. So for now, we will proceed with + * breaking the function into pieces. If we ever learn how to fake those + * required object, we can replace this code then. */ + switch (dev->type) { + case VIR_DOMAIN_DEVICE_GRAPHICS: + ret = qemuDomainChangeGraphics(&driver, vm, dev->data.graphics); + break; + default: + if (virTestGetVerbose()) + fprintf(stderr, "device type '%s' cannot be updated", + virDomainDeviceTypeToString(dev->type)); + break; + } + + return ret; +} + +static int testQemuHotplug(const void *data) { int ret = -1; @@ -72,6 +155,7 @@ testQemuHotplug(const void *data) char *device_xml = NULL; const char *const *tmp; bool fail = test->fail; + bool keep = test->keep; virDomainObjPtr vm = NULL; virDomainDeviceDefPtr dev = NULL; virCapsPtr caps = NULL; @@ -87,17 +171,18 @@ testQemuHotplug(const void *data) if (!(caps = virQEMUDriverGetCapabilities(&driver, false))) goto cleanup; - if (qemuHotplugCreateObjects(driver.xmlopt, &vm, domain_filename) < 0) - goto cleanup; - - priv = vm->privateData; + if (test->vm) { + vm = test->vm; + } else { + if (qemuHotplugCreateObjects(driver.xmlopt, &vm, domain_filename) < 0) + goto cleanup; + } if (virtTestLoadFile(device_filename, &device_xml) < 0) goto cleanup; if (!(dev = virDomainDeviceDefParse(device_xml, vm->def, - caps, driver.xmlopt, - VIR_DOMAIN_XML_INACTIVE))) + caps, driver.xmlopt, 0))) goto cleanup; /* Now is the best time to feed the spoofed monitor with predefined @@ -117,6 +202,7 @@ testQemuHotplug(const void *data) goto cleanup; } + priv = vm->privateData; priv->mon = qemuMonitorTestGetMonitor(test_mon); priv->monJSON = true; @@ -125,20 +211,22 @@ testQemuHotplug(const void *data) * tries to lock it again */ virObjectUnlock(priv->mon); - /* XXX Ideally, we would call qemuDomainUpdateDeviceLive here. But that - * would require us to provide virConnectPtr and virDomainPtr (they're used - * in case of updating a disk device. So for now, we will proceed with - * breaking the function into pieces. If we ever learn how to fake those - * required object, we can replace this code then. */ - switch (dev->type) { - case VIR_DOMAIN_DEVICE_GRAPHICS: - ret = qemuDomainChangeGraphics(&driver, vm, dev->data.graphics); + switch (test->action) { + case ATTACH: + ret = testQemuHotplugAttach(vm, dev); + if (!ret) { + /* avoid @dev double free on success, + * as @dev is part of vm->def now */ + dev = NULL; + } break; - default: - if (virTestGetVerbose()) - fprintf(stderr, "device type '%s' cannot be updated", - virDomainDeviceTypeToString(dev->type)); + + case DETACH: + ret = testQemuHotplugDetach(vm, dev); break; + + case UPDATE: + ret = testQemuHotplugUpdate(vm, dev); } cleanup: @@ -148,7 +236,12 @@ cleanup: /* don't dispose test monitor with VM */ if (priv) priv->mon = NULL; - virObjectUnref(vm); + if (keep) { + test->vm = vm; + } else { + virObjectUnref(vm); + test->vm = NULL; + } virDomainDeviceDefFree(dev); virObjectUnref(caps); qemuMonitorTestFree(test_mon); @@ -159,6 +252,7 @@ static int mymain(void) { int ret = 0; + struct qemuHotplugTestData data = {0}; #if !WITH_YAJL fputs("libvirt not compiled with yajl, skipping this test\n", stderr); @@ -180,26 +274,52 @@ mymain(void) if (VIR_STRDUP_QUIET(driver.config->spicePassword, "123456") < 0) return EXIT_FAILURE; -#define DO_TEST(file, dev, fial, ...) \ - do { \ - const char *my_mon[] = { __VA_ARGS__, NULL}; \ - struct qemuHotplugTestData data = \ - {.domain_filename = file, .device_filename = dev, .fail = fial, \ - .mon = my_mon}; \ - if (virtTestRun(#file, 1, testQemuHotplug, &data) < 0) \ - ret = -1; \ +#define DO_TEST(file, dev, fial, kep, ...) \ + const char *my_mon[] = { __VA_ARGS__, NULL}; \ + data.domain_filename = file; \ + data.device_filename = dev; \ + data.fail = fial; \ + data.mon = my_mon; \ + data.keep = kep; \ + if (virtTestRun(#file, 1, testQemuHotplug, &data) < 0) \ + ret = -1; \ + +#define DO_TEST_ATTACH(file, dev, fial, kep, ...) \ + do { \ + data.action = ATTACH; \ + DO_TEST(file, dev, fial, kep, __VA_ARGS__) \ + } while (0) + +#define DO_TEST_DETACH(file, dev, fial, kep, ...) \ + do { \ + data.action = DETACH; \ + DO_TEST(file, dev, fial, kep, __VA_ARGS__) \ + } while (0) + +#define DO_TEST_UPDATE(file, dev, fial, kep, ...) \ + do { \ + data.action = UPDATE; \ + DO_TEST(file, dev, fial, kep, __VA_ARGS__) \ } while (0) - DO_TEST("graphics-spice", "graphics-spice-nochange", false, NULL); - DO_TEST("graphics-spice-timeout", "graphics-spice-timeout-nochange", false, - "set_password", "{\"return\":{}}", "expire_password", "{\"return\":{}}"); - DO_TEST("graphics-spice-timeout", "graphics-spice-timeout-password", false, - "set_password", "{\"return\":{}}", "expire_password", "{\"return\":{}}"); - DO_TEST("graphics-spice", "graphics-spice-listen", true, NULL); - DO_TEST("graphics-spice-listen-network", "graphics-spice-listen-network", false, - "set_password", "{\"return\":{}}", "expire_password", "{\"return\":{}}"); - /* Strange huh? Currently, only graphics can be testet :-P */ - DO_TEST("disk-cdrom", "disk-cdrom-nochange", true, NULL); + DO_TEST_UPDATE("graphics-spice", "graphics-spice-nochange", false, false, NULL); + DO_TEST_UPDATE("graphics-spice-timeout", "graphics-spice-timeout-nochange", false, false, + "set_password", "{\"return\":{}}", "expire_password", "{\"return\":{}}"); + DO_TEST_UPDATE("graphics-spice-timeout", "graphics-spice-timeout-password", false, false, + "set_password", "{\"return\":{}}", "expire_password", "{\"return\":{}}"); + DO_TEST_UPDATE("graphics-spice", "graphics-spice-listen", true, false, NULL); + DO_TEST_UPDATE("graphics-spice-listen-network", "graphics-spice-listen-network", false, false, + "set_password", "{\"return\":{}}", "expire_password", "{\"return\":{}}"); + /* Strange huh? Currently, only graphics can be updated :-P */ + DO_TEST_UPDATE("disk-cdrom", "disk-cdrom-nochange", true, false, NULL); + + DO_TEST_ATTACH("console-compat-2", "console-virtio", false, true, + "chardev-add", "{\"return\": {\"pty\": \"/dev/pts/26\"}}", + "device_add", "{\"return\": {}}"); + + DO_TEST_DETACH("console-compat-2", "console-virtio", false, false, + "device_del", "{\"return\": {}}", + "chardev-remove", "{\"return\": {}}"); virObjectUnref(driver.caps); virObjectUnref(driver.xmlopt); diff --git a/tests/qemuhotplugtestdata/qemuhotplug-console-virtio.xml b/tests/qemuhotplugtestdata/qemuhotplug-console-virtio.xml new file mode 100644 index 0000000..bfab6ff --- /dev/null +++ b/tests/qemuhotplugtestdata/qemuhotplug-console-virtio.xml @@ -0,0 +1,5 @@ + <console type='pty'> + <source path='/dev/pts/26'/> + <target type='virtio' port='1'/> + <alias name='console1'/> + </console> diff --git a/tests/qemuxml2argvdata/qemuxml2argv-console-compat-2.xml b/tests/qemuxml2argvdata/qemuxml2argv-console-compat-2.xml new file mode 100644 index 0000000..3fc1153 --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-console-compat-2.xml @@ -0,0 +1,122 @@ +<domain type='kvm' id='2'> + <name>f17</name> + <uuid>a1cd52eb-d37f-4717-fc6e-972f0774f4c9</uuid> + <memory unit='KiB'>1048576</memory> + <currentMemory unit='KiB'>1048576</currentMemory> + <vcpu placement='static'>1</vcpu> + <resource> + <partition>/machine</partition> + </resource> + <os> + <type arch='x86_64' machine='pc-i440fx-1.4'>hvm</type> + <boot dev='hd'/> + </os> + <features> + <acpi/> + <apic/> + <pae/> + </features> + <clock offset='utc'/> + <on_poweroff>destroy</on_poweroff> + <on_reboot>restart</on_reboot> + <on_crash>restart</on_crash> + <pm> + <suspend-to-mem enabled='yes'/> + <suspend-to-disk enabled='yes'/> + </pm> + <devices> + <emulator>/usr/bin/qemu-system-x86_64</emulator> + <disk type='file' device='disk'> + <driver name='qemu' type='qcow2' cache='none'/> + <source file='/var/lib/libvirt/images/f17.qcow2'/> + <target dev='vda' bus='virtio'/> + <alias name='virtio-disk0'/> + <address type='pci' domain='0x0000' bus='0x00' slot='0x05' function='0x0'/> + </disk> + <disk type='file' device='cdrom'> + <driver name='qemu' type='raw' cache='none'/> + <source file='/home/user/tmp/Fedora-17-x86_64-Live-KDE.iso'/> + <target dev='hdc' bus='ide'/> + <readonly/> + <alias name='ide0-1-0'/> + <address type='drive' controller='0' bus='1' target='0' unit='0'/> + </disk> + <controller type='ide' index='0'> + <alias name='ide0'/> + <address type='pci' domain='0x0000' bus='0x00' slot='0x01' function='0x1'/> + </controller> + <controller type='usb' index='0'> + <alias name='usb0'/> + <address type='pci' domain='0x0000' bus='0x00' slot='0x01' function='0x2'/> + </controller> + <controller type='pci' index='0' model='pci-root'> + <alias name='pci0'/> + </controller> + <controller type='virtio-serial' index='0'> + <alias name='virtio-serial0'/> + <address type='pci' domain='0x0000' bus='0x00' slot='0x07' function='0x0'/> + </controller> + <interface type='network'> + <mac address='52:54:00:ea:35:6f'/> + <source network='default'/> + <target dev='vnet0'/> + <model type='virtio'/> + <bandwidth> + <inbound average='4000' peak='8000' floor='200' burst='1024'/> + <outbound average='4000' peak='8000' burst='1024'/> + </bandwidth> + <alias name='net0'/> + <address type='pci' domain='0x0000' bus='0x00' slot='0x03' function='0x0'/> + </interface> + <serial type='pty'> + <source path='/dev/pts/22'/> + <target type='isa-serial' port='0'/> + <alias name='serial0'/> + </serial> + <serial type='pty'> + <source path='/dev/pts/25'/> + <target port='0'/> + <alias name='serial1'/> + </serial> + <serial type='tcp'> + <source mode='bind' host='0.0.0.0' service='2445'/> + <protocol type='raw'/> + <target port='1'/> + <alias name='serial2'/> + </serial> + <console type='pty' tty='/dev/pts/22'> + <source path='/dev/pts/22'/> + <target type='serial' port='0'/> + <alias name='serial0'/> + </console> + <channel type='unix'> + <source mode='bind' path='/var/lib/libvirt/qemu/f17x86_64.agent'/> + <target type='virtio' name='org.qemu.guest_agent.0'/> + <alias name='channel0'/> + <address type='virtio-serial' controller='0' bus='0' port='1'/> + </channel> + <input type='tablet' bus='usb'> + <alias name='input0'/> + </input> + <input type='mouse' bus='ps2'/> + <graphics type='vnc' port='5900' autoport='yes' listen='0.0.0.0'> + <listen type='address' address='0.0.0.0'/> + </graphics> + <sound model='ich6'> + <alias name='sound0'/> + <address type='pci' domain='0x0000' bus='0x00' slot='0x04' function='0x0'/> + </sound> + <video> + <model type='cirrus' vram='9216' heads='1'/> + <alias name='video0'/> + <address type='pci' domain='0x0000' bus='0x00' slot='0x02' function='0x0'/> + </video> + <memballoon model='virtio'> + <alias name='balloon0'/> + <address type='pci' domain='0x0000' bus='0x00' slot='0x06' function='0x0'/> + </memballoon> + </devices> + <seclabel type='static' model='dac' relabel='no'> + <label>root:root</label> + </seclabel> +</domain> -- 1.8.1.5

On Fri, Jul 12, 2013 at 07:24:02PM +0200, Michal Privoznik wrote:
The test is currently testing just device update function. However, chardev hotplug is implemented just for device attach and detach. This fact means, the test needs to be rewritten (the majority of the code is still shared). Moreover, we are now able to pass VM among multiple test runs. So for instance, while we add a device in the first run, we can remove it in the second run. --- tests/qemuhotplugtest.c | 194 +++++++++++++++++---- .../qemuhotplug-console-virtio.xml | 5 + .../qemuxml2argv-console-compat-2.xml | 122 +++++++++++++ 3 files changed, 284 insertions(+), 37 deletions(-) create mode 100644 tests/qemuhotplugtestdata/qemuhotplug-console-virtio.xml create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-console-compat-2.xml
ACK Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
participants (3)
-
Daniel P. Berrange
-
John Ferlan
-
Michal Privoznik