[PATCH 0/9] various build flag related fixes
These patches eliminate a few build flags that were necessary back 10 years ago when their associated features had been newly added to the kernel, but are now pointless (because all supported Linuxes have the feature) (1-6), makes it possible to to a successful (and usable, albeit limited in some ways) build on a system that doesn't have libnl3-devel installed (7-8) (macvtap and interface type='hostdev' don't work, but standard tap devices do), and removes a duplicated identifier check that I noticed in meson.build (9). Laine Stump (9): util: remove useless check for IFLA_VF_MAX util: remove extraneous defined(__linux__) when checking for WITH_LIBNL build: eliminate useless WITH_VIRTUALPORT check build: remove check for MACVLAN_MODE_PASSTHRU build: simplify check for WITH_MACVTAP build: eliminate WITH_MACVTAP flag util: fix Linux build when libnl-devel isn't available util: provide non-netlink/libnl alternative for virNetDevGetMaster() build: remove duplicate check for GET_VLAN_VID_CMD libvirt.spec.in | 1 - meson.build | 30 ------------------------ src/util/virnetdev.c | 40 ++++++++++++++++++++++++++------ src/util/virnetdevbridge.c | 6 ++--- src/util/virnetdevip.c | 6 ++--- src/util/virnetdevmacvlan.c | 11 +++------ src/util/virnetdevvportprofile.c | 8 +++---- src/util/virnetlink.c | 4 ++-- src/util/virnetlink.h | 4 ++-- tools/virsh.c | 3 --- 10 files changed, 50 insertions(+), 63 deletions(-) -- 2.26.2
IFLA_VF_MAX was introduced to the Linux kernel in 2.6.35, and was even backported to the RHEL*6* 2.6.32 kernel downstream, so it is present in all supported versions of all Linux distros that libvirt builds on. Additionally, it can't be conditionally compiled out of a kernel. There is no reason to conditionalize any piece of code on presence of IFLA_VF_MAX - if the platform is Linux, it is supported. Signed-off-by: Laine Stump <laine@redhat.com> --- src/util/virnetdev.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/util/virnetdev.c b/src/util/virnetdev.c index e1a4cc2bef..07208a1876 100644 --- a/src/util/virnetdev.c +++ b/src/util/virnetdev.c @@ -1473,7 +1473,7 @@ virNetDevSysfsFile(char **pf_sysfs_device_link G_GNUC_UNUSED, #endif /* !__linux__ */ -#if defined(__linux__) && defined(WITH_LIBNL) && defined(IFLA_VF_MAX) +#if defined(__linux__) && 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) && defined(IFLA_VF_MAX) */ +#else /* defined(__linux__) && defined(WITH_LIBNL) */ int @@ -2309,7 +2309,7 @@ virNetDevSetNetConfig(const char *linkdev G_GNUC_UNUSED, } -#endif /* defined(__linux__) && defined(WITH_LIBNL) && defined(IFLA_VF_MAX) */ +#endif /* defined(__linux__) && defined(WITH_LIBNL) */ VIR_ENUM_IMPL(virNetDevIfState, VIR_NETDEV_IF_STATE_LAST, -- 2.26.2
On 10/1/20 1:14 AM, Laine Stump wrote:
IFLA_VF_MAX was introduced to the Linux kernel in 2.6.35, and was even backported to the RHEL*6* 2.6.32 kernel downstream, so it is present in all supported versions of all Linux distros that libvirt builds on. Additionally, it can't be conditionally compiled out of a kernel. There is no reason to conditionalize any piece of code on presence of IFLA_VF_MAX - if the platform is Linux, it is supported.
Signed-off-by: Laine Stump <laine@redhat.com> --- src/util/virnetdev.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/src/util/virnetdev.c b/src/util/virnetdev.c index e1a4cc2bef..07208a1876 100644 --- a/src/util/virnetdev.c +++ b/src/util/virnetdev.c @@ -1473,7 +1473,7 @@ virNetDevSysfsFile(char **pf_sysfs_device_link G_GNUC_UNUSED,
#endif /* !__linux__ */ -#if defined(__linux__) && defined(WITH_LIBNL) && defined(IFLA_VF_MAX) +#if defined(__linux__) && 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) && defined(IFLA_VF_MAX) */ +#else /* defined(__linux__) && defined(WITH_LIBNL) */
int @@ -2309,7 +2309,7 @@ virNetDevSetNetConfig(const char *linkdev G_GNUC_UNUSED, }
-#endif /* defined(__linux__) && defined(WITH_LIBNL) && defined(IFLA_VF_MAX) */ +#endif /* defined(__linux__) && defined(WITH_LIBNL) */
VIR_ENUM_IMPL(virNetDevIfState, VIR_NETDEV_IF_STATE_LAST,
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
On 10/1/20 4:06 AM, Michal Prívozník wrote:
On 10/1/20 1:14 AM, Laine Stump wrote:
IFLA_VF_MAX was introduced to the Linux kernel in 2.6.35, and was even backported to the RHEL*6* 2.6.32 kernel downstream, so it is present in all supported versions of all Linux distros that libvirt builds on. Additionally, it can't be conditionally compiled out of a kernel. There is no reason to conditionalize any piece of code on presence of IFLA_VF_MAX - if the platform is Linux, it is supported.
Signed-off-by: Laine Stump <laine@redhat.com> --- src/util/virnetdev.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/src/util/virnetdev.c b/src/util/virnetdev.c index e1a4cc2bef..07208a1876 100644 --- a/src/util/virnetdev.c +++ b/src/util/virnetdev.c @@ -1473,7 +1473,7 @@ virNetDevSysfsFile(char **pf_sysfs_device_link G_GNUC_UNUSED, #endif /* !__linux__ */ -#if defined(__linux__) && defined(WITH_LIBNL) && defined(IFLA_VF_MAX) +#if defined(__linux__) && 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) && defined(IFLA_VF_MAX) */ +#else /* defined(__linux__) && defined(WITH_LIBNL) */ int @@ -2309,7 +2309,7 @@ virNetDevSetNetConfig(const char *linkdev G_GNUC_UNUSED, } -#endif /* defined(__linux__) && defined(WITH_LIBNL) && defined(IFLA_VF_MAX) */ +#endif /* defined(__linux__) && defined(WITH_LIBNL) */ VIR_ENUM_IMPL(virNetDevIfState, VIR_NETDEV_IF_STATE_LAST,
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
Well, really I meant to point out in the commit log that MACVLAN_MODE_BRIDGE has also been in the kernel since 2.6.33 and so is always present for all Linuxes.
use_macvtap = true endif
Ah, you're removing the whole block later in the series. Okay then.
Yeah, that too. I really only left the two patches separate in case someone complained about removing the user-controlled flag (although I see 0 use for it, since it doesn't remove any library dependency, and doesn't reduce the size of any binary by a useful amount).
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@redhat.com> --- 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
WITH_VIRTUALPORT just checks that we are building on Linux and that IFLA_PORT_MAX is defined in linux/if_link.h. Back when 802.11Qb[gh] support was added, the IFLA_* stuff was new (introduced in kernel 2.6.35, backported to RHEL6 2.6.32 kernel at some point), and so this extra check was necessary, because libvirt was being built on Linux distros that didn't yet have IFLA_* (e.g. older RHEL6, all RHEL5). It's been in the kernel for a *very* long time now, so all supported versions of all Linux platforms libvirt builds on have it. Note that the above paragraph implies that the conditional compilation should be changed to #if defined(__linux__). However, the astute reader will notice that the code in question is sending and receiving netlink messages, so it really should be conditional on WITH_LIBNL (which implies __linux__) instead, so that's what this patch does. Signed-off-by: Laine Stump <laine@redhat.com> --- meson.build | 8 -------- src/util/virnetdevvportprofile.c | 8 ++++---- tools/virsh.c | 3 --- 3 files changed, 4 insertions(+), 15 deletions(-) diff --git a/meson.build b/meson.build index 2e57a435df..073ea6d49e 100644 --- a/meson.build +++ b/meson.build @@ -1386,14 +1386,6 @@ endif util_dep = cc.find_library('util', required: false) -if not get_option('virtualport').disabled() - if cc.has_header_symbol('linux/if_link.h', 'IFLA_PORT_MAX') - conf.set('WITH_VIRTUALPORT', 1) - elif get_option('virtualport').enabled() - error('Installed linux headers don\'t show support for virtual port support.') - endif -endif - if host_machine.system() == 'windows' ole32_dep = cc.find_library('ole32') oleaut32_dep = cc.find_library('oleaut32') diff --git a/src/util/virnetdevvportprofile.c b/src/util/virnetdevvportprofile.c index 020683fa04..5dae8aa57d 100644 --- a/src/util/virnetdevvportprofile.c +++ b/src/util/virnetdevvportprofile.c @@ -46,7 +46,7 @@ VIR_ENUM_IMPL(virNetDevVPortProfileOp, "no-op", ); -#if WITH_VIRTUALPORT +#if defined(WITH_LIBNL) # include <fcntl.h> @@ -454,7 +454,7 @@ int virNetDevVPortProfileMerge3(virNetDevVPortProfilePtr *result, } -#if WITH_VIRTUALPORT +#if defined(WITH_LIBNL) static struct nla_policy ifla_port_policy[IFLA_PORT_MAX + 1] = { @@ -1343,7 +1343,7 @@ virNetDevVPortProfileDisassociate(const char *macvtap_ifname, return rc; } -#else /* ! WITH_VIRTUALPORT */ +#else /* !WITH_LIBNL */ int virNetDevVPortProfileAssociate(const char *macvtap_ifname G_GNUC_UNUSED, const virNetDevVPortProfile *virtPort G_GNUC_UNUSED, const virMacAddr *macvtap_macaddr G_GNUC_UNUSED, @@ -1369,4 +1369,4 @@ int virNetDevVPortProfileDisassociate(const char *macvtap_ifname G_GNUC_UNUSED, _("Virtual port profile association not supported on this platform")); return -1; } -#endif /* ! WITH_VIRTUALPORT */ +#endif /* !WITH_LIBNL */ diff --git a/tools/virsh.c b/tools/virsh.c index ac9b7ad238..954778b626 100644 --- a/tools/virsh.c +++ b/tools/virsh.c @@ -548,9 +548,6 @@ virshShowVersion(vshControl *ctl G_GNUC_UNUSED) #endif #ifdef WITH_NWFILTER vshPrint(ctl, " Nwfilter"); -#endif -#ifdef WITH_VIRTUALPORT - vshPrint(ctl, " VirtualPort"); #endif vshPrint(ctl, "\n"); -- 2.26.2
On Wed, Sep 30, 2020 at 07:14:38PM -0400, Laine Stump wrote:
WITH_VIRTUALPORT just checks that we are building on Linux and that IFLA_PORT_MAX is defined in linux/if_link.h. Back when 802.11Qb[gh] support was added, the IFLA_* stuff was new (introduced in kernel 2.6.35, backported to RHEL6 2.6.32 kernel at some point), and so this extra check was necessary, because libvirt was being built on Linux distros that didn't yet have IFLA_* (e.g. older RHEL6, all RHEL5). It's been in the kernel for a *very* long time now, so all supported versions of all Linux platforms libvirt builds on have it.
Note that the above paragraph implies that the conditional compilation should be changed to #if defined(__linux__). However, the astute reader will notice that the code in question is sending and receiving netlink messages, so it really should be conditional on WITH_LIBNL (which implies __linux__) instead, so that's what this patch does.
Signed-off-by: Laine Stump <laine@redhat.com> --- meson.build | 8 -------- src/util/virnetdevvportprofile.c | 8 ++++---- tools/virsh.c | 3 --- 3 files changed, 4 insertions(+), 15 deletions(-)
diff --git a/meson.build b/meson.build index 2e57a435df..073ea6d49e 100644 --- a/meson.build +++ b/meson.build @@ -1386,14 +1386,6 @@ endif
util_dep = cc.find_library('util', required: false)
-if not get_option('virtualport').disabled() - if cc.has_header_symbol('linux/if_link.h', 'IFLA_PORT_MAX') - conf.set('WITH_VIRTUALPORT', 1) - elif get_option('virtualport').enabled() - error('Installed linux headers don\'t show support for virtual port support.') - endif -endif
We should also remove the 'virtualport' option from meson_options.txt . Pavel
On 10/1/20 11:04 AM, Pavel Hrdina wrote:
We should also remove the 'virtualport' option from meson_options.txt .
Ah, okay. I hadn't been aware of that file. Fortunately my final sanity builds were delayed while dealing with the daily "school issues", so I haven't pushed yet :-)
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@redhat.com> --- 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
macvtap support was added to the Linux kernel in 2.6.33. libvirt checked for this by looking for MACVLAN_MODE_BRIDGE and IFLA_VF_MAX in linux/if_link.h. This hasn't been necessary for a very long time, so just gate on platform == 'linux' (and be sure to complain if someone tries to enable it on a non-Linux platform). Signed-off-by: Laine Stump <laine@redhat.com> --- meson.build | 17 ++++++----------- 1 file changed, 6 insertions(+), 11 deletions(-) diff --git a/meson.build b/meson.build index 5e0af76bbc..fe08a45b46 100644 --- a/meson.build +++ b/meson.build @@ -1159,20 +1159,15 @@ libxml_dep = dependency('libxml-2.0', version: '>=' + libxml_version) cc = meson.get_compiler('c') 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')) - use_macvtap = true +if host_machine.system() == 'linux' + if not get_option('macvtap').disabled() + conf.set('WITH_MACVTAP', 1) endif - - if get_option('macvtap').enabled() and not use_macvtap - error('Installed linux headers don\'t show support for macvtap device.') +else + if get_option('macvtap').enabled() + error('macvtap is not supported on this platform.') endif endif -if use_macvtap - conf.set('WITH_MACVTAP', 1) -endif netcf_version = '0.1.8' netcf_dep = dependency('netcf', version: '>=' + netcf_version, required: get_option('netcf')) -- 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@redhat.com> --- 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 a Wednesday in 2020, Laine Stump wrote:
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).
The indentation is off here. Jano
Signed-off-by: Laine Stump <laine@redhat.com> --- libvirt.spec.in | 1 - meson.build | 10 ---------- src/util/virnetdevmacvlan.c | 6 +++--- 3 files changed, 3 insertions(+), 14 deletions(-)
On Wed, Sep 30, 2020 at 07:14:41PM -0400, Laine Stump wrote:
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@redhat.com> --- 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
Missing change to meson_options.txt to remove 'macvtap' as well. Pavel
There was one stray bit of code in virnetdev.c that required libnl to build, but wasn't qualified by defined(WITH_LIBNL). Adding that, plus putting a similar check around a static function only used by that aforementioned code, makes libvirt build properly without libnl-devel installed. How useful it is in that state is a separate issue :-) Signed-off-by: Laine Stump <laine@redhat.com> --- src/util/virnetdev.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/src/util/virnetdev.c b/src/util/virnetdev.c index 737081a12b..5221bada7b 100644 --- a/src/util/virnetdev.c +++ b/src/util/virnetdev.c @@ -1058,6 +1058,9 @@ virNetDevSysfsDeviceFile(char **pf_sysfs_device_link, const char *ifname, return 0; } + +# if defined(WITH_LIBNL) + /** * Determine if the device path specified in devpath is a PCI Device * by resolving the 'subsystem'-link in devpath and looking for @@ -1091,6 +1094,7 @@ virNetDevIsPCIDevice(const char *devpath) return STRPREFIX(subsys, "pci"); } + static virPCIDevicePtr virNetDevGetPCIDevice(const char *devName) { @@ -1110,6 +1114,7 @@ virNetDevGetPCIDevice(const char *devName) return virPCIDeviceNew(vfPCIAddr->domain, vfPCIAddr->bus, vfPCIAddr->slot, vfPCIAddr->function); } +# endif /** @@ -2958,7 +2963,7 @@ virNetDevGetEthtoolFeatures(const char *ifname, } -# if WITH_DECL_DEVLINK_CMD_ESWITCH_GET +# if defined(WITH_LIBNL) && WITH_DECL_DEVLINK_CMD_ESWITCH_GET /** * virNetDevGetFamilyId: -- 2.26.2
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@redhat.com> --- 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 1:14 AM, Laine Stump wrote:
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@redhat.com> --- 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,
Whoa, building without LIBNL should fail, n'est-ce pas? I'm not saying your patch is wrong, it's just that I'm surprised we haven't caught this earlier. Michal
On 10/1/20 4:06 AM, Michal Prívozník wrote:
On 10/1/20 1:14 AM, Laine Stump wrote:
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@redhat.com> --- 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,
Whoa, building without LIBNL should fail, n'est-ce pas? I'm not saying your patch is wrong, it's just that I'm surprised we haven't caught this earlier.
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).
Somehow this check was duplicated just below the original. (I was at first skeptical that it's needed at all, since GET_VLAN_VID_CMD was already present in kernel 2.6.32, but then I realized that there is no higher level check for __linux__ around the code that is conditional on WITH_DECL_GET_VLAN_VID_CMD; it only checks for SIOCGIFVLAN and WITH_STRUCT_IFREQ - the latter is also present on *BSD platforms, the former doesn't seem to be anywhere but Linux, but I didn't want to change the effect of the conditional, so I left it in (we could have also replaced WITH_DECL_VET_VLAN_VID_CMD, but possibly there is a non-Linux platform that *does* have it...) Signed-off-by: Laine Stump <laine@redhat.com> --- meson.build | 3 --- 1 file changed, 3 deletions(-) diff --git a/meson.build b/meson.build index a6b6f2d2ee..fa19677fee 100644 --- a/meson.build +++ b/meson.build @@ -774,9 +774,6 @@ symbols = [ [ 'unistd.h', 'SEEK_HOLE' ], - # GET_VLAN_VID_CMD is required for virNetDevGetVLanID - [ 'linux/if_vlan.h', 'GET_VLAN_VID_CMD' ], - # Check for BSD approach for setting MAC addr [ 'net/if_dl.h', 'link_addr', '#include <sys/types.h>\n#include <sys/socket.h>' ], ] -- 2.26.2
On 10/1/20 1:14 AM, Laine Stump wrote:
These patches eliminate a few build flags that were necessary back 10 years ago when their associated features had been newly added to the kernel, but are now pointless (because all supported Linuxes have the feature) (1-6), makes it possible to to a successful (and usable, albeit limited in some ways) build on a system that doesn't have libnl3-devel installed (7-8) (macvtap and interface type='hostdev' don't work, but standard tap devices do), and removes a duplicated identifier check that I noticed in meson.build (9).
Laine Stump (9): util: remove useless check for IFLA_VF_MAX util: remove extraneous defined(__linux__) when checking for WITH_LIBNL build: eliminate useless WITH_VIRTUALPORT check build: remove check for MACVLAN_MODE_PASSTHRU build: simplify check for WITH_MACVTAP build: eliminate WITH_MACVTAP flag util: fix Linux build when libnl-devel isn't available util: provide non-netlink/libnl alternative for virNetDevGetMaster() build: remove duplicate check for GET_VLAN_VID_CMD
libvirt.spec.in | 1 - meson.build | 30 ------------------------ src/util/virnetdev.c | 40 ++++++++++++++++++++++++++------ src/util/virnetdevbridge.c | 6 ++--- src/util/virnetdevip.c | 6 ++--- src/util/virnetdevmacvlan.c | 11 +++------ src/util/virnetdevvportprofile.c | 8 +++---- src/util/virnetlink.c | 4 ++-- src/util/virnetlink.h | 4 ++-- tools/virsh.c | 3 --- 10 files changed, 50 insertions(+), 63 deletions(-)
Reviewed-by: Michal Privoznik <mprivozn@redhat.com> Michal
participants (4)
-
Ján Tomko -
Laine Stump -
Michal Prívozník -
Pavel Hrdina