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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]


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


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

[Index of Archives]     [Linux Virtualization]     [KVM Development]     [CentOS Virtualization]     [Netdev]     [Ethernet Bridging]     [Linux Wireless]     [Kernel Newbies]     [Security]     [Linux for Hams]     [Netfilter]     [Bugtraq]     [Yosemite Forum]     [MIPS Linux]     [ARM Linux]     [Linux RAID]     [Linux Admin]     [Samba]     [Video 4 Linux]

  Powered by Linux