Netfilter-Devel Archive on lore.kernel.org
 help / color / Atom feed
From: Phil Sutter <phil@nwl.cc>
To: Pablo Neira Ayuso <pablo@netfilter.org>
Cc: netfilter-devel@vger.kernel.org
Subject: Re: [iptables PATCH v2 06/10] nft: Introduce struct nft_chain
Date: Tue, 13 Oct 2020 11:56:59 +0200
Message-ID: <20201013095659.GV13016@orbyte.nwl.cc> (raw)
In-Reply-To: <20201012120855.GE26845@salvia>

On Mon, Oct 12, 2020 at 02:08:55PM +0200, Pablo Neira Ayuso wrote:
> On Wed, Sep 23, 2020 at 07:48:45PM +0200, Phil Sutter wrote:
> > Preparing for ordered output of user-defined chains, introduce a local
> > datatype wrapping nftnl_chain. In order to maintain the chain name hash
> > table, introduce nft_chain_list as well and use it instead of
> > nftnl_chain_list.
> > 
> > Put everything into a dedicated source file and provide a bunch of
> > getters for attributes of the embedded libnftnl_chain object.
> [...]
> > diff --git a/iptables/nft-chain.h b/iptables/nft-chain.h
> > new file mode 100644
> > index 0000000000000..818bbf1f4b525
> > --- /dev/null
> > +++ b/iptables/nft-chain.h
> > @@ -0,0 +1,87 @@
> > +#ifndef _NFT_CHAIN_H_
> > +#define _NFT_CHAIN_H_
> > +
> > +#include <libnftnl/chain.h>
> > +#include <libiptc/linux_list.h>
> > +
> > +struct nft_handle;
> > +
> > +struct nft_chain {
> > +	struct list_head	head;
> > +	struct hlist_node	hnode;
> > +	struct nftnl_chain	*nftnl;
> > +};
> > +
> > +#define CHAIN_NAME_HSIZE	512
> > +
> > +struct nft_chain_list {
> > +	struct list_head	list;
> > +	struct hlist_head	names[CHAIN_NAME_HSIZE];
> > +};
> > +
> > +struct nft_chain *nft_chain_alloc(struct nftnl_chain *nftnl);
> > +void nft_chain_free(struct nft_chain *c);
> > +
> > +struct nft_chain_list *nft_chain_list_alloc(void);
> > +void nft_chain_list_free(struct nft_chain_list *list);
> > +void nft_chain_list_del(struct nft_chain *c);
> > +
> > +static inline const char *nft_chain_name(struct nft_chain *c)
> > +{
> > +	return nftnl_chain_get_str(c->nftnl, NFTNL_CHAIN_NAME);
> > +}
> > +
> > +static inline const char *nft_chain_table(struct nft_chain *c)
> > +{
> > +	return nftnl_chain_get_str(c->nftnl, NFTNL_CHAIN_TABLE);
> > +}
> > +
> > +static inline const char *nft_chain_type(struct nft_chain *c)
> > +{
> > +	return nftnl_chain_get_str(c->nftnl, NFTNL_CHAIN_TYPE);
> > +}
> > +
> > +static inline uint32_t nft_chain_prio(struct nft_chain *c)
> > +{
> > +	return nftnl_chain_get_u32(c->nftnl, NFTNL_CHAIN_PRIO);
> > +}
> > +
> > +static inline uint32_t nft_chain_hooknum(struct nft_chain *c)
> > +{
> > +	return nftnl_chain_get_u32(c->nftnl, NFTNL_CHAIN_HOOKNUM);
> > +}
> > +
> > +static inline uint64_t nft_chain_packets(struct nft_chain *c)
> > +{
> > +	return nftnl_chain_get_u64(c->nftnl, NFTNL_CHAIN_PACKETS);
> > +}
> > +
> > +static inline uint64_t nft_chain_bytes(struct nft_chain *c)
> > +{
> > +	return nftnl_chain_get_u64(c->nftnl, NFTNL_CHAIN_BYTES);
> > +}
> > +
> > +static inline bool nft_chain_has_policy(struct nft_chain *c)
> > +{
> > +	return nftnl_chain_is_set(c->nftnl, NFTNL_CHAIN_POLICY);
> > +}
> > +
> > +static inline uint32_t nft_chain_policy(struct nft_chain *c)
> > +{
> > +	return nftnl_chain_get_u32(c->nftnl, NFTNL_CHAIN_POLICY);
> > +}
> > +
> > +static inline uint32_t nft_chain_use(struct nft_chain *c)
> > +{
> > +	return nftnl_chain_get_u32(c->nftnl, NFTNL_CHAIN_USE);
> > +}
> 
> Do you need this wrapper functions now? I mean, the intention is to
> have a native nft_chain structure so nft_chain_use() become:
> 
> static inline uint32_t nft_chain_use(struct nft_chain *c)
> {
> 	return c->use;
> }

