Patchwork [1/4] Revert: add new libnetfilter_queue API for libmnl

login
register
mail settings
Submitter Florian Westphal
Date April 25, 2013, 10:43 a.m.
Message ID <1366886611-21666-2-git-send-email-fw@strlen.de>
Download mbox | patch
Permalink /patch/239482/
State Superseded
Headers show

Comments

Florian Westphal - April 25, 2013, 10:43 a.m.
This is a partial revert of a0c885ae5a79457aa592cb70c27a7dee619762a4

Specifically, it removes the header linux/netfilter/nfnetlink_queue.h
added in that commit.

1), there is already a /usr/include/linux/netfilter/nfnetlink_queue.h,
    which is part of the linux kernel API

2), we already have
    include/libnetfilter_queue/linux_nfnetlink_queue.h

which contains the same definitions/structures/macros, so it makes
little sense to have two headers in libnetfilter_queue that share
almost their entire content.

[ worse, the nfnetlink_queue.h header reverted here actually is
incompatible with mainline kernels, since a few defines have the
wrong value ... ]

Signed-off-by: Florian Westphal <fw@strlen.de>
---
 Makefile.am                               |    2 +-
 configure.ac                              |    3 +-
 include/Makefile.am                       |    2 +-
 include/linux/Makefile.am                 |    1 -
 include/linux/netfilter/Makefile.am       |    1 -
 include/linux/netfilter/nfnetlink_queue.h |   98 -----------------------------
 src/nlmsg.c                               |    2 -
 7 files changed, 3 insertions(+), 106 deletions(-)
 delete mode 100644 include/linux/Makefile.am
 delete mode 100644 include/linux/netfilter/Makefile.am
 delete mode 100644 include/linux/netfilter/nfnetlink_queue.h
Pablo Neira - April 26, 2013, 1:36 a.m.
Hi Florian,

On Thu, Apr 25, 2013 at 12:43:28PM +0200, Florian Westphal wrote:
> This is a partial revert of a0c885ae5a79457aa592cb70c27a7dee619762a4
> 
> Specifically, it removes the header linux/netfilter/nfnetlink_queue.h
> added in that commit.

The idea for caching that file header is to make sure
libnetfilter_queue compiles out of the box, without external linux
kernel headers installed in the system. The idea is to make sure the
libraries compilation does not break in case there's an old library
header installed in the system.

> 1), there is already a /usr/include/linux/netfilter/nfnetlink_queue.h,
>     which is part of the linux kernel API

As said, that file may not be available or may be stale.

> 2), we already have
>     include/libnetfilter_queue/linux_nfnetlink_queue.h

That was added initially for the old libnetfilter_queue API, I'd like
to get rid of it. It's currently being installed and it results in
(very likely) two duplicated headers in the system (the one from the
linux kernel headers and this one from libnetfilter_queue).

Once the old API is deprecated (we should do that anytime soon), we
can get finally rid of linux_nfnetlink_queue.h

> which contains the same definitions/structures/macros, so it makes
> little sense to have two headers in libnetfilter_queue that share
> almost their entire content.
> 
> [ worse, the nfnetlink_queue.h header reverted here actually is
> incompatible with mainline kernels, since a few defines have the
> wrong value ... ]

Then, please refresh it.
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Florian Westphal - April 26, 2013, 7:32 a.m.
Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> On Thu, Apr 25, 2013 at 12:43:28PM +0200, Florian Westphal wrote:
> > This is a partial revert of a0c885ae5a79457aa592cb70c27a7dee619762a4
> > 
> > Specifically, it removes the header linux/netfilter/nfnetlink_queue.h
> > added in that commit.
> 
> The idea for caching that file header is to make sure
> libnetfilter_queue compiles out of the box, without external linux
> kernel headers installed in the system. The idea is to make sure the
> libraries compilation does not break in case there's an old library
> header installed in the system.
> 
> > 1), there is already a /usr/include/linux/netfilter/nfnetlink_queue.h,
> >     which is part of the linux kernel API
> 
> As said, that file may not be available or may be stale.

