Re: [PATCH nft v2 0/5] src: mnl: rework list hooks infra

[Pablo Neira Ayuso <pablo@netfilter.org>]

Hi Florian,

A few suggestions:

- nft list hooks could probably take 'ip' family as default for
  consistency with other commands? There is 'nft list ruleset' which
  is special because it is family agnostic. Otherwise print all
  hooks with 'nft list hooks'? leaving netdev out of the picture
  unless 'device' is specified. I guess this approach you follow
  is to be conservative with the existing behaviour.

- Maybe plain reject 'device' for arp and bridge families?

  nft list hooks arp device enp0s25
  # device keyword (enp0s25) unexpected for this family

  instead of displaying a warning? I guess you are being conservative
  again here, that is fine.

- I understand you don't like the inet/ingress hack. It is there to
  address the shortcomming of not allowing sets to be shared accross
  families, and it does not even address it fully. I admit it is not
  the best approach, I'd like to explore better ones to address the
  need for set sharing.

  Going back to your approach: this is a bit low level, it exposes
  internal implementation details, such as the inet syntactic sugar
  (this is all hidden behind after the {register,unregister}_hook API).
  I understand that my attempt to describe the pipeline was
  incomplete, but I still wonder if, from user POV, it might makes
  sense at least to show the inet/ingress hook when listing the inet
  family to give an idea of what hook is registered according to
  priority. I would still show the inet/ingress in netdev family too
  (yes, it would be redundant).

Documentation looks good.

Thanks!

On Wed, Jul 31, 2024 at 06:51:00PM +0200, Florian Westphal wrote:
> Turns out that not only was 'nft list hooks' mostly undocumented,
> there was also confusion on what it should do.
> 
> First, clean this code up and make it strictly a tool to dump
> the NFPROTO_X registered functions.
> 
> Then, remove the 'hook' function argument, this was still passed
> from back in the day when one could ask to only dump e.g.
> ipv4 prerouting.  This ability is of little value, so don't restore
> this but instead just remove the leftover code.
> 
> Next, allow dumping of netdev:egress hooks.
> Lastly, document this in more detail and make it clear that this
> dumps the netfilter hooks registered for the protocol families,
> and nothing else.
> 
> Once this gets applied I intend to make
> 'nft list hooks netdev'
> 
> dump device hooks for all interfaces, if any, instead of a
> 'no device provided' warning.
> 
> Florian Westphal (5):
>   src: mnl: clean up hook listing code
>   src: mnl: make family specification more strict when listing
>   src: drop obsolete hook argument form hook dump functions
>   src: add egress support for 'list hooks'
>   doc: add documentation about list hooks feature
> 
>  Makefile.am                 |   1 +
>  doc/additional-commands.txt | 116 ++++++++++++++++++++++++++++
>  doc/nft.txt                 |  63 +--------------
>  include/mnl.h               |   2 +-
>  src/mnl.c                   | 150 ++++++++++++++----------------------
>  src/rule.c                  |   6 +-
>  6 files changed, 179 insertions(+), 159 deletions(-)
>  create mode 100644 doc/additional-commands.txt
> 
> -- 
> 2.44.2
> 
>