Do not free it in the caller (qemuDomainAttachDeviceFlags), because it has no way of knowing if it hasn't been cleaned up by qemuProcessStop on monitor EOF. https://bugzilla.redhat.com/show_bug.cgi?id=1161024 --- src/qemu/qemu_driver.c | 3 +-- src/qemu/qemu_hotplug.c | 14 ++++++++++---- tests/qemuhotplugtest.c | 1 + 3 files changed, 12 insertions(+), 6 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index ee5e4f5..333b2bf 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -6997,8 +6997,7 @@ qemuDomainAttachDeviceLive(virDomainObjPtr vm, case VIR_DOMAIN_DEVICE_CHR: ret = qemuDomainAttachChrDevice(driver, vm, dev->data.chr); - if (!ret) - dev->data.chr = NULL; + dev->data.chr = NULL; break; case VIR_DOMAIN_DEVICE_NONE: diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index 62a9cba..f4eca30 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -1484,18 +1484,19 @@ int qemuDomainAttachChrDevice(virQEMUDriverPtr driver, char *devstr = NULL; char *charAlias = NULL; bool need_remove = false; + bool need_free = true; if (!virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_DEVICE)) { virReportError(VIR_ERR_OPERATION_INVALID, "%s", _("qemu does not support -device")); - return ret; + goto cleanup; } if (qemuAssignDeviceChrAlias(vmdef, chr, -1) < 0) - return ret; + goto cleanup; if (qemuBuildChrDeviceStr(&devstr, vm->def, chr, priv->qemuCaps) < 0) - return ret; + goto cleanup; if (virAsprintf(&charAlias, "char%s", chr->info.alias) < 0) goto cleanup; @@ -1503,6 +1504,7 @@ int qemuDomainAttachChrDevice(virQEMUDriverPtr driver, if (qemuDomainChrInsert(vmdef, chr) < 0) goto cleanup; need_remove = true; + need_free = false; qemuDomainObjEnterMonitor(driver, vm); if (qemuMonitorAttachCharDev(priv->mon, charAlias, &chr->source) < 0) { @@ -1534,8 +1536,12 @@ int qemuDomainAttachChrDevice(virQEMUDriverPtr driver, audit: virDomainAuditChardev(vm, NULL, chr, "attach", ret == 0); cleanup: - if (ret < 0 && need_remove) + if (ret < 0 && need_remove) { qemuDomainChrRemove(vmdef, chr); + need_free = true; + } + if (need_free) + virDomainChrDefFree(chr); VIR_FREE(charAlias); VIR_FREE(devstr); return ret; diff --git a/tests/qemuhotplugtest.c b/tests/qemuhotplugtest.c index 12a7f71..c914004 100644 --- a/tests/qemuhotplugtest.c +++ b/tests/qemuhotplugtest.c @@ -114,6 +114,7 @@ testQemuHotplugAttach(virDomainObjPtr vm, break; case VIR_DOMAIN_DEVICE_CHR: ret = qemuDomainAttachChrDevice(&driver, vm, dev->data.chr); + dev->data.chr = NULL; break; default: if (virTestGetVerbose()) -- 2.0.5

On 01/27/2015 04:27 PM, Michal Privoznik wrote: Yeah, it's a shame the author of this function didn't follow the convention. I'll take a look at the *ChrInsert function if they can be easily split up into pre-monitor and post-monitor parts. Otherwise, I'd rather not risk breaking it and use this patch. Jan