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

[Antonio Quartulli <antonio@openvpn.net>]

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@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.

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.