On 06/11/2024 02:18, Sergey Ryazanov wrote:
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@xxxxxxxxxxx
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:
https://github.com/OpenVPN/linux-kernel-ovpn
As I promised many months ago I am starting publishing some nit picks
regarding the series.
Thanks and welcome back!
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.
I'll go through them all and judge what's meaningful to add to v12 and
what can be postponed for later improvements.
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.
True, but to be honest at this point I am fine with sticking to RTNL,
also because we will soon introduce the ability to create 'persistent'
ifaces, which a user should be able to create before starting openvpn.
Going through RTNL for this is the best choice IMHO, therefore we have
an extra use case in favour of this approach (next to what Jiri already
mentioned).
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.
I see this was already discussed with Sabrina under another patch and I
have the same opinion.
To me the switch/case approach looks cleaner and I truly like it,
especially when enums are involved.
ops/callbacks are fine when they can be redefined at runtime (i.e. a
proto that can be registered by another module), but this is not the
case here.
I also feel that with ops it's not easy to understand what call is truly
being made by just looking at the caller context and reading can be harder.
So I truly prefer to stick to this schema.
Thanks a lot for sharing your point though.
Regards,
--
Antonio Quartulli
OpenVPN Inc.
