[libvirt] [PATCH] qemu: Fix a regression of attaching device
qemuDomainModifyDeviceFlags: Changing "ret" to "0", but don't reset it to "-1" on failure, it's not good idea to change the value of "ret" in the codes to do some condition checking. This patch fix it. --- src/qemu/qemu_driver.c | 6 +++--- 1 files changed, 3 insertions(+), 3 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 0a6b48e..ae11c68 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -4892,10 +4892,10 @@ qemuDomainModifyDeviceFlags(virDomainPtr dom, const char *xml, _("unknown domain modify action %d"), action); break; } - } else - ret = 0; + } - if (!ret && (flags & VIR_DOMAIN_AFFECT_LIVE)) { + if (!(flags & VIR_DOMAIN_AFFECT_CONFIG) && + (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, -- 1.7.6
On 07/13/2011 09:17 AM, Osier Yang wrote:
qemuDomainModifyDeviceFlags: Changing "ret" to "0", but don't reset it to "-1" on failure, it's not good idea to change the value of "ret" in the codes to do some condition checking. This patch fix it.
Can you identify which commit introduced the regression, and what behavior actually broke as a result? The commit message will be more useful with that information.
--- src/qemu/qemu_driver.c | 6 +++--- 1 files changed, 3 insertions(+), 3 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 0a6b48e..ae11c68 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -4892,10 +4892,10 @@ qemuDomainModifyDeviceFlags(virDomainPtr dom, const char *xml, _("unknown domain modify action %d"), action); break; } - } else - ret = 0; + }
- if (!ret && (flags & VIR_DOMAIN_AFFECT_LIVE)) { + if (!(flags & VIR_DOMAIN_AFFECT_CONFIG) && + (flags & VIR_DOMAIN_AFFECT_LIVE)) {
I'm not sure I agree with this change. The logic flow before this patch is: if config make the change in a copy, or record that the change failed if live, and either no config or change to config copy is happy make the change live, or record that change failed if config, and either no live or change to live is happy commit the changed copy made earlier Your proposed change makes it so that we can now change live even if we are going to fail to change the config, which is not a good idea. Why not instead do: if config { make change in a copy if ret < 0 goto error } if live { make change in live if ret < 0 goto error } if config commit the changed copy made earlier -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
At 07/13/2011 11:03 PM, Eric Blake Write:
On 07/13/2011 09:17 AM, Osier Yang wrote:
qemuDomainModifyDeviceFlags: Changing "ret" to "0", but don't reset it to "-1" on failure, it's not good idea to change the value of "ret" in the codes to do some condition checking. This patch fix it.
Can you identify which commit introduced the regression, and what behavior actually broke as a result? The commit message will be more useful with that information.
Commit da1eba6b introduced this regression. I can reproduce this bug as the following: # cat usb.xml #The format of the xml file is wrong. <disk type='file' device='disk'> <driver name='qemu' type='raw'/> <source file='/var/lib/libvirt/images/test2.img'/> <target dev='ubdb' bus='usb'/> <disk> # virsh attach-device vm1 usb.xml Device attached successfully The command successed, but it failed actually.
--- src/qemu/qemu_driver.c | 6 +++--- 1 files changed, 3 insertions(+), 3 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 0a6b48e..ae11c68 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -4892,10 +4892,10 @@ qemuDomainModifyDeviceFlags(virDomainPtr dom, const char *xml, _("unknown domain modify action %d"), action); break; } - } else - ret = 0; + }
- if (!ret && (flags & VIR_DOMAIN_AFFECT_LIVE)) { + if (!(flags & VIR_DOMAIN_AFFECT_CONFIG) && + (flags & VIR_DOMAIN_AFFECT_LIVE)) {
I'm not sure I agree with this change. The logic flow before this patch is:
I do not agree with this change too.
if config make the change in a copy, or record that the change failed if live, and either no config or change to config copy is happy make the change live, or record that change failed if config, and either no live or change to live is happy commit the changed copy made earlier
Your proposed change makes it so that we can now change live even if we are going to fail to change the config, which is not a good idea.
Why not instead do:
if config { make change in a copy if ret < 0 goto error } if live { make change in live if ret < 0 goto error } if config commit the changed copy made earlier
Agree with this way, but I have anohter simple way to fix this problem: if !ret && live { reset ret to -1 ... }
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
于 2011年07月13日 23:03, Eric Blake 写道:
On 07/13/2011 09:17 AM, Osier Yang wrote:
qemuDomainModifyDeviceFlags: Changing "ret" to "0", but don't reset it to "-1" on failure, it's not good idea to change the value of "ret" in the codes to do some condition checking. This patch fix it. Can you identify which commit introduced the regression, and what behavior actually broke as a result? The commit message will be more useful with that information.
--- src/qemu/qemu_driver.c | 6 +++--- 1 files changed, 3 insertions(+), 3 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 0a6b48e..ae11c68 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -4892,10 +4892,10 @@ qemuDomainModifyDeviceFlags(virDomainPtr dom, const char *xml, _("unknown domain modify action %d"), action); break; } - } else - ret = 0; + }
- if (!ret&& (flags& VIR_DOMAIN_AFFECT_LIVE)) { + if (!(flags& VIR_DOMAIN_AFFECT_CONFIG)&& + (flags& VIR_DOMAIN_AFFECT_LIVE)) { I'm not sure I agree with this change. The logic flow before this patch is:
if config make the change in a copy, or record that the change failed if live, and either no config or change to config copy is happy make the change live, or record that change failed if config, and either no live or change to live is happy commit the changed copy made earlier
Your proposed change makes it so that we can now change live even if we are going to fail to change the config, which is not a good idea.
Yes, I didn't follow with the complete meaning of commit da1eba6b pointed by Wen, a updated patch is following.
Why not instead do:
if config { make change in a copy if ret< 0 goto error } if live { make change in live if ret< 0 goto error } if config commit the changed copy made earlier
participants (3)
-
Eric Blake -
Osier Yang -
Wen Congyang