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
