On 10/22/19 9:24 PM, Laine Stump wrote:
> <interface> devices (virDomainNetDef) are a bit different from other
> types of devices in that their actual type may come from a network (in
> the form of a port connection), and that doesn't happen until the
> domain is started. This means that any validation of an <interface> at
> parse time needs to be a bit liberal in what it accepts - when
> type='network', you could think that something is/isn't allowed, but
> once the domain is started and a port is created by the configured
> network, the opposite might be true.
>
> To solve this problem hypervisor drivers need to do an extra
> validation step when the domain is being started. I recently (commit
> 3cff23f7, libvirt 5.7.0) added a function to peform such validation
> for all interfaces to the QEMU driver -
> qemuDomainValidateActualNetDef() - but while that function is a good
> single point to call for the multiple places that need to "start" an
> interface (domain startup, device hotplug, device update), it can't be
> called by the other hypervisor drivers, since 1) it's in the QEMU
> driver, and 2) it contains some checks specific to QEMU. For
> validation that applies to network devices on *all* hypervisors, we
> need yet another interface validation function that can be called by
> any hypervisor driver (not just QEMU) right after its network port has
> been created during domain startup or hotplug. This patch adds that
> function - virDomainNetDefRuntimeValidate(), in the conf directory,
> and calls it in appropriate places in the QEMU, lxc, and libxl
> drivers.
>
> The new function, virDomainNetDefRuntimeValidate(), should contain
> network device validation that 1) is hypervisor agnostic, and 2) can't
> be done until we know the "actual type" of an interface.
>
> There is no framework for validation at domain startup as there is for
> post-parse validation, but I don't want to create a whole elaborate
> system that will only be used by one type of device. For that reason,
> I just made a single function that should be called directly from the
> hypervisors, when they are initializing interfaces to start a domain,
> right after conditionally allocating the network port (and regardless
> of whether or not that was actually needed). In the case of the QEMU
> driver, qemuDomainValidateActualNetDef() is already called in all the
> appropriate places, so we can just call the new function from
> there. In the case of the other hypervisors, we search for
> virDomainNetAllocateActualDevice() (which is the hypervisor-agnostic
> function that calls virNetworkPortCreateXML()), and add the call to our
> new function right after that.
>
> The new function itself could be plunked down into many places in the
> code, but we already have 3 validation functions for network devices
> in 2 different places (not counting any basic validation done in
> virDomainNetDefParseXML() itself):
>
> 1) post-parse hypervisor-agnostic
> (virDomainNetDefValidate() - domain_conf.c:6145)
> 2) post-parse hypervisor-specific
> (qemuDomainDeviceDefValidateNetwork() - qemu_domain.c:5498)
> 3) domain-start hypervisor-specific
> (qemuDomainValidateActualNetDef() - qemu_domain.c:5390)
>
> I placed (3) right next to (2) when I added it, specifically to avoid
> spreading validation all over the code. For the same reason, I decided
> to put this new function right next to (1) - this way if someone needs
> to add validation specific to qemu, they go to one location, and if
> they need to add validation applying to everyone, they go to the
> other. It looks a bit strange to have a public function in between a
> bunch of statics, but I think it's better than the alternative of
> further fragmentation. (I'm open to other ideas though, of course.)
>
> Signed-off-by: Laine Stump <laine@xxxxxxxxxx>
> ---
> src/conf/domain_conf.c | 22 ++++++++++++++++++++++
> src/conf/domain_conf.h | 3 +++
> src/libvirt_private.syms | 1 +
> src/libxl/libxl_domain.c | 4 ++++
> src/libxl/libxl_driver.c | 4 ++++
> src/lxc/lxc_driver.c | 4 ++++
> src/lxc/lxc_process.c | 4 ++++
> src/qemu/qemu_domain.c | 6 ++++++
> 8 files changed, 48 insertions(+)
>
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index 01a1a74946..c063f40a4c 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -6094,6 +6094,28 @@ virDomainRedirdevDefValidate(const virDomainDef *def,
> }
>
>
> +int
> +virDomainNetDefRuntimeValidate(const virDomainNetDef *net G_GNUC_UNUSED)
> +{
> + /* Unlike virDomainNetDefValidate(), which is a static function
> + * called internally to this file, virDomainActualNetDefValidate()
> + * is a public function that can be called from a hypervisor after
> + * it has completely setup the NetDef for use by a domain,
> + * including possibly allocating a port from the network driver
> + * (which could change the effective/"actual" type of the NetDef,
> + * thus changing what should/shouldn't be allowed by validation).
> + *
The comment here claims the function is named
virDomainActualNetDefValidate. I think that naming is more consistent
rather than invent a new 'runtime' naming scheme, event though runtime
is more semanticly descriptive. (though TBH I wouldn't mind
s/actual/runtime/ in the codebase anyways, the 'actual' naming always
confuses me)
Comment should be fixed either way, I'll leave it up to you whether to
actually rename the function. Series:
Reviewed-by: Cole Robinson <crobinso@xxxxxxxxxx>
I think there's a conflict in patch 7, git am gave me some confusing
results, so make sure the code you are moving is incorporating and
possible new changes that landed there since this was posted.
- Cole
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
