Hi Jozsef,
On Sat, 14 Jul 2018 14:23:54 +0200 (CEST)
Jozsef Kadlecsik <kadlec@xxxxxxxxxxxxxxxxx> wrote:
> > -const char *
> > -ip_set_name_byindex(struct net *net, ip_set_id_t index)
> > +void
> > +ip_set_name_byindex(struct net *net, ip_set_id_t index, char *name)
> > {
> > - const struct ip_set *set = ip_set_rcu_get(net, index);
> > + struct ip_set *set = ip_set_rcu_get(net, index);
> >
> > BUG_ON(!set);
> > - BUG_ON(set->ref == 0);
> >
> > - /* Referenced, so it's safe */
> > - return set->name;
> > + __ip_set_get_netlink(set);
> > + strncpy(name, set->name, IPSET_MAXNAMELEN);
> > + __ip_set_put_netlink(set);
> > }
> > EXPORT_SYMBOL_GPL(ip_set_name_byindex);
>
> This is my main concern about the patch: it boils down to *four* locking
> operations, for every single list members in a list type of set. It costs
> too much!
>
> ip_set_name_byindex() is used in ip_set_list_set.c only, so please use
> write_lock_bh()/write_unlock_bh() directly, that should be sufficient.
I see. I thought it would be "cleaner" this way, but I didn't consider
it's unnecessarily expensive to do that.
I'll send v3 in a moment (also addressing the rest of your comments).
Thanks for the review and all the pointers!
--
Stefano
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
