On 10/1/20 1:14 AM, Laine Stump wrote: This should be squashed in: diff --git i/meson.build w/meson.build index 073ea6d49e..592f5882fa 100644 --- i/meson.build +++ w/meson.build @@ -1161,8 +1161,7 @@ m_dep = cc.find_library('m', required : false) use_macvtap = false if not get_option('macvtap').disabled() - if (cc.has_header_symbol('linux/if_link.h', 'MACVLAN_MODE_BRIDGE') and - cc.has_header_symbol('linux/if_link.h', 'IFLA_VF_MAX')) + if cc.has_header_symbol('linux/if_link.h', 'MACVLAN_MODE_BRIDGE') and use_macvtap = true endif Ah, you're removing the whole block later in the series. Okay then. Michal

WITH_LIBNL will only be defined on Linux platforms (because libnl is a library written to encapsulate parts of netlink, which is a Linux-only API), so it's redundant to write: #if defined(__linux__) && defined(WITH_LIBNL) We can just check for WITH_LIBNL. Signed-off-by: Laine Stump <laine(a)redhat.com&gt; --- src/util/virnetdev.c | 10 +++++----- src/util/virnetdevbridge.c | 6 +++--- src/util/virnetdevip.c | 6 +++--- src/util/virnetlink.c | 4 ++-- src/util/virnetlink.h | 4 ++-- 5 files changed, 15 insertions(+), 15 deletions(-) diff --git a/src/util/virnetdev.c b/src/util/virnetdev.c index 07208a1876..737081a12b 100644 --- a/src/util/virnetdev.c +++ b/src/util/virnetdev.c @@ -885,7 +885,7 @@ int virNetDevGetIndex(const char *ifname G_GNUC_UNUSED, #endif /* ! SIOCGIFINDEX */ -#if defined(__linux__) && defined(WITH_LIBNL) +#if defined(WITH_LIBNL) /** * virNetDevGetMaster: * @ifname: name of interface we're interested in @@ -929,7 +929,7 @@ virNetDevGetMaster(const char *ifname G_GNUC_UNUSED, } -#endif /* defined(__linux__) && defined(WITH_LIBNL) */ +#endif /* defined(WITH_LIBNL) */ #if defined(SIOCGIFVLAN) && defined(WITH_STRUCT_IFREQ) && WITH_DECL_GET_VLAN_VID_CMD @@ -1473,7 +1473,7 @@ virNetDevSysfsFile(char **pf_sysfs_device_link G_GNUC_UNUSED, #endif /* !__linux__ */ -#if defined(__linux__) && defined(WITH_LIBNL) +#if defined(WITH_LIBNL) static virMacAddr zeroMAC = { .addr = { 0, 0, 0, 0, 0, 0 } }; @@ -2266,7 +2266,7 @@ virNetDevSetNetConfig(const char *linkdev, int vf, } -#else /* defined(__linux__) && defined(WITH_LIBNL) */ +#else /* defined(WITH_LIBNL) */ int @@ -2309,7 +2309,7 @@ virNetDevSetNetConfig(const char *linkdev G_GNUC_UNUSED, } -#endif /* defined(__linux__) && defined(WITH_LIBNL) */ +#endif /* defined(WITH_LIBNL) */ VIR_ENUM_IMPL(virNetDevIfState, VIR_NETDEV_IF_STATE_LAST, diff --git a/src/util/virnetdevbridge.c b/src/util/virnetdevbridge.c index 9fec770fb6..d475e4c43d 100644 --- a/src/util/virnetdevbridge.c +++ b/src/util/virnetdevbridge.c @@ -441,7 +441,7 @@ virNetDevBridgeCreateWithIoctl(const char *brname, } #endif -#if defined(__linux__) && defined(WITH_LIBNL) +#if defined(WITH_LIBNL) int virNetDevBridgeCreate(const char *brname, const virMacAddr *mac) @@ -552,7 +552,7 @@ virNetDevBridgeDeleteWithIoctl(const char *brname) #endif -#if defined(__linux__) && defined(WITH_LIBNL) +#if defined(WITH_LIBNL) int virNetDevBridgeDelete(const char *brname) { @@ -976,7 +976,7 @@ virNetDevBridgeSetVlanFiltering(const char *brname G_GNUC_UNUSED, #endif -#if defined(__linux__) && defined(WITH_LIBNL) +#if defined(WITH_LIBNL) # ifndef NTF_SELF # define NTF_SELF 0x02 diff --git a/src/util/virnetdevip.c b/src/util/virnetdevip.c index 4887ccfcf1..a69567da6f 100644 --- a/src/util/virnetdevip.c +++ b/src/util/virnetdevip.c @@ -52,7 +52,7 @@ VIR_LOG_INIT("util.netdevip"); -#if defined(__linux__) && defined(WITH_LIBNL) +#if defined(WITH_LIBNL) static int virNetDevGetIPAddressBinary(virSocketAddr *addr, void **data, size_t *len) @@ -373,7 +373,7 @@ virNetDevIPRouteAdd(const char *ifname, } -#else /* defined(__linux__) && defined(WITH_LIBNL) */ +#else /* defined(WITH_LIBNL) */ int @@ -494,7 +494,7 @@ virNetDevIPRouteAdd(const char *ifname, return 0; } -#endif /* defined(__linux__) && defined(HAVE_LIBNL) */ +#endif /* defined(HAVE_LIBNL) */ #if defined(__linux__) diff --git a/src/util/virnetlink.c b/src/util/virnetlink.c index be444baa56..ea06db51ee 100644 --- a/src/util/virnetlink.c +++ b/src/util/virnetlink.c @@ -40,7 +40,7 @@ VIR_LOG_INIT("util.netlink"); #define NETLINK_ACK_TIMEOUT_S (2*1000) -#if defined(__linux__) && defined(WITH_LIBNL) +#if defined(WITH_LIBNL) /* State for a single netlink event handle */ struct virNetlinkEventHandle { int watch; @@ -1387,4 +1387,4 @@ virNetlinkGetErrorCode(struct nlmsghdr *resp G_GNUC_UNUSED, return -EINVAL; } -#endif /* __linux__ */ +#endif /* WITH_LIBNL */ diff --git a/src/util/virnetlink.h b/src/util/virnetlink.h index 40f27524f6..7121eac43e 100644 --- a/src/util/virnetlink.h +++ b/src/util/virnetlink.h @@ -22,7 +22,7 @@ #include "internal.h" #include "virmacaddr.h" -#if defined(__linux__) && defined(WITH_LIBNL) +#if defined(WITH_LIBNL) # include <netlink/msg.h> @@ -36,7 +36,7 @@ struct sockaddr_nl; struct nlattr; struct nlmsghdr; -#endif /* __linux__ */ +#endif /* WITH_LIBNL */ #define NETLINK_MSG_NEST_START(msg, container, attrtype) \ do { \ -- 2.26.2

