4:38 p.m.
In order to make a client-only build successful on RHEL4, commit
3ed2e54 modified src/util/virnetdev.c so that the functional version
of virNetDevGetVLanID() was only compiled if GET_VLAN_VID_CMD was
defined. However, it is *never* defined, but is only an enum value, so
the proper version was no longer compiled even on platforms that
support it. This resulted in the vlan tag not being properly set for
guest traffic on VEPA mode guest macvtap interfaces that were bound to
a vlan interface (that's the only place that libvirt currently uses
virNetDevGetVLanID)
Since there is no way to compile conditionally based on the presence
of an enum value, this patch modifies configure.ac to check for said
enum value with AC_CHECK_DECLS(), which #defines HAVE_GET_VLAN_VID_CMD
if it's successful compiling a test program that uses
GET_VLAN_VID_CMD. We can then make the compilation of
virNetDevGetVLanID() conditional on that #define instead.
---
Difference from V1:
Use the amazingly compact AC_CHECK_DECLS() instead
of deprecated AC_TRY_COMPILE() + extra trappings.
configure.ac | 2 ++
src/util/virnetdev.c | 4 ++--
2 files changed, 4 insertions(+), 2 deletions(-)
diff --git a/configure.ac b/configure.ac
index 7ecbee9..0d505d3 100644
--- a/configure.ac
+++ b/configure.ac
@@ -2470,6 +2470,8 @@ if test "$with_virtualport" != "no"; then
fi
AM_CONDITIONAL([WITH_VIRTUALPORT], [test "$with_virtualport" =
"yes"])
+dnl GET_VLAN_VID_CMD is required for virNetDevGetVLanID
+AC_CHECK_DECLS([GET_VLAN_VID_CMD], [], [], [[#include <linux/if_vlan.h>]])
dnl netlink library
diff --git a/src/util/virnetdev.c b/src/util/virnetdev.c
index e74fc5f..72f52c7 100644
--- a/src/util/virnetdev.c
+++ b/src/util/virnetdev.c
@@ -1,5 +1,5 @@
/*
- * Copyright (C) 2007-2013 Red Hat, Inc.
+ * Copyright (C) 2007-2014 Red Hat, Inc.
*
* This library is free software; you can redistribute it and/or
* modify it under the terms of the GNU Lesser General Public
@@ -732,7 +732,7 @@ int virNetDevGetIndex(const char *ifname ATTRIBUTE_UNUSED,
#endif /* ! SIOCGIFINDEX */
-#if defined(SIOCGIFVLAN) && defined(HAVE_STRUCT_IFREQ) &&
defined(GET_VLAN_VID_CMD)
+#if defined(SIOCGIFVLAN) && defined(HAVE_STRUCT_IFREQ) &&
defined(HAVE_GET_VLAN_VID_CMD)
int virNetDevGetVLanID(const char *ifname, int *vlanid)
{
struct vlan_ioctl_args vlanargs = {
--
1.8.5.3
4:49 p.m.
On 02/10/2014 03:38 PM, Laine Stump wrote:
In order to make a client-only build successful on RHEL4, commit
3ed2e54 modified src/util/virnetdev.c so that the functional version
of virNetDevGetVLanID() was only compiled if GET_VLAN_VID_CMD was
defined. However, it is *never* defined, but is only an enum value, so
the proper version was no longer compiled even on platforms that
support it. This resulted in the vlan tag not being properly set for
guest traffic on VEPA mode guest macvtap interfaces that were bound to
a vlan interface (that's the only place that libvirt currently uses
virNetDevGetVLanID)
Since there is no way to compile conditionally based on the presence
of an enum value, this patch modifies configure.ac to check for said
enum value with AC_CHECK_DECLS(), which #defines HAVE_GET_VLAN_VID_CMD
if it's successful compiling a test program that uses
GET_VLAN_VID_CMD. We can then make the compilation of
virNetDevGetVLanID() conditional on that #define instead.
---
Difference from V1:
Use the amazingly compact AC_CHECK_DECLS() instead
of deprecated AC_TRY_COMPILE() + extra trappings.
configure.ac | 2 ++
src/util/virnetdev.c | 4 ++--
2 files changed, 4 insertions(+), 2 deletions(-)
Almost!
-#if defined(SIOCGIFVLAN) && defined(HAVE_STRUCT_IFREQ) &&
defined(GET_VLAN_VID_CMD)
+#if defined(SIOCGIFVLAN) && defined(HAVE_STRUCT_IFREQ) &&
defined(HAVE_GET_VLAN_VID_CMD)
s/defined(HAVE_GET_VLAN_VID_CMD)/HAVE_GET_VLAN_VID_CMD/
AC_CHECK_DECLS unconditionally defines the variable to either 0 or 1, so
checking if it is defined will still fail to compile on older headers
that lack the enum; what you want is to check that it has a non-zero value.
ACK with that fix.
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
6:10 p.m.
On 02/11/2014 12:49 AM, Eric Blake wrote:
On 02/10/2014 03:38 PM, Laine Stump wrote:
> In order to make a client-only build successful on RHEL4, commit
> 3ed2e54 modified src/util/virnetdev.c so that the functional version
> of virNetDevGetVLanID() was only compiled if GET_VLAN_VID_CMD was
> defined. However, it is *never* defined, but is only an enum value, so
> the proper version was no longer compiled even on platforms that
> support it. This resulted in the vlan tag not being properly set for
> guest traffic on VEPA mode guest macvtap interfaces that were bound to
> a vlan interface (that's the only place that libvirt currently uses
> virNetDevGetVLanID)
>
> Since there is no way to compile conditionally based on the presence
> of an enum value, this patch modifies configure.ac to check for said
> enum value with AC_CHECK_DECLS(), which #defines HAVE_GET_VLAN_VID_CMD
> if it's successful compiling a test program that uses
> GET_VLAN_VID_CMD. We can then make the compilation of
> virNetDevGetVLanID() conditional on that #define instead.
> ---
>
> Difference from V1:
>
> Use the amazingly compact AC_CHECK_DECLS() instead
> of deprecated AC_TRY_COMPILE() + extra trappings.
>
> configure.ac | 2 ++
> src/util/virnetdev.c | 4 ++--
> 2 files changed, 4 insertions(+), 2 deletions(-)
Almost!
>
> -#if defined(SIOCGIFVLAN) && defined(HAVE_STRUCT_IFREQ) &&
defined(GET_VLAN_VID_CMD)
> +#if defined(SIOCGIFVLAN) && defined(HAVE_STRUCT_IFREQ) &&
defined(HAVE_GET_VLAN_VID_CMD)
s/defined(HAVE_GET_VLAN_VID_CMD)/HAVE_GET_VLAN_VID_CMD/
As a matter of fact, "HAVE_DECL_GET_VLAN_VID_CMD". I fixed that and
pushed. Thanks for the tutorial :-)
AC_CHECK_DECLS unconditionally defines the variable to either 0 or 1, so
checking if it is defined will still fail to compile on older headers
that lack the enum;
Well, it would have compiled successfully, but compiled the wrong code :-)
what you want is to check that it has a non-zero value.
Now that I've seen this, it bugs me that all HAVE_* #defines aren't
consistent about this - some are undefined if the capability is missing,
and some are #defined, but to 0.
10:30 p.m.
On 02/10/2014 05:10 PM, Laine Stump wrote:
>
> AC_CHECK_DECLS unconditionally defines the variable to either 0 or 1, so
> checking if it is defined will still fail to compile on older headers
> that lack the enum;
Well, it would have compiled successfully, but compiled the wrong code :-)
> what you want is to check that it has a non-zero value.
Now that I've seen this, it bugs me that all HAVE_* #defines aren't
consistent about this - some are undefined if the capability is missing,
and some are #defined, but to 0.
It's historical baggage of autoconf - the various checks were written at
different times, but autoconf can't change the older checks to always
define the macro because of back-compat. The general rule of thumb is
HAVE_DECL_ is always defined, any other HAVE_ is likely undefined if the
feature is missing.
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
3938
days inactive
3939
days old
3 comments
2 participants
participants (2)
-
Eric Blake -
Laine Stump