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