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

[Shuah Khan <skhan@linuxfoundation.org>]

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@kernel.org
>>> Cc: linux-kselftest@vger.kernel.org
>>> Signed-off-by: Antonio Quartulli <antonio@openvpn.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