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,
--
Antonio Quartulli
OpenVPN Inc.
