| Message ID | 20171019081847.16171-5-phil@nwl.cc |
|---|---|
| State | Changes Requested |
| Delegated to: | Pablo Neira |
| Headers | show |
| Series | libnftables preparations | expand |
On Thu, Oct 19, 2017 at 10:18:44AM +0200, Phil Sutter wrote: > This simplifies CLI code and allows to reduce libnftables API by not > exporting nft_run(). > > Since nft_run_cmd_from_buffer() takes care of scanner initialization and > libmnl socket passed to cli_init() is present as nft_ctx field as well, > signature of cli_init() can be reduced to just take nft_ctx pointer as > single argument. libmnl socket is indeed in nft_ctx, but we're planning a mode that allows to expose the mnl_socket for advanced handling. In that scenario, nft->nf_sock will be null. So I would prefer we don't do changes that we have to undo once the advanced API is in place. -- 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 02:15:34PM +0200, Pablo Neira Ayuso wrote: > On Thu, Oct 19, 2017 at 10:18:44AM +0200, Phil Sutter wrote: > > This simplifies CLI code and allows to reduce libnftables API by not > > exporting nft_run(). > > > > Since nft_run_cmd_from_buffer() takes care of scanner initialization and > > libmnl socket passed to cli_init() is present as nft_ctx field as well, > > signature of cli_init() can be reduced to just take nft_ctx pointer as > > single argument. > > libmnl socket is indeed in nft_ctx, but we're planning a mode that > allows to expose the mnl_socket for advanced handling. In that > scenario, nft->nf_sock will be null. > > So I would prefer we don't do changes that we have to undo once the > advanced API is in place. IMHO this doesn't contradict what the patch does. Right now we only have the "simple API", and the patch changes src/cli.c to use just that. CLI code doesn't need anything which is not fulfilled by simple API at this point, so I'd say changing it to use advanced API should be done when we implement features (e.g. transaction control) there. What do you think? 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:10:18PM +0200, Phil Sutter wrote: > On Fri, Oct 20, 2017 at 02:15:34PM +0200, Pablo Neira Ayuso wrote: > > On Thu, Oct 19, 2017 at 10:18:44AM +0200, Phil Sutter wrote: > > > This simplifies CLI code and allows to reduce libnftables API by not > > > exporting nft_run(). > > > > > > Since nft_run_cmd_from_buffer() takes care of scanner initialization and > > > libmnl socket passed to cli_init() is present as nft_ctx field as well, > > > signature of cli_init() can be reduced to just take nft_ctx pointer as > > > single argument. > > > > libmnl socket is indeed in nft_ctx, but we're planning a mode that > > allows to expose the mnl_socket for advanced handling. In that > > scenario, nft->nf_sock will be null. > > > > So I would prefer we don't do changes that we have to undo once the > > advanced API is in place. > > IMHO this doesn't contradict what the patch does. Right now we only have > the "simple API", and the patch changes src/cli.c to use just that. CLI > code doesn't need anything which is not fulfilled by simple API at this > point, so I'd say changing it to use advanced API should be done when we > implement features (e.g. transaction control) there. > > What do you think? I have no strong objection against this, I just would like we don't lose track of the high level API, and that one will need to expose the netlink socket. So all these calls we will end up needed the nf_sock parameter again at some point. I don't have any strong opinion against this, just an observation. -- 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:18:07PM +0200, Pablo Neira Ayuso wrote: > On Fri, Oct 20, 2017 at 07:10:18PM +0200, Phil Sutter wrote: > > On Fri, Oct 20, 2017 at 02:15:34PM +0200, Pablo Neira Ayuso wrote: > > > On Thu, Oct 19, 2017 at 10:18:44AM +0200, Phil Sutter wrote: > > > > This simplifies CLI code and allows to reduce libnftables API by not > > > > exporting nft_run(). > > > > > > > > Since nft_run_cmd_from_buffer() takes care of scanner initialization and > > > > libmnl socket passed to cli_init() is present as nft_ctx field as well, > > > > signature of cli_init() can be reduced to just take nft_ctx pointer as > > > > single argument. > > > > > > libmnl socket is indeed in nft_ctx, but we're planning a mode that > > > allows to expose the mnl_socket for advanced handling. In that > > > scenario, nft->nf_sock will be null. > > > > > > So I would prefer we don't do changes that we have to undo once the > > > advanced API is in place. > > > > IMHO this doesn't contradict what the patch does. Right now we only have > > the "simple API", and the patch changes src/cli.c to use just that. CLI > > code doesn't need anything which is not fulfilled by simple API at this > > point, so I'd say changing it to use advanced API should be done when we > > implement features (e.g. transaction control) there. > > > > What do you think? > > I have no strong objection against this, I just would like we don't > lose track of the high level API, and that one will need to expose the > netlink socket. So all these calls we will end up needed the nf_sock > parameter again at some point. > > I don't have any strong opinion against this, just an observation. I'd suggest to review things again once we have a common view of how the advanced API will look like. Right now, CLI is fine with using the simple API as proposed by my patches, so passing it ctx->nf_sock separately just to have it either ignore it or reassign it to ctx->nf_sock again doesn't make sense to me. 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/cli.h b/include/cli.h index 3ae1c459bce2d..3780e0917969d 100644 --- a/include/cli.h +++ b/include/cli.h @@ -5,11 +5,9 @@ struct parser_state; #ifdef HAVE_LIBREADLINE -extern int cli_init(struct nft_ctx *nft, struct mnl_socket *nf_sock, - struct parser_state *state); +extern int cli_init(struct nft_ctx *nft); #else -static inline int cli_init(struct nft_ctx *nft, struct mnl_socket *nf_sock, - struct parser_state *state) +static inline int cli_init(struct nft_ctx *nft) { return -1; } diff --git a/include/nftables/nftables.h b/include/nftables/nftables.h index fbc6fd4252a97..b91219d423df9 100644 --- a/include/nftables/nftables.h +++ b/include/nftables/nftables.h @@ -9,7 +9,6 @@ #ifndef LIB_NFTABLES_H #define LIB_NFTABLES_H -struct parser_state; struct mnl_socket; struct nft_cache { @@ -79,10 +78,6 @@ void nft_ctx_free(struct nft_ctx *ctx); FILE *nft_ctx_set_output(struct nft_ctx *ctx, FILE *fp); void nft_ctx_flush_cache(struct nft_ctx *ctx); -int nft_run(struct nft_ctx *nft, struct mnl_socket *nf_sock, - void *scanner, struct parser_state *state, - struct list_head *msgs); - int nft_run_cmd_from_buffer(struct nft_ctx *nft, char *buf, size_t buflen); int nft_run_cmd_from_filename(struct nft_ctx *nft, const char *filename); diff --git a/src/cli.c b/src/cli.c index cadc3af6e8034..37351f2f8b04f 100644 --- a/src/cli.c +++ b/src/cli.c @@ -35,15 +35,7 @@ #define CMDLINE_HISTFILE ".nft.history" -static const struct input_descriptor indesc_cli = { - .type = INDESC_CLI, - .name = "<cli>", -}; - -static struct parser_state *state; static struct nft_ctx *cli_nft; -static struct mnl_socket *cli_nf_sock; -static void *scanner; static char histfile[PATH_MAX]; static char *multiline; static bool eof; @@ -134,14 +126,9 @@ static void cli_complete(char *line) xfree(line); line = s; - parser_init(cli_nf_sock, &cli_nft->cache, state, &msgs, - cli_nft->debug_mask, &cli_nft->output); - scanner_push_buffer(scanner, &indesc_cli, line); - nft_run(cli_nft, cli_nf_sock, scanner, state, &msgs); - erec_print_list(&cli_nft->output, &msgs, cli_nft->debug_mask); + nft_run_cmd_from_buffer(cli_nft, line, len + 2); xfree(line); - cache_release(&cli_nft->cache); - iface_cache_release(); + nft_ctx_flush_cache(cli_nft); } static char **cli_completion(const char *text, int start, int end) @@ -149,12 +136,10 @@ static char **cli_completion(const char *text, int start, int end) return NULL; } -int cli_init(struct nft_ctx *nft, struct mnl_socket *nf_sock, - struct parser_state *_state) +int cli_init(struct nft_ctx *nft) { const char *home; - cli_nf_sock = nf_sock; cli_nft = nft; rl_readline_name = "nft"; rl_instream = stdin; @@ -171,9 +156,6 @@ int cli_init(struct nft_ctx *nft, struct mnl_socket *nf_sock, read_history(histfile); history_set_pos(history_length); - state = _state; - scanner = scanner_init(state); - while (!eof) rl_callback_read_char(); return 0; diff --git a/src/libnftables.c b/src/libnftables.c index 0de50c854d572..d88c299c3647e 100644 --- a/src/libnftables.c +++ b/src/libnftables.c @@ -74,9 +74,9 @@ out: return ret; } -int nft_run(struct nft_ctx *nft, struct mnl_socket *nf_sock, - void *scanner, struct parser_state *state, - struct list_head *msgs) +static int nft_run(struct nft_ctx *nft, struct mnl_socket *nf_sock, + void *scanner, struct parser_state *state, + struct list_head *msgs) { struct cmd *cmd, *next; int ret; diff --git a/src/main.c b/src/main.c index a7da460f28ca5..3c107181305c7 100644 --- a/src/main.c +++ b/src/main.c @@ -168,7 +168,6 @@ int main(int argc, char * const *argv) char *buf = NULL, *filename = NULL; unsigned int len; bool interactive = false; - struct parser_state state; int i, val, rc; nft = nft_ctx_new(NFT_CTX_DEFAULT); @@ -272,7 +271,7 @@ int main(int argc, char * const *argv) } else if (filename != NULL) { rc = nft_run_cmd_from_filename(nft, filename); } else if (interactive) { - if (cli_init(nft, nft->nf_sock, &state) < 0) { + if (cli_init(nft) < 0) { fprintf(stderr, "%s: interactive CLI not supported in this build\n", argv[0]); exit(NFT_EXIT_FAILURE);
This simplifies CLI code and allows to reduce libnftables API by not exporting nft_run(). Since nft_run_cmd_from_buffer() takes care of scanner initialization and libmnl socket passed to cli_init() is present as nft_ctx field as well, signature of cli_init() can be reduced to just take nft_ctx pointer as single argument. Note that this change introduces two (possibly unwanted) side-effects: * Input descriptor passed to scanner_push_buffer() is changed from the CLI-specific one to the one used by nft_run_cmd_from_buffer(). In practice though, this doesn't make a difference: input descriptor types INDESC_CLI and INDESC_BUFFER are treated equally by erec_print(). Also, scanner_push_buffer() NULLs input descriptor name, so that is not used at all in latter code. * Error messages are printed to stderr instead of cli_nft->output. This could be fixed by introducing an 'error_output' field in nft_ctx for nft_run_cmd_from_buffer() to use when printing error messages. Signed-off-by: Phil Sutter <phil@nwl.cc> --- include/cli.h | 6 ++---- include/nftables/nftables.h | 5 ----- src/cli.c | 24 +++--------------------- src/libnftables.c | 6 +++--- src/main.c | 3 +-- 5 files changed, 9 insertions(+), 35 deletions(-)