Re: [nft PATCH 0/8] Convert tests/py to use libnftables directly
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();
|
Re: [nft PATCH 0/8] Convert tests/py to use libnftables directly
Hi Phil, On Tue, Apr 10, 2018 at 07:00:18PM +0200, Phil Sutter wrote: > This series of patches results in tests/py/nft-test.py using libnftables > API directly via ctypes module which leads to significant performance > improvement of the test script. Series looks good, applied. 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? * 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. So what I'm suggesting is: Can we turn 'struct cookie' into the default way to print messages? 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? If you think these two comments are relevant, please follow up with patches. Thanks. > The first five patches are preparational work: > > * Patch 1 fixes a missed conversion to nft_print() in src/ct.c which > became obvious after nft-test.py started using libnftables API and > therefore ignored anything not written into output_fp. > > * Patch 2 moves a necessary workaround from main() into > run_cmd_from_buffer() which is required to accept non-newline > terminated strings. While being at it, reading input from files > without trailing newline is fixed as well. (Yes, this never worked.) > > * Patch 3 makes error output configurable by users, which is required if > one wants to redirect stderr without dirty hacks. > > * Patch 4 adds convenience functions for using fopencookie() to buffer > standard and error output. Doing so from within Python code is > non-trivial and therefore an unnecessary burden on (already chastened) > Python programmers. > > * Patch 5 simplifies the code introduced in patch 4. Though since it also > increases the size of struct nft_ctx quite a bit, it is kept separate > in case that side-effect is unwanted upstream. > > Finally, patch 6 introduces a simple Python class Nftables and changes > nft-test.py accordingly to make good use of it. Since we might at some > point want to ship that class as a Python module, it is placed in a new > topdir subdirectory named 'py'. > > Patches 7 and 8 are more or less fall-out from the actual > implementation. A bit of cleanup in the first one and a minor > enhancement in the latter. > > Phil Sutter (8): > ct: Fix output_fp bypass in ct_print() > libnftables: Fix for input without trailing newline > libnftables: Introduce nft_ctx_set_error() > libnftables: Support buffering output and error > libnftables: Simplify cookie integration > tests/py: Use libnftables instead of calling nft binary > tests/py: Review print statements in nft-test.py > tests/py: Allow passing multiple files to nft-test.py > > include/nftables.h| 17 ++- > include/nftables/nftables.h | 9 ++ > py/.gitignore | 1 + > py/nftables.py| 224 ++ > src/ct.c | 2 +- > src/erec.c| 2 +- > src/libnftables.c | 135 - > src/main.c| 5 +- > src/scanner.l | 2 +- > tests/py/any/ct.t | 14 +-- > tests/py/any/ct.t.payload | 12 +- > tests/py/any/log.t| 2 +- > tests/py/any/log.t.payload| 2 +- > tests/py/any/meta.t | 4 +- > tests/py/arp/arp.t| 2 +- > tests/py/arp/arp.t.payload| 2 +- > tests/py/arp/arp.t.payload.netdev | 2 +- > tests/py/inet/tcp.t | 2 +- > tests/py/inet/tcp.t.payload | 2 +- > tests/py/ip/ip.t | 6 +- > tests/py/ip/ip.t.payload | 6 +- > tests/py/ip/ip.t.payload.bridge | 6 +- > tests/py/ip/ip.t.payload.inet | 6 +- > tests/py/ip/ip.t.payload.netdev | 6 +- > tests/py/ip/objects.t | 4 +- > tests/py/nft-test.py | 247 > ++ > 26 files changed, 545 insertions(+), 177 deletions(-) > create mode 100644 py/.gitignore > create mode 100644 py/nftables.py > > -- > 2.16.1 > -- To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[nft PATCH 0/8] Convert tests/py to use libnftables directly
This series of patches results in tests/py/nft-test.py using libnftables API directly via ctypes module which leads to significant performance improvement of the test script. The first five patches are preparational work: * Patch 1 fixes a missed conversion to nft_print() in src/ct.c which became obvious after nft-test.py started using libnftables API and therefore ignored anything not written into output_fp. * Patch 2 moves a necessary workaround from main() into run_cmd_from_buffer() which is required to accept non-newline terminated strings. While being at it, reading input from files without trailing newline is fixed as well. (Yes, this never worked.) * Patch 3 makes error output configurable by users, which is required if one wants to redirect stderr without dirty hacks. * Patch 4 adds convenience functions for using fopencookie() to buffer standard and error output. Doing so from within Python code is non-trivial and therefore an unnecessary burden on (already chastened) Python programmers. * Patch 5 simplifies the code introduced in patch 4. Though since it also increases the size of struct nft_ctx quite a bit, it is kept separate in case that side-effect is unwanted upstream. Finally, patch 6 introduces a simple Python class Nftables and changes nft-test.py accordingly to make good use of it. Since we might at some point want to ship that class as a Python module, it is placed in a new topdir subdirectory named 'py'. Patches 7 and 8 are more or less fall-out from the actual implementation. A bit of cleanup in the first one and a minor enhancement in the latter. Phil Sutter (8): ct: Fix output_fp bypass in ct_print() libnftables: Fix for input without trailing newline libnftables: Introduce nft_ctx_set_error() libnftables: Support buffering output and error libnftables: Simplify cookie integration tests/py: Use libnftables instead of calling nft binary tests/py: Review print statements in nft-test.py tests/py: Allow passing multiple files to nft-test.py include/nftables.h| 17 ++- include/nftables/nftables.h | 9 ++ py/.gitignore | 1 + py/nftables.py| 224 ++ src/ct.c | 2 +- src/erec.c| 2 +- src/libnftables.c | 135 - src/main.c| 5 +- src/scanner.l | 2 +- tests/py/any/ct.t | 14 +-- tests/py/any/ct.t.payload | 12 +- tests/py/any/log.t| 2 +- tests/py/any/log.t.payload| 2 +- tests/py/any/meta.t | 4 +- tests/py/arp/arp.t| 2 +- tests/py/arp/arp.t.payload| 2 +- tests/py/arp/arp.t.payload.netdev | 2 +- tests/py/inet/tcp.t | 2 +- tests/py/inet/tcp.t.payload | 2 +- tests/py/ip/ip.t | 6 +- tests/py/ip/ip.t.payload | 6 +- tests/py/ip/ip.t.payload.bridge | 6 +- tests/py/ip/ip.t.payload.inet | 6 +- tests/py/ip/ip.t.payload.netdev | 6 +- tests/py/ip/objects.t | 4 +- tests/py/nft-test.py | 247 ++ 26 files changed, 545 insertions(+), 177 deletions(-) create mode 100644 py/.gitignore create mode 100644 py/nftables.py -- 2.16.1 -- To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
