On 06/10/2016 11:33 AM, Michal Privoznik wrote:
The problem is this: when working on redirdev detach, I've noticed that even though I've passed device alias in the input device XML, it got transformed into inactive in qemuDomainDetachDeviceLive() while in qemuDomainDetachDeviceConfig() it was the active version of it.
Apparently, if you are doing:
virsh # detach-device --config --live domain device.xml
our code correctly detects that you want to detach the device from both config and live domain definition. However, due to some internal implementation we need to make a copy of the device that user had provided. And to do that, we call virDomainDeviceDefCopy(). This function basically formats the
If you take off the (), then I assume that function name fits on the previous line. The blank space just looks odd.
device into XML and then parse it again. But there's a problem,
s/parse/parses
it's formatting the XML with VIR_DOMAIN_DEF_FORMAT_INACTIVE flag which means that no live data are present in the copy. So we have
s/are/is
a (possibly live) device definition that we pass to code detaching it from inactive XML, and inactive device definition that we pass to code detaching it from active XML. This makes no sense.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_driver.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)
Hazards of cut-n-paste code? And perhaps adjustments to the Copy function after original implementation? I didn't dig on history. Why is this not a problem for Attach and Update? What about LXC which also uses the Copy function. BTW: There's a comment: /* If we are affecting both CONFIG and LIVE * create a deep copy of device as adding * to CONFIG takes one instance. */ just above the code you changed that has I think an obvious adjustment to "deleting from"... Similarly the update code could use "updating" I also think for each comment (I read code and don't necessarily hunt for the commit message) that summarizes your investigation: "NB: The 'dev' is the actual/live device, while 'dev_copy' will be a somewhat reduced copy generated by using VIR_DOMAIN_DEF_PARSE_INACTIVE which means conditionally parsed/formatted fields will not be copied." (I assume that means stuff we've generated, that isn't part of the XML, but is part of the virDmainDef) I think my summary is - all 3 should use 'dev' for live and 'dev_copy' for config. John
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 65e96be..8255d7b 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -8523,22 +8523,22 @@ qemuDomainDetachDeviceFlags(virDomainPtr dom, if (!vmdef) goto endjob;
- if (virDomainDefCompatibleDevice(vmdef, dev, + if (virDomainDefCompatibleDevice(vmdef, dev_copy, VIR_DOMAIN_DEVICE_ACTION_DETACH) < 0) goto endjob;
- if ((ret = qemuDomainDetachDeviceConfig(vmdef, dev, caps, + if ((ret = qemuDomainDetachDeviceConfig(vmdef, dev_copy, caps, parse_flags, driver->xmlopt)) < 0) goto endjob; }
if (flags & VIR_DOMAIN_AFFECT_LIVE) { - if (virDomainDefCompatibleDevice(vm->def, dev_copy, + if (virDomainDefCompatibleDevice(vm->def, dev, VIR_DOMAIN_DEVICE_ACTION_DETACH) < 0) goto endjob;
- if ((ret = qemuDomainDetachDeviceLive(vm, dev_copy, dom)) < 0) + if ((ret = qemuDomainDetachDeviceLive(vm, dev, dom)) < 0) goto endjob; /* * update domain status forcibly because the domain status may be