2:34 p.m.
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(a)solinno.co.uk>
> ---
> 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(a)redhat.com>
>
>> +}
>> +
>> #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 &&