5:42 p.m.
The refactoring in 6a5978833a5 and df3d8c362d3 was incomplete
as it accidentally moved macvtap related code out of a
#if WITH_MACVTAP in a #if __linux__ block. To fix this move
ifaceMacvtapLinkAdd and ifaceMacvtapLinkDump back under
#if WITH_MACVTAP.
Also nlComm was moved from #if WITH_MACVTAP to #if __linux__
but configure.ac was not updated to match this, as libnl is
now required on Linux always because of this.
Also related to libnl, libvirt_lxc missed LIBNL_CFLAGS in it's
CFLAGS.
---
configure.ac | 6 +++---
src/Makefile.am | 1 +
src/util/interface.c | 6 +++---
3 files changed, 7 insertions(+), 6 deletions(-)
diff --git a/configure.ac b/configure.ac
index f816696..4c81e3b 100644
--- a/configure.ac
+++ b/configure.ac
@@ -2372,10 +2372,10 @@ dnl netlink library
LIBNL_CFLAGS=""
LIBNL_LIBS=""
-if test "$with_macvtap" = "yes"; then
+if test "$with_linux" = "yes"; then
PKG_CHECK_MODULES([LIBNL], [libnl-1 >= $LIBNL_REQUIRED], [
], [
- AC_MSG_ERROR([libnl-devel >= $LIBNL_REQUIRED is required for macvtap
support])
+ AC_MSG_ERROR([libnl-devel >= $LIBNL_REQUIRED is required])
])
fi
@@ -2574,7 +2574,7 @@ AC_MSG_NOTICE([ pcap: $LIBPCAP_CFLAGS $LIBPCAP_LIBS])
else
AC_MSG_NOTICE([ pcap: no])
fi
-if test "$with_macvtap" = "yes" ; then
+if test "$with_linux" = "yes" ; then
AC_MSG_NOTICE([ nl: $LIBNL_CFLAGS $LIBNL_LIBS])
else
AC_MSG_NOTICE([ nl: no])
diff --git a/src/Makefile.am b/src/Makefile.am
index b292f9f..9e69f37 100644
--- a/src/Makefile.am
+++ b/src/Makefile.am
@@ -1243,6 +1243,7 @@ libvirt_lxc_CFLAGS = \
$(CAPNG_CFLAGS) \
$(YAJL_CFLAGS) \
$(AUDIT_CFLAGS) \
+ $(LIBNL_CFLAGS) \
-I@top_srcdir@/src/conf \
$(AM_CFLAGS)
endif
diff --git a/src/util/interface.c b/src/util/interface.c
index 02544d9..03f9367 100644
--- a/src/util/interface.c
+++ b/src/util/interface.c
@@ -494,7 +494,7 @@ ifaceSetMacaddr(const char *ifname ATTRIBUTE_UNUSED,
/**
- * ifaceLinkAdd
+ * ifaceMacvtapLinkAdd
*
* @type: The type of device, i.e., "macvtap"
* @macaddress: The MAC address of the device
@@ -510,7 +510,7 @@ ifaceSetMacaddr(const char *ifname ATTRIBUTE_UNUSED,
*
* Returns 0 on success, -1 on fatal error.
*/
-#if __linux__
+#if WITH_MACVTAP
int
ifaceMacvtapLinkAdd(const char *type,
const unsigned char *macaddress, int macaddrsize,
@@ -758,7 +758,7 @@ ifaceLinkDel(const char *ifname ATTRIBUTE_UNUSED)
#endif
-#if __linux__
+#if WITH_MACVTAP
static struct nla_policy ifla_policy[IFLA_MAX + 1] =
{
[IFLA_VF_PORTS] = { .type = NLA_NESTED },
--
1.7.0.4
5:59 p.m.
On 06/22/2011 04:42 PM, Matthias Bolte wrote:
The refactoring in 6a5978833a5 and df3d8c362d3 was incomplete
as it accidentally moved macvtap related code out of a
#if WITH_MACVTAP in a #if __linux__ block. To fix this move
ifaceMacvtapLinkAdd and ifaceMacvtapLinkDump back under
#if WITH_MACVTAP.
Also nlComm was moved from #if WITH_MACVTAP to #if __linux__
but configure.ac was not updated to match this, as libnl is
now required on Linux always because of this.
Also related to libnl, libvirt_lxc missed LIBNL_CFLAGS in it's
CFLAGS.
---
configure.ac | 6 +++---
src/Makefile.am | 1 +
src/util/interface.c | 6 +++---
3 files changed, 7 insertions(+), 6 deletions(-)
diff --git a/configure.ac b/configure.ac
index f816696..4c81e3b 100644
--- a/configure.ac
+++ b/configure.ac
@@ -2372,10 +2372,10 @@ dnl netlink library
LIBNL_CFLAGS=""
LIBNL_LIBS=""
-if test "$with_macvtap" = "yes"; then
+if test "$with_linux" = "yes"; then
Do we want this to be:
if test "$with_macvtap" = "yes" || test "$with_linux" =
"yes"; then
PKG_CHECK_MODULES([LIBNL], [libnl-1 >= $LIBNL_REQUIRED], [
], [
- AC_MSG_ERROR([libnl-devel >= $LIBNL_REQUIRED is required for macvtap
support])
+ AC_MSG_ERROR([libnl-devel >= $LIBNL_REQUIRED is required])
RHEL 5 only has libnl-devel 1.0. Is libnl 1.1 a hard-and-fast
requirement, or will things still work if we relax the required libnl
version to accommodate RHEL 5?
])
fi
@@ -2574,7 +2574,7 @@ AC_MSG_NOTICE([ pcap: $LIBPCAP_CFLAGS $LIBPCAP_LIBS])
else
AC_MSG_NOTICE([ pcap: no])
fi
-if test "$with_macvtap" = "yes" ; then
+if test "$with_linux" = "yes" ; then
Same question on the conditional.
+++ b/src/util/interface.c
@@ -494,7 +494,7 @@ ifaceSetMacaddr(const char *ifname ATTRIBUTE_UNUSED,
/**
- * ifaceLinkAdd
+ * ifaceMacvtapLinkAdd
*
* @type: The type of device, i.e., "macvtap"
* @macaddress: The MAC address of the device
@@ -510,7 +510,7 @@ ifaceSetMacaddr(const char *ifname ATTRIBUTE_UNUSED,
*
* Returns 0 on success, -1 on fatal error.
*/
-#if __linux__
+#if WITH_MACVTAP
int
ifaceMacvtapLinkAdd(const char *type,
const unsigned char *macaddress, int macaddrsize,
@@ -758,7 +758,7 @@ ifaceLinkDel(const char *ifname ATTRIBUTE_UNUSED)
#endif
-#if __linux__
+#if WITH_MACVTAP
These hunks look okay, but I doubt they work in isolation without fixing
the other questions I raised.
--
Eric Blake eblake(a)redhat.com +1-801-349-2682
Libvirt virtualization library http://libvirt.org
7:45 p.m.
On 06/22/2011 06:42 PM, Matthias Bolte wrote:
The refactoring in 6a5978833a5 and df3d8c362d3 was incomplete
as it accidentally moved macvtap related code out of a
#if WITH_MACVTAP in a #if __linux__ block. To fix this move
ifaceMacvtapLinkAdd and ifaceMacvtapLinkDump back under
#if WITH_MACVTAP.
Also nlComm was moved from #if WITH_MACVTAP to #if __linux__
but configure.ac was not updated to match this, as libnl is
now required on Linux always because of this.
So with your changes it's still required for all Linux builds? That's
potentially not good at all. RHEL5 uses libnl 1.0, which is not ABI
compatible with libnl 1.1 (which everything in libvirt is written to).
(Up to now we've been saved the headache because macvtap isn't
enabled/supported on RHEL5.)
The upstream libnl maintainer (I'm Cc'ing him) has told me it wouldn't
be difficult to make libvirt's libnl usage compatible with all versions
of libnl, but he didn't elaborate. If we're lucky, the libnl usage in
that bit of code will require no change, but we definitely shouldn't let
ourselves get to a release that requires libnl for all Linux builds
without resolving this.
(Thomas - the code in question is in src/util/netlink.[ch] of
git://libvirt.org/libvirt.git)
2:17 a.m.
2011/6/23 Laine Stump <laine(a)laine.org>:
On 06/22/2011 06:42 PM, Matthias Bolte wrote:
>
> The refactoring in 6a5978833a5 and df3d8c362d3 was incomplete
> as it accidentally moved macvtap related code out of a
> #if WITH_MACVTAP in a #if __linux__ block. To fix this move
> ifaceMacvtapLinkAdd and ifaceMacvtapLinkDump back under
> #if WITH_MACVTAP.
>
> Also nlComm was moved from #if WITH_MACVTAP to #if __linux__
> but configure.ac was not updated to match this, as libnl is
> now required on Linux always because of this.
So with your changes it's still required for all Linux builds? That's
potentially not good at all. RHEL5 uses libnl 1.0, which is not ABI
compatible with libnl 1.1 (which everything in libvirt is written to). (Up
to now we've been saved the headache because macvtap isn't enabled/supported
on RHEL5.)
Well, the whole problem was introduced by Stefans cleanup commits. Is
there some higher level reason for this refactoring? There probably
is, but If not then the simplest fix is to revert the offending
commits.
--
Matthias Bolte
http://photron.blogspot.com
5:12 a.m.
Matthias Bolte <matthias.bolte(a)googlemail.com> wrote on 06/23/2011
03:17:32 AM:
2011/6/23 Laine Stump <laine(a)laine.org>:
> On 06/22/2011 06:42 PM, Matthias Bolte wrote:
>>
>> The refactoring in 6a5978833a5 and df3d8c362d3 was incomplete
>> as it accidentally moved macvtap related code out of a
>> #if WITH_MACVTAP in a #if __linux__ block. To fix this move
>> ifaceMacvtapLinkAdd and ifaceMacvtapLinkDump back under
>> #if WITH_MACVTAP.
>>
>> Also nlComm was moved from #if WITH_MACVTAP to #if __linux__
>> but configure.ac was not updated to match this, as libnl is
>> now required on Linux always because of this.
>
> So with your changes it's still required for all Linux builds? That's
> potentially not good at all. RHEL5 uses libnl 1.0, which is not ABI
> compatible with libnl 1.1 (which everything in libvirt is written to).
(Up
> to now we've been saved the headache because macvtap
isn't
enabled/supported
> on RHEL5.)
Well, the whole problem was introduced by Stefans cleanup commits. Is
there some higher level reason for this refactoring? There probably
is, but If not then the simplest fix is to revert the offending
commits.
I apologize for this. I am fixing this now.
Stefan
5:29 a.m.
On Wed, Jun 22, 2011 at 08:45:23PM -0400, Laine Stump wrote:
The upstream libnl maintainer (I'm Cc'ing him) has told me
it
wouldn't be difficult to make libvirt's libnl usage compatible with
all versions of libnl, but he didn't elaborate. If we're lucky, the
libnl usage in that bit of code will require no change, but we
definitely shouldn't let ourselves get to a release that requires
libnl for all Linux builds without resolving this.
(Thomas - the code in question is in src/util/netlink.[ch] of
git://libvirt.org/libvirt.git)
I just looked at the code briefly. I don't think any of the API
that you use has changed between 2.x and 3.x so no changes should
be needed.
However:
*respbuflen = nl_recv(nlhandle, &nladdr, respbuf, NULL);
if (*respbuflen <= 0) {
virReportSystemError(errno,
"%s", _("nl_recv failed"));
rc = -1;
}
You could simplify this a lot by using nl_wait_for_ack() in
all cases except where you fetch the link which would have
to use nl_recvmsgs() and a have a callback do the parsing.
Still, the code as-is looks solid, all you could do is ditch
a few dozen lines of code.
4901
days inactive
4902
days old
5 comments
5 participants
participants (5)
-
Eric Blake -
Laine Stump -
Matthias Bolte -
Stefan Berger -
Thomas Graf