Re: [libvirt] [PATCH v2 1/4] domain_conf: allow address type='none' to unassign PCI hostdevs
On 11/22/19 12:13 PM, Daniel P. Berrangé wrote:
On Thu, Nov 21, 2019 at 07:19:14PM -0300, Daniel Henrique Barboza
wrote:
[...]
>
> The use of a new 'unassigned' flag in virDomainHostdevDef makes
> it easier to distinguish between the case we're handling here
> versus the case in which a PCI hostdev has no <address> element.
> Both are interpreted as VIR_DOMAIN_DEF_FORMAT_ADDRESS_NONE in
> the existing code, thus reducing the logic to a flag makes
> it easier to handle this new use case. In the next patch we'll
> use the 'unassigned' flag to filter the hostdevs being
> initialized in the QEMU command line.
This paragraph illustrates why I think this approach is a bad
idea. It is overloading ADDRESS_NONE to have 2 separate meanings.
When we've done such things in the past we've usually ended up
regretting it.
We're using ADDRESS_NONE as our signal that the user does not
care about the address and thus libvirt should automatically
assign one. That's consistent across all the different types
of devices that we have.
I don't want to see us add special semantics for this just
for host devices.
The fact that you stil lneeded a "bool unassigned" flag to
be stored in the virDomainHostdevDef is a clear sign that
this must be exposed as a distinct XML attribute.
I see you in fact did exactly this in the previous version of
this patch series, so I'd like to go back to that version.
Do you suggest to keep exactly like it was in v1, an extra parameter
in the subsys element? We can for example create a new address type,
"unassigned", to avoid overloading ADDRESS_NONE like I'm doing here.
I'm suggesting this because, back in v1, I had to take a peak in the
<address> element to avoid the case where the user would assign an
address to an unassigned device. Since I'll need to took in the <address>
element anyway, a new "unassigned" address type would make it easier
to cover this cases. It's also a bit less verbose than adding
"assigned='yes|no'" in the end of the <hostdev> element.
I could also say that this would also be easier for other device types
to use, but I can't come up with a valid use case for anything other than
a PCI hostdev to be unassignable.
Thanks,
DHB
Regards,
Daniel