Re: [PATCH net-next v11 02/23] net: introduce OpenVPN Data Channel Offload (ovpn)

[Sergey Ryazanov]

On 15.11.2024 11:56, Antonio Quartulli wrote:
> On 06/11/2024 01:31, Sergey Ryazanov wrote:
> > > As explained above, in case of P2MP mode, OpenVPN will use the main 
> > > system
> > > routing table to decide which packet goes to which peer. This implies
> > > that no routing table was re-implemented in the `ovpn` kernel module.
> > >
> > > This kernel module can be enabled by selecting the CONFIG_OVPN entry
> > > in the networking drivers section.
> >
> > Most of the above text has no relation to the patch itself. Should it 
> > be moved to the cover letter?
>
> I think this needs to be in the git history.
> We are introducing a new kernel module and this is the presentation, so 
> I expect this to live in git.

Sure! I hope Jakub can literally merge the series as a branch with the 
merge commit containing the text from the cover later. If it's not Ok, 
then keeping the introduction in the first patch is a good idea.

> This was the original text when ovpn was a 1/1 patch.
> I can better clarify what this patch is doing and what comes in 
> following patches, if that can help.

The text itself looks a nice introduction into the functionality, keep 
it please.

> > > +/* Driver info */
> > > +#define DRV_DESCRIPTION    "OpenVPN data channel offload (ovpn)"
> > > +#define DRV_COPYRIGHT    "(C) 2020-2024 OpenVPN, Inc."
> >
> > nit: these strings are used only once for MODULE_{DESCRIPTION,AUTHOR} 
> > below. Can we directly use strings to avoid levels of indirection?
>
> I liked to have these defines at the top as if they were some form of 
> greeting :) But I can move them down and drop the constants.

715 of 784 network driver modules directly specify the description. I 
see mostly old drivers are using a dedicated macro. But it's up to you 
how to say hello to future code readers.

> > > --- a/include/uapi/linux/udp.h
> > > +++ b/include/uapi/linux/udp.h
> > > @@ -43,5 +43,6 @@ struct udphdr {
> > >   #define UDP_ENCAP_GTP1U        5 /* 3GPP TS 29.060 */
> > >   #define UDP_ENCAP_RXRPC        6
> > >   #define TCP_ENCAP_ESPINTCP    7 /* Yikes, this is really xfrm encap 
> > > types. */
> > > +#define UDP_ENCAP_OVPNINUDP    8 /* OpenVPN traffic */
> >
> > nit: this specific change does not belong to this specific patch.
>
> Right. Like for the Kconfig, I wanted to keep "general" changes and 
> things that touch the rest of the kernel in this patch.

Ok. I believe it's not a technical problem to introduce it here. The 
module will be buildable. It's more about facilitating reviewers' life. 
A code, introduced just in time, saves from two kinds of wonderings: (a) 
do we need this at all, when you see an unused definition, and (b) what 
it exactly means, when you see a user two patches after. Anyway, it was 
just a nit pick, and it's up to you how to introduce the module.

--
Sergey