Ander Juaristi <a@xxxxxxxxxxxx> wrote:
> This patch implements the delete operation from the ruleset.
>
> It implements a new delete() function in nft_set_rhash. It is simpler
> to use than the already existing remove(), because it only takes the set
> and the key as arguments, whereas remove() expects a full
> nft_set_elem structure.
>
> Signed-off-by: Ander Juaristi <a@xxxxxxxxxxxx>
> ---
> include/net/netfilter/nf_tables.h | 17 ++++++---
> include/uapi/linux/netfilter/nf_tables.h | 1 +
> net/netfilter/nft_dynset.c | 44 +++++++++++++++++-------
> net/netfilter/nft_set_hash.c | 19 ++++++++++
> 4 files changed, 63 insertions(+), 18 deletions(-)
>
> diff --git a/include/net/netfilter/nf_tables.h b/include/net/netfilter/nf_tables.h
> index 9e8493aad49d..ddea26ba14b1 100644
> --- a/include/net/netfilter/nf_tables.h
> +++ b/include/net/netfilter/nf_tables.h
> @@ -286,24 +286,25 @@ struct nft_expr;
> /**
> * struct nft_set_ops - nf_tables set operations
> *
> + * @update: update an element if exists, add it if doesn't exist
> + * @delete: delete an element
> * @lookup: look up an element within the set
I would suggest:
> * @lookup: look up an element within the set
> + * @update: update an element if exists, add it if doesn't exist
> + * @delete: delete an element
> + *
> + * Operations update and delete have simpler interfaces, and currently
> + * only used in the packet path. All the rest are for the control plane.
Not correct, update/delete/lookup are packet path.
> */
> struct nft_set_ops {
You could add a comment here,
/* data plane / fast path */
> - bool (*lookup)(const struct net *net,
> - const struct nft_set *set,
> - const u32 *key,
> - const struct nft_set_ext **ext);
This looks confusing, why is this being moved around?
You can keep it where it is, its most likely the most frequently called op.
> bool (*update)(struct nft_set *set,
> const u32 *key,
> void *(*new)(struct nft_set *,
> @@ -312,7 +313,13 @@ struct nft_set_ops {
> const struct nft_expr *expr,
> struct nft_regs *regs,
> const struct nft_set_ext **ext);
> + bool (*delete)(const struct nft_set *set,
> + const u32 *key);
Adding delete here looks good.
> + bool (*lookup)(const struct net *net,
> + const struct nft_set *set,
> + const u32 *key,
> + const struct nft_set_ext **ext);
I would keep it at the start, no need to move it.
You could add a comment here,
/* control plane / slow path */
Also, please run your patches through scripts/checkpatch.pl.
> +static bool nft_rhash_delete(const struct nft_set *set,
> + const u32 *key)
> +{
> + struct nft_rhash *priv = nft_set_priv(set);
> + struct nft_rhash_elem *he;
> + struct nft_rhash_cmp_arg arg = {
> + .genmask = NFT_GENMASK_ANY,
> + .set = set,
> + .key = key,
> + };
> +
> + he = rhashtable_lookup(&priv->ht, &arg, nft_rhash_params);
> + if (he == NULL)
> + return false;
> +
> + return (rhashtable_remove_fast(&priv->ht, &he->node, nft_rhash_params) == 0);
No need for those ().
return rhashtable_remove_fast( ...) == 0;
Other than that this looks ready for merging.
