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@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@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);