On 03/03/2025 14:08, Sabrina Dubroca wrote:
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)
Yeah, makes sense!
+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?
Ouch, good catch.
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.
/* 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.
Thanks!
Cheers,