Potentially, yes. I played with "pre-extracting" some attributes from
nftnl_chain object once, populating pointers in nft_chain object and
making above getters just return that pointer.

> at some point?
> 
> Sorry but I don't see this is happening in this batch?

I'm not entirely sure it is much use. In fact, we get nftnl_chain
objects when fetching cache from kernel and when pushing changes to
kernel we need nftnl_chain objects again. So unless we're concerned with
memory use in between or we get rid of the need for nftnl_chain objects,
dropping and recreating them is just extra work.

> I remember the original intention was to support for sorting chains,
> so the listing is predictable. But this batch is updating more things
> than that and I don't see a clear connection with the goal.

The nft_chain struct is needed mostly to allow for sorting the chain
list without adding code to libnftnl. This was your request before this
respin, you claimed it is important to keep libnftnl code size small. So
nft_chain is mostly a reimplementation of nftnl_chain_list, including
chain name hash.

The getter introduction is handy as I had many change like:

- nftnl_rule_set_str(rule, NFTNL_RULE_TABLE,
-                    nftnl_chain_get_str(c, NFTNL_CHAIN_TABLE));
+ nftnl_rule_set_str(rule, NFTNL_RULE_TABLE,
+                    nftnl_chain_get_str(c->nftnl, NFTNL_CHAIN_TABLE));

Given that I have to touch those lines anyway, with introducing a getter
things become a bit cleaner also:

- nftnl_rule_set_str(rule, NFTNL_RULE_TABLE,
-                    nftnl_chain_get_str(c, NFTNL_CHAIN_TABLE));
+ nftnl_rule_set_str(rule, NFTNL_RULE_TABLE, nft_chain_table(c));

Thinking about it, I could introduce the getters in a separate, early
patch. This would reduce the nft_chain introduction patch quite a bit.
But since these getters are (in the current form) specific to nft_chain
objects, I find it all belongs together.

Cheers, Phil

  reply index

Thread overview: 20+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-09-23 17:48 [iptables PATCH v2 00/10] nft: Sorted chain listing et al Phil Sutter
2020-09-23 17:48 ` [iptables PATCH v2 01/10] nft: Fix selective chain compatibility checks Phil Sutter
2020-10-12 11:54   ` Pablo Neira Ayuso
2020-10-13  9:29     ` Phil Sutter
2020-09-23 17:48 ` [iptables PATCH v2 02/10] nft: Implement nft_chain_foreach() Phil Sutter
2020-10-12 12:01   ` Pablo Neira Ayuso
2020-10-13  9:40     ` Phil Sutter
2020-09-23 17:48 ` [iptables PATCH v2 03/10] nft: cache: Introduce nft_cache_add_chain() Phil Sutter
2020-10-12 12:02   ` Pablo Neira Ayuso
2020-09-23 17:48 ` [iptables PATCH v2 04/10] nft: Eliminate nft_chain_list_get() Phil Sutter
2020-10-12 12:03   ` Pablo Neira Ayuso
2020-10-13  9:44     ` Phil Sutter
2020-09-23 17:48 ` [iptables PATCH v2 05/10] nft: cache: Move nft_chain_find() over Phil Sutter
2020-09-23 17:48 ` [iptables PATCH v2 06/10] nft: Introduce struct nft_chain Phil Sutter
2020-10-12 12:08   ` Pablo Neira Ayuso
2020-10-13  9:56     ` Phil Sutter [this message]
2020-09-23 17:48 ` [iptables PATCH v2 07/10] nft: Introduce a dedicated base chain array Phil Sutter
2020-09-23 17:48 ` [iptables PATCH v2 08/10] nft: cache: Sort custom chains by name Phil Sutter
2020-09-23 17:48 ` [iptables PATCH v2 09/10] tests: shell: Drop any dump sorting in place Phil Sutter
2020-09-23 17:48 ` [iptables PATCH v2 10/10] nft: Avoid pointless table/chain creation Phil Sutter

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20201013095659.GV13016@orbyte.nwl.cc \
    --to=phil@nwl.cc \
    --cc=netfilter-devel@vger.kernel.org \
    --cc=pablo@netfilter.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Netfilter-Devel Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/netfilter-devel/0 netfilter-devel/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 netfilter-devel netfilter-devel/ https://lore.kernel.org/netfilter-devel \
		netfilter-devel@vger.kernel.org
	public-inbox-index netfilter-devel

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.netfilter-devel


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git