Hi Pablo,
On Wed, Apr 11, 2018 at 09:39:01AM +0200, Pablo Neira Ayuso wrote:
> Two comments, both related to 'struct cookie':
>
> * How is 'struct cookie' going to be used? Do you have follow up
> patches to update the codebase to use this?
It's already there, please have a look at
nft_ctx_{un,}buffer_{output,error} functions and those they call
(init_cookie, exit_cookie). The API interface they establish can be used
as such:
| struct nft_ctx *ctx;
| const char *cmd;
| int rc;
|
| ctx = nft_ctx_new(0);
| nft_buffer_output(ctx);
| nft_buffer_error(ctx);
|
| cmd = "list ruleset";
| rc = nft_run_cmd_from_buffer(ctx, cmd, sizeof(cmd));
|
| if (rc) {
| printf("FAIL: nftables says: %s\n",
| nft_ctx_get_error_buffer(ctx));
| } else {
| /* this prints 'list ruleset' output */
| printf("%s\n", nft_ctx_get_output_buffer(ctx));
| }
|
| nft_ctx_free(ctx);
So these nft_ctx_buffer_* and nft_ctx_get_*_buffer functions are an
alternative to nft_ctx_set_{output,error} functions for cases where it
is more convenient to have all output available in a char buffer instead
of being written to an open FILE pointer.
> * Can we avoid the union around struct cookie and FILE *fp? I mean, I
> don't think it makes sense to start exposing lots of toggles through
> the API, the more we expose, the more combinations are possible and
> then more chances we get to support all kind of strange combinations
> in the future.
That union is simply for convenience: {init,exit}_cookie functions take
a pointer to struct cookie as parameter and since it is part of that
union they get access to the "standard" FILE pointer which is used if
no buffering was requested.
That union is introduced by commit 4885331de84cb ("libnftables: Simplify
cookie integration"), a followup to 40d94ca1dcd8e ("libnftables: Support
buffering output and error") - please have a look at the latter to see
how things were without it.
> So what I'm suggesting is: Can we turn 'struct cookie' into the
> default way to print messages?
Sure, we could do that. OTOH there is e.g. nft cli, which doesn't care
about what's printed to stdout and stderr. Changing the default would
mean cli has to manually print to stdout and stderr after each command.
Internally, this buffering infrastructure is an add-on to FILE pointer
output we already have (which was introduced as the quickest path from
plain printf's everywhere to giving API users control over library
output).
I really think we should keep the FILE pointer output (i.e. nft_print())
internally, simply because changing everything to snprintf() is tedious
due to buffer length checks everywhere (I'm having libnftnl's
SNPRINTF_BUFFER_SIZE macro in mind ;).
The buffering add-on doesn't necessarily add complexity to API users'
implementations - prior to it, there were two options:
* Don't care and go with the default, i.e. output goes to stdout and
stderr.
* Provide a FILE pointer to print into, i.e. call nft_ctx_set_output().
The add-on adds a third, completely separate option to them:
* Get output in a buffer, i.e. call nft_ctx_buffer_output() and use
nft_ctx_get_output_buffer() afterwards to receive a pointer to the
buffer.
> Regarding the name 'struct cookie', is this refering to something
> related to Python? I mean, this name tells me very little. What I
> can see there is a new class that allows us to do buffering, so
> probably using 'struct nft_buffer' or such sound more convenient to
> me?
I chose it simply because it all evolves around fopencookie(3), which
provides the needed functionality (create a FILE pointer with
customizable write() backend). In the documentation, the opaque data
pointer passed around to callbacks is referred to as 'cookie'.
On Wed, Apr 11, 2018 at 09:48:25AM +0200, Pablo Neira Ayuso wrote:
> On Tue, Apr 10, 2018 at 07:00:21PM +0200, Phil Sutter wrote:
[...]
> > diff --git a/src/libnftables.c b/src/libnftables.c
> > index 8bf989b08cc54..d6622d51aba33 100644
> > --- a/src/libnftables.c
> > +++ b/src/libnftables.c
> > @@ -169,6 +169,7 @@ struct nft_ctx *nft_ctx_new(uint32_t flags)
> > init_list_head(&ctx->cache.list);
> > ctx->flags = flags;
> > ctx->output.output_fp = stdout;
> > + ctx->output.error_fp = stderr;
>
> I understand you may need this new toggle. But can we probably
> reasonable defaults? I mean, if no error_fp is specified, then use the
> one set by output_fp.
This was already the default, see this chunk from commit 4176e24e14f07
("libnftables: Introduce nft_ctx_set_error()"):
| @@ -297,9 +309,7 @@ int nft_run_cmd_from_buffer(struct nft_ctx *nft, char *buf, size_t buflen)
| if (nft_run(nft, nft->nf_sock, scanner, &state, &msgs) != 0)
| rc = -1;
|
| - fp = nft_ctx_set_output(nft, stderr);
| erec_print_list(&nft->output, &msgs, nft->debug_mask);
| - nft_ctx_set_output(nft, fp);
| scanner_destroy(scanner);
| iface_cache_release();
| free(nlbuf);
So for printing error records, output_fp was temporarily changed to
stderr. So error_fp defaulting to stderr is backwards compatible.
> As said, it would be good if you can review the existing toggles and
> see if we can stop exposing some of those through API if possible. I
> would just expose the bare minimum and provide reasonable defaults so
> we most people in the world will not need to call n+1 different setter
> functions.
Understood. In this case though, I think the combination of possible
FILE pointer output and output buffering into a library managed buffer
is worth keeping. Looking at nftables.h though, I guess we could reduce
more complexity by combining all the nft_ctx_output_* toggles into a
single one which accepts a bit field. With bits being: stateless,
ip2name, handle, echo and maybe also dry-run, although that's strictly
not the same category. What do you think?
Thanks, Phil
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