On Wed, Sep 30, 2020 at 07:14:38PM -0400, Laine Stump wrote: We should also remove the 'virtualport' option from meson_options.txt . Pavel

macvlan support was added to the Linux kernel in 2.6.33, but MACVLAN_MODE_PASSTHRU wasn't added until 2.6.38, so a workaround had been put in place to define that constant on those few systems where it was missing. It's useful like was probably 6 months at most, but it's been there for over 10 years. Signed-off-by: Laine Stump <laine(a)redhat.com&gt; --- meson.build | 4 ---- src/util/virnetdevmacvlan.c | 5 ----- 2 files changed, 9 deletions(-) diff --git a/meson.build b/meson.build index 073ea6d49e..5e0af76bbc 100644 --- a/meson.build +++ b/meson.build @@ -1172,10 +1172,6 @@ if not get_option('macvtap').disabled() endif if use_macvtap conf.set('WITH_MACVTAP', 1) - - if cc.has_header_symbol('linux/if_link.h', 'MACVLAN_MODE_PASSTHRU') - conf.set('WITH_DECL_MACVLAN_MODE_PASSTHRU', 1) - endif endif netcf_version = '0.1.8' diff --git a/src/util/virnetdevmacvlan.c b/src/util/virnetdevmacvlan.c index 143e1ab98c..889cbb2293 100644 --- a/src/util/virnetdevmacvlan.c +++ b/src/util/virnetdevmacvlan.c @@ -47,11 +47,6 @@ VIR_ENUM_IMPL(virNetDevMacVLanMode, # include <linux/if_tun.h> # include <math.h> -/* Older kernels lacked this enum value. */ -# if !WITH_DECL_MACVLAN_MODE_PASSTHRU -# define MACVLAN_MODE_PASSTHRU 8 -# endif - # include "viralloc.h" # include "virlog.h" # include "viruuid.h" -- 2.26.2

