| Message ID | 20170819152420.22563-6-eric@regit.org |
|---|---|
| State | Changes Requested |
| Delegated to: | Pablo Neira |
| Headers | show |
On Sat, Aug 19, 2017 at 05:24:07PM +0200, Eric Leblond wrote: > Signed-off-by: Eric Leblond <eric@regit.org> > --- > include/nftables/nftables.h | 3 +++ > src/libnftables.c | 26 +++++++++++++++++++++++++- > src/main.c | 19 ++++++++----------- > 3 files changed, 36 insertions(+), 12 deletions(-) > > diff --git a/include/nftables/nftables.h b/include/nftables/nftables.h > index cfa60fe..63150ba 100644 > --- a/include/nftables/nftables.h > +++ b/include/nftables/nftables.h > @@ -20,4 +20,7 @@ void nft_global_deinit(void); > struct nft_ctx *nft_context_new(void); > void nft_context_free(struct nft_ctx *nft); > > +int nft_run_command_from_buffer(struct nft_ctx *nft, struct nft_cache *cache, > + char *buf, size_t buflen); Can we probably have something like: nft_import_from_buffer(ctx, ...) nft_import_from_file(ctx, ...) That initializes the context structure. Then, a generic: nft_run(ctx) I suggest you make a patch to add these to nftables in first place. I mean, just send a patch that adds these functions as static to src/main.c to start with it. Just as a cleanup to prepare thing, we can integrate this asap meanwhile we keep discussing library details, so we reduce the size/burden of your patchset as we iterate over it. To integrate things more quickly. 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
On Mon, Aug 21, 2017 at 10:23:44AM +0200, Pablo Neira Ayuso wrote: > On Sat, Aug 19, 2017 at 05:24:07PM +0200, Eric Leblond wrote: > > Signed-off-by: Eric Leblond <eric@regit.org> > > --- > > include/nftables/nftables.h | 3 +++ > > src/libnftables.c | 26 +++++++++++++++++++++++++- > > src/main.c | 19 ++++++++----------- > > 3 files changed, 36 insertions(+), 12 deletions(-) > > > > diff --git a/include/nftables/nftables.h b/include/nftables/nftables.h > > index cfa60fe..63150ba 100644 > > --- a/include/nftables/nftables.h > > +++ b/include/nftables/nftables.h > > @@ -20,4 +20,7 @@ void nft_global_deinit(void); > > struct nft_ctx *nft_context_new(void); > > void nft_context_free(struct nft_ctx *nft); > > > > +int nft_run_command_from_buffer(struct nft_ctx *nft, struct nft_cache *cache, > > + char *buf, size_t buflen); > > Can we probably have something like: > > nft_import_from_buffer(ctx, ...) > nft_import_from_file(ctx, ...) > > That initializes the context structure. Then, a generic: > > nft_run(ctx) > > I suggest you make a patch to add these to nftables in first place. > I mean, just send a patch that adds these functions as static to > src/main.c to start with it. Just as a cleanup to prepare thing, we > can integrate this asap meanwhile we keep discussing library details, > so we reduce the size/burden of your patchset as we iterate over it. > To integrate things more quickly. Actually, every nft_import_xyz() would just provide the batch support we need, ie. it should be possible to call nft_import_xyz() several times before we call nft_run(). The nft_import_xyz() calls would to the scanner, parsing and evaluation phases. The nft_run(nlsock, ...) would just push this into the kernel - so this function would be wrapping the netlink layer handling... I would say this nft_run() become nft_compile() instead and we take the underneath netlink codebase to do the handling away from this... Otherwise, we will have to add branches to deal with non-blocking IO. Then, at some point to start looking like libnl... and that is not good ;-) I can help with preparation patches, let's just agree on what needs to be done so we don't overlap each other. 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 Mon, 2017-08-21 at 10:45 +0200, Pablo Neira Ayuso wrote: > On Mon, Aug 21, 2017 at 10:23:44AM +0200, Pablo Neira Ayuso wrote: > > On Sat, Aug 19, 2017 at 05:24:07PM +0200, Eric Leblond wrote: > > > Signed-off-by: Eric Leblond <eric@regit.org> > > > --- > > > include/nftables/nftables.h | 3 +++ > > > src/libnftables.c | 26 +++++++++++++++++++++++++- > > > src/main.c | 19 ++++++++----------- > > > 3 files changed, 36 insertions(+), 12 deletions(-) > > > > > > diff --git a/include/nftables/nftables.h > > > b/include/nftables/nftables.h > > > index cfa60fe..63150ba 100644 > > > --- a/include/nftables/nftables.h > > > +++ b/include/nftables/nftables.h > > > @@ -20,4 +20,7 @@ void nft_global_deinit(void); > > > struct nft_ctx *nft_context_new(void); > > > void nft_context_free(struct nft_ctx *nft); > > > > > > +int nft_run_command_from_buffer(struct nft_ctx *nft, struct > > > nft_cache *cache, > > > + char *buf, size_t buflen); > > > > Can we probably have something like: > > > > nft_import_from_buffer(ctx, ...) > > nft_import_from_file(ctx, ...) > > > > That initializes the context structure. Then, a generic: > > > > nft_run(ctx) > > > > I suggest you make a patch to add these to nftables in first place. > > I mean, just send a patch that adds these functions as static to > > src/main.c to start with it. Just as a cleanup to prepare thing, we > > can integrate this asap meanwhile we keep discussing library > > details, > > so we reduce the size/burden of your patchset as we iterate over > > it. > > To integrate things more quickly. > > Actually, every nft_import_xyz() would just provide the batch support > we need, ie. it should be possible to call nft_import_xyz() several > times before we call nft_run(). > > The nft_import_xyz() calls would to the scanner, parsing and > evaluation phases. > > The nft_run(nlsock, ...) would just push this into the kernel - so > this function would be wrapping the netlink layer handling... I like the idea to hide the batch inside nft_import function. But I still don't like the idea of having the user handling 2 things (nft_ctx and netlink socket) to be able to send a simple command to the kernel. I understand your problem with netlink handling. What I would suggest is to add a flag to nft_context_new function so user can specify "I'm handling the nf socket". Then we can have something like nft_commit(context, ...) OR for advanced users: nft_run(nf_sock, context, ...) I don't like the naming but I think you get the idea. > I would say this nft_run() become nft_compile() instead and we take > the underneath netlink codebase to do the handling away from this... > > Otherwise, we will have to add branches to deal with non-blocking IO. > > Then, at some point to start looking like libnl... and that is not > good ;-) > > I can help with preparation patches, let's just agree on what needs > to > be done so we don't overlap each other. OK sure, I'm busy at work and I'm gonna answers to your other mails tonight. And then come to you to see how we can share the load. ++
On Mon, Aug 21, 2017 at 11:06:19AM +0200, Eric Leblond wrote: > Hi, > > On Mon, 2017-08-21 at 10:45 +0200, Pablo Neira Ayuso wrote: > > On Mon, Aug 21, 2017 at 10:23:44AM +0200, Pablo Neira Ayuso wrote: > > > On Sat, Aug 19, 2017 at 05:24:07PM +0200, Eric Leblond wrote: > > > > Signed-off-by: Eric Leblond <eric@regit.org> > > > > --- > > > > include/nftables/nftables.h | 3 +++ > > > > src/libnftables.c | 26 +++++++++++++++++++++++++- > > > > src/main.c | 19 ++++++++----------- > > > > 3 files changed, 36 insertions(+), 12 deletions(-) > > > > > > > > diff --git a/include/nftables/nftables.h > > > > b/include/nftables/nftables.h > > > > index cfa60fe..63150ba 100644 > > > > --- a/include/nftables/nftables.h > > > > +++ b/include/nftables/nftables.h > > > > @@ -20,4 +20,7 @@ void nft_global_deinit(void); > > > > struct nft_ctx *nft_context_new(void); > > > > void nft_context_free(struct nft_ctx *nft); > > > > > > > > +int nft_run_command_from_buffer(struct nft_ctx *nft, struct > > > > nft_cache *cache, > > > > + char *buf, size_t buflen); > > > > > > Can we probably have something like: > > > > > > nft_import_from_buffer(ctx, ...) > > > nft_import_from_file(ctx, ...) > > > > > > That initializes the context structure. Then, a generic: > > > > > > nft_run(ctx) > > > > > > I suggest you make a patch to add these to nftables in first place. > > > I mean, just send a patch that adds these functions as static to > > > src/main.c to start with it. Just as a cleanup to prepare thing, we > > > can integrate this asap meanwhile we keep discussing library > > > details, > > > so we reduce the size/burden of your patchset as we iterate over > > > it. > > > To integrate things more quickly. > > > > Actually, every nft_import_xyz() would just provide the batch support > > we need, ie. it should be possible to call nft_import_xyz() several > > times before we call nft_run(). > > > > The nft_import_xyz() calls would to the scanner, parsing and > > evaluation phases. > > > > The nft_run(nlsock, ...) would just push this into the kernel - so > > this function would be wrapping the netlink layer handling... > > I like the idea to hide the batch inside nft_import function. But I > still don't like the idea of having the user handling 2 things (nft_ctx > and netlink socket) to be able to send a simple command to the kernel. > > I understand your problem with netlink handling. What I would suggest > is to add a flag to nft_context_new function so user can specify "I'm > handling the nf socket". > > Then we can have something like > > nft_commit(context, ...) > > OR for advanced users: > > nft_run(nf_sock, context, ...) > > I don't like the naming but I think you get the idea. I'm fine if you want to provide a 'shortcut' function that does it all in one go, but it should be a composite of the _advanced functions_. In a nutshell: we provide a simple API for people that don't want to deal with IO at all, that's good. Then, an API that allows people to deal with IO themselves - advanced stuff. Simple API functions would be made of composites of the advance ones. Everytime I see IO hidding underneath layers of abstractions, it just means problems. In terms of maintainance, you end up with complex codebase with lots of switches/toggles that users can turn on and off, then the library code needs to handle all of those combinations... If the IO logic is placed on the side of client for advanced stuff, we simply don't have to maintain all that complexity. At least, until it is proven that some specific IO handling is good enough to be generalized to be placed in the library. So we expose a simple API that does it all in one shot for people that don't care about netlink, but you have to promise me it will always stay simple forever. No room to extend the _simple API_ with flags/knobs that users can turn on/off to slightly change the behaviour in some aspect, OK? :-) -- 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 Mon, 2017-08-21 at 11:44 +0200, Pablo Neira Ayuso wrote: > On Mon, Aug 21, 2017 at 11:06:19AM +0200, Eric Leblond wrote: > > Hi, > > > > On Mon, 2017-08-21 at 10:45 +0200, Pablo Neira Ayuso wrote: > > > On Mon, Aug 21, 2017 at 10:23:44AM +0200, Pablo Neira Ayuso > > > wrote: > > > > On Sat, Aug 19, 2017 at 05:24:07PM +0200, Eric Leblond wrote: > > > > > Signed-off-by: Eric Leblond <eric@regit.org> > > > > > --- > > > > > include/nftables/nftables.h | 3 +++ > > > > > src/libnftables.c | 26 +++++++++++++++++++++++++- > > > > > src/main.c | 19 ++++++++----------- > > > > > 3 files changed, 36 insertions(+), 12 deletions(-) > > > > > > > > > > diff --git a/include/nftables/nftables.h > > > > > b/include/nftables/nftables.h > > > > > index cfa60fe..63150ba 100644 > > > > > --- a/include/nftables/nftables.h > > > > > +++ b/include/nftables/nftables.h > > > > > @@ -20,4 +20,7 @@ void nft_global_deinit(void); > > > > > struct nft_ctx *nft_context_new(void); > > > > > void nft_context_free(struct nft_ctx *nft); > > > > > > > > > > +int nft_run_command_from_buffer(struct nft_ctx *nft, struct > > > > > nft_cache *cache, > > > > > + char *buf, size_t buflen); > > > > > > > > Can we probably have something like: > > > > > > > > nft_import_from_buffer(ctx, ...) > > > > nft_import_from_file(ctx, ...) > > > > > > > > That initializes the context structure. Then, a generic: > > > > > > > > nft_run(ctx) > > > > > > > > I suggest you make a patch to add these to nftables in first > > > > place. > > > > I mean, just send a patch that adds these functions as static > > > > to > > > > src/main.c to start with it. Just as a cleanup to prepare > > > > thing, we > > > > can integrate this asap meanwhile we keep discussing library > > > > details, > > > > so we reduce the size/burden of your patchset as we iterate > > > > over > > > > it. > > > > To integrate things more quickly. > > > > > > Actually, every nft_import_xyz() would just provide the batch > > > support > > > we need, ie. it should be possible to call nft_import_xyz() > > > several > > > times before we call nft_run(). > > > > > > The nft_import_xyz() calls would to the scanner, parsing and > > > evaluation phases. > > > > > > The nft_run(nlsock, ...) would just push this into the kernel - > > > so > > > this function would be wrapping the netlink layer handling... > > > > I like the idea to hide the batch inside nft_import function. But I > > still don't like the idea of having the user handling 2 things > > (nft_ctx > > and netlink socket) to be able to send a simple command to the > > kernel. > > > > I understand your problem with netlink handling. What I would > > suggest > > is to add a flag to nft_context_new function so user can specify > > "I'm > > handling the nf socket". > > > > Then we can have something like > > > > nft_commit(context, ...) > > > > OR for advanced users: > > > > nft_run(nf_sock, context, ...) > > > > I don't like the naming but I think you get the idea. > > I'm fine if you want to provide a 'shortcut' function that does it > all > in one go, but it should be a composite of the _advanced functions_. > > In a nutshell: we provide a simple API for people that don't want to > deal with IO at all, that's good. Then, an API that allows people to > deal with IO themselves - advanced stuff. Simple API functions would > be made of composites of the advance ones. OK, I'm good with this approach and it will please the "I'm afraid of netlink" club ;) > Everytime I see IO hidding underneath layers of abstractions, it just > means problems. In terms of maintainance, you end up with complex > codebase with lots of switches/toggles that users can turn on and > off, > then the library code needs to handle all of those combinations... > > If the IO logic is placed on the side of client for advanced stuff, > we > simply don't have to maintain all that complexity. At least, until it > is proven that some specific IO handling is good enough to be > generalized to be placed in the library. > > So we expose a simple API that does it all in one shot for people > that > don't care about netlink, but you have to promise me it will always > stay simple forever. No room to extend the _simple API_ with > flags/knobs that users can turn on/off to slightly change the > behaviour in some aspect, OK? :-) I think we can all have as a guideline for libnftables that all advanced things are going to the advanced functions. The simple functions must provide something appealing in term of features but have to remain really simple. This make me think I still don't know how to deal with sets. I'm not planning to work on it but I think it is a feature that should be available in the simple functions. But we are dealing with possibly complex object so this can be really messy. BR,
On Mon, Aug 21, 2017 at 09:21:06PM +0200, Eric Leblond wrote: > On Mon, 2017-08-21 at 11:44 +0200, Pablo Neira Ayuso wrote: > > On Mon, Aug 21, 2017 at 11:06:19AM +0200, Eric Leblond wrote: [...] > > In a nutshell: we provide a simple API for people that don't want to > > deal with IO at all, that's good. Then, an API that allows people to > > deal with IO themselves - advanced stuff. Simple API functions would > > be made of composites of the advance ones. > > OK, I'm good with this approach and it will please the "I'm afraid of > netlink" club ;) OK, so we agree on the API policy then. [...] > I think we can all have as a guideline for libnftables that all > advanced things are going to the advanced functions. The simple > functions must provide something appealing in term of features but have > to remain really simple. Fine with it. > This make me think I still don't know how to deal with sets. I'm not > planning to work on it but I think it is a feature that should be > available in the simple functions. But we are dealing with possibly > complex object so this can be really messy. What's your concern with sets? -- 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 Tue, 2017-08-22 at 14:37 +0200, Pablo Neira Ayuso wrote: > On Mon, Aug 21, 2017 at 09:21:06PM +0200, Eric Leblond wrote: > > On Mon, 2017-08-21 at 11:44 +0200, Pablo Neira Ayuso wrote: > > > On Mon, Aug 21, 2017 at 11:06:19AM +0200, Eric Leblond wrote: > > [...] > > > In a nutshell: we provide a simple API for people that don't want > > > to > > > deal with IO at all, that's good. Then, an API that allows people > > > to > > > deal with IO themselves - advanced stuff. Simple API functions > > > would > > > be made of composites of the advance ones. > > > > OK, I'm good with this approach and it will please the "I'm afraid > > of > > netlink" club ;) > > OK, so we agree on the API policy then. > > [...] > > I think we can all have as a guideline for libnftables that all > > advanced things are going to the advanced functions. The simple > > functions must provide something appealing in term of features but > > have > > to remain really simple. > > Fine with it. > > > This make me think I still don't know how to deal with sets. I'm > > not > > planning to work on it but I think it is a feature that should be > > available in the simple functions. But we are dealing with possibly > > complex object so this can be really messy. > > What's your concern with sets? None fundamental really. It is just I don't see how we can build an easy API with set that can looks like "ipv4_addr . ipv4addr . inet_service". The use needs to be able to build the set object (could be a string) AND to parse it. This last part is the most complex I think. Maybe the JSON formatting could help here. ++
diff --git a/include/nftables/nftables.h b/include/nftables/nftables.h index cfa60fe..63150ba 100644 --- a/include/nftables/nftables.h +++ b/include/nftables/nftables.h @@ -20,4 +20,7 @@ void nft_global_deinit(void); struct nft_ctx *nft_context_new(void); void nft_context_free(struct nft_ctx *nft); +int nft_run_command_from_buffer(struct nft_ctx *nft, struct nft_cache *cache, + char *buf, size_t buflen); + #endif diff --git a/src/libnftables.c b/src/libnftables.c index 6756c0f..9701660 100644 --- a/src/libnftables.c +++ b/src/libnftables.c @@ -7,7 +7,6 @@ * */ -#include <nftables/nftables.h> #include <string.h> #include <errno.h> #include <nftables.h> @@ -18,6 +17,8 @@ #include <libmnl/libmnl.h> #include <mnl.h> +#include <nftables/nftables.h> + #include <unistd.h> #include <fcntl.h> @@ -71,3 +72,26 @@ void nft_context_free(struct nft_ctx *nft) netlink_close_sock(nft->nf_sock); xfree(nft); } + +static const struct input_descriptor indesc_cmdline = { + .type = INDESC_BUFFER, + .name = "<cmdline>", +}; + +int nft_run_command_from_buffer(struct nft_ctx *nft, struct nft_cache *cache, + char *buf, size_t buflen) +{ + int rc = NFT_EXIT_SUCCESS; + struct parser_state state; + LIST_HEAD(msgs); + void *scanner; + + parser_init(nft->nf_sock, cache, &state, &msgs); + scanner = scanner_init(&state); + scanner_push_buffer(scanner, &indesc_cmdline, buf); + + if (nft_run(nft, nft->nf_sock, cache, scanner, &state, &msgs) != 0) + rc = NFT_EXIT_FAILURE; + + return rc; +} diff --git a/src/main.c b/src/main.c index ee5566c..f863dec 100644 --- a/src/main.c +++ b/src/main.c @@ -18,8 +18,8 @@ #include <fcntl.h> #include <sys/types.h> -#include <nftables/nftables.h> #include <nftables.h> +#include <nftables/nftables.h> #include <utils.h> #include <parser.h> #include <rule.h> @@ -182,11 +182,6 @@ static const struct { }; #endif -static const struct input_descriptor indesc_cmdline = { - .type = INDESC_BUFFER, - .name = "<cmdline>", -}; - static int nft_netlink(struct nft_ctx *nft, struct nft_cache *cache, struct parser_state *state, struct list_head *msgs, struct mnl_socket *nf_sock) @@ -385,9 +380,10 @@ int main(int argc, char * const *argv) strcat(buf, " "); } strcat(buf, "\n"); - parser_init(nft->nf_sock, &cache, &state, &msgs); - scanner = scanner_init(&state); - scanner_push_buffer(scanner, &indesc_cmdline, buf); + rc = nft_run_command_from_buffer(nft, &cache, buf, len + 2); + if (rc < 0) + return rc; + goto libout; } else if (filename != NULL) { rc = cache_update(nft->nf_sock, &cache, CMD_INVALID, &msgs); if (rc < 0) @@ -397,6 +393,8 @@ int main(int argc, char * const *argv) scanner = scanner_init(&state); if (scanner_read_file(scanner, filename, &internal_location) < 0) goto out; + if (nft_run(nft, nft->nf_sock, &cache, scanner, &state, &msgs) != 0) + rc = NFT_EXIT_FAILURE; } else if (interactive) { if (cli_init(nft, nft->nf_sock, &cache, &state) < 0) { fprintf(stderr, "%s: interactive CLI not supported in this build\n", @@ -409,11 +407,10 @@ int main(int argc, char * const *argv) exit(NFT_EXIT_FAILURE); } - if (nft_run(nft, nft->nf_sock, &cache, scanner, &state, &msgs) != 0) - rc = NFT_EXIT_FAILURE; out: scanner_destroy(scanner); erec_print_list(stderr, &msgs); +libout: xfree(buf); cache_release(&cache); iface_cache_release();
Signed-off-by: Eric Leblond <eric@regit.org> --- include/nftables/nftables.h | 3 +++ src/libnftables.c | 26 +++++++++++++++++++++++++- src/main.c | 19 ++++++++----------- 3 files changed, 36 insertions(+), 12 deletions(-)