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@xxxxxxxxxx>
> ---
> 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
>
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list