This flag was originally created to indicate that either 1) the build platform wasn't linux, 2) the build platform was linux, but the kernel was too old to have macvtap support; since there was already a switch there, the ability to also disable it in case 3) the kernel supported macvtap but the user didn't want it, was added in. I don't think that (3) was ever an intentional goal, just something that grew naturally out of having the flag there in the first place (unless possibly the original author wanted a way to quickly disable their new code in case it caused regressions elsewhere). Now that the check for (2) has been removed, WITH_MACVTAP is just checking (1) and (3), but (3) is pointless (since it adds almost nothing extra in size to the code). We can therfore eliminate the WITH_MACVTAP flag, as it is equivalent to __linux__. *However*, macvtap/macvlan devices are created using netlink messages, and any netlink interaction in libvirt requires libnl. So what we *really* need is to check WITH_LIBNL (which itself implies __linux__, as libnl is only useful/available on Linux). Signed-off-by: Laine Stump <laine(a)redhat.com&gt; --- libvirt.spec.in | 1 - meson.build | 10 ---------- src/util/virnetdevmacvlan.c | 6 +++--- 3 files changed, 3 insertions(+), 14 deletions(-) diff --git a/libvirt.spec.in b/libvirt.spec.in index c4a7c30737..aa2bc84be9 100644 --- a/libvirt.spec.in +++ b/libvirt.spec.in @@ -1167,7 +1167,6 @@ export SOURCE_DATE_EPOCH=$(stat --printf='%Y' %{_specdir}/%{name}.spec) -Dyajl=enabled \ %{?arg_sanlock} \ -Dlibpcap=enabled \ - -Dmacvtap=enabled \ -Daudit=enabled \ -Ddtrace=enabled \ %{?arg_firewalld} \ diff --git a/meson.build b/meson.build index fe08a45b46..a6b6f2d2ee 100644 --- a/meson.build +++ b/meson.build @@ -1159,16 +1159,6 @@ libxml_dep = dependency('libxml-2.0', version: '>=' + libxml_version) cc = meson.get_compiler('c') m_dep = cc.find_library('m', required : false) -if host_machine.system() == 'linux' - if not get_option('macvtap').disabled() - conf.set('WITH_MACVTAP', 1) - endif -else - if get_option('macvtap').enabled() - error('macvtap is not supported on this platform.') - endif -endif - netcf_version = '0.1.8' netcf_dep = dependency('netcf', version: '>=' + netcf_version, required: get_option('netcf')) if netcf_dep.found() diff --git a/src/util/virnetdevmacvlan.c b/src/util/virnetdevmacvlan.c index 889cbb2293..25eb53c5da 100644 --- a/src/util/virnetdevmacvlan.c +++ b/src/util/virnetdevmacvlan.c @@ -40,7 +40,7 @@ VIR_ENUM_IMPL(virNetDevMacVLanMode, "passthrough", ); -#if WITH_MACVTAP +#if defined(WITH_LIBNL) # include <fcntl.h> # include <net/if.h> @@ -1051,7 +1051,7 @@ int virNetDevMacVLanRestartWithVPortProfile(const char *cr_ifname, } -#else /* ! WITH_MACVTAP */ +#else /* ! WITH_LIBNL */ bool virNetDevMacVLanIsMacvtap(const char *ifname G_GNUC_UNUSED) { virReportSystemError(ENOSYS, "%s", @@ -1157,4 +1157,4 @@ void virNetDevMacVLanReserveName(const char *name G_GNUC_UNUSED) virReportSystemError(ENOSYS, "%s", _("Cannot create macvlan devices on this platform")); } -#endif /* ! WITH_MACVTAP */ +#endif /* ! WITH_LIBNL */ -- 2.26.2

On Wed, Sep 30, 2020 at 07:14:41PM -0400, Laine Stump wrote: Missing change to meson_options.txt to remove 'macvtap' as well. Pavel

Lack of this one function (which is called for each active tap device every time libvirtd is started) is the one thing preventing a "WITHOUT_LIBNL" build of libvirt from being useful. With this alternate implementation, guests using standard tap devices will work properly. Signed-off-by: Laine Stump <laine(a)redhat.com&gt; --- src/util/virnetdev.c | 23 ++++++++++++++++++++++- 1 file changed, 22 insertions(+), 1 deletion(-) diff --git a/src/util/virnetdev.c b/src/util/virnetdev.c index 5221bada7b..c43823c747 100644 --- a/src/util/virnetdev.c +++ b/src/util/virnetdev.c @@ -915,9 +915,30 @@ virNetDevGetMaster(const char *ifname, char **master) return 0; } +#elif defined(__linux__) -#else +/* libnl isn't available, so we can't use netlink. + * Fall back to using sysfs + */ +int +virNetDevGetMaster(const char *ifname, char **master) +{ + g_autofree char *path = NULL; + g_autofree char *canonical = NULL; + + if (virNetDevSysfsFile(&path, ifname, "master") < 0) + return -1; + if (!(canonical = virFileCanonicalizePath(path))) + return -1; + + *master = g_path_get_basename(canonical); + + VIR_DEBUG("IFLA_MASTER for %s is %s", ifname, *master ? *master : "(none)"); + return 0; +} + +#else int virNetDevGetMaster(const char *ifname G_GNUC_UNUSED, -- 2.26.2

On 10/1/20 4:06 AM, Michal Prívozník wrote: Yes, at two different levels: 1) A standard configure would try to enable "macvtap" and "virtualport" by default, and the checks for both of those would generate an error (saying "libnl is required for this feature") if libnl3-devel wasn't installed. 2) since commit 8708ca01 in 2017, and ending with the patch just prior to this one, if you removed libnl3-devel from your system, and then had --without-macvtap --without-virtualport (or whatever the flags were on the configure commandline - I've forgotten) then make would fail with a compile error in virnetdev.c. But nobody ever does either of those (because "Why would you?"). I guess I should add the above details to the commit logs, now that I've taken the time to look it up and try it :-) Earlier patches in this series eliminate the virtualport and macvtap flags, and patch 7 fixes the compile error, so finally a build without libnl can succeed. It was only after I did that, and tried installing a build w/o libnl and firing it up that I saw the errors (well, I had suspected that's what would happen after a manual inspection of places we were using netlink, but installing and running it verified the suspicion).