Fwd: Re: [PATCH virt-install v1 3/3] virtxml: prevent defining same hostdev again

[Shalini Chellathurai Saroja]

On 5/28/21 10:05 PM, Cole Robinson wrote:
> On 5/28/21 8:06 AM, Shalini Chellathurai Saroja wrote:
>> On 5/19/21 12:49 AM, Cole Robinson wrote:
>>> On 4/14/21 11:18 AM, Shalini Chellathurai Saroja wrote:
>>>> Currently, it is possible to add same device multiple times, when the
>>>> guest domain is in shut-off state. This patch prevents this unexpected
>>>> behavior for all hostdev devices, including mdev devices.
>>>>
>>> If this is an invalid config, these types of validation checks should
>>> live in libvirt so all clients benefit from them, and libvirt is the one
>>> source of truth for what's supported or not.
>> Hello Cole,
>>
>> Thank you for the review.
>>
>> This validation check is available in libvirt, for this to work, the
>> flag "VIR_DOMAIN_AFFECT_CONFIG" has to be set, when the virtual server
>> is not in running state.
>>
> It's kind of complicated.
>
> AFFECT_CONFIG for an offline VM (or the offline XML of a running VM)
> should in theory be identical to the process we currently use: dumpxml,
> modify XML, redefine. Anything else is a bug IMO. My reasoning is that
> if AFFECT_CONFIG does anything special besides hitting the XML define
> code path, then there's logic locked into the AFFECT_CONFIG code path
> which isn't available to the many other ways of editing the XML.
>
> So for example, if there is a validation check on coldplugging mdev
> devices in libvirt, and it's only accessible through the attachDevice
> API, that's a libvirt bug IMO. If the check is worth doing at coldplug
> time, it's also worth doing at XML define time, and should be part of
> the generic virDomainDefValidate infrastructure. If it's not worth doing
> at XML validate time for some reasons, there should be no argument for
> why it makes sense at coldplug time.
>
> That's my theory anyways. I think other libvirt devs will agree but I'm
> not sure.
>
> AFFECT_CONFIG in general is not a good fit for virt-install, for the
> reasons you show below: we can't rely on it being implemented
> consistently. It's not implemented in the test driver, and it's not
> implemented for all devices even in the qemu driver. So we would have to
> either punt the option to the user by explicitly exposing the flag, or
> try to keep track internally which cases it works for and what cases it
> doesn't, neither of which ideas I like.
>
> So I think if for this case, libvirt already has the validation you
> want, but it's only in the attachDevice code path, that's an opportunity
> to improve libvirt. Move that check to
> domain_validate.c:virDomainHostdevDefValidate, or possibly to
> virDomainDefValidateInternal , do something like what
> virDomainDefDuplicateDiskInfoValidate does
>
> Hopefully that makes sense
>
> Thanks,
> Cole

Hello Cole,

I agree that this validation check should be included in libvirt for 
defining the XML too,

So I am dropping this commit. Thank you for the explanation.

-- 
Kind regards
Shalini Chellathurai Saroja
Linux on Z and Virtualization Development
Vorsitzende des Aufsichtsrats: Gregor Pillen
Geschäftsführung: Dirk Wittkopp
Sitz der Gesellschaft: Böblingen
Registergericht: Amtsgericht Stuttgart, HRB 243294

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://listman.redhat.com/archives/virt-tools-list/attachments/20210531/a6790a00/attachment.htm>