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

Reply via email to