Re: [libvirt] [PATCH v2 2/3] conf: Reintroduce action to virDomainDefCompatibleDevice
On 06/26/2018 11:35 AM, Michal Privoznik wrote:
On 06/26/2018 04:30 PM, John Ferlan wrote:
>
>
> On 06/26/2018 06:21 AM, Michal Privoznik wrote:
>> This was lost in c57f3fd2f8999d17e01. But now we are going to
>> need it again.
>
> Looks like that commit also "lost" the compatible device check on detach
> too (e.g. ACTION_DETACH) without explanation. Whether at the time it
> then became the last consumer of virDomainDeviceAction is another question.
There's no need to check for device compatibility on device detach, is
there? The virDomainDefCompatibleDevice() merely checks whether new
device that we are trying to hot-plug or change would not break
assumptions made by other parts of domain config. These can't be broken
if we're removing a device.
Well it was there at some point in time and it's removal (or reason
therein) wasn't provided in either the bz or the commit message from the
referenced patch. So that led me down the path of curiosity.
Maybe use this opportunity to add to the commit message that restoring
the DETACH checks isn't necessary for the reasons you've outlined
although it's not that important.
>
> So, since you're bringing this back to life - should those checks be
> reinstated into the Detach code (lxc, qemu)? If not, surely
> ACTION_DETACH then still isn't used.
Yes, it is unused. I can remove it, although for completion sake I
rather have it in.
No need to remove it now, but since it's not used, perhaps eventually
some trivial patch will wind it's way through the system. When that
patch makes it's way through maybe it can have the explanation for why
DETACH checking wasn't necessary.
>
> Can I assume you tested bz1439991 with the new condition?
>
Yes you can. I've tested this.
Michal
OK, so considered this patch,
Reviewed-by: John Ferlan <jferlan(a)redhat.com>
John