netdev - Re: [PATCH net-next v9 23/23] testing/selftest: add test tool and scripts for ovpn module

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <ad45c95c-c5dc-42e5-b270-4dd0a1d0aa75@openvpn.net>
Date: Fri, 18 Oct 2024 10:20:33 +0200
From: Antonio Quartulli <antonio@...nvpn.net>
To: Shuah Khan <skhan@...uxfoundation.org>
Cc: netdev@...r.kernel.org, linux-kernel@...r.kernel.org,
 openvpn-devel@...ts.sourceforge.net, linux-kselftest@...r.kernel.org,
 Eric Dumazet <edumazet@...gle.com>, Jakub Kicinski <kuba@...nel.org>,
 Paolo Abeni <pabeni@...hat.com>, Donald Hunter <donald.hunter@...il.com>,
 Shuah Khan <shuah@...nel.org>, sd@...asysnail.net, ryazanov.s.a@...il.com,
 Andrew Lunn <andrew@...n.ch>
Subject: Re: [PATCH net-next v9 23/23] testing/selftest: add test tool and
 scripts for ovpn module

On 17/10/2024 23:40, Shuah Khan wrote:
> On 10/17/24 05:27, Antonio Quartulli wrote:
>> On 16/10/2024 23:14, Shuah Khan wrote:
>>> On 10/15/24 19:03, Antonio Quartulli wrote:
>>>> The ovpn-cli tool can be compiled and used as selftest for the ovpn
>>>> kernel module.
>>>>
>>>> It implements the netlink API and can thus be integrated in any
>>>> script for more automated testing.
>>>>
>>>> Along with the tool, 2 scripts are added that perform basic
>>>> functionality tests by means of network namespaces.
>>>>
>>>> The scripts can be performed in sequence by running run.sh
>>>>
>>>> Cc: shuah@...nel.org
>>>> Cc: linux-kselftest@...r.kernel.org
>>>> Signed-off-by: Antonio Quartulli <antonio@...nvpn.net>
>>>
>>> I almost gave my Reviewed-by when I saw the very long argument parsing
>>> in the main() - please see comment below under main().
>>>
>>> Let's simply the logic using getopt() - it is way too long and
>>> complex.
>>
>> Shuan,
>>
>> while looking into this I got the feeling that getopt() may not be the 
>> right tool for this parser.
>>
>> The ovpn-cli tool doesn't truly excpect "options" with their arguments 
>> on the command line, but it rather takes a "command" followed by 
>> command-specific arguments/modifiers. More like the 'ip' tool (from 
>> iproute2).
>>
>> The large if/else block is checking for the specified command.
>> Moreover commands are *mutually exclusive*.
>>
>> Converting this logic to getopt() seems quite complicated as I'd need to:
>> * keep track of the first specified command (which may be in any 
>> position)
>> * prevent other commands to be thrown on the command line
>> * come up with an option for each command-specific argument (and make 
>> sure only those required by the specified command are present)
>>
> 
> Thank for looking into it. I would like to make a suggestion to
> add a parse() routine and move this logic there instead of making
> the main() very long. It will be easier to read the code as well.

Ok, I get your point. Let me work something out :)
Thanks again for your feedback!

Regards,

> 
>> Are you sure this is the right path to follow?
>>
>> The 'ip' tool also implements something similar after all.
>>
> 
> Sometimes argument parsing takes on life as new options get
> added. It starts out as a couple if else conditionals and
> expands - when I see a long argument parsing code, I like
> to pause and ask the question. Sounds like you case is more
> complex.
> 
> thanks,
> -- Shuah

-- 
Antonio Quartulli
OpenVPN Inc.