Yes, but at this time the system-wide header is used, and not this copy.
Compile of libnetfilter_queue breaks with the other patches applied
because the new attribute is missing from the system header.

> > 2), we already have
> >     include/libnetfilter_queue/linux_nfnetlink_queue.h
> 
> That was added initially for the old libnetfilter_queue API, I'd like
> to get rid of it. It's currently being installed and it results in
> (very likely) two duplicated headers in the system (the one from the
> linux kernel headers and this one from libnetfilter_queue).

Hrm, yes.  I thought that was intentional.
Or are you saying that you basically just want to rename
linux_nfnetlink_queue.h and carry a copy of nfnetlink_queue.h instead?

> Once the old API is deprecated (we should do that anytime soon), we
> can get finally rid of linux_nfnetlink_queue.h

I guess what confused me is that there is nothing related to the old
api in that header, so we might as well keep it?

> > which contains the same definitions/structures/macros, so it makes
> > little sense to have two headers in libnetfilter_queue that share
> > almost their entire content.
> > 
> > [ worse, the nfnetlink_queue.h header reverted here actually is
> > incompatible with mainline kernels, since a few defines have the
> > wrong value ... ]
> 
> Then, please refresh it.

Alright, but its not enough; the build system would require
additional treatment to give precedence to that header when compiling
the library.
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Pablo Neira - April 26, 2013, 9:37 a.m.
On Fri, Apr 26, 2013 at 09:32:39AM +0200, Florian Westphal wrote:
> Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> > On Thu, Apr 25, 2013 at 12:43:28PM +0200, Florian Westphal wrote:
> > > This is a partial revert of a0c885ae5a79457aa592cb70c27a7dee619762a4
> > > 
> > > Specifically, it removes the header linux/netfilter/nfnetlink_queue.h
> > > added in that commit.
> > 
> > The idea for caching that file header is to make sure
> > libnetfilter_queue compiles out of the box, without external linux
> > kernel headers installed in the system. The idea is to make sure the
> > libraries compilation does not break in case there's an old library
> > header installed in the system.
> > 
> > > 1), there is already a /usr/include/linux/netfilter/nfnetlink_queue.h,
> > >     which is part of the linux kernel API
> > 
> > As said, that file may not be available or may be stale.
> 
> Yes, but at this time the system-wide header is used, and not this copy.
> Compile of libnetfilter_queue breaks with the other patches applied
> because the new attribute is missing from the system header.

Hm, it's using the local cache copy here.

I tested it by adding some new (random) attribute definition
(NFQA_XYZ) to the locally cached header file, and using it from some
of the source files (e.g. nlmsg.c).

> > > 2), we already have
> > >     include/libnetfilter_queue/linux_nfnetlink_queue.h
> > 
> > That was added initially for the old libnetfilter_queue API, I'd like
> > to get rid of it. It's currently being installed and it results in
> > (very likely) two duplicated headers in the system (the one from the
> > linux kernel headers and this one from libnetfilter_queue).
> 
> Hrm, yes.  I thought that was intentional.
> Or are you saying that you basically just want to rename
> linux_nfnetlink_queue.h and carry a copy of nfnetlink_queue.h instead?

We cannot rename that header. Some people are including it into it
into their source code and that will break their compilation.

We can add some #warning in that file to say that people should use
cache some fresh kernel header in their source code tree, or simply
tell them to use the system kernel headers.

Regards.
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Florian Westphal - April 26, 2013, 10:02 a.m.
Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> > Yes, but at this time the system-wide header is used, and not this copy.
> > Compile of libnetfilter_queue breaks with the other patches applied
> > because the new attribute is missing from the system header.
> 
> Hm, it's using the local cache copy here.

You're right.

