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

[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.

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 at redhat.com    +1-801-349-2682
Libvirt virtualization library http://libvirt.org

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 619 bytes
Desc: OpenPGP digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20110713/0201044f/attachment-0001.sig>