Re: [PATCH] util: Check libnl function availability

Akihiko Odaki <akihiko.odaki@xxxxxxxxxx> · Fri, 7 Mar 2025 22:21:40 +0900

On 2025/03/07 22:11, Laine Stump wrote:
On 3/7/25 6:48 AM, Akihiko Odaki wrote:
virNetDevBridgeSetupVlans() calls virNetlinkBridgeVlanFilterSet(),
which requires libnl. Use the function only when libnl is available
to avoid breaking builds.

Signed-off-by: Akihiko Odaki <akihiko.odaki@xxxxxxxxxx>

I'm curious what is the Linux platform where we don't have libnl available?

I didn't have libnl3-devel installed.

Another option is to require libnl3-devel in Mesa; it will give nicer 
error messages to anyone trying to build libvirt without libnl and 
allows us to drop WITH_LIBNL checks.


Also, in this case you've removed the ability to add ports to a bridge, 
even if the request has no vlan info. If there really are Linux distros 
that don't have libnl then we don't want to completely disable the 
ability to add a tap to a bridge - instead we should move the check for 
vlan == NULL from the top of virNetDevBridgeSetupVlans() to the place 
where virNetDevBridgeSetupVlans() is called (in 
virNetDevBridgeAddPort()). Then virNetDeBridgeSetupVlans should be 
placed inside  an #if defined(WITH_LIBNL) that has an #else alternative 
implementation just reporting that vlans can't be set on host bridge 
ports on this platform - this entire set of 2 
virNetDevBridgeeSetupVlan() functions should be defined immediately 
preceding the definition of virNetDebBridgeAddPort(), inside the same 
#if as that function.

So in short, you should have:

#if defined(WITH_STRUCT_IFREQ) && defined(SIOCBRADDIF)
# if defined(WITH_LIBNL)
static int
virNetDevBridgeSetupVlans()
{
   /* existing virNetDevBridgeSetupVlans() body except NULL check */
}
# else
static int
virNetDevBridgeSetupVlans()
{
    virReportSystemError(ENOSYS, "%s",
                          _("Cannot configure vlans on host bridge ports
                            on this platform"));
                          /* *un*break previous two lines!! */
     return -1;}
}
# fi


int
virNetDevBridgeAddPort(...)
{
   /* existing body of this function, except at the end do this: */
   if (virtVlan && virtVlan->nTags)
       return virNetDevBridgeSetupVlans(ifname, virtVlan);
   else
       return 0;
}
#elif defined(WITH_BSD_BRIDGE_MGMT)
...


This way you'll stil have the ability to add ports to a bridge when 
libnl isn't available, but will get a runtime error if someone tries to 
configure vlan stuff for a port.

---
  src/util/virnetdevbridge.c | 122 +++++++++++++++++++++ 
+-----------------------
  1 file changed, 61 insertions(+), 61 deletions(-)

diff --git a/src/util/virnetdevbridge.c b/src/util/virnetdevbridge.c
index 
c79d0c79b7b16a0a070b5013ed7df9d0a63b1c38..70e82a0029634ddf44c2b933577bf783c89f035c 100644
--- a/src/util/virnetdevbridge.c
+++ b/src/util/virnetdevbridge.c
@@ -313,66 +313,6 @@ 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) {
-        if (error != 0) {
-            virReportSystemError(-error,
-                                 _("error removing vlan filter from 
interface %1$s"),
-                                 ifname);
-        }
-        return -1;
-    }
-
-    // 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;
-            }
-
-            if (virNetlinkBridgeVlanFilterSet(ifname, RTM_SETLINK, 
flags,
-                                              virtVlan->nativeTag, 
&error) < 0) {
-                goto error;
-            }
-        }
-
-        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 error;
-                }
-        }
-    } 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 error;
-        }
-    }
-
-    return 0;
-
- error:
-    if (error != 0)
-        virReportSystemError(-error, _("error adding vlan filter to 
interface %1$s"), ifname);
-    return -1;
-}
-
  #else
  int
@@ -651,7 +591,67 @@ int virNetDevBridgeDelete(const char *brname 
G_GNUC_UNUSED)
   *
   * Returns 0 in case of success or an errno code in case of failure.
   */
-#if defined(WITH_STRUCT_IFREQ) && defined(SIOCBRADDIF)
+#if defined(WITH_STRUCT_IFREQ) && defined(SIOCBRADDIF) && 
defined(WITH_LIBNL)
+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) {
+        if (error != 0) {
+            virReportSystemError(-error,
+                                 _("error removing vlan filter from 
interface %1$s"),
+                                 ifname);
+        }
+        return -1;
+    }
+
+    // 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;
+            }
+
+            if (virNetlinkBridgeVlanFilterSet(ifname, RTM_SETLINK, 
flags,
+                                              virtVlan->nativeTag, 
&error) < 0) {
+                goto error;
+            }
+        }
+
+        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 error;
+                }
+        }
+    } 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 error;
+        }
+    }
+
+    return 0;
+
+ error:
+    if (error != 0)
+        virReportSystemError(-error, _("error adding vlan filter to 
interface %1$s"), ifname);
+    return -1;
+}
+
  int virNetDevBridgeAddPort(const char *brname,
                             const char *ifname,
                             const virNetDevVlan *virtVlan)

---
base-commit: e5299ddf86121d3c792ca271ffcb54900eb19dc3
change-id: 20250304-nl-605e05b8c5f2

Best regards,








