| Message ID | 20171019081847.16171-7-phil@nwl.cc |
|---|---|
| State | Changes Requested |
| Delegated to: | Pablo Neira |
| Headers | show |
| Series | libnftables preparations | expand |
On Thu, Oct 19, 2017 at 10:18:46AM +0200, Phil Sutter wrote: > In order to keep the API simple, remove INCLUDE_PATHS_MAX restraint and > dynamically allocate nft_ctx field include_paths instead. > > Signed-off-by: Phil Sutter <phil@nwl.cc> > --- > include/nftables/nftables.h | 6 +++--- > src/libnftables.c | 34 ++++++++++++++++++++++++++++++++-- > src/main.c | 9 ++++----- > src/scanner.l | 4 +--- > 4 files changed, 40 insertions(+), 13 deletions(-) > > diff --git a/include/nftables/nftables.h b/include/nftables/nftables.h > index f0c9bbf3ba3fe..a752f20d74132 100644 > --- a/include/nftables/nftables.h > +++ b/include/nftables/nftables.h > @@ -17,8 +17,6 @@ struct nft_cache { > uint32_t seqnum; > }; > > -#define INCLUDE_PATHS_MAX 16 > - > struct output_ctx { > unsigned int numeric; > unsigned int stateless; > @@ -30,7 +28,7 @@ struct output_ctx { > > struct nft_ctx { > struct mnl_socket *nf_sock; > - const char *include_paths[INCLUDE_PATHS_MAX]; > + char **include_paths; > unsigned int num_include_paths; > unsigned int parser_max_errors; > unsigned int debug_mask; > @@ -78,6 +76,8 @@ void nft_ctx_free(struct nft_ctx *ctx); > > FILE *nft_ctx_set_output(struct nft_ctx *ctx, FILE *fp); > void nft_ctx_set_dry_run(struct nft_ctx *ctx, bool dry); > +int nft_ctx_add_include_path(struct nft_ctx *ctx, const char *path); > +void nft_ctx_clear_include_paths(struct nft_ctx *ctx); > > void nft_ctx_flush_cache(struct nft_ctx *ctx); > > diff --git a/src/libnftables.c b/src/libnftables.c > index 817f537e32618..2f4275c9a0a94 100644 > --- a/src/libnftables.c > +++ b/src/libnftables.c > @@ -6,10 +6,13 @@ > * published by the Free Software Foundation. > * > */ > +#define _GNU_SOURCE > #include <erec.h> > #include <errno.h> > #include <mnl.h> > #include <parser.h> > +#include <stdio.h> > +#include <stdlib.h> > #include <string.h> > #include <utils.h> > #include <iface.h> > @@ -122,6 +125,33 @@ static void nft_exit(void) > mark_table_exit(); > } > > +int nft_ctx_add_include_path(struct nft_ctx *ctx, const char *path) Do we want to accept runtime addition/removal of include paths? I mean, I would just make it nft_ctx_set_include_path(), then add an unsetter, so we simplify this. Let me know if I'm overlooking anything, 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
Hi, On Fri, Oct 20, 2017 at 02:17:00PM +0200, Pablo Neira Ayuso wrote: > On Thu, Oct 19, 2017 at 10:18:46AM +0200, Phil Sutter wrote: [...] > > +int nft_ctx_add_include_path(struct nft_ctx *ctx, const char *path) > > Do we want to accept runtime addition/removal of include paths? Not necessarily, but src/main.c does just that: It calls nft_ctx_new() first, then adds include paths as it parses them from command line. > I mean, I would just make it nft_ctx_set_include_path(), then add an > unsetter, so we simplify this. The counterpart to nft_ctx_add_include_path() is nft_ctx_clear_include_paths(), which just drops all the previously set ones. Does that meet your understanding of an unsetter, or am I missing something? The reason why this patch is a bit more complicated is because I wanted to get rid of the hard upper limit of include paths to avoid introducing a getter for number of set include paths or to make it necessary for applications (read: src/main.c) to check what return code nft_ctx_add_include_path() returned to print a reasonable error message. 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
On Fri, Oct 20, 2017 at 07:16:20PM +0200, Phil Sutter wrote: > Hi, > > On Fri, Oct 20, 2017 at 02:17:00PM +0200, Pablo Neira Ayuso wrote: > > On Thu, Oct 19, 2017 at 10:18:46AM +0200, Phil Sutter wrote: > [...] > > > +int nft_ctx_add_include_path(struct nft_ctx *ctx, const char *path) > > > > Do we want to accept runtime addition/removal of include paths? > > Not necessarily, but src/main.c does just that: It calls nft_ctx_new() > first, then adds include paths as it parses them from command line. So it's more like a one time call to set up the include path, right? So I think semantically this is just another setter. This _add_ name made me think you can keep adding including path one after another anytime. > > I mean, I would just make it nft_ctx_set_include_path(), then add an > > unsetter, so we simplify this. > > The counterpart to nft_ctx_add_include_path() is > nft_ctx_clear_include_paths(), which just drops all the previously set > ones. Does that meet your understanding of an unsetter, or am I missing > something? Do we have a usecase for nft_ctx_clear_include_paths(). If we don't - I don't see any at least from my side - I'd prefer, to keep it back. > The reason why this patch is a bit more complicated is because I wanted > to get rid of the hard upper limit of include paths to avoid introducing > a getter for number of set include paths or to make it necessary for > applications (read: src/main.c) to check what return code > nft_ctx_add_include_path() returned to print a reasonable error message. I'm fine with removing the upper limit, but that is a different thing. My only concerns are related to the API we provide to set include paths. -- 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 Fri, Oct 20, 2017 at 09:16:43PM +0200, Pablo Neira Ayuso wrote: > On Fri, Oct 20, 2017 at 07:16:20PM +0200, Phil Sutter wrote: > > Hi, > > > > On Fri, Oct 20, 2017 at 02:17:00PM +0200, Pablo Neira Ayuso wrote: > > > On Thu, Oct 19, 2017 at 10:18:46AM +0200, Phil Sutter wrote: > > [...] > > > > +int nft_ctx_add_include_path(struct nft_ctx *ctx, const char *path) > > > > > > Do we want to accept runtime addition/removal of include paths? > > > > Not necessarily, but src/main.c does just that: It calls nft_ctx_new() > > first, then adds include paths as it parses them from command line. > > So it's more like a one time call to set up the include path, right? > So I think semantically this is just another setter. This _add_ name > made me think you can keep adding including path one after another > anytime. Yes, the API (or specifically, nft_ctx_add_include_path()) allows that. The only alternative I could think of would be to introduce something like: | int nft_ctx_set_include_paths(struct nft_ctx *ctx, const char **paths) Which means src/main.c would have to take care of populating the char ** array itself in order to later pass it in one go to the setter. Fine with me, you decide! :) > > > I mean, I would just make it nft_ctx_set_include_path(), then add an > > > unsetter, so we simplify this. > > > > The counterpart to nft_ctx_add_include_path() is > > nft_ctx_clear_include_paths(), which just drops all the previously set > > ones. Does that meet your understanding of an unsetter, or am I missing > > something? > > Do we have a usecase for nft_ctx_clear_include_paths(). If we don't > - I don't see any at least from my side - I'd prefer, to keep it back. It's only used in nft_ctx_free() for now, just because it's convenient. If you don't want to export it (yet), I can make it static so code readability is kept but it won't be available to applications. > > The reason why this patch is a bit more complicated is because I wanted > > to get rid of the hard upper limit of include paths to avoid introducing > > a getter for number of set include paths or to make it necessary for > > applications (read: src/main.c) to check what return code > > nft_ctx_add_include_path() returned to print a reasonable error message. > > I'm fine with removing the upper limit, but that is a different thing. > My only concerns are related to the API we provide to set include > paths. OK, cool. So we only have to agree about above items. 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
diff --git a/include/nftables/nftables.h b/include/nftables/nftables.h index f0c9bbf3ba3fe..a752f20d74132 100644 --- a/include/nftables/nftables.h +++ b/include/nftables/nftables.h @@ -17,8 +17,6 @@ struct nft_cache { uint32_t seqnum; }; -#define INCLUDE_PATHS_MAX 16 - struct output_ctx { unsigned int numeric; unsigned int stateless; @@ -30,7 +28,7 @@ struct output_ctx { struct nft_ctx { struct mnl_socket *nf_sock; - const char *include_paths[INCLUDE_PATHS_MAX]; + char **include_paths; unsigned int num_include_paths; unsigned int parser_max_errors; unsigned int debug_mask; @@ -78,6 +76,8 @@ void nft_ctx_free(struct nft_ctx *ctx); FILE *nft_ctx_set_output(struct nft_ctx *ctx, FILE *fp); void nft_ctx_set_dry_run(struct nft_ctx *ctx, bool dry); +int nft_ctx_add_include_path(struct nft_ctx *ctx, const char *path); +void nft_ctx_clear_include_paths(struct nft_ctx *ctx); void nft_ctx_flush_cache(struct nft_ctx *ctx); diff --git a/src/libnftables.c b/src/libnftables.c index 817f537e32618..2f4275c9a0a94 100644 --- a/src/libnftables.c +++ b/src/libnftables.c @@ -6,10 +6,13 @@ * published by the Free Software Foundation. * */ +#define _GNU_SOURCE #include <erec.h> #include <errno.h> #include <mnl.h> #include <parser.h> +#include <stdio.h> +#include <stdlib.h> #include <string.h> #include <utils.h> #include <iface.h> @@ -122,6 +125,33 @@ static void nft_exit(void) mark_table_exit(); } +int nft_ctx_add_include_path(struct nft_ctx *ctx, const char *path) +{ + char **tmp; + int pcount = ctx->num_include_paths; + + tmp = realloc(ctx->include_paths, (pcount + 1) * sizeof(char *)); + if (!tmp) + return -1; + + ctx->include_paths = tmp; + + if (asprintf(&ctx->include_paths[pcount], "%s", path) < 0) + return -1; + + ctx->num_include_paths++; + return 0; +} + +void nft_ctx_clear_include_paths(struct nft_ctx *ctx) +{ + while (ctx->num_include_paths) + xfree(ctx->include_paths[--ctx->num_include_paths]); + + xfree(ctx->include_paths); + ctx->include_paths = NULL; +} + static void nft_ctx_netlink_init(struct nft_ctx *ctx) { ctx->nf_sock = netlink_open_sock(); @@ -134,8 +164,7 @@ struct nft_ctx *nft_ctx_new(uint32_t flags) nft_init(); ctx = xzalloc(sizeof(struct nft_ctx)); - ctx->include_paths[0] = DEFAULT_INCLUDE_PATH; - ctx->num_include_paths = 1; + nft_ctx_add_include_path(ctx, DEFAULT_INCLUDE_PATH); ctx->parser_max_errors = 10; init_list_head(&ctx->cache.list); ctx->flags = flags; @@ -158,6 +187,7 @@ void nft_ctx_free(struct nft_ctx *ctx) netlink_close_sock(ctx->nf_sock); nft_ctx_flush_cache(ctx); + nft_ctx_clear_include_paths(ctx); xfree(ctx); nft_exit(); } diff --git a/src/main.c b/src/main.c index 8359367b78654..de5c115757f44 100644 --- a/src/main.c +++ b/src/main.c @@ -196,13 +196,12 @@ int main(int argc, char * const *argv) interactive = true; break; case OPT_INCLUDEPATH: - if (nft->num_include_paths >= INCLUDE_PATHS_MAX) { - fprintf(stderr, "Too many include paths " - "specified, max. %u\n", - INCLUDE_PATHS_MAX - 1); + if (nft_ctx_add_include_path(nft, optarg)) { + fprintf(stderr, + "Failed to add include path '%s'\n", + optarg); exit(NFT_EXIT_FAILURE); } - nft->include_paths[nft->num_include_paths++] = optarg; break; case OPT_NUMERIC: if (++nft->output.numeric > NUMERIC_ALL) { diff --git a/src/scanner.l b/src/scanner.l index 594073660c6b1..ee09775ebf1d9 100644 --- a/src/scanner.l +++ b/src/scanner.l @@ -794,9 +794,7 @@ int scanner_include_file(struct nft_ctx *nft, void *scanner, int ret = -1; if (search_in_include_path(filename)) { - for (i = 0; i < INCLUDE_PATHS_MAX; i++) { - if (nft->include_paths[i] == NULL) - break; + for (i = 0; i < nft->num_include_paths; i++) { ret = snprintf(buf, sizeof(buf), "%s/%s", nft->include_paths[i], filename); if (ret < 0 || ret >= PATH_MAX) {
In order to keep the API simple, remove INCLUDE_PATHS_MAX restraint and dynamically allocate nft_ctx field include_paths instead. Signed-off-by: Phil Sutter <phil@nwl.cc> --- include/nftables/nftables.h | 6 +++--- src/libnftables.c | 34 ++++++++++++++++++++++++++++++++-- src/main.c | 9 ++++----- src/scanner.l | 4 +--- 4 files changed, 40 insertions(+), 13 deletions(-)