On 28.01.2015 10:14, Ján Tomko wrote:
https://bugzilla.redhat.com/show_bug.cgi?id=1161024
This way the device is in vmdef only if ret = 0 and the caller (qemuDomainAttachDeviceFlags) does not free it.
Otherwise it might get double freed by qemuProcessStop and qemuDomainAttachDeviceFlags if the domain crashed in monitor after we've added it to vm->def. --- qemuDomainChrInsertPreAllocCleanup is always called, not just when qemuDomainChrPreInsert was called before. But unless I missed something, the configuration where nserials == 0, nconsoles == 1 should not happen after qemu's PostParse callback.
src/qemu/qemu_hotplug.c | 34 +++++++++++----------------------- 1 file changed, 11 insertions(+), 23 deletions(-)
diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index 2ea30f5..033b281 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -1523,59 +1523,47 @@ int qemuDomainAttachChrDevice(virQEMUDriverPtr driver, virDomainDefPtr vmdef = vm->def; char *devstr = NULL; char *charAlias = NULL; - bool need_remove = false;
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;
- if (qemuDomainChrInsert(vmdef, chr) < 0) + if (qemuDomainChrPreInsert(vmdef, chr) < 0) goto cleanup; - need_remove = true;
qemuDomainObjEnterMonitor(driver, vm); if (qemuMonitorAttachCharDev(priv->mon, charAlias, &chr->source) < 0) { - if (qemuDomainObjExitMonitor(driver, vm) < 0) { - need_remove = false; - ret = -1; - goto cleanup; - } + ignore_value(qemuDomainObjExitMonitor(driver, vm)); goto audit; }
if (devstr && qemuMonitorAddDevice(priv->mon, devstr) < 0) { /* detach associated chardev on error */ qemuMonitorDetachCharDev(priv->mon, charAlias); - if (qemuDomainObjExitMonitor(driver, vm) < 0) { - need_remove = false; - ret = -1; - goto cleanup; - } + ignore_value(qemuDomainObjExitMonitor(driver, vm)); goto audit; } - if (qemuDomainObjExitMonitor(driver, vm) < 0) { - need_remove = false; - ret = -1; - goto cleanup; - } + if (qemuDomainObjExitMonitor(driver, vm) < 0) + goto audit;
+ qemuDomainChrInsertPreAlloced(vm->def, chr); ret = 0; audit: virDomainAuditChardev(vm, NULL, chr, "attach", ret == 0); cleanup: - if (ret < 0 && need_remove) - qemuDomainChrRemove(vmdef, chr); + if (ret < 0 && virDomainObjIsActive(vm)) + qemuDomainChrInsertPreAllocCleanup(vm->def, chr);
It took me a while to see if this is safe. We can jump here even if vm->def hasn't been touched at all, e.g. if qemu is missing the DEVICE capability. However, if that's the case, there's currently no way for vm->def to contain one console but no serial line.
VIR_FREE(charAlias); VIR_FREE(devstr); return ret;
Michal