| Message ID | 20170816204310.3371-2-eric@regit.org |
|---|---|
| State | Changes Requested |
| Delegated to: | Pablo Neira |
| Headers | show |
On Wed, Aug 16, 2017 at 10:42:55PM +0200, Eric Leblond wrote: [...] > diff --git a/src/libnftables.c b/src/libnftables.c > new file mode 100644 > index 0000000..215179a > --- /dev/null > +++ b/src/libnftables.c > @@ -0,0 +1,53 @@ > +/* > + * Copyright (c) 2017 Eric Leblond <eric@regit.org> > + * > + * This program is free software; you can redistribute it and/or modify > + * it under the terms of the GNU General Public License version 2 as > + * published by the Free Software Foundation. > + * > + */ > + > +#include <nftables/nftables.h> > +#include <string.h> > +#include <errno.h> > +#include <nftables.h> > +#include <parser.h> > +#include <iface.h> > +#include <netlink.h> > +#include <erec.h> > +#include <libmnl/libmnl.h> > +#include <mnl.h> > + > +#include <unistd.h> > +#include <fcntl.h> > + > + > +unsigned int max_errors = 10; > +unsigned int numeric_output; > +unsigned int ip2name_output; > +unsigned int handle_output; > +#ifdef DEBUG > +unsigned int debug_level; > +#endif I guess these global variables have to die. I tried static linking (for quick testing without installing) which didn't work because max_errors is then redefined here. (There is a stale one in main.c which you probably just forgot to remove.) Maybe max_errors could become part of struct nft_ctx? Alternatively I would make it static and add a getter to be used from parser_bison.y. The remaining ones apart from debug_level are leftovers, they live in struct output_ctx now. I am not sure where debug_level really belongs to. One one hand, it has something to do with printing, so struct output_ctx. On the other, nft_ctx would have to be made reachable from all places where debug output happens. Maybe the whole debugging infrastructure (i.e. debug data dumpers and printers) should be exported to the application? > + > +void nft_global_init(void) > +{ > + mark_table_init(); > + realm_table_rt_init(); > + devgroup_table_init(); > + realm_table_meta_init(); > + ct_label_table_init(); > + gmp_init(); > +#ifdef HAVE_LIBXTABLES > + xt_init(); > +#endif > +} > + > +void nft_global_deinit(void) > +{ > + ct_label_table_exit(); > + realm_table_rt_exit(); > + devgroup_table_exit(); > + realm_table_meta_exit(); > + mark_table_exit(); > +} How about calling these from nft_context_new() and nft_context_free()? Cheers, Phil -- 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
Hi, On Thu, 2017-08-17 at 10:57 +0200, Phil Sutter wrote: > On Wed, Aug 16, 2017 at 10:42:55PM +0200, Eric Leblond wrote: > [...] > > diff --git a/src/libnftables.c b/src/libnftables.c > > new file mode 100644 > > index 0000000..215179a > > --- /dev/null > > +++ b/src/libnftables.c > > @@ -0,0 +1,53 @@ > > +/* > > + * Copyright (c) 2017 Eric Leblond <eric@regit.org> > > + * > > + * This program is free software; you can redistribute it and/or > > modify > > + * it under the terms of the GNU General Public License version 2 > > as > > + * published by the Free Software Foundation. > > + * > > + */ > > + > > +#include <nftables/nftables.h> > > +#include <string.h> > > +#include <errno.h> > > +#include <nftables.h> > > +#include <parser.h> > > +#include <iface.h> > > +#include <netlink.h> > > +#include <erec.h> > > +#include <libmnl/libmnl.h> > > +#include <mnl.h> > > + > > +#include <unistd.h> > > +#include <fcntl.h> > > + > > + > > +unsigned int max_errors = 10; > > +unsigned int numeric_output; > > +unsigned int ip2name_output; > > +unsigned int handle_output; > > +#ifdef DEBUG > > +unsigned int debug_level; > > +#endif > > I guess these global variables have to die. I tried static linking > (for > quick testing without installing) which didn't work because > max_errors > is then redefined here. (There is a stale one in main.c which you > probably just forgot to remove.) > > Maybe max_errors could become part of struct nft_ctx? Alternatively I > would make it static and add a getter to be used from parser_bison.y. I'm gonna remove the all but max_errors and see how I could addmax_errors somewhere. > The remaining ones apart from debug_level are leftovers, they live in > struct output_ctx now. Indeed. Good catch. > I am not sure where debug_level really belongs to. One one hand, it > has > something to do with printing, so struct output_ctx. On the other, > nft_ctx would have to be made reachable from all places where debug > output happens. > > Maybe the whole debugging infrastructure (i.e. debug data dumpers and > printers) should be exported to the application? > > > + > > +void nft_global_init(void) > > +{ > > + mark_table_init(); > > + realm_table_rt_init(); > > + devgroup_table_init(); > > + realm_table_meta_init(); > > + ct_label_table_init(); > > + gmp_init(); > > +#ifdef HAVE_LIBXTABLES > > + xt_init(); > > +#endif > > +} > > + > > +void nft_global_deinit(void) > > +{ > > + ct_label_table_exit(); > > + realm_table_rt_exit(); > > + devgroup_table_exit(); > > + realm_table_meta_exit(); > > + mark_table_exit(); > > +} > > How about calling these from nft_context_new() and > nft_context_free()? I want to be able to have multiple context for a single process. Hence I defined a global init and deinit. But maybe it does not really make sense and could be attached to each context or init could be done at first usage. ++
Hey, On Thu, Aug 17, 2017 at 07:09:02PM +0200, Eric Leblond wrote: > On Thu, 2017-08-17 at 10:57 +0200, Phil Sutter wrote: > > On Wed, Aug 16, 2017 at 10:42:55PM +0200, Eric Leblond wrote: [...] > > > +void nft_global_init(void) > > > +{ > > > + mark_table_init(); > > > + realm_table_rt_init(); > > > + devgroup_table_init(); > > > + realm_table_meta_init(); > > > + ct_label_table_init(); > > > + gmp_init(); > > > +#ifdef HAVE_LIBXTABLES > > > + xt_init(); > > > +#endif > > > +} > > > + > > > +void nft_global_deinit(void) > > > +{ > > > + ct_label_table_exit(); > > > + realm_table_rt_exit(); > > > + devgroup_table_exit(); > > > + realm_table_meta_exit(); > > > + mark_table_exit(); > > > +} > > > > How about calling these from nft_context_new() and > > nft_context_free()? > > I want to be able to have multiple context for a single process. Hence > I defined a global init and deinit. But maybe it does not really make > sense and could be attached to each context or init could be done at > first usage. My idea was to implement simple reference counting to see whether the library was already initialized and whether it is safe to deinit. Of course this needs some serialization for thread-safety. Or maybe the deinit can be ignored completely and nft_global_init() just has to check whether data is already initialized or not. Cheers, Phil -- 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
Hi, On Thu, 2017-08-17 at 19:13 +0200, Phil Sutter wrote: > Hey, > > On Thu, Aug 17, 2017 at 07:09:02PM +0200, Eric Leblond wrote: > > On Thu, 2017-08-17 at 10:57 +0200, Phil Sutter wrote: > > > On Wed, Aug 16, 2017 at 10:42:55PM +0200, Eric Leblond wrote: > > [...] > > > > +void nft_global_init(void) > > > > +{ > > > > + mark_table_init(); > > > > + realm_table_rt_init(); > > > > + devgroup_table_init(); > > > > + realm_table_meta_init(); > > > > + ct_label_table_init(); > > > > + gmp_init(); > > > > +#ifdef HAVE_LIBXTABLES > > > > + xt_init(); > > > > +#endif > > > > +} > > > > + > > > > +void nft_global_deinit(void) > > > > +{ > > > > + ct_label_table_exit(); > > > > + realm_table_rt_exit(); > > > > + devgroup_table_exit(); > > > > + realm_table_meta_exit(); > > > > + mark_table_exit(); > > > > +} > > > > > > How about calling these from nft_context_new() and > > > nft_context_free()? > > > > I want to be able to have multiple context for a single process. > > Hence > > I defined a global init and deinit. But maybe it does not really > > make > > sense and could be attached to each context or init could be done > > at > > first usage. > > My idea was to implement simple reference counting to see whether the > library was already initialized and whether it is safe to deinit. Of > course this needs some serialization for thread-safety. > > Or maybe the deinit can be ignored completely and nft_global_init() > just > has to check whether data is already initialized or not. I have used numerous libraries providing a global init or deinit. For now we could easily skip it with your approach or another but I prefer we have it existing so it is available later in case of needs. ++
On Saturday 2017-08-19 10:43, Eric Leblond wrote: >>> Hence I defined a global init and deinit. But maybe it does not >>> really make sense and could be attached to each context or init >>> could be done at first usage. >> >> My idea was to implement simple reference counting to see whether >> the library was already initialized and whether it is safe to >> deinit. Of course this needs some serialization for thread-safety. >> >> Or maybe the deinit can be ignored completely and >> nft_global_init() just has to check whether data is already >> initialized or not. > >I have used numerous libraries providing a global init or deinit. >For now we could easily skip it with your approach or another but I >prefer we have it existing so it is available later in case of >needs. If a global init - more correctly, a singleton init - is needed, can't it just implicitly be called from make_me_a_new_context()? -- 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
On Sat, Aug 19, 2017 at 09:07:55PM +0200, Jan Engelhardt wrote: > > On Saturday 2017-08-19 10:43, Eric Leblond wrote: > > >>> Hence I defined a global init and deinit. But maybe it does not > >>> really make sense and could be attached to each context or init > >>> could be done at first usage. > >> > >> My idea was to implement simple reference counting to see whether > >> the library was already initialized and whether it is safe to > >> deinit. Of course this needs some serialization for thread-safety. > >> > >> Or maybe the deinit can be ignored completely and > >> nft_global_init() just has to check whether data is already > >> initialized or not. > > > >I have used numerous libraries providing a global init or deinit. > >For now we could easily skip it with your approach or another but I > >prefer we have it existing so it is available later in case of > >needs. > > If a global init - more correctly, a singleton init - is needed, > can't it just implicitly be called from make_me_a_new_context()? I would advocate for placing the nft_global_init() and deinit() code into the ctx object too. Those _init() and _deinit() are indeed initializing context information, such as iproute/ct label maps. So they are context after all. -- 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
diff --git a/configure.ac b/configure.ac index bef6c0b..91273ce 100644 --- a/configure.ac +++ b/configure.ac @@ -56,6 +56,9 @@ then exit 1 fi +AM_PROG_AR +AM_PROG_LIBTOOL + AC_CHECK_PROG(DOCBOOK2X_MAN, [docbook2x-man], [docbook2x-man], [no]) AC_CHECK_PROG(DOCBOOK2MAN, [docbook2man], [docbook2man], [no]) AC_CHECK_PROG(DB2X_DOCBOOK2MAN, [db2x_docbook2man], [db2x_docbook2man], [no]) @@ -146,6 +149,7 @@ AC_CONFIG_FILES([ \ include/linux/netfilter_bridge/Makefile \ include/linux/netfilter_ipv4/Makefile \ include/linux/netfilter_ipv6/Makefile \ + include/nftables/Makefile \ doc/Makefile \ files/Makefile \ files/nftables/Makefile \ diff --git a/include/Makefile.am b/include/Makefile.am index 5dd73d8..caa6961 100644 --- a/include/Makefile.am +++ b/include/Makefile.am @@ -1,4 +1,4 @@ -SUBDIRS = linux +SUBDIRS = linux nftables noinst_HEADERS = cli.h \ datatype.h \ diff --git a/include/nftables/Makefile.am b/include/nftables/Makefile.am new file mode 100644 index 0000000..a14b188 --- /dev/null +++ b/include/nftables/Makefile.am @@ -0,0 +1,2 @@ +pkginclude_HEADERS = nftables.h + diff --git a/include/nftables/nftables.h b/include/nftables/nftables.h new file mode 100644 index 0000000..4ba16f0 --- /dev/null +++ b/include/nftables/nftables.h @@ -0,0 +1,20 @@ +/* + * Copyright (c) 2017 Eric Leblond <eric@regit.org> + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License version 2 as + * published by the Free Software Foundation. + * + */ + +#ifndef LIB_NFTABLES_H +#define LIB_NFTABLES_H + +#include <stdlib.h> +#include <stdint.h> +#include <stdbool.h> + +void nft_global_init(void); +void nft_global_deinit(void); + +#endif diff --git a/src/Makefile.am b/src/Makefile.am index 99eef7b..a340d39 100644 --- a/src/Makefile.am +++ b/src/Makefile.am @@ -27,8 +27,9 @@ parser_bison.o scanner.o: AM_CFLAGS += -Wno-missing-prototypes -Wno-missing-decl BUILT_SOURCES = parser_bison.h -nft_SOURCES = main.c \ - rule.c \ +lib_LTLIBRARIES = libnftables.la + +libnftables_la_SOURCES = rule.c \ statement.c \ datatype.c \ expression.c \ @@ -54,23 +55,28 @@ nft_SOURCES = main.c \ iface.c \ services.c \ mergesort.c \ + libnftables.c \ scanner.l \ tcpopt.c \ parser_bison.y -if BUILD_CLI -nft_SOURCES += cli.c -endif - if BUILD_MINIGMP mini-gmp.o: AM_CFLAGS += -Wno-sign-compare -nft_SOURCES += mini-gmp.c +libnftables_la_SOURCES += mini-gmp.c endif -nft_LDADD = ${LIBMNL_LIBS} ${LIBNFTNL_LIBS} +nft_SOURCES = main.c + +if BUILD_CLI +nft_SOURCES += cli.c +endif + +libnftables_la_LIBADD = ${LIBMNL_LIBS} ${LIBNFTNL_LIBS} if BUILD_XTABLES -nft_SOURCES += xt.c -nft_LDADD += ${XTABLES_LIBS} +libnftables_la_SOURCES += xt.c +libnftables_la_LIBADD += ${XTABLES_LIBS} endif + +nft_LDADD = libnftables.la diff --git a/src/libnftables.c b/src/libnftables.c new file mode 100644 index 0000000..215179a --- /dev/null +++ b/src/libnftables.c @@ -0,0 +1,53 @@ +/* + * Copyright (c) 2017 Eric Leblond <eric@regit.org> + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License version 2 as + * published by the Free Software Foundation. + * + */ + +#include <nftables/nftables.h> +#include <string.h> +#include <errno.h> +#include <nftables.h> +#include <parser.h> +#include <iface.h> +#include <netlink.h> +#include <erec.h> +#include <libmnl/libmnl.h> +#include <mnl.h> + +#include <unistd.h> +#include <fcntl.h> + + +unsigned int max_errors = 10; +unsigned int numeric_output; +unsigned int ip2name_output; +unsigned int handle_output; +#ifdef DEBUG +unsigned int debug_level; +#endif + +void nft_global_init(void) +{ + mark_table_init(); + realm_table_rt_init(); + devgroup_table_init(); + realm_table_meta_init(); + ct_label_table_init(); + gmp_init(); +#ifdef HAVE_LIBXTABLES + xt_init(); +#endif +} + +void nft_global_deinit(void) +{ + ct_label_table_exit(); + realm_table_rt_exit(); + devgroup_table_exit(); + realm_table_meta_exit(); + mark_table_exit(); +} diff --git a/src/main.c b/src/main.c index 8883959..dde3104 100644 --- a/src/main.c +++ b/src/main.c @@ -18,6 +18,7 @@ #include <fcntl.h> #include <sys/types.h> +#include <nftables/nftables.h> #include <nftables.h> #include <utils.h> #include <parser.h> @@ -272,28 +273,6 @@ err1: return ret; } -void nft_init(void) -{ - mark_table_init(); - realm_table_rt_init(); - devgroup_table_init(); - realm_table_meta_init(); - ct_label_table_init(); - gmp_init(); -#ifdef HAVE_LIBXTABLES - xt_init(); -#endif -} - -void nft_exit(void) -{ - ct_label_table_exit(); - realm_table_rt_exit(); - devgroup_table_exit(); - realm_table_meta_exit(); - mark_table_exit(); -} - int main(int argc, char * const *argv) { struct parser_state state; @@ -309,7 +288,7 @@ int main(int argc, char * const *argv) memset(&cache, 0, sizeof(cache)); init_list_head(&cache.list); - nft_init(); + nft_global_init(); nf_sock = netlink_open_sock(); while (1) { val = getopt_long(argc, argv, OPTSTRING, options, NULL); @@ -440,7 +419,7 @@ out: cache_release(&cache); iface_cache_release(); netlink_close_sock(nf_sock); - nft_exit(); + nft_global_deinit(); return rc; }
Add global init and deinit functions. Signed-off-by: Eric Leblond <eric@regit.org> --- configure.ac | 4 ++++ include/Makefile.am | 2 +- include/nftables/Makefile.am | 2 ++ include/nftables/nftables.h | 20 +++++++++++++++++ src/Makefile.am | 26 +++++++++++++--------- src/libnftables.c | 53 ++++++++++++++++++++++++++++++++++++++++++++ src/main.c | 27 +++------------------- 7 files changed, 99 insertions(+), 35 deletions(-) create mode 100644 include/nftables/Makefile.am create mode 100644 include/nftables/nftables.h create mode 100644 src/libnftables.c