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: <9837c95a-2366-4733-b26a-9bfd27261f56@linuxfoundation.org>
Date: Thu, 17 Oct 2024 15:40:51 -0600
From: Shuah Khan <skhan@...uxfoundation.org>
To: Antonio Quartulli <antonio@...nvpn.net>
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>, Shuah Khan <skhan@...uxfoundation.org>
Subject: Re: [PATCH net-next v9 23/23] testing/selftest: add test tool and
 scripts for ovpn module

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.

> 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