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@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)) {
+ virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+ _("interface %s - <portOptions isolated='yes'/> is not supported for network interfaces with type='%s'"),
+ 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));
+ return -1;
+ }
+ }
+ return 0;
+}
+
+
int
virDomainActualNetDefValidate(const virDomainNetDef *net)
{
@@ -6291,6 +6332,11 @@ virDomainActualNetDefValidate(const virDomainNetDef *net)
return -1;
}
+ if (virDomainNetDefValidatePortOptions(macstr, actualType, vport,
+ virDomainNetGetActualPortOptionsIsolated(net)) < 0) {
+ return -1;
+ }
+
return 0;
}
@@ -6298,6 +6344,10 @@ virDomainActualNetDefValidate(const virDomainNetDef *net)
static int
virDomainNetDefValidate(const virDomainNetDef *net)
{
+ char macstr[VIR_MAC_STRING_BUFLEN];
+
+ virMacAddrFormat(&net->mac, macstr);
+
if ((net->hostIP.nroutes || net->hostIP.nips) &&
net->type != VIR_DOMAIN_NET_TYPE_ETHERNET) {
virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
@@ -6331,6 +6381,12 @@ virDomainNetDefValidate(const virDomainNetDef *net)
return -1;
}
}
+
+ if (virDomainNetDefValidatePortOptions(macstr, net->type, net->virtPortProfile,
+ net->isolatedPort) < 0) {
+ return -1;
+ }
+
return 0;
}
--
2.24.1
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@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)
>+ 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.
How about:
... - isolated ports are not supported ...
>+ 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@redhat.com>
Jano
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@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@redhat.com>
>
> Jano
On Tue, Feb 18, 2020 at 10:08:42PM -0500, Laine Stump wrote:
>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@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
[...]
>>>+ 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... ?
My reasoning was that it takes more space - especially the ='yes'
part is redundant. And I thought we never do that, which is apparently
not true:
$ git grep '_(.*"<' src/conf/ | wc -l
7
>
>>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.
Right, leave it in then.
>
>(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.)
Not that I know of.
Jano
© 2016 - 2026 Red Hat, Inc.