Hello, On 2025-01-08 07:26, Laine Stump wrote:
On 1/2/25 4:29 AM, Leigh Brown wrote:Add virNetDevBridgeSetupVlans function to configures a bridge interface using the passed virNetDevVlan struct. Add virVlan parameter to the Linux version of virNetDevBridgeAddPort and call virNetDevBridgeSetupVlans to set up the required vlan configuration. Update callers of virNetDevBridgeAddPort to pass NULL for now. Signed-off-by: Leigh Brown <leigh@xxxxxxxxxxxxx>--- src/lxc/lxc_process.c | 2 +-src/util/virnetdevbridge.c | 75 ++++++++++++++++++++++++++++++++++++--src/util/virnetdevbridge.h | 4 +- src/util/virnetdevtap.c | 2 +- 4 files changed, 76 insertions(+), 7 deletions(-) diff --git a/src/lxc/lxc_process.c b/src/lxc/lxc_process.c index c2982244f0..7c760cec40 100644 --- a/src/lxc/lxc_process.c +++ b/src/lxc/lxc_process.c @@ -289,7 +289,7 @@ virLXCProcessSetupInterfaceTap(virDomainDef *vm,vport, virDomainNetGetActualVlan(net)) < 0)return NULL; } else { - if (virNetDevBridgeAddPort(brname, parentVeth) < 0) + if (virNetDevBridgeAddPort(brname, parentVeth, NULL) < 0) return NULL;if (virDomainNetGetActualPortOptionsIsolated(net) == VIR_TRISTATE_BOOL_YES &&diff --git a/src/util/virnetdevbridge.c b/src/util/virnetdevbridge.c index 5fd88f3195..80f028e9b7 100644 --- a/src/util/virnetdevbridge.c +++ b/src/util/virnetdevbridge.c@@ -313,6 +313,65 @@ virNetDevBridgePortSetIsolated(const char *brname, return virNetDevBridgePortSet(brname, ifname, "isolated", enable ? 1 : 0);} +static int+virNetDevBridgeSetupVlans(const char *ifname, const virNetDevVlan *virtVlan)+{ + int error = 0; + unsigned short flags; + + if (!virtVlan || !virtVlan->nTags) + return 0; ++ // The interface will have been automatically added to vlan 1, so remove it + if (virNetlinkBridgeVlanFilterSet(ifname, RTM_DELLINK, 0, 1, &error) < 0)+ goto err_delete; + + // If trunk mode, add the native VLAN then add the others, if any + if (virtVlan->trunk) { + size_t i; + + if (virtVlan->nativeTag) { + flags = BRIDGE_VLAN_INFO_PVID;+ if (virtVlan->nativeMode == VIR_NATIVE_VLAN_MODE_UNTAGGED ||+ virtVlan->nativeMode == VIR_NATIVE_VLAN_MODE_DEFAULT) + flags |= BRIDGE_VLAN_INFO_UNTAGGED;I think some people aren't as pedantic about it as I am (since I often encounter braceless code like this), but the libvirt coding guidelines (at https://libvirt.org/coding-style.html) do require adding { } when the conditional expression itself takes up multiple lines at the same indentation level, even if the code inside the conditional is just a single line.
I will update.
++ if (virNetlinkBridgeVlanFilterSet(ifname, RTM_SETLINK, flags, + virtVlan->nativeTag, &error) < 0)+ goto err_add;(For some reason, the coding guidelines *don't* require braces in this case (they say it's optional because the difference in indentation "makes it obvious it is a single line"), but I still prefer braces in this case as well)
I will update.
+ } + + for (i = 0; i < virtVlan->nTags; i++) { + if (virtVlan->tag[i] != virtVlan->nativeTag)+ if (virNetlinkBridgeVlanFilterSet(ifname, RTM_SETLINK, 0, + virtVlan->tag[i], &error) < 0)+ goto err_add; + } + } else { + // In native mode, add the single VLAN as pvid untagged + flags = BRIDGE_VLAN_INFO_PVID | BRIDGE_VLAN_INFO_UNTAGGED; + if (virNetlinkBridgeVlanFilterSet(ifname, RTM_SETLINK, flags,+ virtVlan->tag[0], &error) < 0)+ goto err_add; + }(NB: I'm assuming that the netlink commands you're sending actually do the correct thing (i.e. identical to what is done for the same config when connecting to an OVS bridge) in all the cases (trunking, etc), as I don't use vlans myself. What you have looks like it makes sense though :-))
I've not used OVS but I read the documentation and I believe the behaviour is the same.
+ + return 0; + + err_add: + if (error != 0) + virReportSystemError(-error,+ _("error adding vlan filter to interface %1$s"),+ ifname); + return -1; + + err_delete: + if (error != 0) + virReportSystemError(-error,+ _("error removing vlan filter from interface %1$s"),+ ifname); + return -1;Although there are a few other instances of a goto label with a name other than "cleanup" or "error", we try to keep it to those as much as possible. Since there's only a single "goto err_delete", maybe we could just move that error reporting and return up to the place that jumps to it (and then rename err_add to error)? (see the "Use of goto" section in the coding-style document I referenced above)If you are okay with these two changes and want to make them today, I can push an updated version tonight, or if you're okay with them but don't have the time to make those changes today, I can do it for you before pushing; let me know one way or the other.
Thanks, I will make the changes and send an updated version.
Assuming those changes: Reviewed-by: Laine Stump <laine@xxxxxxxxxx>+} + #else int@@ -593,7 +652,8 @@ int virNetDevBridgeDelete(const char *brname G_GNUC_UNUSED)*/ #if defined(WITH_STRUCT_IFREQ) && defined(SIOCBRADDIF) int virNetDevBridgeAddPort(const char *brname, - const char *ifname) + const char *ifname, + const virNetDevVlan *virtVlan) { struct ifreq ifr; VIR_AUTOCLOSE fd = -1; @@ -613,14 +673,20 @@ int virNetDevBridgeAddPort(const char *brname, return -1; } - return 0; + return virNetDevBridgeSetupVlans(ifname, virtVlan); } #elif defined(WITH_BSD_BRIDGE_MGMT) int virNetDevBridgeAddPort(const char *brname, - const char *ifname) + const char *ifname, + const virNetDevVlan *virtVlan) { struct ifbreq req = { 0 }; + if (virtVlan) {+ virReportSystemError(ENOSYS, "%s", _("Not supported on this platform"));+ return -1; + } + if (virStrcpyStatic(req.ifbr_ifsname, ifname) < 0) { virReportSystemError(ERANGE,_("Network interface name '%1$s' is too long"),@@ -638,7 +704,8 @@ int virNetDevBridgeAddPort(const char *brname, } #else int virNetDevBridgeAddPort(const char *brname, - const char *ifname) + const char *ifname, + const virNetDevVlan *virtVlan) { virReportSystemError(ENOSYS,_("Unable to add bridge %1$s port %2$s"), brname, ifname);diff --git a/src/util/virnetdevbridge.h b/src/util/virnetdevbridge.h index db4099bf0b..5f51656abe 100644 --- a/src/util/virnetdevbridge.h +++ b/src/util/virnetdevbridge.h @@ -20,6 +20,7 @@ #include "internal.h" #include "virmacaddr.h" +#include "virnetdevvlan.h" int virNetDevBridgeCreate(const char *brname, const virMacAddr *mac) @@ -28,7 +29,8 @@ int virNetDevBridgeDelete(const char *brname) ATTRIBUTE_NONNULL(1) G_GNUC_WARN_UNUSED_RESULT; int virNetDevBridgeAddPort(const char *brname, - const char *ifname) + const char *ifname, + const virNetDevVlan *virtVlan)ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) G_GNUC_WARN_UNUSED_RESULT;int virNetDevBridgeRemovePort(const char *brname, diff --git a/src/util/virnetdevtap.c b/src/util/virnetdevtap.c index 2701ba6dfc..a9573eb8e1 100644 --- a/src/util/virnetdevtap.c +++ b/src/util/virnetdevtap.c @@ -483,7 +483,7 @@ virNetDevTapAttachBridge(const char *tapname, return -1; } } else { - if (virNetDevBridgeAddPort(brname, tapname) < 0) + if (virNetDevBridgeAddPort(brname, tapname, NULL) < 0) return -1; if (isolatedPort == VIR_TRISTATE_BOOL_YES &&
