Hi Antonio,
On 29.10.2024 12:47, Antonio Quartulli wrote:
Notable changes from v10:
- extended commit message of 23/23 with brief description of the output
- Link to v10: https://lore.kernel.org/r/20241025-b4-ovpn-v10-0-b87530777be7@openvpn.net
Please note that some patches were already reviewed by Andre Lunn, Donald Hunter and Shuah Khan. They have retained the Reviewed-by tag since no major code modification has happened since the review.
The latest code can also be found at:
As I promised many months ago I am starting publishing some nit picks regarding the series. The review was started when series was V3 "rebasing" the review to every next version to publish it at once. But I lost this race to the new version releasing velocity :) So, I am going to publish it patch-by-patch.
Anyway you and all participants have done a great progress toward making accelerator part of the kernel. Most of considerable things already resolved so do not wait me please to finish picking every nit.
Regarding "big" topics I have only two concerns: link creation using RTNL and a switch statement usage. In the corresponding thread, I asked Jiri to clarify that "should" regarding .newlink implementation. Hope he will have a chance to find a time to reply.
For the 'switch' statement, I see a repeating pattern of handling mode-or family-specific cases like this:
int ovpn_peer_add(struct ovpn_struct *ovpn, struct ovpn_peer *peer) { switch (ovpn->mode) { case OVPN_MODE_MP: return ovpn_peer_add_mp(ovpn, peer); case OVPN_MODE_P2P: return ovpn_peer_add_p2p(ovpn, peer); default: return -EOPNOTSUPP; } }
or
void ovpn_encrypt_post(void *data, int ret) { ... switch (peer->sock->sock->sk->sk_protocol) { case IPPROTO_UDP: ovpn_udp_send_skb(peer->ovpn, peer, skb); break; case IPPROTO_TCP: ovpn_tcp_send_skb(peer, skb); break; default: /* no transport configured yet */ goto err; } ... }
or
void ovpn_peer_keepalive_work(...) { ... switch (ovpn->mode) { case OVPN_MODE_MP: next_run = ovpn_peer_keepalive_work_mp(ovpn, now); break; case OVPN_MODE_P2P: next_run = ovpn_peer_keepalive_work_p2p(ovpn, now); break; } ... }
Did you consider to implement mode specific operations as a set of operations like this:
ovpn_ops { int (*peer_add)(struct ovpn_struct *ovpn, struct ovpn_peer *peer); int (*peer_del)(struct ovpn_peer *peer, enum ovpn_del_peer_reason reason); void (*send_skb)(struct ovpn_peer *peer, struct sk_buff *skb); time64_t (*keepalive_work)(...); };
Initialize them during the interface creation and invoke these operations indirectly. E.g.
int ovpn_peer_add(struct ovpn_struct *ovpn, struct ovpn_peer *peer) { return ovpn->ops->peer_add(ovpn, peer); }
void ovpn_encrypt_post(void *data, int ret) { ... ovpn->ops->send_skb(peer, skb); ... }
void ovpn_peer_keepalive_work(...) { ... next_run = ovpn->ops->keepalive_work(ovpn, now); ... }
Anyway the module has all these option values in advance during the network interface creation phase and I believe replacing 'switch' statements with indirect calls can make code easy to read.
-- Sergey