[libvirt] [PATCH v2] qemu: Fix a regression of attaching device
The regression is introduced by Commit da1eba6b, the new codes with this commit doesn't reset "ret" to "-1" when it fails on parsing the device XML (live device attachment) This patch changes the codes to reset the "ret" and "-1", and also changes the codes so that it don't modify "ret" for condition checking. How to reproduce: <disk type='oops' device='disk'> <driver name='qemu' type='raw'/> <source file='/var/lib/libvirt/images/test.img'/> <target dev='vda' bus='virtio'/> </disk> Device attached successfully --- src/qemu/qemu_driver.c | 23 +++++++++++++++++------ 1 files changed, 17 insertions(+), 6 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 3d4207e..73118fd 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -4903,16 +4903,20 @@ qemuDomainModifyDeviceFlags(virDomainPtr dom, const char *xml, _("unknown domain modify action %d"), action); break; } - } else - ret = 0; - if (!ret && (flags & VIR_DOMAIN_AFFECT_LIVE)) { + if (ret == -1) + goto endjob; + } + + if (flags & VIR_DOMAIN_AFFECT_LIVE) { /* If dev exists it was created to modify the domain config. Free it. */ virDomainDeviceDefFree(dev); dev = virDomainDeviceDefParse(driver->caps, vm->def, xml, VIR_DOMAIN_XML_INACTIVE); - if (dev == NULL) + if (dev == NULL) { + ret = -1; goto endjob; + } switch (action) { case QEMU_DEVICE_ATTACH: @@ -4927,18 +4931,25 @@ qemuDomainModifyDeviceFlags(virDomainPtr dom, const char *xml, default: qemuReportError(VIR_ERR_INTERNAL_ERROR, _("unknown domain modify action %d"), action); + ret = -1; break; } + + if (ret == -1) + goto endjob; /* * update domain status forcibly because the domain status may be * changed even if we attach the device failed. For example, a * For example, a new controller may be created. */ - if (virDomainSaveStatus(driver->caps, driver->stateDir, vm) < 0) + if (virDomainSaveStatus(driver->caps, driver->stateDir, vm) < 0) { ret = -1; + goto endjob; + } } + /* Finally, if no error until here, we can save config. */ - if (!ret && (flags & VIR_DOMAIN_AFFECT_CONFIG)) { + if (flags & VIR_DOMAIN_AFFECT_CONFIG) { ret = virDomainSaveConfig(driver->configDir, vmdef); if (!ret) { virDomainObjAssignDef(vm, vmdef, false); -- 1.7.6
On 07/15/2011 09:16 AM, Osier Yang wrote:
The regression is introduced by Commit da1eba6b, the new codes with this commit doesn't reset "ret" to "-1" when it fails on parsing the device XML (live device attachment)
This patch changes the codes to reset the "ret" and "-1", and also changes the codes so that it don't modify "ret" for condition checking.
How to reproduce:
<disk type='oops' device='disk'> <driver name='qemu' type='raw'/> <source file='/var/lib/libvirt/images/test.img'/> <target dev='vda' bus='virtio'/> </disk>
Device attached successfully --- src/qemu/qemu_driver.c | 23 +++++++++++++++++------ 1 files changed, 17 insertions(+), 6 deletions(-)
ACK. The commit message is better this time, and the flow of logic, although requiring more lines, is easier to understand.
/* * update domain status forcibly because the domain status may be * changed even if we attach the device failed. For example, a
While you're touching this area of code, s/even if we attach the device failed/even we failed to attach the device/
* For example, a new controller may be created.
Also, delete the redundant "For example, a". -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
于 2011年07月15日 23:13, Eric Blake 写道:
On 07/15/2011 09:16 AM, Osier Yang wrote:
The regression is introduced by Commit da1eba6b, the new codes with this commit doesn't reset "ret" to "-1" when it fails on parsing the device XML (live device attachment)
This patch changes the codes to reset the "ret" and "-1", and also changes the codes so that it don't modify "ret" for condition checking.
How to reproduce:
<disk type='oops' device='disk'> <driver name='qemu' type='raw'/> <source file='/var/lib/libvirt/images/test.img'/> <target dev='vda' bus='virtio'/> </disk>
Device attached successfully --- src/qemu/qemu_driver.c | 23 +++++++++++++++++------ 1 files changed, 17 insertions(+), 6 deletions(-) ACK. The commit message is better this time, and the flow of logic, although requiring more lines, is easier to understand.
/* * update domain status forcibly because the domain status may be * changed even if we attach the device failed. For example, a
While you're touching this area of code,
s/even if we attach the device failed/even we failed to attach the device/
* For example, a new controller may be created.
Also, delete the redundant "For example, a".
Thanks, pushed with better commit message and the comments updated. Regards Osier
participants (2)
-
Eric Blake -
Osier Yang