[libvirt] [PATCH 0/2] Really fix the crash on qemu crash in chardev hotplug
Ján Tomko (2): Always free the device in AttachChrDevice Unify cleanup path in AttachChrDevice src/qemu/qemu_driver.c | 3 +-- src/qemu/qemu_hotplug.c | 40 +++++++++++++++++++++------------------- tests/qemuhotplugtest.c | 1 + 3 files changed, 23 insertions(+), 21 deletions(-) -- 2.0.5
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 27.01.2015 14:45, Ján Tomko wrote:
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;
I think it would be clearer if we make qemuDomainAttachChrDevice to add the device into vm->def only on success like the rest of attach function do. There's nothing wrong with the code though. Michal
On 01/27/2015 04:27 PM, Michal Privoznik wrote:
On 27.01.2015 14:45, Ján Tomko wrote:
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;
I think it would be clearer if we make qemuDomainAttachChrDevice to add the device into vm->def only on success like the rest of attach function do.
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
There's nothing wrong with the code though.
Michal
Add a 'crashed' label to reuse the cleanup path on domain crash. --- src/qemu/qemu_hotplug.c | 26 +++++++++++--------------- 1 file changed, 11 insertions(+), 15 deletions(-) diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index f4eca30..f5490e2 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -1508,29 +1508,20 @@ int qemuDomainAttachChrDevice(virQEMUDriverPtr driver, qemuDomainObjEnterMonitor(driver, vm); if (qemuMonitorAttachCharDev(priv->mon, charAlias, &chr->source) < 0) { - if (qemuDomainObjExitMonitor(driver, vm) < 0) { - need_remove = false; - ret = -1; - goto cleanup; - } + if (qemuDomainObjExitMonitor(driver, vm) < 0) + goto crashed; 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; - } + if (qemuDomainObjExitMonitor(driver, vm) < 0) + goto crashed; goto audit; } - if (qemuDomainObjExitMonitor(driver, vm) < 0) { - need_remove = false; - ret = -1; - goto cleanup; - } + if (qemuDomainObjExitMonitor(driver, vm) < 0) + goto crashed; ret = 0; audit: @@ -1545,6 +1536,11 @@ int qemuDomainAttachChrDevice(virQEMUDriverPtr driver, VIR_FREE(charAlias); VIR_FREE(devstr); return ret; + + crashed: + need_remove = false; + ret = -1; + goto cleanup; } static int -- 2.0.5
participants (2)
-
Ján Tomko -
Michal Privoznik