On Mon, May 12, 2025 at 15:37:12 +0200, Michal Privoznik via Devel wrote: Hmm seems that it was used only from src/util/virnetdevbridge.c thus the linker didn't need it outside. I presume you want to mock or use it; please mention that in the commit message because the justification there doens't make sense unless you want to use it outside of src/util/ Reviewed-by: Peter Krempa <pkrempa(a)redhat.com&gt;

From: Michal Privoznik <mprivozn(a)redhat.com&gt; In virnetlink.c there are two sections: the first one when building WITH_LIBNL support, the other that provides stubs for functions declared in the corresponding header file when building without netlink support. But the stub implementation for virNetlinkBridgeVlanFilterSet() was missing. Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com&gt; --- src/util/virnetlink.c | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/src/util/virnetlink.c b/src/util/virnetlink.c index 206646d9d7..2555457cd2 100644 --- a/src/util/virnetlink.c +++ b/src/util/virnetlink.c @@ -1344,6 +1344,17 @@ virNetlinkNewLink(const char *ifname G_GNUC_UNUSED, } +int +virNetlinkBridgeVlanFilterSet(const char *ifname G_GNUC_UNUSED, + int cmd G_GNUC_UNUSED, + const unsigned short fflags G_GNUC_UNUSED, + const short vid G_GNUC_UNUSED, + int *error G_GNUC_UNUSED) +{ + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _(unsupported)); + return -1; +} + int virNetlinkGetNeighbor(void **nlData G_GNUC_UNUSED, uint32_t src_pid G_GNUC_UNUSED, -- 2.49.0

On 5/13/25 09:53, Peter Krempa wrote: No, this needs to be anything else but 'flags' otherwise our sc_flags_usage syntax-check rule will complain. unusedflags perhaps? Good point. Indeed, I went with consistency over correctness. Michal

From: Michal Privoznik <mprivozn(a)redhat.com&gt; We still use C89 style of comments. Fix C99 style of comments used in virNetDevBridgeSetupVlans(). Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com&gt; --- src/util/virnetdevbridge.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/util/virnetdevbridge.c b/src/util/virnetdevbridge.c index c79d0c79b7..3b54e2cb1e 100644 --- a/src/util/virnetdevbridge.c +++ b/src/util/virnetdevbridge.c @@ -322,7 +322,7 @@ virNetDevBridgeSetupVlans(const char *ifname, const virNetDevVlan *virtVlan) if (!virtVlan || !virtVlan->nTags) return 0; - // The interface will have been automatically added to vlan 1, so remove it + /* The interface will have been automatically added to vlan 1, so remove it. */ if (virNetlinkBridgeVlanFilterSet(ifname, RTM_DELLINK, 0, 1, &error) < 0) { if (error != 0) { virReportSystemError(-error, @@ -332,7 +332,7 @@ virNetDevBridgeSetupVlans(const char *ifname, const virNetDevVlan *virtVlan) return -1; } - // If trunk mode, add the native VLAN then add the others, if any + /* If trunk mode, add the native VLAN then add the others, if any. */ if (virtVlan->trunk) { size_t i; @@ -357,7 +357,7 @@ virNetDevBridgeSetupVlans(const char *ifname, const virNetDevVlan *virtVlan) } } } else { - // In native mode, add the single VLAN as pvid untagged + /* 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) { -- 2.49.0

From: Michal Privoznik <mprivozn(a)redhat.com&gt; The whole point of virnetlink.h is that it hides away the build time dependency on netlink. It wraps netlink functions in our functions which then have a stub implementation in case netlink support was disabled. Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com&gt; --- src/util/virnetdevbridge.c | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/src/util/virnetdevbridge.c b/src/util/virnetdevbridge.c index 3b54e2cb1e..70e78a9be6 100644 --- a/src/util/virnetdevbridge.c +++ b/src/util/virnetdevbridge.c @@ -23,6 +23,7 @@ #include "virerror.h" #include "virfile.h" #include "virlog.h" +#include "virnetlink.h" #include "virstring.h" #ifdef WITH_NET_IF_H @@ -30,9 +31,6 @@ #endif #ifdef __linux__ -# if defined(WITH_LIBNL) -# include "virnetlink.h" -# endif # include <linux/sockios.h> # include <linux/param.h> /* HZ */ # include <linux/in6.h> -- 2.49.0

From: Michal Privoznik <mprivozn(a)redhat.com&gt; Currently, virNetlinkBridgeVlanFilterSet() takes @cmd as the second argument where either RTM_SETLINK or RTM_DELLINK is expected. Both of these constants come from linux/rtnetlink.h and thus are undefined when building without netlink. This design also clashes with the whole point of virnetlink: to offload netlink dependency onto a single file. Therefore, drop the argument, turn virNetlinkBridgeVlanFilterSet() into just setter, effectively, and introduce virNetlinkBridgeVlanFilterDel() for the case when RTM_DELLINK would be passed as @cmd. Resolves: https://gitlab.com/libvirt/libvirt/-/issues/770 Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com&gt; --- src/libvirt_private.syms | 1 + src/util/virnetdevbridge.c | 8 ++--- src/util/virnetlink.c | 73 +++++++++++++++++++++++++++++++++----- src/util/virnetlink.h | 5 ++- 4 files changed, 74 insertions(+), 13 deletions(-) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 727b4b2caa..67eb17a527 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -3037,6 +3037,7 @@ virNetDevVPortProfileOpTypeToString; # util/virnetlink.h +virNetlinkBridgeVlanFilterDel; virNetlinkBridgeVlanFilterSet; virNetlinkCommand; virNetlinkDelLink; diff --git a/src/util/virnetdevbridge.c b/src/util/virnetdevbridge.c index 70e78a9be6..385eccf2d8 100644 --- a/src/util/virnetdevbridge.c +++ b/src/util/virnetdevbridge.c @@ -321,7 +321,7 @@ virNetDevBridgeSetupVlans(const char *ifname, const virNetDevVlan *virtVlan) return 0; /* The interface will have been automatically added to vlan 1, so remove it. */ - if (virNetlinkBridgeVlanFilterSet(ifname, RTM_DELLINK, 0, 1, &error) < 0) { + if (virNetlinkBridgeVlanFilterDel(ifname, 1, &error) < 0) { if (error != 0) { virReportSystemError(-error, _("error removing vlan filter from interface %1$s"), @@ -341,7 +341,7 @@ virNetDevBridgeSetupVlans(const char *ifname, const virNetDevVlan *virtVlan) flags |= BRIDGE_VLAN_INFO_UNTAGGED; } - if (virNetlinkBridgeVlanFilterSet(ifname, RTM_SETLINK, flags, + if (virNetlinkBridgeVlanFilterSet(ifname, flags, virtVlan->nativeTag, &error) < 0) { goto error; } @@ -349,7 +349,7 @@ virNetDevBridgeSetupVlans(const char *ifname, const virNetDevVlan *virtVlan) for (i = 0; i < virtVlan->nTags; i++) { if (virtVlan->tag[i] != virtVlan->nativeTag) - if (virNetlinkBridgeVlanFilterSet(ifname, RTM_SETLINK, 0, + if (virNetlinkBridgeVlanFilterSet(ifname, 0, virtVlan->tag[i], &error) < 0) { goto error; } @@ -357,7 +357,7 @@ virNetDevBridgeSetupVlans(const char *ifname, const virNetDevVlan *virtVlan) } 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, + if (virNetlinkBridgeVlanFilterSet(ifname, flags, virtVlan->tag[0], &error) < 0) { goto error; } diff --git a/src/util/virnetlink.c b/src/util/virnetlink.c index 2555457cd2..c7404279bc 100644 --- a/src/util/virnetlink.c +++ b/src/util/virnetlink.c @@ -702,7 +702,7 @@ virNetlinkDelLink(const char *ifname, virNetlinkTalkFallback fallback) } /** - * virNetlinkBridgeVlanFilterSet: + * virNetlinkBridgeVlanFilterHelper: * * @ifname: name of the link * @cmd: netlink command, either RTM_SETLINK or RTM_DELLINK @@ -717,12 +717,12 @@ virNetlinkDelLink(const char *ifname, virNetlinkTalkFallback fallback) * non-zero, then a netlink failure occurred, but no error message * is generated leaving it up to the caller to handle the condition. */ -int -virNetlinkBridgeVlanFilterSet(const char *ifname, - int cmd, - const unsigned short flags, - const short vid, - int *error) +static int +virNetlinkBridgeVlanFilterHelper(const char *ifname, + int cmd, + const unsigned short flags, + const short vid, + int *error) { struct ifinfomsg ifm = { .ifi_family = PF_BRIDGE }; struct bridge_vlan_info vinfo = { .flags = flags, .vid = vid }; @@ -767,6 +767,55 @@ virNetlinkBridgeVlanFilterSet(const char *ifname, return 0; } + +/** + * virNetlinkBridgeVlanFilterSet: + * + * @ifname: name of the link + * @flags: flags to use when adding the vlan filter + * @vid: vlan id to add + * @error: netlink error code + * + * Add a vlan filter from an interface associated with a bridge. + * + * Returns 0 on success, -1 on error. Additionally, if the @error is + * non-zero, then a netlink failure occurred, but no error message + * is generated leaving it up to the caller to handle the condition. + */ +int +virNetlinkBridgeVlanFilterSet(const char *ifname, + const unsigned short flags, + const short vid, + int *error) +{ + return virNetlinkBridgeVlanFilterHelper(ifname, RTM_SETLINK, + flags, vid, error); +} + + +/** + * virNetlinkBridgeVlanFilterDel: + * + * @ifname: name of the link + * @vid: vlan id to remove + * @error: netlink error code + * + * Remove a vlan filter from an interface associated with a bridge. + * + * Returns 0 on success, -1 on error. Additionally, if the @error is + * non-zero, then a netlink failure occurred, but no error message + * is generated leaving it up to the caller to handle the condition. + */ +int +virNetlinkBridgeVlanFilterDel(const char *ifname, + const short vid, + int *error) +{ + return virNetlinkBridgeVlanFilterHelper(ifname, + RTM_DELLINK, 0, vid, error); +} + + /** * virNetlinkGetNeighbor: * @@ -1346,7 +1395,6 @@ virNetlinkNewLink(const char *ifname G_GNUC_UNUSED, int virNetlinkBridgeVlanFilterSet(const char *ifname G_GNUC_UNUSED, - int cmd G_GNUC_UNUSED, const unsigned short fflags G_GNUC_UNUSED, const short vid G_GNUC_UNUSED, int *error G_GNUC_UNUSED) @@ -1355,6 +1403,15 @@ virNetlinkBridgeVlanFilterSet(const char *ifname G_GNUC_UNUSED, return -1; } +int +virNetlinkBridgeVlanFilterDel(const char *ifname G_GNUC_UNUSED, + const short vid G_GNUC_UNUSED, + int *error G_GNUC_UNUSED) +{ + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _(unsupported)); + return -1; +} + int virNetlinkGetNeighbor(void **nlData G_GNUC_UNUSED, uint32_t src_pid G_GNUC_UNUSED, diff --git a/src/util/virnetlink.h b/src/util/virnetlink.h index 327fb426a1..74d4f5b613 100644 --- a/src/util/virnetlink.h +++ b/src/util/virnetlink.h @@ -78,11 +78,14 @@ typedef int (*virNetlinkTalkFallback)(const char *ifname); int virNetlinkDelLink(const char *ifname, virNetlinkTalkFallback fallback); int virNetlinkBridgeVlanFilterSet(const char *ifname, - int cmd, const unsigned short flags, const short vid, int *error); +int virNetlinkBridgeVlanFilterDel(const char *ifname, + const short vid, + int *error); + int virNetlinkGetErrorCode(struct nlmsghdr *resp, unsigned int recvbuflen); int virNetlinkDumpLink(const char *ifname, int ifindex, -- 2.49.0