Re: [PATCH net-next v20 15/25] ovpn: implement multi-peer support

[Sabrina Dubroca]

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