xtables-restore: Review chain handling

There is no need to "delete" (actually, remove from cache) a chain if
noflush wasn't given: While handling the corresponding table line,
'table_flush' callback has already taken care of that.

This .chain_del indirection is not required since d1eb4d587297
("iptables-compat: chains are purge out already from table flush").

Streamlining the code further, move syntax checks to the top. If these
concede, there are three cases to distinguish:

A) Given chain name matches a builtin one in current table, so assume it
   exists already and just set policy and counters.

B) Noflush was given and the (custom) chain exists already, flush it.

C) Custom chain was either flushed (noflush not given) or didn't exist
   before, create it.

Signed-off-by: Phil Sutter <phil@nwl.cc>
Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>

diff --git a/iptables/nft-shared.h b/iptables/nft-shared.h
index 388abb97303ab2a45c565dbd3fcd99ca6796e2e5..019c1f20e2939e46bdc81ce1c8bef2dab44a81c9 100644 (file)
--- a/iptables/nft-shared.h
+++ b/iptables/nft-shared.h
@@ -245,8 +245,6 @@ struct nft_xt_restore_cb {
        void (*table_new)(struct nft_handle *h, const char *table);
        struct nftnl_chain_list *(*chain_list)(struct nft_handle *h,
                                               const char *table);
-       void (*chain_del)(struct nftnl_chain_list *clist, const char *curtable,
-                         const char *chain);
        int (*chain_user_flush)(struct nft_handle *h,
                                struct nftnl_chain_list *clist,
                                const char *table, const char *chain);

diff --git a/iptables/xtables-restore.c b/iptables/xtables-restore.c
index 642876d6c70aceb2e4d602d72184c2bfaa5eba08..4e00ed86be06d05ff7851f8326677a78782533c4 100644 (file)
--- a/iptables/xtables-restore.c
+++ b/iptables/xtables-restore.c
@@ -68,21 +68,6 @@ static struct nftnl_chain_list *get_chain_list(struct nft_handle *h,
        return chain_list;
 }
 
-static void chain_delete(struct nftnl_chain_list *clist, const char *curtable,
-                        const char *chain)
-{
-       struct nftnl_chain *chain_obj;
-
-       chain_obj = nft_chain_list_find(clist, chain);
-       /* This chain has been found, delete from list. Later
-        * on, unvisited chains will be purged out.
-        */
-       if (chain_obj != NULL) {
-               nftnl_chain_list_del(chain_obj);
-               nftnl_chain_free(chain_obj);
-       }
-}
-
 struct nft_xt_restore_cb restore_cb = {
        .chain_list     = get_chain_list,
        .commit         = nft_commit,
@@ -90,7 +75,6 @@ struct nft_xt_restore_cb restore_cb = {
        .table_new      = nft_table_new,
        .table_flush    = nft_table_flush,
        .chain_user_flush = nft_chain_user_flush,
-       .chain_del      = chain_delete,
        .do_command     = do_commandx,
        .chain_set      = nft_chain_set,
        .chain_user_add = nft_chain_user_add,
@@ -183,7 +167,6 @@ void xtables_restore_parse(struct nft_handle *h,
                        /* New chain. */
                        char *policy, *chain = NULL;
                        struct xt_counters count = {};
-                       bool chain_exists = false;
 
                        chain = strtok(buffer+1, " \t\n");
                        DEBUGP("line %u, chain '%s'\n", line, chain);
@@ -194,21 +177,6 @@ void xtables_restore_parse(struct nft_handle *h,
                                exit(1);
                        }
 
-                       if (noflush == 0) {
-                               if (cb->chain_del)
-                                       cb->chain_del(chain_list, curtable->name,
-                                                     chain);
-                       } else if (nft_chain_list_find(chain_list, chain)) {
-                               chain_exists = true;
-                               /* Apparently -n still flushes existing user
-                                * defined chains that are redefined. Otherwise,
-                                * leave them as is.
-                                */
-                               if (cb->chain_user_flush)
-                                       cb->chain_user_flush(h, chain_list,
-                                                            curtable->name, chain);
-                       }
-
                        if (strlen(chain) >= XT_EXTENSION_MAXNAMELEN)
                                xtables_error(PARAMETER_PROBLEM,
                                           "Invalid chain name `%s' "
@@ -246,24 +214,28 @@ void xtables_restore_parse(struct nft_handle *h,
                                }
                                DEBUGP("Setting policy of chain %s to %s\n",
                                       chain, policy);
-                               ret = 1;
 
-                       } else {
-                               if (!chain_exists &&
-                                   cb->chain_user_add &&
-                                   cb->chain_user_add(h, chain,
-                                                      curtable->name) < 0) {
-                                       if (errno == EEXIST)
-                                               continue;
+                       } else if (noflush &&
+                                  nftnl_chain_list_lookup_byname(chain_list, chain)) {
+                               /* Apparently -n still flushes existing user
+                                * defined chains that are redefined. Otherwise,
+                                * leave them as is.
+                                */
+                               if (cb->chain_user_flush)
+                                       cb->chain_user_flush(h, chain_list,
+                                                            curtable->name, chain);
+                       } else if (cb->chain_user_add &&
+                                  cb->chain_user_add(h, chain,
+                                                     curtable->name) < 0) {
+                               if (errno == EEXIST)
+                                       continue;
 
-                                       xtables_error(PARAMETER_PROBLEM,
-                                                     "cannot create chain "
-                                                     "'%s' (%s)\n", chain,
-                                                     strerror(errno));
-                               }
-                               continue;
+                               xtables_error(PARAMETER_PROBLEM,
+                                             "cannot create chain "
+                                             "'%s' (%s)\n", chain,
+                                             strerror(errno));
                        }
-
+                       ret = 1;
                } else if (in_table) {
                        int a;
                        char *pcnt = NULL;
@@ -496,7 +468,6 @@ struct nft_xt_restore_cb ebt_restore_cb = {
        .table_new      = nft_table_new,
        .table_flush    = nft_table_flush,
        .chain_user_flush = nft_chain_user_flush,
-       .chain_del      = chain_delete,
        .do_command     = do_commandeb,
        .chain_set      = nft_chain_set,
        .chain_user_add = nft_chain_user_add,
@@ -542,7 +513,6 @@ struct nft_xt_restore_cb arp_restore_cb = {
        .table_new      = nft_table_new,
        .table_flush    = nft_table_flush,
        .chain_user_flush = nft_chain_user_flush,
-       .chain_del      = chain_delete,
        .do_command     = do_commandarp,
        .chain_set      = nft_chain_set,
        .chain_user_add = nft_chain_user_add,