Re: [nft v2 PATCH 3/3] py: add input_{set,get}_flags() API to helpers

[Thomas Haller <thaller@redhat.com>]

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