[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] [PATCH] qemu: Fix a regression of attaching device



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


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]