[libvirt] [PATCHv2 2/2] hotplug: only add a chardev to vmdef after monitor call
Michal Privoznik
mprivozn at redhat.com
Wed Jan 28 10:36:48 UTC 2015
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
More information about the libvir-list
mailing list