> > Hrm, yes.  I thought that was intentional.
> > Or are you saying that you basically just want to rename
> > linux_nfnetlink_queue.h and carry a copy of nfnetlink_queue.h instead?
> 
> We cannot rename that header. Some people are including it into it
> into their source code and that will break their compilation.

Yes, I meant "in the long run, when old API is removed".
I got rid of the revert and it works for me after refreshing
with a copy from net-next.

I hope the V2 i posted is in the direction you had in mind.
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Pablo Neira - April 26, 2013, 10:12 a.m.
On Fri, Apr 26, 2013 at 12:02:47PM +0200, Florian Westphal wrote:
[...]
> > We cannot rename that header. Some people are including it into it
> > into their source code and that will break their compilation.
> 
> Yes, I meant "in the long run, when old API is removed".
> I got rid of the revert and it works for me after refreshing
> with a copy from net-next.

Feel free to post a patch for that refresh.

> I hope the V2 i posted is in the direction you had in mind.

Looks good, only missing some explanation in the doxygen documentation
on the "csum not ready" thing.

Thanks.
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

diff --git a/Makefile.am b/Makefile.am
index 6b4ef77..1230dc1 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -1,6 +1,6 @@ 
 ACLOCAL_AMFLAGS = -I m4
 
-EXTRA_DIST = $(man_MANS) include/linux
+EXTRA_DIST = $(man_MANS)
 
 SUBDIRS = src utils include examples
 
diff --git a/configure.ac b/configure.ac
index 07747a6..649060d 100644
--- a/configure.ac
+++ b/configure.ac
@@ -32,6 +32,5 @@  PKG_CHECK_MODULES([LIBMNL], [libmnl >= 1.0.3])
 dnl Output the makefiles
 AC_CONFIG_FILES([Makefile src/Makefile utils/Makefile examples/Makefile
         libnetfilter_queue.pc doxygen.cfg
-	include/Makefile include/libnetfilter_queue/Makefile
-	include/linux/Makefile include/linux/netfilter/Makefile])
+	include/Makefile include/libnetfilter_queue/Makefile])
 AC_OUTPUT
