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 -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list