Re: [xtables-arptables PATCH v2 3/5] nft: nft_xtables_config_load() called only in nft_init()

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

Hi,

On Wed, Jul 24, 2013 at 12:36:27PM +0300, Tomasz Bursztyka wrote:
> Hi Pablo,
> 
> >>+int nft_init(struct nft_handle *h, struct builtin_table *t,
> >>>+	     const char *filename)
> >                          ^^^^^^^^
> >
> >why do we need this new parameter?
> >
> >The optional /etc/xtables.conf file should contain the definition for
> >all the families, that includes IPv4, IPv6, bridge and ARP.
> 
> My mistake, I advised that. Imho, it's not relevant to load
> everything for all families if the user is using only
> xtables-iptables for instance.
> 
> Then ok, let's have one file, but we could make a change in
> nft_xtables_config_load() so it would load only the tables of the
> current family.
> It's a quick fix to do actually.

Yes, that's the way to go.

> >>>  {
> >>>  	h->nl = mnl_socket_open(NETLINK_NETFILTER);
> >>>  	if (h->nl == NULL) {
> >>>@@ -388,6 +389,16 @@ int nft_init(struct nft_handle *h, struct builtin_table *t)
> >>>  	h->portid = mnl_socket_get_portid(h->nl);
> >>>  	h->tables = t;
> >>>  >+	/* If built-in chains don't exist for this table, create
> >>them */
> >>>+	if (nft_xtables_config_load(h, filename, 0) < 0) {
> >>>+		int i;
> >>>+
> >>>+		if (h->tables != NULL) {
> >>>+			for (i=0; i<TABLES_MAX; i++)
> >>>+				nft_chain_builtin_init(h, h->tables[i].name,
> >>>+						       NULL, NF_ACCEPT);
> >>>+		}
> >>>+	}
> >
> >I don't see what we get by moving nft_xtables_config_load here.
> 
> Why not calling this at only one unique place instead of multiple ones?
> You need to call that anyway currently as soon as you list/change
> the rule set.
> It's relevant to move that here, makes code clearer.

OK, but there are other chunks in this patch that are unclear to me:

@@ -1100,6 +1100,11 @@  int do_commandx(struct nft_handle *h, int
argc, char *argv[], char **table)
        if (h->ops == NULL)
                xtables_error(PARAMETER_PROBLEM, "Unknown family");
 
+       if (h->tables == NULL) {
+                if (nft_init(h, tables, XTABLES_CONFIG_DEFAULT) < 0)
+                        xtables_error(OTHER_PROBLEM, "Could not initialize nftables layer.");
+        }
+
        h->ops->post_parse(command, &cs, &args);
 
        if (command == CMD_REPLACE &&

By looking at patch 1, h->tables always point to some array of tables,
so that should always evaluates true.

Please, resolve all open issue and get back to me with a patch for
this if you think the time is worth to discuss this small
refactorization.

Thanks.