[libvirt] [PATCH] Use correct pci addresses during device-detach

[John Ferlan]

Although you posted the same patch twice, I chose this one to respond to...

On 10/09/2015 01:17 AM, Nitesh_Konkar wrote:
> From: Nitesh_Konkar <niteshkonkar at in.ibm.com>

^^ Unnecessary in commit message and different from the S-o-b below...

> 
> The attach-device on live and persistent copies can be done independently.
> Thus devices can end up having different pci addresses in live and persistent
> copies. The detach device should try to detach the device from their respective
> addresses instead of using the same from live/persistent.
> 

Sometimes it's helpful to provide an example, in this case I assume your
example is an attach-device --config using one XML file and address
followed by an attach-device --live using a different XML file and
different PCI address - is that right?

Then your contention is if someone does a "detach-device --live
--config" they should expect this to succeed using the same XML file?
Which XML - the one they used for the "--live" or the one they used for
"--config"? Obviously I think using the "--live" one for "--config" will
fail and vice-versa.

I think you need to follow the history of the Update & Detach API's - in
particular, I think you'll find commit id '1e0534a7'. In there Michal
notes that parsing twice is wrong. Although I suppose using 'vmdef'
instead of 'vm->def' for the config when both live & config are being
used may provide the results you expect (unless of course you're working
on a transient domain in which case vmdef == vm->def).

Anyway, consider that Detach (and Update as well btw) can work on either
the --config or --live or both. If both then, then both XML's should
match. If they don't, then I'm going to assume this patch was generated
because of some failure scenario, but without the example I can only
make assumptions.

I'm not so sure this patch is valid based on the above (written after I
looked at the code for a while and made comments below). I've kept my
comments, but I think you need to convince me more of the need by
providing examples. I don't think you can expect to attach using
different XML and then expect one detach (or update) to work properly.
You've already 'decided' that the config and live will be different,
then you must manage them differently.  Beyond that as the man page
notes, if your XML description of the device isn't specific enough, you
could match something you don't expect. Likewise, if you do have
specific XML descriptions, then you shouldn't expect to match something
different when trying to update both config and live at the same time.
Personally, in that case, I'd assume it becomes hypervisor dependent and
the 'live' XML is used to update the 'config' device (as long as there
isn't a PCI address conflict...).

John



> Signed-off-by:nitkon12 at linux.vnet.ibm.com
> ---
>  src/driver-nodedev.h   |  1 +
>  src/qemu/qemu_driver.c | 25 ++++++++++---------------
>  2 files changed, 11 insertions(+), 15 deletions(-)
> 
> diff --git a/src/driver-nodedev.h b/src/driver-nodedev.h
> index e846612..dea79bd 100644
> --- a/src/driver-nodedev.h
> +++ b/src/driver-nodedev.h
> @@ -59,6 +59,7 @@ typedef char *
>  typedef char *
>  (*virDrvNodeDeviceGetParent)(virNodeDevicePtr dev); 
> +
>  typedef int
>  (*virDrvNodeDeviceNumOfCaps)(virNodeDevicePtr dev);

?? Spurious change.  Should be removed from patch.

> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> index f133b45..6fd58c2 100644
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c
> @@ -8708,23 +8708,12 @@ static int qemuDomainDetachDeviceFlags(virDomainPtr dom, const char *xml,
>          !(flags & VIR_DOMAIN_AFFECT_LIVE))
>          parse_flags |= VIR_DOMAIN_DEF_PARSE_INACTIVE;
>  

Note this setting of parse_flags only when update the config. I didn't
dig into why this is different than the update path, but it may be
important w/r/t your changes.

> -    dev = dev_copy = virDomainDeviceDefParse(xml, vm->def,
> +    dev_copy = virDomainDeviceDefParse(xml, vm->def,
>                                               caps, driver->xmlopt,
>                                               parse_flags);

Need to change alignment of remainder of arguments

Also 'dev_copy' isn't really a copy is it?  Why not name it 'dev_live'
or just keep it as 'dev' instead?

> -    if (dev == NULL)
> +    if (dev_copy == NULL)
>          goto endjob;

Could all be one statement - "if (!(dev_live = xxxx(arg, arg, arg,
                                                    arg, arg)))"
>  
> -    if (flags & VIR_DOMAIN_AFFECT_CONFIG &&
> -        flags & VIR_DOMAIN_AFFECT_LIVE) {
> -        /* If we are affecting both CONFIG and LIVE
> -         * create a deep copy of device as adding
> -         * to CONFIG takes one instance.
> -         */
> -        dev_copy = virDomainDeviceDefCopy(dev, vm->def, caps, driver->xmlopt);
> -        if (!dev_copy)
> -            goto endjob;
> -    }
> -

This essentially removes the changes from commit id '1e0534a77'

>      if (priv->qemuCaps)
>          qemuCaps = virObjectRef(priv->qemuCaps);
>      else if (!(qemuCaps = virQEMUCapsCacheLookup(driver->qemuCapsCache, vm->def->emulator)))
> @@ -8736,6 +8725,13 @@ static int qemuDomainDetachDeviceFlags(virDomainPtr dom, const char *xml,
>          if (!vmdef)
>              goto endjob;

Existing, but could be "if (!(vmdef = ...)))"

>  
> +        dev = virDomainDeviceDefParse(xml, vmdef,
> +                                             caps, driver->xmlopt,
> +                                             parse_flags);

More argument alignment issues.

You'll also see that commit id '1e0534a77' used vm->def for both while
you're changing to 'vmdef' here. Although in thinking about this -
perhaps this is what should have been used originally for the config
path. Still if at this point "vmdef == vm->def", then this parse is
theoretically no different than above (although Michal's commit message
seems to indicate differently and I didn't dig into the Parse to follow
why...).

Maybe this becomes :

   if (vmdef == vm->def)
        dev_config = virDomainDeviceDefCopy(dev_live,...)
   else
        dev_config = virDomainDeviceDefParse(xml, vmdef, ...)


> +        if (!dev)
> +            goto endjob;
> +
> +

You could have combine into one statement, e.g.:

    if (vmdef == vm->def) {
        if (!(dev_config = virDomainDeviceDefCopy(dev_live,...)
            goto endjob;
    } else {
        if (!(dev_config = virDomainDeviceDefParse(xml, vmdef, ...)))
            goto endjob;
    }

>          if (virDomainDefCompatibleDevice(vmdef, dev,
>                                           VIR_DOMAIN_DEVICE_ACTION_DETACH) < 0)
>              goto endjob;
> @@ -8777,8 +8773,7 @@ static int qemuDomainDetachDeviceFlags(virDomainPtr dom, const char *xml,
>   cleanup:
>      virObjectUnref(qemuCaps);
>      virDomainDefFree(vmdef);
> -    if (dev != dev_copy)
> -        virDomainDeviceDefFree(dev_copy);
> +    virDomainDeviceDefFree(dev_copy);

dev_config and dev_live

>      virDomainDeviceDefFree(dev);
>      virDomainObjEndAPI(&vm);
>      virObjectUnref(caps);
>