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

[Sabrina Dubroca <sd@queasysnail.net>]

Hello, a few minor coding style nits on this patch.

2025-02-27, 02:21:40 +0100, Antonio Quartulli wrote:
> @@ -197,9 +254,16 @@ static int ovpn_netdev_notifier_call(struct notifier_block *nb,
>  		netif_carrier_off(dev);
>  		ovpn->registered = false;
>  
> -		if (ovpn->mode == OVPN_MODE_P2P)
> +		switch (ovpn->mode) {
> +		case OVPN_MODE_P2P:
>  			ovpn_peer_release_p2p(ovpn, NULL,
>  					      OVPN_DEL_PEER_REASON_TEARDOWN);
> +			break;
> +		case OVPN_MODE_MP:
> +			ovpn_peers_free(ovpn, NULL,
> +					OVPN_DEL_PEER_REASON_TEARDOWN);
> +			break;
> +		}

nit: maybe that switch could be done inside ovpn_peers_free, since
both places calling ovpn_peers_free do the same thing?
(it would also be more consistent with the rest of the peer-related
functions that are wrappers for the _mp/_p2p variant, rather than
pushing the switch down to the caller)


> +void ovpn_peers_free(struct ovpn_priv *ovpn, struct sock *sk,
> +		     enum ovpn_del_peer_reason reason)
> +{
> +	struct ovpn_socket *ovpn_sock;
> +	LLIST_HEAD(release_list);
> +	struct ovpn_peer *peer;
> +	struct hlist_node *tmp;
> +	bool skip;
> +	int bkt;
> +
> +	spin_lock_bh(&ovpn->lock);
> +	hash_for_each_safe(ovpn->peers->by_id, bkt, tmp, peer, hash_entry_id) {
> +		/* if a socket was passed as argument, skip all peers except
> +		 * those using it
> +		 */
> +		if (sk) {
> +			skip = true;
> +
> +			rcu_read_lock();
> +			ovpn_sock = rcu_access_pointer(peer->sock);

rcu_dereference, since you're actually accessing ovpn_sock->sock
afterwards?

> +			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:

	hash_for_each_safe(ovpn->peers->by_id, bkt, tmp, peer, hash_entry_id) {
		bool remove = true;

		/* 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)

-- 
Sabrina