diff --git a/include/Makefile.am b/include/Makefile.am
index 54ea0b4..1e766d5 100644
--- a/include/Makefile.am
+++ b/include/Makefile.am
@@ -1 +1 @@ 
-SUBDIRS= libnetfilter_queue linux
+SUBDIRS= libnetfilter_queue
diff --git a/include/linux/Makefile.am b/include/linux/Makefile.am
deleted file mode 100644
index 38eb109..0000000
--- a/include/linux/Makefile.am
+++ /dev/null
@@ -1 +0,0 @@ 
-SUBDIRS = netfilter
diff --git a/include/linux/netfilter/Makefile.am b/include/linux/netfilter/Makefile.am
deleted file mode 100644
index d0937cb..0000000
--- a/include/linux/netfilter/Makefile.am
+++ /dev/null
@@ -1 +0,0 @@ 
-noinst_HEADERS = nfnetlink_queue.h
diff --git a/include/linux/netfilter/nfnetlink_queue.h b/include/linux/netfilter/nfnetlink_queue.h
deleted file mode 100644
index da44b33..0000000
--- a/include/linux/netfilter/nfnetlink_queue.h
+++ /dev/null
@@ -1,98 +0,0 @@ 
-#ifndef _NFNETLINK_QUEUE_H
-#define _NFNETLINK_QUEUE_H
-
-#include <linux/types.h>
-#include <linux/netfilter/nfnetlink.h>
-
-enum nfqnl_msg_types {
-	NFQNL_MSG_PACKET,		/* packet from kernel to userspace */
-	NFQNL_MSG_VERDICT,		/* verdict from userspace to kernel */
-	NFQNL_MSG_CONFIG,		/* connect to a particular queue */
-	NFQNL_MSG_VERDICT_BATCH,	/* batchv from userspace to kernel */
-
-	NFQNL_MSG_MAX
-};
-
-struct nfqnl_msg_packet_hdr {
-	__be32		packet_id;	/* unique ID of packet in queue */
-	__be16		hw_protocol;	/* hw protocol (network order) */
-	__u8	hook;		/* netfilter hook */
-} __attribute__ ((packed));
-
-struct nfqnl_msg_packet_hw {
-	__be16		hw_addrlen;
-	__u16	_pad;
-	__u8	hw_addr[8];
-};
-
-struct nfqnl_msg_packet_timestamp {
-	__aligned_be64	sec;
-	__aligned_be64	usec;
-};
-
-enum nfqnl_attr_type {
-	NFQA_UNSPEC,
-	NFQA_PACKET_HDR,
-	NFQA_VERDICT_HDR,		/* nfqnl_msg_verdict_hrd */
-	NFQA_MARK,			/* __u32 nfmark */
-	NFQA_TIMESTAMP,			/* nfqnl_msg_packet_timestamp */
-	NFQA_IFINDEX_INDEV,		/* __u32 ifindex */
-	NFQA_IFINDEX_OUTDEV,		/* __u32 ifindex */
-	NFQA_IFINDEX_PHYSINDEV,		/* __u32 ifindex */
-	NFQA_IFINDEX_PHYSOUTDEV,	/* __u32 ifindex */
-	NFQA_HWADDR,			/* nfqnl_msg_packet_hw */
-	NFQA_PAYLOAD,			/* opaque data payload */
-	NFQA_CT,			/* nf_conntrack_netlink.h */
-	NFQA_CT_INFO,			/* enum ip_conntrack_info */
-
-	__NFQA_MAX
-};
-#define NFQA_MAX (__NFQA_MAX - 1)
-
-struct nfqnl_msg_verdict_hdr {
-	__be32 verdict;
-	__be32 id;
-};
-
-
-enum nfqnl_msg_config_cmds {
-	NFQNL_CFG_CMD_NONE,
-	NFQNL_CFG_CMD_BIND,
-	NFQNL_CFG_CMD_UNBIND,
-	NFQNL_CFG_CMD_PF_BIND,
-	NFQNL_CFG_CMD_PF_UNBIND,
-};
-
-struct nfqnl_msg_config_cmd {
-	__u8	command;	/* nfqnl_msg_config_cmds */
-	__u8	_pad;
-	__be16		pf;		/* AF_xxx for PF_[UN]BIND */
-};
-
-enum nfqnl_config_mode {
-	NFQNL_COPY_NONE,
-	NFQNL_COPY_META,
-	NFQNL_COPY_PACKET,
-};
-
-struct nfqnl_msg_config_params {
-	__be32		copy_range;
-	__u8	copy_mode;	/* enum nfqnl_config_mode */
-} __attribute__ ((packed));
-
-enum nfqnl_flags {
-	NFQNL_F_NONE		= 0,
-	NFQNL_F_CONNTRACK	= (1 << 0),
-};
-
-enum nfqnl_attr_config {
-	NFQA_CFG_UNSPEC,
-	NFQA_CFG_CMD,			/* nfqnl_msg_config_cmd */
-	NFQA_CFG_PARAMS,		/* nfqnl_msg_config_params */
-	NFQA_CFG_QUEUE_MAXLEN,		/* __u32 */
-	NFQA_CFG_FLAGS,			/* __u32 */
-	__NFQA_CFG_MAX
-};
-#define NFQA_CFG_MAX (__NFQA_CFG_MAX-1)
-
-#endif /* _NFNETLINK_QUEUE_H */
diff --git a/src/nlmsg.c b/src/nlmsg.c
index 6c4a139..e592ebd 100644
--- a/src/nlmsg.c
+++ b/src/nlmsg.c
@@ -19,8 +19,6 @@ 
 #define __aligned_le64 __le64 __attribute__((aligned(8)))
 #endif
 
-#include <linux/netfilter/nfnetlink_queue.h>
-
 #include <libnetfilter_queue/libnetfilter_queue.h>
 
 #include "internal.h"