On Thu, Dec 20, 2018 at 04:09:12PM +0100, Phil Sutter wrote:
> @@ -1195,12 +1182,14 @@ err:
> return NULL;
> }
>
> -static struct nftnl_rule_list *nft_rule_list_get(struct nft_handle *h);
> +static struct nftnl_chain *
> +nft_chain_find(struct nft_handle *h, const char *table, const char *chain);
>
> int
> nft_rule_append(struct nft_handle *h, const char *chain, const char *table,
> void *data, uint64_t handle, bool verbose)
> {
> + struct nftnl_chain *c;
> struct nftnl_rule *r;
> int type;
>
> @@ -1228,10 +1217,9 @@ nft_rule_append(struct nft_handle *h, const char *chain, const char *table,
> if (verbose)
> h->ops->print_rule(r, 0, FMT_PRINT_RULE);
>
> - if (!nft_rule_list_get(h))
> - return 0;
> -
> - nftnl_rule_list_add_tail(r, h->rule_cache);
> + c = nft_chain_find(h, table, chain);
> + if (c)
> + nftnl_chain_rule_add_tail(r, c);
I think we always have a chain here.
> return 1;
Because we return success in any case.
[...]
> @@ -2024,16 +2028,16 @@ next:
> int nft_rule_check(struct nft_handle *h, const char *chain,
> const char *table, void *data, bool verbose)
> {
> - struct nftnl_rule_list *list;
> + struct nftnl_chain *c;
> struct nftnl_rule *r;
>
> nft_fn = nft_rule_check;
>
> - list = nft_rule_list_get(h);
> - if (list == NULL)
> + c = nft_chain_find(h, table, chain);
> + if (!c)
errno = ENOENT?
Or chain is assumed to be found always?
> return 0;
>
> - r = nft_rule_find(h, list, chain, table, data, -1);
> + r = nft_rule_find(h, c, data, -1);
> if (r == NULL) {
> errno = ENOENT;
> return 0;
> @@ -2048,16 +2052,18 @@ int nft_rule_delete(struct nft_handle *h, const char *chain,
> const char *table, void *data, bool verbose)
> {
> int ret = 0;
> + struct nftnl_chain *c;
> struct nftnl_rule *r;
> - struct nftnl_rule_list *list;
>
> nft_fn = nft_rule_delete;
>
> - list = nft_rule_list_get(h);
> - if (list == NULL)
> + c = nft_chain_find(h, table, chain);
> + if (!c) {
> + errno = ENOENT;
Here you set it to ENOENT and elsewhere.
> return 0;
> + }
>
> - r = nft_rule_find(h, list, chain, table, data, -1);
> + r = nft_rule_find(h, c, data, -1);
> if (r != NULL) {
> ret =__nft_rule_del(h, r);
> if (ret < 0)
> @@ -2136,9 +2144,9 @@ int nft_rule_insert(struct nft_handle *h, const char *chain,
> goto err;
>
> if (handle)
> - nftnl_rule_list_insert_at(new_rule, r);
> + nftnl_chain_rule_insert_at(new_rule, r);
I wonder why we need nftnl_chain_rule_insert_at() if there is no chain
parameter, see below.
> @@ -2149,16 +2157,18 @@ int nft_rule_delete_num(struct nft_handle *h, const char *chain,
> const char *table, int rulenum, bool verbose)
> {
> int ret = 0;
> + struct nftnl_chain *c;
> struct nftnl_rule *r;
> - struct nftnl_rule_list *list;
>
> nft_fn = nft_rule_delete_num;
>
> - list = nft_rule_list_get(h);
> - if (list == NULL)
> + c = nft_chain_find(h, table, chain);
> + if (!c) {
> + errno = ENOENT;
> return 0;
> + }
>
> - r = nft_rule_find(h, list, chain, table, NULL, rulenum);
> + r = nft_rule_find(h, c, NULL, rulenum);
> if (r != NULL) {
> DEBUGP("deleting rule by number %d\n", rulenum);
> ret = __nft_rule_del(h, r);
> @@ -2174,16 +2184,18 @@ int nft_rule_replace(struct nft_handle *h, const char *chain,
> const char *table, void *data, int rulenum, bool verbose)
> {
> int ret = 0;
> + struct nftnl_chain *c;
> struct nftnl_rule *r;
> - struct nftnl_rule_list *list;
>
> nft_fn = nft_rule_replace;
>
> - list = nft_rule_list_get(h);
> - if (list == NULL)
> + c = nft_chain_find(h, table, chain);
> + if (!c) {
> + errno = ENOENT;
> return 0;
> + }
>
> - r = nft_rule_find(h, list, chain, table, data, rulenum);
> + r = nft_rule_find(h, c, data, rulenum);
> if (r != NULL) {
> DEBUGP("replacing rule with handle=%llu\n",
> (unsigned long long)
Here in nft_rule_replace() I can see we still use
nftnl_rule_list_del() to remove an entry from the cache.
No problem, since internally nftnl_rule_list_del() does the same as
nftnl_chain_rule_del().
We can probably deprecate nftnl_rule_list at some point, and rely
entirely on nftnl_chain_rule_add() and so on.
[...]
> @@ -3143,19 +3106,8 @@ static int nft_is_expr_compatible(struct nftnl_expr *expr, void *data)
> return -1;
> }
>
> -struct nft_is_rule_compatible_data {
> - const char *tablename;
> -};
> -
> static int nft_is_rule_compatible(struct nftnl_rule *rule, void *data)
> {
> - const char *table = nftnl_rule_get_str(rule, NFTNL_RULE_TABLE);
> - struct nft_is_rule_compatible_data *d = data;
> -
> - /* ignore rules belonging to a different table */
> - if (strcmp(table, d->tablename))
> - return 0;
> -
> return nftnl_expr_foreach(rule, nft_is_expr_compatible, NULL);
> }
This change I think it doesn't belong here, anyway, codebase looks
better now, but for 'git annotate' purpose, better if we can avoid
this in the future.
Thanks a lot for your work.
