[libvirt PATCH 00/10] support BR_ISOLATED flag for guest interfaces attached to a Linux host bridge
https://bugzilla.redhat.com/1727263 Since Linux kernel 4.18, the Linux host bridge has had a flag BR_ISOLATED that can be applied to individual ports. When this flag is set for a port, traffic is blocked between that port and any other port that also has the BR_ISOLATED flag set. libvirt domain interface config now supports setting this flag via the <portOptions isolated='yes'/> setting. It can also be set for all connections to a particular libvirt network by setting the same option in the network config - since the port for the host itself does not have BR_ISOLATED set, the guests can communicate with the host and the outside world, but guests on that network can't communicate with each other. This feature works for QEMU and LXC guests with interfaces attached to a Linux host bridge. (I had contemplated (and experimented with) putting this new flag in the <virtualport> element to avoid creating a new element, but that ended up creating lots of extra code since none of the existing virtualport types would support this new flag, Linux host bridges already work with *no* <virtualport> (much less a virtualport type), and there are some attributes in the <virtualport> parameters subelement that are always autogenerated if there is no virtualport type specified, so I would needed to add a new virtualport type for Linux host bridge, which seems redundant as that information is already implicit in the interface's connection type. etc. etc. It all just turned into a big mess, and starting over fresh with something generic (and hopefully expandable in a sensible way) seemed cleaner). (I am of course open to suggestions though!) Laine Stump (10): schema: trivial indentation fix schema: add missing vlan element to networkport RNG qemu: save/restore original error when recovering from failed bridge attach util: query/set BR_ISOLATED flag on netdevs attached to bridge conf: parse/format <portOptions isolated='yes|no'/> network: propagate <portOptions isolated='yes'/> between network and domain qemu/lxc: plumb isolatedPort from config down through bridge attachment qemu: support updating <portOptions isolated='yes|no'/> during device update conf: extra validation for <portOptions isolated='yes'/> docs: add info about <portOptions isolated='yes'/> to news file docs/news.xml | 21 +++++ docs/schemas/domaincommon.rng | 3 + docs/schemas/network.rng | 9 ++- docs/schemas/networkcommon.rng | 11 +++ docs/schemas/networkport.rng | 6 ++ src/bhyve/bhyve_command.c | 1 + src/conf/domain_conf.c | 79 +++++++++++++++++++ src/conf/domain_conf.h | 4 + src/conf/network_conf.c | 32 ++++++++ src/conf/network_conf.h | 9 +++ src/conf/virnetworkportdef.c | 3 + src/conf/virnetworkportdef.h | 1 + src/libvirt_private.syms | 3 + src/lxc/lxc_process.c | 10 +++ src/network/bridge_driver.c | 4 + src/qemu/qemu_hotplug.c | 47 +++++++++-- src/qemu/qemu_interface.c | 1 + src/util/virnetdevbridge.c | 46 +++++++++++ src/util/virnetdevbridge.h | 9 +++ src/util/virnetdevtap.c | 17 +++- src/util/virnetdevtap.h | 3 + tests/bhyvexml2argvmock.c | 1 + tests/networkxml2xmlin/isolated-ports.xml | 7 ++ tests/networkxml2xmlout/isolated-ports.xml | 7 ++ tests/networkxml2xmltest.c | 1 + tests/qemuxml2argvdata/net-isolated-port.xml | 34 ++++++++ .../net-isolated-port.x86_64-latest.xml | 63 +++++++++++++++ tests/qemuxml2xmltest.c | 1 + 28 files changed, 423 insertions(+), 10 deletions(-) create mode 100644 tests/networkxml2xmlin/isolated-ports.xml create mode 100644 tests/networkxml2xmlout/isolated-ports.xml create mode 100644 tests/qemuxml2argvdata/net-isolated-port.xml create mode 100644 tests/qemuxml2xmloutdata/net-isolated-port.x86_64-latest.xml -- 2.24.1
Signed-off-by: Laine Stump <laine@redhat.com> --- docs/schemas/network.rng | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/docs/schemas/network.rng b/docs/schemas/network.rng index 56937d6a4e..677ec77724 100644 --- a/docs/schemas/network.rng +++ b/docs/schemas/network.rng @@ -237,9 +237,9 @@ <optional> <ref name="bandwidth"/> </optional> - <optional> - <ref name="vlan"/> - </optional> + <optional> + <ref name="vlan"/> + </optional> </interleave> </element> </zeroOrMore> -- 2.24.1
On Sun, Feb 16, 2020 at 11:22:50PM -0500, Laine Stump wrote:
Signed-off-by: Laine Stump <laine@redhat.com> --- docs/schemas/network.rng | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
This is in the data structure and the parse/format functions, and is getting passed all around correctly, it just was omitted from the RNG, which hasn't been noticed because no human is creating <networkport> XML, and so it's never getting validated against the schema. Signed-off-by: Laine Stump <laine@redhat.com> --- docs/schemas/networkport.rng | 3 +++ 1 file changed, 3 insertions(+) diff --git a/docs/schemas/networkport.rng b/docs/schemas/networkport.rng index 8cfc043a40..ea43c03d41 100644 --- a/docs/schemas/networkport.rng +++ b/docs/schemas/networkport.rng @@ -29,6 +29,9 @@ <optional> <ref name="bandwidth"/> </optional> + <optional> + <ref name="vlan"/> + </optional> <optional> <ref name="plug"/> </optional> -- 2.24.1
On Sun, Feb 16, 2020 at 11:22:51PM -0500, Laine Stump wrote:
This is in the data structure and the parse/format functions, and is getting passed all around correctly, it just was omitted from the RNG, which hasn't been noticed because no human is creating <networkport> XML, and so it's never getting validated against the schema.
Signed-off-by: Laine Stump <laine@redhat.com> --- docs/schemas/networkport.rng | 3 +++ 1 file changed, 3 insertions(+)
diff --git a/docs/schemas/networkport.rng b/docs/schemas/networkport.rng
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
Not only was the original error code destroyed in the case of encountering an error during recovery from a failed attach to the bridge (and then *that* error was destroyed by logging a *second* error about the failure to recover - virNetDevBridgeAddPort() already logs an error, so the one about failing to recover was redundant), but if the recovery was successful, the function would then return success to the caller even though it had failed. Signed-off-by: Laine Stump <laine@redhat.com> --- src/qemu/qemu_hotplug.c | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-) diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index c840889968..6395826c69 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -3352,14 +3352,13 @@ qemuDomainChangeNetBridge(virDomainObjPtr vm, ret = virNetDevBridgeAddPort(newbridge, olddev->ifname); virDomainAuditNet(vm, NULL, newdev, "attach", ret == 0); if (ret < 0) { + virErrorPtr err; + + virErrorPreserveLast(&err); ret = virNetDevBridgeAddPort(oldbridge, olddev->ifname); virDomainAuditNet(vm, NULL, olddev, "attach", ret == 0); - if (ret < 0) { - virReportError(VIR_ERR_OPERATION_FAILED, - _("unable to recover former state by adding port " - "to bridge %s"), oldbridge); - } - return ret; + virErrorRestore(&err); + return -1; } /* caller will replace entire olddev with newdev in domain nets list */ return 0; -- 2.24.1
On Sun, Feb 16, 2020 at 11:22:52PM -0500, Laine Stump wrote:
Not only was the original error code destroyed in the case of encountering an error during recovery from a failed attach to the bridge (and then *that* error was destroyed by logging a *second* error about the failure to recover - virNetDevBridgeAddPort() already logs an error, so the one about failing to recover was redundant), but if the recovery was successful, the function would then return success to the caller even though it had failed.
Fixes: 2711ac87160d7ac7d550c57f4339e6c6749942fa (overwritten errors were introduced along with this functionality) Fixes: 6bde0a1a37424c84492658223ff845b1ebb0e25c (the wrong return value was introduced by a refactor)
Signed-off-by: Laine Stump <laine@redhat.com> --- src/qemu/qemu_hotplug.c | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
When this flag is set for an interface attached to a bridge, traffic to/from the specified interface can only enter/exit the bridge via another attached interface that *doesn't* have the BR_ISOLATED flag set. This can be used to permit guests to communicate with the rest of the network, but not with each other. Signed-off-by: Laine Stump <laine@redhat.com> --- src/libvirt_private.syms | 2 ++ src/util/virnetdevbridge.c | 46 ++++++++++++++++++++++++++++++++++++++ src/util/virnetdevbridge.h | 9 ++++++++ 3 files changed, 57 insertions(+) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index dc0449d1d8..5d043041e0 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -2552,8 +2552,10 @@ virNetDevBridgeFDBDel; virNetDevBridgeGetSTP; virNetDevBridgeGetSTPDelay; virNetDevBridgeGetVlanFiltering; +virNetDevBridgePortGetIsolated; virNetDevBridgePortGetLearning; virNetDevBridgePortGetUnicastFlood; +virNetDevBridgePortSetIsolated; virNetDevBridgePortSetLearning; virNetDevBridgePortSetUnicastFlood; virNetDevBridgeRemovePort; diff --git a/src/util/virnetdevbridge.c b/src/util/virnetdevbridge.c index 769289ae0b..d15e81daeb 100644 --- a/src/util/virnetdevbridge.c +++ b/src/util/virnetdevbridge.c @@ -311,6 +311,30 @@ virNetDevBridgePortSetUnicastFlood(const char *brname, } +int +virNetDevBridgePortGetIsolated(const char *brname, + const char *ifname, + bool *enable) +{ + unsigned long value; + + if (virNetDevBridgePortGet(brname, ifname, "isolated", &value) < 0) + return -1; + + *enable = !!value; + return 0; +} + + +int +virNetDevBridgePortSetIsolated(const char *brname, + const char *ifname, + bool enable) +{ + return virNetDevBridgePortSet(brname, ifname, "isolated", enable ? 1 : 0); +} + + #else int virNetDevBridgePortGetLearning(const char *brname G_GNUC_UNUSED, @@ -354,6 +378,28 @@ virNetDevBridgePortSetUnicastFlood(const char *brname G_GNUC_UNUSED, _("Unable to set bridge port unicast_flood on this platform")); return -1; } + + +int +virNetDevBridgePortGetIsolated(const char *brname G_GNUC_UNUSED, + const char *ifname G_GNUC_UNUSED, + bool *enable G_GNUC_UNUSED) +{ + virReportSystemError(ENOSYS, "%s", + _("Unable to get bridge port isolated on this platform")); + return -1; +} + + +int +virNetDevBridgePortSetIsolated(const char *brname G_GNUC_UNUSED, + const char *ifname G_GNUC_UNUSED, + bool enable G_GNUC_UNUSED) +{ + virReportSystemError(ENOSYS, "%s", + _("Unable to set bridge port isolated on this platform")); + return -1; +} #endif diff --git a/src/util/virnetdevbridge.h b/src/util/virnetdevbridge.h index 8137914da8..db4099bf0b 100644 --- a/src/util/virnetdevbridge.h +++ b/src/util/virnetdevbridge.h @@ -73,6 +73,15 @@ int virNetDevBridgePortSetUnicastFlood(const char *brname, const char *ifname, bool enable) ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) G_GNUC_WARN_UNUSED_RESULT; +int virNetDevBridgePortGetIsolated(const char *brname, + const char *ifname, + bool *enable) + ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(3) + G_GNUC_WARN_UNUSED_RESULT; +int virNetDevBridgePortSetIsolated(const char *brname, + const char *ifname, + bool enable) + ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) G_GNUC_WARN_UNUSED_RESULT; typedef enum { VIR_NETDEVBRIDGE_FDB_FLAG_ROUTER = (1 << 0), -- 2.24.1
On Sun, Feb 16, 2020 at 11:22:53PM -0500, Laine Stump wrote:
When this flag is set for an interface attached to a bridge, traffic to/from the specified interface can only enter/exit the bridge via another attached interface that *doesn't* have the BR_ISOLATED flag set. This can be used to permit guests to communicate with the rest of the network, but not with each other.
Signed-off-by: Laine Stump <laine@redhat.com> --- src/libvirt_private.syms | 2 ++ src/util/virnetdevbridge.c | 46 ++++++++++++++++++++++++++++++++++++++ src/util/virnetdevbridge.h | 9 ++++++++ 3 files changed, 57 insertions(+)
@@ -354,6 +378,28 @@ virNetDevBridgePortSetUnicastFlood(const char *brname G_GNUC_UNUSED, _("Unable to set bridge port unicast_flood on this platform")); return -1; } + + +int +virNetDevBridgePortGetIsolated(const char *brname G_GNUC_UNUSED, + const char *ifname G_GNUC_UNUSED, + bool *enable G_GNUC_UNUSED)
Indentation is off here.
+{ + virReportSystemError(ENOSYS, "%s", + _("Unable to get bridge port isolated on this platform")); + return -1; +} + + +int
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
This is a very simple thing to parse and format, but needs to be done in 4 places, so two trivial utility functions have been made that can be called from all the higher level parser/formatters: <domain><interface> <domain><interface><actual> (only in domain status) <network> <networkport> Signed-off-by: Laine Stump <laine@redhat.com> --- docs/schemas/domaincommon.rng | 3 + docs/schemas/network.rng | 3 + docs/schemas/networkcommon.rng | 11 ++++ docs/schemas/networkport.rng | 3 + src/conf/domain_conf.c | 19 ++++++ src/conf/domain_conf.h | 4 ++ src/conf/network_conf.c | 32 ++++++++++ src/conf/network_conf.h | 9 +++ src/conf/virnetworkportdef.c | 3 + src/conf/virnetworkportdef.h | 1 + src/libvirt_private.syms | 1 + tests/networkxml2xmlin/isolated-ports.xml | 7 +++ tests/networkxml2xmlout/isolated-ports.xml | 7 +++ tests/networkxml2xmltest.c | 1 + tests/qemuxml2argvdata/net-isolated-port.xml | 34 ++++++++++ .../net-isolated-port.x86_64-latest.xml | 63 +++++++++++++++++++ tests/qemuxml2xmltest.c | 1 + 17 files changed, 202 insertions(+) create mode 100644 tests/networkxml2xmlin/isolated-ports.xml create mode 100644 tests/networkxml2xmlout/isolated-ports.xml create mode 100644 tests/qemuxml2argvdata/net-isolated-port.xml create mode 100644 tests/qemuxml2xmloutdata/net-isolated-port.x86_64-latest.xml diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng index 29b6b95357..5bb8281a59 100644 --- a/docs/schemas/domaincommon.rng +++ b/docs/schemas/domaincommon.rng @@ -3159,6 +3159,9 @@ <optional> <ref name="vlan"/> </optional> + <optional> + <ref name="portOptions"/> + </optional> <optional> <element name="teaming"> <choice> diff --git a/docs/schemas/network.rng b/docs/schemas/network.rng index 677ec77724..60453225d6 100644 --- a/docs/schemas/network.rng +++ b/docs/schemas/network.rng @@ -332,6 +332,9 @@ <optional> <ref name="vlan"/> </optional> + <optional> + <ref name="portOptions"/> + </optional> <!-- <ip> element --> <zeroOrMore> diff --git a/docs/schemas/networkcommon.rng b/docs/schemas/networkcommon.rng index fd1aac6485..c0eeb5f2c5 100644 --- a/docs/schemas/networkcommon.rng +++ b/docs/schemas/networkcommon.rng @@ -280,4 +280,15 @@ </attribute> </element> </define> + + <define name="portOptions"> + <element name="portOptions"> + <optional> + <attribute name="isolated"> + <ref name="virYesNo"/> + </attribute> + </optional> + </element> + </define> + </grammar> diff --git a/docs/schemas/networkport.rng b/docs/schemas/networkport.rng index ea43c03d41..031c5241f0 100644 --- a/docs/schemas/networkport.rng +++ b/docs/schemas/networkport.rng @@ -32,6 +32,9 @@ <optional> <ref name="vlan"/> </optional> + <optional> + <ref name="portOptions"/> + </optional> <optional> <ref name="plug"/> </optional> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 7a0d126784..176550b62f 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -11480,6 +11480,9 @@ virDomainActualNetDefParseXML(xmlNodePtr node, if (vlanNode && virNetDevVlanParse(vlanNode, ctxt, &actual->vlan) < 0) goto error; + if (virNetworkPortOptionsParseXML(ctxt, &actual->isolatedPort) < 0) + goto error; + *def = g_steal_pointer(&actual); ret = 0; error: @@ -12376,6 +12379,9 @@ virDomainNetDefParseXML(virDomainXMLOptionPtr xmlopt, goto error; } + if (virNetworkPortOptionsParseXML(ctxt, &def->isolatedPort) < 0) + goto error; + cleanup: virDomainActualNetDefFree(actual); virHashFree(filterparams); @@ -25453,6 +25459,7 @@ virDomainActualNetDefContentsFormat(virBufferPtr buf, return -1; if (virNetDevBandwidthFormat(virDomainNetGetActualBandwidth(def), 0, buf) < 0) return -1; + virNetworkPortOptionsFormat(virDomainNetGetActualPortOptionsIsolated(def), buf); return 0; } @@ -25829,6 +25836,7 @@ virDomainNetDefFormat(virBufferPtr buf, return -1; if (virNetDevBandwidthFormat(def->bandwidth, 0, buf) < 0) return -1; + virNetworkPortOptionsFormat(def->isolatedPort, buf); /* ONLY for internal status storage - format the ActualNetDef * as a subelement of <interface> so that no persistent config @@ -29906,6 +29914,17 @@ virDomainNetGetActualVlan(const virDomainNetDef *iface) } +virTristateBool +virDomainNetGetActualPortOptionsIsolated(const virDomainNetDef *iface) +{ + if (iface->type == VIR_DOMAIN_NET_TYPE_NETWORK && + iface->data.network.actual) { + return iface->data.network.actual->isolatedPort; + } + return iface->isolatedPort; +} + + bool virDomainNetGetActualTrustGuestRxFilters(const virDomainNetDef *iface) { diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 867a9c7661..cdc4d25700 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -928,6 +928,7 @@ struct _virDomainActualNetDef { virNetDevBandwidthPtr bandwidth; virNetDevVlan vlan; int trustGuestRxFilters; /* enum virTristateBool */ + virTristateBool isolatedPort; unsigned int class_id; /* class ID for bandwidth 'floor' */ }; @@ -1032,6 +1033,7 @@ struct _virDomainNetDef { virNetDevBandwidthPtr bandwidth; virNetDevVlan vlan; int trustGuestRxFilters; /* enum virTristateBool */ + virTristateBool isolatedPort; int linkstate; unsigned int mtu; virNetDevCoalescePtr coalesce; @@ -3239,6 +3241,8 @@ const virNetDevBandwidth * virDomainNetGetActualBandwidth(const virDomainNetDef *iface); const virNetDevVlan *virDomainNetGetActualVlan(const virDomainNetDef *iface); bool virDomainNetGetActualTrustGuestRxFilters(const virDomainNetDef *iface); +virTristateBool +virDomainNetGetActualPortOptionsIsolated(const virDomainNetDef *iface); const char *virDomainNetGetModelString(const virDomainNetDef *net); int virDomainNetSetModelString(virDomainNetDefPtr et, const char *model); diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c index 1f14a964a2..e9cc9bb55a 100644 --- a/src/conf/network_conf.c +++ b/src/conf/network_conf.c @@ -1172,6 +1172,26 @@ virNetworkIPDefParseXML(const char *networkName, } +int +virNetworkPortOptionsParseXML(xmlXPathContextPtr ctxt, + virTristateBool *isolatedPort) +{ + g_autofree char *str = NULL; + int tmp = VIR_TRISTATE_BOOL_ABSENT; + + if ((str = virXPathString("string(./portOptions/@isolated)", ctxt))) { + if ((tmp = virTristateBoolTypeFromString(str)) <= 0) { + virReportError(VIR_ERR_XML_ERROR, + _("unknown port isolated value '%s'"), str); + return -1; + } + } + + *isolatedPort = tmp; + return 0; +} + + static int virNetworkPortGroupParseXML(virPortGroupDefPtr def, xmlNodePtr node, @@ -1725,6 +1745,9 @@ virNetworkDefParseXML(xmlXPathContextPtr ctxt, if (vlanNode && virNetDevVlanParse(vlanNode, ctxt, &def->vlan) < 0) goto error; + if (virNetworkPortOptionsParseXML(ctxt, &def->isolatedPort) < 0) + goto error; + /* Parse bridge information */ def->bridge = virXPathString("string(./bridge[1]/@name)", ctxt); def->bridgeZone = virXPathString("string(./bridge[1]/@zone)", ctxt); @@ -2331,6 +2354,14 @@ virNetworkIPDefFormat(virBufferPtr buf, return 0; } +void +virNetworkPortOptionsFormat(virTristateBool isolatedPort, + virBufferPtr buf) +{ + if (isolatedPort != VIR_TRISTATE_BOOL_ABSENT) + virBufferAsprintf(buf, "<portOptions isolated='%s'/>\n", + virTristateBoolTypeToString(isolatedPort)); +} static int virPortGroupDefFormat(virBufferPtr buf, @@ -2608,6 +2639,7 @@ virNetworkDefFormatBuf(virBufferPtr buf, return -1; if (virNetDevBandwidthFormat(def->bandwidth, 0, buf) < 0) return -1; + virNetworkPortOptionsFormat(def->isolatedPort, buf); for (i = 0; i < def->nips; i++) { if (virNetworkIPDefFormat(buf, &def->ips[i]) < 0) diff --git a/src/conf/network_conf.h b/src/conf/network_conf.h index d5dd8480db..db7243eef5 100644 --- a/src/conf/network_conf.h +++ b/src/conf/network_conf.h @@ -272,6 +272,7 @@ struct _virNetworkDef { virNetDevBandwidthPtr bandwidth; virNetDevVlan vlan; int trustGuestRxFilters; /* enum virTristateBool */ + virTristateBool isolatedPort; /* Application-specific custom metadata */ xmlNodePtr metadata; @@ -377,6 +378,14 @@ virNetworkConfigFile(const char *dir, void virNetworkSetBridgeMacAddr(virNetworkDefPtr def); +int +virNetworkPortOptionsParseXML(xmlXPathContextPtr ctxt, + virTristateBool *isolatedPort); + +void +virNetworkPortOptionsFormat(virTristateBool isolatedPort, + virBufferPtr buf); + VIR_ENUM_DECL(virNetworkForward); #define VIR_CONNECT_LIST_NETWORKS_FILTERS_ACTIVE \ diff --git a/src/conf/virnetworkportdef.c b/src/conf/virnetworkportdef.c index 28a58ad8f8..a0705a8322 100644 --- a/src/conf/virnetworkportdef.c +++ b/src/conf/virnetworkportdef.c @@ -161,6 +161,8 @@ virNetworkPortDefParseXML(xmlXPathContextPtr ctxt) if (vlanNode && virNetDevVlanParse(vlanNode, ctxt, &def->vlan) < 0) return NULL; + if (virNetworkPortOptionsParseXML(ctxt, &def->isolatedPort) < 0) + return NULL; trustGuestRxFilters = virXPathString("string(./rxfilters/@trustGuest)", ctxt); @@ -360,6 +362,7 @@ virNetworkPortDefFormatBuf(virBufferPtr buf, virNetDevBandwidthFormat(def->bandwidth, def->class_id, buf); if (virNetDevVlanFormat(&def->vlan, buf) < 0) return -1; + virNetworkPortOptionsFormat(def->isolatedPort, buf); if (def->trustGuestRxFilters) virBufferAsprintf(buf, "<rxfilters trustGuest='%s'/>\n", virTristateBoolTypeToString(def->trustGuestRxFilters)); diff --git a/src/conf/virnetworkportdef.h b/src/conf/virnetworkportdef.h index f5ba337fc9..78cf2c1ba4 100644 --- a/src/conf/virnetworkportdef.h +++ b/src/conf/virnetworkportdef.h @@ -60,6 +60,7 @@ struct _virNetworkPortDef { unsigned int class_id; /* class ID for bandwidth 'floor' */ virNetDevVlan vlan; int trustGuestRxFilters; /* enum virTristateBool */ + virTristateBool isolatedPort; int plugtype; /* virNetworkPortPlugType */ union { diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 5d043041e0..8f3312d0df 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -513,6 +513,7 @@ virDomainNetGetActualBridgeName; virDomainNetGetActualDirectDev; virDomainNetGetActualDirectMode; virDomainNetGetActualHostdev; +virDomainNetGetActualPortOptionsIsolated; virDomainNetGetActualTrustGuestRxFilters; virDomainNetGetActualType; virDomainNetGetActualVirtPortProfile; diff --git a/tests/networkxml2xmlin/isolated-ports.xml b/tests/networkxml2xmlin/isolated-ports.xml new file mode 100644 index 0000000000..be21f155b2 --- /dev/null +++ b/tests/networkxml2xmlin/isolated-ports.xml @@ -0,0 +1,7 @@ +<network> + <name>port-isolation-test</name> + <uuid>81ff0d90-c91e-6742-64da-4a736edb9a9b</uuid> + <bridge name="br0"/> + <forward mode="bridge"/> + <portOptions isolated="yes"/> +</network> diff --git a/tests/networkxml2xmlout/isolated-ports.xml b/tests/networkxml2xmlout/isolated-ports.xml new file mode 100644 index 0000000000..eed4461574 --- /dev/null +++ b/tests/networkxml2xmlout/isolated-ports.xml @@ -0,0 +1,7 @@ +<network> + <name>port-isolation-test</name> + <uuid>81ff0d90-c91e-6742-64da-4a736edb9a9b</uuid> + <forward mode='bridge'/> + <bridge name='br0'/> + <portOptions isolated='yes'/> +</network> diff --git a/tests/networkxml2xmltest.c b/tests/networkxml2xmltest.c index f784b90c69..ec679e72ee 100644 --- a/tests/networkxml2xmltest.c +++ b/tests/networkxml2xmltest.c @@ -160,6 +160,7 @@ mymain(void) DO_TEST("metadata"); DO_TEST("set-mtu"); DO_TEST("dnsmasq-options"); + DO_TEST("isolated-ports"); return ret == 0 ? EXIT_SUCCESS : EXIT_FAILURE; } diff --git a/tests/qemuxml2argvdata/net-isolated-port.xml b/tests/qemuxml2argvdata/net-isolated-port.xml new file mode 100644 index 0000000000..1d53c0cd6b --- /dev/null +++ b/tests/qemuxml2argvdata/net-isolated-port.xml @@ -0,0 +1,34 @@ +<domain type='qemu'> + <name>q35-test</name> + <uuid>11dbdcdd-4c3b-482b-8903-9bdb8c0a2774</uuid> + <memory unit='KiB'>2097152</memory> + <currentMemory unit='KiB'>2097152</currentMemory> + <vcpu placement='static' cpuset='0-1'>2</vcpu> + <os> + <type arch='x86_64' machine='q35'>hvm</type> + <boot dev='hd'/> + </os> + <clock offset='utc'/> + <on_poweroff>destroy</on_poweroff> + <on_reboot>restart</on_reboot> + <on_crash>destroy</on_crash> + <devices> + <emulator>/usr/bin/qemu-system-x86_64</emulator> + <disk type='block' device='disk'> + <source dev='/dev/HostVG/QEMUGuest1'/> + <target dev='sda' bus='sata'/> + <address type='drive' controller='0' bus='0' target='0' unit='0'/> + </disk> + <controller type='pci' index='0' model='pcie-root'/> + <interface type='network'> + <mac address='52:54:00:d6:c0:0b'/> + <source network='default'/> + <portOptions isolated='yes'/> + <model type='virtio'/> + </interface> + <video> + <model type='qxl' ram='65536' vram='32768' vgamem='8192' heads='1'/> + </video> + <memballoon model='none'/> + </devices> +</domain> diff --git a/tests/qemuxml2xmloutdata/net-isolated-port.x86_64-latest.xml b/tests/qemuxml2xmloutdata/net-isolated-port.x86_64-latest.xml new file mode 100644 index 0000000000..0605d36a87 --- /dev/null +++ b/tests/qemuxml2xmloutdata/net-isolated-port.x86_64-latest.xml @@ -0,0 +1,63 @@ +<domain type='qemu'> + <name>q35-test</name> + <uuid>11dbdcdd-4c3b-482b-8903-9bdb8c0a2774</uuid> + <memory unit='KiB'>2097152</memory> + <currentMemory unit='KiB'>2097152</currentMemory> + <vcpu placement='static' cpuset='0-1'>2</vcpu> + <os> + <type arch='x86_64' machine='q35'>hvm</type> + <boot dev='hd'/> + </os> + <cpu mode='custom' match='exact' check='none'> + <model fallback='forbid'>qemu64</model> + </cpu> + <clock offset='utc'/> + <on_poweroff>destroy</on_poweroff> + <on_reboot>restart</on_reboot> + <on_crash>destroy</on_crash> + <devices> + <emulator>/usr/bin/qemu-system-x86_64</emulator> + <disk type='block' device='disk'> + <driver name='qemu' type='raw'/> + <source dev='/dev/HostVG/QEMUGuest1'/> + <target dev='sda' bus='sata'/> + <address type='drive' controller='0' bus='0' target='0' unit='0'/> + </disk> + <controller type='pci' index='0' model='pcie-root'/> + <controller type='usb' index='0' model='qemu-xhci'> + <address type='pci' domain='0x0000' bus='0x02' slot='0x00' function='0x0'/> + </controller> + <controller type='sata' index='0'> + <address type='pci' domain='0x0000' bus='0x00' slot='0x1f' function='0x2'/> + </controller> + <controller type='pci' index='1' model='pcie-root-port'> + <model name='pcie-root-port'/> + <target chassis='1' port='0x10'/> + <address type='pci' domain='0x0000' bus='0x00' slot='0x02' function='0x0' multifunction='on'/> + </controller> + <controller type='pci' index='2' model='pcie-root-port'> + <model name='pcie-root-port'/> + <target chassis='2' port='0x11'/> + <address type='pci' domain='0x0000' bus='0x00' slot='0x02' function='0x1'/> + </controller> + <controller type='pci' index='3' model='pcie-root-port'> + <model name='pcie-root-port'/> + <target chassis='3' port='0x12'/> + <address type='pci' domain='0x0000' bus='0x00' slot='0x02' function='0x2'/> + </controller> + <interface type='network'> + <mac address='52:54:00:d6:c0:0b'/> + <source network='default'/> + <portOptions isolated='yes'/> + <model type='virtio'/> + <address type='pci' domain='0x0000' bus='0x01' slot='0x00' function='0x0'/> + </interface> + <input type='mouse' bus='ps2'/> + <input type='keyboard' bus='ps2'/> + <video> + <model type='qxl' ram='65536' vram='32768' vgamem='8192' heads='1' primary='yes'/> + <address type='pci' domain='0x0000' bus='0x00' slot='0x01' function='0x0'/> + </video> + <memballoon model='none'/> + </devices> +</domain> diff --git a/tests/qemuxml2xmltest.c b/tests/qemuxml2xmltest.c index 4b1699db7e..073a7e5565 100644 --- a/tests/qemuxml2xmltest.c +++ b/tests/qemuxml2xmltest.c @@ -462,6 +462,7 @@ mymain(void) DO_TEST("net-virtio-teaming-network", QEMU_CAPS_VIRTIO_NET_FAILOVER, QEMU_CAPS_DEVICE_VFIO_PCI); + DO_TEST_CAPS_LATEST("net-isolated-port"); DO_TEST("net-hostdev", NONE); DO_TEST("net-hostdev-bootorder", NONE); DO_TEST("net-hostdev-vfio", QEMU_CAPS_DEVICE_VFIO_PCI); -- 2.24.1
On Sun, Feb 16, 2020 at 11:22:54PM -0500, Laine Stump wrote:
This is a very simple thing to parse and format, but needs to be done in 4 places, so two trivial utility functions have been made that can be called from all the higher level parser/formatters:
<domain><interface> <domain><interface><actual> (only in domain status) <network> <networkport>
Signed-off-by: Laine Stump <laine@redhat.com> --- docs/schemas/domaincommon.rng | 3 + docs/schemas/network.rng | 3 + docs/schemas/networkcommon.rng | 11 ++++ docs/schemas/networkport.rng | 3 + src/conf/domain_conf.c | 19 ++++++ src/conf/domain_conf.h | 4 ++ src/conf/network_conf.c | 32 ++++++++++ src/conf/network_conf.h | 9 +++ src/conf/virnetworkportdef.c | 3 + src/conf/virnetworkportdef.h | 1 + src/libvirt_private.syms | 1 + tests/networkxml2xmlin/isolated-ports.xml | 7 +++ tests/networkxml2xmlout/isolated-ports.xml | 7 +++ tests/networkxml2xmltest.c | 1 + tests/qemuxml2argvdata/net-isolated-port.xml | 34 ++++++++++ .../net-isolated-port.x86_64-latest.xml | 63 +++++++++++++++++++ tests/qemuxml2xmltest.c | 1 + 17 files changed, 202 insertions(+) create mode 100644 tests/networkxml2xmlin/isolated-ports.xml create mode 100644 tests/networkxml2xmlout/isolated-ports.xml create mode 100644 tests/qemuxml2argvdata/net-isolated-port.xml create mode 100644 tests/qemuxml2xmloutdata/net-isolated-port.x86_64-latest.xml
Not a fan of multi-word elements, because they bring up our inconsistency in using camelCase vs snake_case. But I assume you chose the name to make it compatible with all four containing elements. Would something like: <networkport> <port isolated='yes'/> </networkport> look too odd? Code-wise: Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
On 2/18/20 12:39 PM, Ján Tomko wrote:
On Sun, Feb 16, 2020 at 11:22:54PM -0500, Laine Stump wrote:
This is a very simple thing to parse and format, but needs to be done in 4 places, so two trivial utility functions have been made that can be called from all the higher level parser/formatters:
<domain><interface> <domain><interface><actual> (only in domain status) <network> <networkport>
Signed-off-by: Laine Stump <laine@redhat.com> --- docs/schemas/domaincommon.rng | 3 + docs/schemas/network.rng | 3 + docs/schemas/networkcommon.rng | 11 ++++ docs/schemas/networkport.rng | 3 + src/conf/domain_conf.c | 19 ++++++ src/conf/domain_conf.h | 4 ++ src/conf/network_conf.c | 32 ++++++++++ src/conf/network_conf.h | 9 +++ src/conf/virnetworkportdef.c | 3 + src/conf/virnetworkportdef.h | 1 + src/libvirt_private.syms | 1 + tests/networkxml2xmlin/isolated-ports.xml | 7 +++ tests/networkxml2xmlout/isolated-ports.xml | 7 +++ tests/networkxml2xmltest.c | 1 + tests/qemuxml2argvdata/net-isolated-port.xml | 34 ++++++++++ .../net-isolated-port.x86_64-latest.xml | 63 +++++++++++++++++++ tests/qemuxml2xmltest.c | 1 + 17 files changed, 202 insertions(+) create mode 100644 tests/networkxml2xmlin/isolated-ports.xml create mode 100644 tests/networkxml2xmlout/isolated-ports.xml create mode 100644 tests/qemuxml2argvdata/net-isolated-port.xml create mode 100644 tests/qemuxml2xmloutdata/net-isolated-port.x86_64-latest.xml
Not a fan of multi-word elements, because they bring up our inconsistency in using camelCase vs snake_case.
Yeah, it always bothers me when I see a multiword element or attribute for that reason. I always use camelCase because I remember asking about which is preferred > 10 years ago and being told that we wanted to have camelCase in libvirt XML. That could even be a false memory, but it has always stuck with me.
But I assume you chose the name to make it compatible with all four containing elements.
Would something like: <networkport> <port isolated='yes'/> </networkport> look too odd?
ummmm.... God I *HATE* coming up with element and attribute names! (That's the only response I can think of right now since it's late in the day. Let me sleep on it, but in the end I was expecting, even *hoping* someone would object to portOptions and propose an alternative, and yours doesn't really sound any *worse* than mine, so it might be worthwhile to use it just so I wouldn't have to shoulder the blame :-)
Code-wise: Reviewed-by: Ján Tomko <jtomko@redhat.com>
Jano
On 2/18/20 10:14 PM, Laine Stump wrote:
On 2/18/20 12:39 PM, Ján Tomko wrote:
On Sun, Feb 16, 2020 at 11:22:54PM -0500, Laine Stump wrote:
This is a very simple thing to parse and format, but needs to be done in 4 places, so two trivial utility functions have been made that can be called from all the higher level parser/formatters:
<domain><interface> <domain><interface><actual> (only in domain status) <network> <networkport>
Signed-off-by: Laine Stump <laine@redhat.com> --- docs/schemas/domaincommon.rng | 3 + docs/schemas/network.rng | 3 + docs/schemas/networkcommon.rng | 11 ++++ docs/schemas/networkport.rng | 3 + src/conf/domain_conf.c | 19 ++++++ src/conf/domain_conf.h | 4 ++ src/conf/network_conf.c | 32 ++++++++++ src/conf/network_conf.h | 9 +++ src/conf/virnetworkportdef.c | 3 + src/conf/virnetworkportdef.h | 1 + src/libvirt_private.syms | 1 + tests/networkxml2xmlin/isolated-ports.xml | 7 +++ tests/networkxml2xmlout/isolated-ports.xml | 7 +++ tests/networkxml2xmltest.c | 1 + tests/qemuxml2argvdata/net-isolated-port.xml | 34 ++++++++++ .../net-isolated-port.x86_64-latest.xml | 63 +++++++++++++++++++ tests/qemuxml2xmltest.c | 1 + 17 files changed, 202 insertions(+) create mode 100644 tests/networkxml2xmlin/isolated-ports.xml create mode 100644 tests/networkxml2xmlout/isolated-ports.xml create mode 100644 tests/qemuxml2argvdata/net-isolated-port.xml create mode 100644 tests/qemuxml2xmloutdata/net-isolated-port.x86_64-latest.xml
Not a fan of multi-word elements, because they bring up our inconsistency in using camelCase vs snake_case.
Yeah, it always bothers me when I see a multiword element or attribute for that reason. I always use camelCase because I remember asking about which is preferred > 10 years ago and being told that we wanted to have camelCase in libvirt XML. That could even be a false memory, but it has always stuck with me.
But I assume you chose the name to make it compatible with all four containing elements.
Would something like: <networkport> <port isolated='yes'/> </networkport> look too odd?
ummmm.... God I *HATE* coming up with element and attribute names! (That's the only response I can think of right now since it's late in the day. Let me sleep on it, but in the end I was expecting, even *hoping* someone would object to portOptions and propose an alternative, and yours doesn't really sound any *worse* than mine, so it might be worthwhile to use it just so I wouldn't have to shoulder the blame :-)
Okay, I've come up with nothing better, so unless anyone else thinks of something else that's better between now and when I wake up tomorrow, I'm going to push it using "<port isolated='blah'/>" as Jan suggests rather than "<portOptions isolated='yes'/>". Last chance to object! (unless you feel like reverting patches between now and the end of the month)
Code-wise: Reviewed-by: Ján Tomko <jtomko@redhat.com>
Jano
Similar to the way that the <vlan>, <bandwidth>, and <virtualport> elements and the trustGuestRxFilters attribute in a <network> (or in the appropriate <portgroup> element of a <network> can be applied to a port when it is allocated for a domain's network interface, this patch checks for a configured value of <portOptions isolated="yes|no"/> in either the domain <interface> or in the network, setting isolatedPort in the <networkport> to the first one it finds (the setting from the domain's <interface> is preferred). This, in turn, is passed back to the domain when a port is allocated, so that the domain will use that setting. (One difference from <vlan>, <bandwidth>, <virtualport>, and trustGuestRxFilters, is that all of those can be set in a <portgroup> so that they can be applied only to a subset of interfaces connected to the network. This didn't really make sense for the isolated setting due to the way that it's implemented in Linux - the BR_ISOLATED flag will prevent traffic from passing between two ports that both have BR_ISOLATED set, but traffic can still go between those ports and other ports that *don't* have BR_ISOLATED. (It would be nice if all traffic from a BR_ISOLATED port could be blocked except traffic going to/from a designated egress port or ports, but instead the entire feature is implemented as a single flag. Because of this, it's really only useful if all the ports on a network are isolated, so setting it for a subset has no practical utility.) Signed-off-by: Laine Stump <laine@redhat.com> --- src/conf/domain_conf.c | 3 +++ src/network/bridge_driver.c | 3 +++ 2 files changed, 6 insertions(+) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 176550b62f..dd35522370 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -30800,6 +30800,7 @@ virDomainNetDefToNetworkPort(virDomainDefPtr dom, if (virNetDevVlanCopy(&port->vlan, &iface->vlan) < 0) return NULL; + port->isolatedPort = iface->isolatedPort; port->trustGuestRxFilters = iface->trustGuestRxFilters; return g_steal_pointer(&port); @@ -30899,6 +30900,7 @@ virDomainNetDefActualFromNetworkPort(virDomainNetDefPtr iface, if (virNetDevVlanCopy(&actual->vlan, &port->vlan) < 0) goto error; + actual->isolatedPort = port->isolatedPort; actual->class_id = port->class_id; actual->trustGuestRxFilters = port->trustGuestRxFilters; @@ -31038,6 +31040,7 @@ virDomainNetDefActualToNetworkPort(virDomainDefPtr dom, if (virNetDevVlanCopy(&port->vlan, &actual->vlan) < 0) return NULL; + port->isolatedPort = actual->isolatedPort; port->class_id = actual->class_id; port->trustGuestRxFilters = actual->trustGuestRxFilters; diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c index 94212eec77..e26c5a4879 100644 --- a/src/network/bridge_driver.c +++ b/src/network/bridge_driver.c @@ -4532,6 +4532,9 @@ networkAllocatePort(virNetworkObjPtr obj, port->trustGuestRxFilters = netdef->trustGuestRxFilters; } + if (port->isolatedPort == VIR_TRISTATE_BOOL_ABSENT) + port->isolatedPort = netdef->isolatedPort; + /* merge virtualports from interface, network, and portgroup to * arrive at actual virtualport to use */ -- 2.24.1
On Sun, Feb 16, 2020 at 11:22:55PM -0500, Laine Stump wrote:
Similar to the way that the <vlan>, <bandwidth>, and <virtualport> elements and the trustGuestRxFilters attribute in a <network> (or in the appropriate <portgroup> element of a <network> can be applied to a port when it is allocated for a domain's network interface, this patch checks for a configured value of <portOptions isolated="yes|no"/> in either the domain <interface> or in the network, setting isolatedPort in the <networkport> to the first one it finds (the setting from the domain's <interface> is preferred). This, in turn, is passed back to the domain when a port is allocated, so that the domain will use that setting.
(One difference from <vlan>, <bandwidth>, <virtualport>, and trustGuestRxFilters, is that all of those can be set in a <portgroup> so that they can be applied only to a subset of interfaces connected to the network. This didn't really make sense for the isolated setting due to the way that it's implemented in Linux - the BR_ISOLATED flag will prevent traffic from passing between two ports that both have BR_ISOLATED set, but traffic can still go between those ports and other ports that *don't* have BR_ISOLATED. (It would be nice if all traffic from a BR_ISOLATED port could be blocked except traffic going to/from a designated egress port or ports, but instead the entire feature is implemented as a single flag. Because of this, it's really only useful if all the ports on a network are isolated, so setting it for a subset has no practical utility.)
Signed-off-by: Laine Stump <laine@redhat.com> --- src/conf/domain_conf.c | 3 +++ src/network/bridge_driver.c | 3 +++ 2 files changed, 6 insertions(+)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
This patch pushes the isolatedPort setting from the <interface> down all the way to the callers of virNetDevBridgeAddPort(), and sets BR_ISOLATED on the port (using virNetDevBridgePortSetIsolated()) after the port has been successfully added to the bridge. Signed-off-by: Laine Stump <laine@redhat.com> --- src/bhyve/bhyve_command.c | 1 + src/conf/domain_conf.c | 1 + src/lxc/lxc_process.c | 10 ++++++++++ src/network/bridge_driver.c | 1 + src/qemu/qemu_hotplug.c | 16 ++++++++++++++++ src/qemu/qemu_interface.c | 1 + src/util/virnetdevtap.c | 17 ++++++++++++++++- src/util/virnetdevtap.h | 3 +++ tests/bhyvexml2argvmock.c | 1 + 9 files changed, 50 insertions(+), 1 deletion(-) diff --git a/src/bhyve/bhyve_command.c b/src/bhyve/bhyve_command.c index a8bfc0aa72..2df7b60115 100644 --- a/src/bhyve/bhyve_command.c +++ b/src/bhyve/bhyve_command.c @@ -95,6 +95,7 @@ bhyveBuildNetArgStr(virConnectPtr conn, def->uuid, NULL, NULL, 0, virDomainNetGetActualVirtPortProfile(net), virDomainNetGetActualVlan(net), + virDomainNetGetActualPortOptionsIsolated(net), NULL, 0, NULL, VIR_NETDEV_TAP_CREATE_IFUP | VIR_NETDEV_TAP_CREATE_PERSIST) < 0) { goto cleanup; diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index dd35522370..30b2a53b83 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -31146,6 +31146,7 @@ virDomainNetNotifyActualDevice(virConnectPtr conn, &iface->mac, dom->uuid, virDomainNetGetActualVirtPortProfile(iface), virDomainNetGetActualVlan(iface), + virDomainNetGetActualPortOptionsIsolated(iface), iface->mtu, NULL)); } } diff --git a/src/lxc/lxc_process.c b/src/lxc/lxc_process.c index da6df86834..6851b3e3e2 100644 --- a/src/lxc/lxc_process.c +++ b/src/lxc/lxc_process.c @@ -303,6 +303,16 @@ virLXCProcessSetupInterfaceTap(virDomainDefPtr vm, } else { if (virNetDevBridgeAddPort(brname, parentVeth) < 0) return NULL; + + if (virDomainNetGetActualPortOptionsIsolated(net) == VIR_TRISTATE_BOOL_YES && + virNetDevBridgePortSetIsolated(brname, parentVeth, true) < 0) { + virErrorPtr err; + + virErrorPreserveLast(&err); + ignore_value(virNetDevBridgeRemovePort(brname, parentVeth)); + virErrorRestore(&err); + return NULL; + } } } diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c index e26c5a4879..27d9a24de9 100644 --- a/src/network/bridge_driver.c +++ b/src/network/bridge_driver.c @@ -2489,6 +2489,7 @@ networkStartNetworkVirtual(virNetworkDriverStatePtr driver, if (virNetDevTapCreateInBridgePort(def->bridge, &macTapIfName, &def->mac, NULL, NULL, &tapfd, 1, NULL, NULL, + VIR_TRISTATE_BOOL_NO, NULL, def->mtu, NULL, VIR_NETDEV_TAP_CREATE_USE_MAC_FOR_BRIDGE | VIR_NETDEV_TAP_CREATE_IFUP | diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index 6395826c69..af892255c7 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -3350,12 +3350,28 @@ qemuDomainChangeNetBridge(virDomainObjPtr vm, } ret = virNetDevBridgeAddPort(newbridge, olddev->ifname); + if (ret == 0 && + virDomainNetGetActualPortOptionsIsolated(newdev) == VIR_TRISTATE_BOOL_YES) { + + ret = virNetDevBridgePortSetIsolated(newbridge, olddev->ifname, true); + if (ret < 0) { + virErrorPtr err; + + virErrorPreserveLast(&err); + ignore_value(virNetDevBridgeRemovePort(newbridge, olddev->ifname)); + virErrorRestore(&err); + } + } virDomainAuditNet(vm, NULL, newdev, "attach", ret == 0); if (ret < 0) { virErrorPtr err; virErrorPreserveLast(&err); ret = virNetDevBridgeAddPort(oldbridge, olddev->ifname); + if (ret == 0 && + virDomainNetGetActualPortOptionsIsolated(olddev) == VIR_TRISTATE_BOOL_YES) { + ignore_value(virNetDevBridgePortSetIsolated(newbridge, olddev->ifname, true)); + } virDomainAuditNet(vm, NULL, olddev, "attach", ret == 0); virErrorRestore(&err); return -1; diff --git a/src/qemu/qemu_interface.c b/src/qemu/qemu_interface.c index 74d4782599..8a01eecd83 100644 --- a/src/qemu/qemu_interface.c +++ b/src/qemu/qemu_interface.c @@ -568,6 +568,7 @@ qemuInterfaceBridgeConnect(virDomainDefPtr def, def->uuid, tunpath, tapfd, *tapfdSize, virDomainNetGetActualVirtPortProfile(net), virDomainNetGetActualVlan(net), + virDomainNetGetActualPortOptionsIsolated(net), net->coalesce, 0, NULL, tap_create_flags) < 0) { virDomainAuditNetDevice(def, net, tunpath, false); diff --git a/src/util/virnetdevtap.c b/src/util/virnetdevtap.c index 84d91428e7..7bd30ea0f9 100644 --- a/src/util/virnetdevtap.c +++ b/src/util/virnetdevtap.c @@ -505,6 +505,7 @@ virNetDevTapAttachBridge(const char *tapname, const unsigned char *vmuuid, const virNetDevVPortProfile *virtPortProfile, const virNetDevVlan *virtVlan, + virTristateBool isolatedPort, unsigned int mtu, unsigned int *actualMTU) { @@ -545,6 +546,16 @@ virNetDevTapAttachBridge(const char *tapname, } else { if (virNetDevBridgeAddPort(brname, tapname) < 0) return -1; + + if (isolatedPort == VIR_TRISTATE_BOOL_YES && + virNetDevBridgePortSetIsolated(brname, tapname, true) < 0) { + virErrorPtr err; + + virErrorPreserveLast(&err); + ignore_value(virNetDevBridgeRemovePort(brname, tapname)); + virErrorRestore(&err); + return -1; + } } return 0; @@ -574,6 +585,7 @@ virNetDevTapReattachBridge(const char *tapname, const unsigned char *vmuuid, const virNetDevVPortProfile *virtPortProfile, const virNetDevVlan *virtVlan, + virTristateBool isolatedPort, unsigned int mtu, unsigned int *actualMTU) { @@ -611,6 +623,7 @@ virNetDevTapReattachBridge(const char *tapname, macaddr, vmuuid, virtPortProfile, virtVlan, + isolatedPort, mtu, actualMTU) < 0) return -1; @@ -660,6 +673,7 @@ int virNetDevTapCreateInBridgePort(const char *brname, size_t tapfdSize, const virNetDevVPortProfile *virtPortProfile, const virNetDevVlan *virtVlan, + virTristateBool isolatedPort, virNetDevCoalescePtr coalesce, unsigned int mtu, unsigned int *actualMTU, @@ -697,7 +711,8 @@ int virNetDevTapCreateInBridgePort(const char *brname, goto error; if (virNetDevTapAttachBridge(*ifname, brname, macaddr, vmuuid, - virtPortProfile, virtVlan, mtu, actualMTU) < 0) { + virtPortProfile, virtVlan, + isolatedPort, mtu, actualMTU) < 0) { goto error; } diff --git a/src/util/virnetdevtap.h b/src/util/virnetdevtap.h index cae8e61861..c6bd9285ba 100644 --- a/src/util/virnetdevtap.h +++ b/src/util/virnetdevtap.h @@ -65,6 +65,7 @@ virNetDevTapAttachBridge(const char *tapname, const unsigned char *vmuuid, const virNetDevVPortProfile *virtPortProfile, const virNetDevVlan *virtVlan, + virTristateBool isolatedPort, unsigned int mtu, unsigned int *actualMTU) ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(3) @@ -77,6 +78,7 @@ virNetDevTapReattachBridge(const char *tapname, const unsigned char *vmuuid, const virNetDevVPortProfile *virtPortProfile, const virNetDevVlan *virtVlan, + virTristateBool isolatedPort, unsigned int mtu, unsigned int *actualMTU) ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(3) @@ -91,6 +93,7 @@ int virNetDevTapCreateInBridgePort(const char *brname, size_t tapfdSize, const virNetDevVPortProfile *virtPortProfile, const virNetDevVlan *virtVlan, + virTristateBool isolatedPort, virNetDevCoalescePtr coalesce, unsigned int mtu, unsigned int *actualMTU, diff --git a/tests/bhyvexml2argvmock.c b/tests/bhyvexml2argvmock.c index 2a552f9f47..25b97f5e04 100644 --- a/tests/bhyvexml2argvmock.c +++ b/tests/bhyvexml2argvmock.c @@ -28,6 +28,7 @@ int virNetDevTapCreateInBridgePort(const char *brname G_GNUC_UNUSED, size_t tapfdSize G_GNUC_UNUSED, const virNetDevVPortProfile *virtPortProfile G_GNUC_UNUSED, const virNetDevVlan *virtVlan G_GNUC_UNUSED, + virTristateBool isolatedPort G_GNUC_UNUSED, virNetDevCoalescePtr coalesce G_GNUC_UNUSED, unsigned int mtu G_GNUC_UNUSED, unsigned int *actualMTU G_GNUC_UNUSED, -- 2.24.1
On Sun, Feb 16, 2020 at 11:22:56PM -0500, Laine Stump wrote:
This patch pushes the isolatedPort setting from the <interface> down all the way to the callers of virNetDevBridgeAddPort(), and sets BR_ISOLATED on the port (using virNetDevBridgePortSetIsolated()) after the port has been successfully added to the bridge.
Signed-off-by: Laine Stump <laine@redhat.com> --- src/bhyve/bhyve_command.c | 1 + src/conf/domain_conf.c | 1 + src/lxc/lxc_process.c | 10 ++++++++++ src/network/bridge_driver.c | 1 + src/qemu/qemu_hotplug.c | 16 ++++++++++++++++ src/qemu/qemu_interface.c | 1 + src/util/virnetdevtap.c | 17 ++++++++++++++++- src/util/virnetdevtap.h | 3 +++ tests/bhyvexml2argvmock.c | 1 + 9 files changed, 50 insertions(+), 1 deletion(-)
diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index 6395826c69..af892255c7 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -3350,12 +3350,28 @@ qemuDomainChangeNetBridge(virDomainObjPtr vm, }
ret = virNetDevBridgeAddPort(newbridge, olddev->ifname); + if (ret == 0 && + virDomainNetGetActualPortOptionsIsolated(newdev) == VIR_TRISTATE_BOOL_YES) { + + ret = virNetDevBridgePortSetIsolated(newbridge, olddev->ifname, true); + if (ret < 0) { + virErrorPtr err; + + virErrorPreserveLast(&err); + ignore_value(virNetDevBridgeRemovePort(newbridge, olddev->ifname)); + virErrorRestore(&err); + } + } virDomainAuditNet(vm, NULL, newdev, "attach", ret == 0); if (ret < 0) { virErrorPtr err;
virErrorPreserveLast(&err); ret = virNetDevBridgeAddPort(oldbridge, olddev->ifname); + if (ret == 0 && + virDomainNetGetActualPortOptionsIsolated(olddev) == VIR_TRISTATE_BOOL_YES) { + ignore_value(virNetDevBridgePortSetIsolated(newbridge, olddev->ifname, true));
Should this use 'oldbridge' instead of 'newbridge'?
+ } virDomainAuditNet(vm, NULL, olddev, "attach", ret == 0); virErrorRestore(&err); return -1;
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
On 2/18/20 12:46 PM, Ján Tomko wrote:
On Sun, Feb 16, 2020 at 11:22:56PM -0500, Laine Stump wrote:
This patch pushes the isolatedPort setting from the <interface> down all the way to the callers of virNetDevBridgeAddPort(), and sets BR_ISOLATED on the port (using virNetDevBridgePortSetIsolated()) after the port has been successfully added to the bridge.
Signed-off-by: Laine Stump <laine@redhat.com> --- src/bhyve/bhyve_command.c | 1 + src/conf/domain_conf.c | 1 + src/lxc/lxc_process.c | 10 ++++++++++ src/network/bridge_driver.c | 1 + src/qemu/qemu_hotplug.c | 16 ++++++++++++++++ src/qemu/qemu_interface.c | 1 + src/util/virnetdevtap.c | 17 ++++++++++++++++- src/util/virnetdevtap.h | 3 +++ tests/bhyvexml2argvmock.c | 1 + 9 files changed, 50 insertions(+), 1 deletion(-)
diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index 6395826c69..af892255c7 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -3350,12 +3350,28 @@ qemuDomainChangeNetBridge(virDomainObjPtr vm, }
ret = virNetDevBridgeAddPort(newbridge, olddev->ifname); + if (ret == 0 && + virDomainNetGetActualPortOptionsIsolated(newdev) == VIR_TRISTATE_BOOL_YES) { + + ret = virNetDevBridgePortSetIsolated(newbridge, olddev->ifname, true); + if (ret < 0) { + virErrorPtr err; + + virErrorPreserveLast(&err); + ignore_value(virNetDevBridgeRemovePort(newbridge, olddev->ifname)); + virErrorRestore(&err); + } + } virDomainAuditNet(vm, NULL, newdev, "attach", ret == 0); if (ret < 0) { virErrorPtr err;
virErrorPreserveLast(&err); ret = virNetDevBridgeAddPort(oldbridge, olddev->ifname); + if (ret == 0 && + virDomainNetGetActualPortOptionsIsolated(olddev) == VIR_TRISTATE_BOOL_YES) { + ignore_value(virNetDevBridgePortSetIsolated(newbridge, olddev->ifname, true));
Should this use 'oldbridge' instead of 'newbridge'?
Whoops! Cut/paste error. (At least I removed the part about being a Navy Seal and having a certain set of skills)
+ } virDomainAuditNet(vm, NULL, olddev, "attach", ret == 0); virErrorRestore(&err); return -1;
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Jano
This setting can be updating very easily on an already active interface by just changing it in sysfs. If the bridge used for connection is also changed, there is no need to separately update it, because the new setting isf done as a part of connecting to the bridge anyway. Signed-off-by: Laine Stump <laine@redhat.com> --- src/qemu/qemu_hotplug.c | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+) diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index af892255c7..9800491755 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -3481,6 +3481,7 @@ qemuDomainChangeNet(virQEMUDriverPtr driver, bool needBandwidthSet = false; bool needCoalesceChange = false; bool needVlanUpdate = false; + bool needIsolatedPortChange = false; int ret = -1; int changeidx = -1; g_autoptr(virConnect) conn = NULL; @@ -3805,6 +3806,11 @@ qemuDomainChangeNet(virQEMUDriverPtr driver, needVlanUpdate = true; } + if (virDomainNetGetActualPortOptionsIsolated(olddev) != + virDomainNetGetActualPortOptionsIsolated(newdev)) { + needIsolatedPortChange = true; + } + if (olddev->linkstate != newdev->linkstate) needLinkStateChange = true; @@ -3851,6 +3857,20 @@ qemuDomainChangeNet(virQEMUDriverPtr driver, * determined that the rest of newdev is equivalent to olddev, * so move newdev into place */ needReplaceDevDef = true; + + /* this is already updated as a part of reconnecting the bridge */ + needIsolatedPortChange = false; + } + + if (needIsolatedPortChange) { + const char *bridge = virDomainNetGetActualBridgeName(newdev); + bool isolatedOn = (virDomainNetGetActualPortOptionsIsolated(newdev) == + VIR_TRISTATE_BOOL_YES); + + if (virNetDevBridgePortSetIsolated(bridge, newdev->ifname, isolatedOn) < 0) + goto cleanup; + + needReplaceDevDef = true; } if (needFilterChange) { -- 2.24.1
On Sun, Feb 16, 2020 at 11:22:57PM -0500, Laine Stump wrote:
This setting can be updating very easily on an already active interface by just changing it in sysfs. If the bridge used for connection is also changed, there is no need to separately update it, because the new setting isf done as a part of connecting to the bridge
s/isf/is/
anyway.
Signed-off-by: Laine Stump <laine@redhat.com> --- src/qemu/qemu_hotplug.c | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
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
Signed-off-by: Laine Stump <laine@redhat.com> --- docs/news.xml | 21 +++++++++++++++++++++ 1 file changed, 21 insertions(+) diff --git a/docs/news.xml b/docs/news.xml index 5aa9d081a7..97a455d721 100644 --- a/docs/news.xml +++ b/docs/news.xml @@ -82,6 +82,27 @@ "type" and "persistent" attributes. </description> </change> + <change> + <summary> + support BR_ISOLATED flag for guest interfaces attached to a Linux host bridge + </summary> + <description> + Since Linux kernel 4.18, the Linux host bridge has had a + flag BR_ISOLATED that can be applied to individual + ports. When this flag is set for a port, traffic is blocked + between that port and any other port that also has the + BR_ISOLATED flag set. libvirt domain interface config now + supports setting this flag via the <portOptions + isolated='yes'/> setting. It can also be set for all + connections to a particular libvirt network by setting the + same option in the network config - since the port for the + host itself does not have BR_ISOLATED set, the guests can + communicate with the host and the outside world, but guests + on that network can't communicate with each other. This + feature works for QEMU and LXC guests with interfaces + attached to a Linux host bridge. + </description> + </change> <change> <summary> qemu: Introduce the 'armvtimer' timer type -- 2.24.1
On Sun, Feb 16, 2020 at 11:22:59PM -0500, Laine Stump wrote:
Signed-off-by: Laine Stump <laine@redhat.com> --- docs/news.xml | 21 +++++++++++++++++++++ 1 file changed, 21 insertions(+)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
participants (2)
-
Ján Tomko -
Laine Stump