On Fri, 2023-07-14 at 11:59 +0200, Phil Sutter wrote:
> Hi Thomas,
>
> On Fri, Jul 14, 2023 at 10:48:53AM +0200, Thomas Haller wrote:
> > Note that the corresponding API for output flags does not expose
> > the
> > plain numeric flags. Instead, it exposes the underlying, flag-based
> > C
> > API more directly.
> >
> > Reasons:
> >
> > - a flags property has the benefits that adding new flags is very
> > light
> > weight. Otherwise, every addition of a flag requires new API.
> > That new
> > API increases the documentation and what the user needs to
> > understand.
> > With a flag API, we just need new documentation what the new flag
> > is.
> > It's already clear how to use it.
> >
> > - opinionated, also the usage of "many getter/setter API" is not
> > have
> > better usability. Its convenient when we can do similar things
> > (setting
> > a boolean flag) depending on an argument of a function, instead
> > of
> > having different functions.
> >
> > Compare
> >
> > ctx.set_reversedns_output(True)
> > ctx.set_handle_output(True)
> >
> > with
> >
> > ctx.ouput_set_flags(NFT_CTX_OUTPUT_REVERSEDNS |
> > NFT_CTX_OUTPUT_HANDLE)
> >
> > Note that the vast majority of users of this API will just create
> > one
> > nft_ctx instance and set the flags once. Each user application
> > probably has only one place where they call the setter once. So
> > while I think flags have better usability, it doesn't matter much
> > either way.
> >
> > - if individual properties are preferable over flags, then the C
> > API
> > should also do that. In other words, the Python API should be
> > similar
> > to the underlying C API.
> >
> > - I don't understand how to do this best. Is Nftables.output_flags
> > public API? It appears to be, as it has no underscore. Why does
> > this
> > additional mapping from function (get_reversedns_output()) to
> > name
> > ("reversedns") to number (1<<0) exist?
>
> I don't recall why I chose to add setters/getters for individual
> output
> flags instead of expecting users to do bit-fiddling. Maybe the latter
> is
> not as common among Python users. :)
>
> On the other hand, things are a bit inconsistent already, see
> set_debug() method.
>
> Maybe we could turn __{get,set}_output_flag() public and make them
> accept an array of strings or numbers just like set_debug()? If you
> then adjust your input flag API accordingly, things become consistent
> (enough?), without breaking existing users.
>
> FWIW, I find
>
> > ctx.set_output_flags(["reversedns", "stateless"])
>
> nicer than
>
> > ctx.set_output_flags(REVERSEDNS | STATELESS)
>
> at least with a Python hat on. WDYT?
Hi Phil,
I see set_debug().
So we can do:
nft.set_debug("netlink")
or
nft.set_debug(("netlink", "scanner"))
but to me, that is not an improvement over plain
nft.output_set_debug(nftables.NFT_DEBUG_NETLINK | nftables.NFT_DEBUG_SCANNER)
(which would be a thin layer over the underlying, documented C API).
I like set_debug() better than the __set_output_flag() approach,
because the flags are an argument of one function, instead of multiple
set-flag-xyz() functions. I don't like very much that
- the "set_debug()" name does not resemble the underlying
nft_ctx_output_set_debug() name.
- it encourages using string literals as arguments (instead of
Python constants which I can grep for and find with ctags).
- it requires extra some code to translate from one domain
(the list of names/ints)) to another (plain integer flags), when
the user could just as well use the flags directly.
Anyway. I don't really mind either way. I will do whatever we agree
upon.
Thanks,
Thomas
