Re: [PATCH] util: Check libnl function availability

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(a)daynix.com&gt; I'm curious what is the Linux platform where we don't have libnl available?

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,