Re: [libvirt PATCH 09/10] conf: extra validation for <portOptions isolated='yes'/>
On 2/18/20 12:52 PM, Ján Tomko wrote:
On Sun, Feb 16, 2020 at 11:22:58PM -0500, Laine Stump wrote:
> During the hypervisor-agnostic validation of network devices, verify
> that the interface type is either "network" or "bridge", and that
if
> there is any <virtualport>, that it doesn't have any type associated
> with it.
>
> This needs to be done both for the parse-time validation and for
> runtime validation (after a port has been acquired from any associated
> network), because an interface with type='network' could have an
> actual type at runtime of "hostdev" or "direct", neither of
which
> support isolated='true' (yet). Likewise, if an interface is
> type='network', then at runtime a <virtualport> with a type that
> doesn't support isolated='yes' (e.g. "openvswitch",
"802.1Qbh" -
> currently *none* of the available virtualport types support it)
>
> Signed-off-by: Laine Stump <laine(a)redhat.com>
> ---
> src/conf/domain_conf.c | 56 ++++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 56 insertions(+)
>
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index 30b2a53b83..f8ce7d519d 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -6239,6 +6239,47 @@ virDomainRedirdevDefValidate(const virDomainDef
> *def,
> }
>
>
> +static int
> +virDomainNetDefValidatePortOptions(const char *macstr,
> + virDomainNetType type,
> + const virNetDevVPortProfile *vport,
> + virTristateBool isolatedPort)
> +{
> + /*
> + * This function can be called for either a config interface
> + * object (NetDef) or a runtime interface object (ActualNetDef),
> + * by calling it with either, e.g., the "type" (what is in the
> + * config) or the "actualType" (what is determined at runtime by
> + * acquiring a port from the network).
> + */
> + /*
> + * port isolation can only be set for an interface that is
> + * connected to a Linux host bridge (either a libvirt-managed
> + * network, or plain type='bridge')
> + */
> + if (isolatedPort == VIR_TRISTATE_BOOL_YES) {
> + if (!(type == VIR_DOMAIN_NET_TYPE_NETWORK ||
> + type == VIR_DOMAIN_NET_TYPE_BRIDGE)) {
consider:
if (type != VIR_DOMAIN_NET_TYPE_NETWORK &&
type != VIR_DOMAIN_NET_TYPE_BRIDGE)
Tomatoes, potatoes, sure :-)
> + virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> + _("interface %s - <portOptions
> isolated='yes'/> is not supported for network interfaces with
> type='%s'"),
Please don't put XML snippets in the error message.
Because... ?
How about:
... - isolated ports are not supported ...
Wouldn't that make it more likely that the word "isolated" (and maybe
even "port") would be translated, and the resulting error message less
useful to the end user? If it's explicitly an XML element or attribute,
then at least a translator who knew a bit about libvirt would realize
that it's the name of an element/attribute and shouldn't be translated.
(But maybe there was a discussion about this somewhere, it was debated,
and I missed it. If that's become a rule of log messages then of course
I'll follow it.)
> + macstr, virDomainNetTypeToString(type));
> + return -1;
> + }
> + /*
> + * also not allowed for anything with <virtualport> setting
> + * (openvswitch or 802.11Qb[gh])
> + */
> + if (vport && vport->virtPortType !=
> VIR_NETDEV_VPORT_PROFILE_NONE) {
> + virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> + _("interface %s - <portOptions
> isolated='yes'/> is not supported for network interfaces with
> virtualport type='%s'"),
> + macstr,
> virNetDevVPortTypeToString(vport->virtPortType));
Same here.
> + return -1;
> + }
> + }
> + return 0;
> +}
> +
> +
> int
> virDomainActualNetDefValidate(const virDomainNetDef *net)
> {
Reviewed-by: Ján Tomko <jtomko(a)redhat.com>
Jano