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
>
>
