2025-03-03, 15:45:23 +0100, Antonio Quartulli wrote:
> On 03/03/2025 14:08, Sabrina Dubroca wrote:
> > > + if (ovpn_sock && ovpn_sock->sock->sk == sk)
> > > + skip = false;
> > > + rcu_read_unlock();
> > > +
> > > + if (skip)
> > > + continue;
> >
> >
> > The skip/continue logic looks a tiny bit strange to me, maybe this:
>
> Hehe, it's like a double negation. I agree it can be improved.
>
> >
> > hash_for_each_safe(ovpn->peers->by_id, bkt, tmp, peer, hash_entry_id) {
> > bool remove = true;
>
> does the netdev coding style allow to use locally scoped variables?
> Or should I declare everything at the beginning of the function?
>
> I had this rule in mind, but it may have been eliminated by now.
Based on a few samples from net/core/dev.c, I'd say it's allowed:
https://web.git.kernel.org/pub/scm/linux/kernel/git/netdev/net-next.git/tree/net/core/dev.c?id=357660d7596b#n4634
https://web.git.kernel.org/pub/scm/linux/kernel/git/netdev/net-next.git/tree/net/core/dev.c?id=357660d7596b#n11404
https://web.git.kernel.org/pub/scm/linux/kernel/git/netdev/net-next.git/tree/net/core/dev.c?id=357660d7596b#n12319
https://web.git.kernel.org/pub/scm/linux/kernel/git/netdev/net-next.git/tree/net/core/dev.c?id=357660d7596b#n12531
> >
> > /* if a socket was passed as argument, skip all peers except
> > * those using it
> > */
> > if (sk) {
> > rcu_read_lock();
> > ovpn_sock = rcu_dereference(peer->sock);
> > remove = ovpn_sock && ovpn_sock->sock->sk == sk;
> > rcu_read_unlock();
> > }
> >
> > if (remove)
> > ovpn_peer_remove(peer, reason, &release_list);
> > }
> >
> >
> > (only if you agree it looks better - if it's my opinion against yours,
> > ignore me since it's really just coding style/taste)
>
> Yours look simpler/cleaner. I'll go with it.
ok :)
--
Sabrina
