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@xxxxxxxxxx
> https://www.redhat.com/mailman/listinfo/libvir-list
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list