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

[Sergey Ryazanov <ryazanov.s.a@gmail.com>]

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