Re: [dpdk-dev] [PATCH v1 3/3] test/devargs: add devargs test cases

["Gaëtan Rivet" <grive@u256.net>]

On Tue, Oct 5, 2021, at 17:54, Xueming Li wrote:
> Initial version to test Global devargs syntax.
>
> Signed-off-by: Xueming Li <xuemingl@nvidia.com>

Hi,

The test is a very nice addition, absolutely required.
I have however two remarks on the coverage and the implementation, below.

> ---
>  app/test/autotest_data.py |   6 ++
>  app/test/meson.build      |   2 +
>  app/test/test_devargs.c   | 147 ++++++++++++++++++++++++++++++++++++++
>  3 files changed, 155 insertions(+)
>  create mode 100644 app/test/test_devargs.c
>
> diff --git a/app/test/autotest_data.py b/app/test/autotest_data.py
> index 302d6374c16..3b841e71918 100644
> --- a/app/test/autotest_data.py
> +++ b/app/test/autotest_data.py
> @@ -785,6 +785,12 @@
>          "Func":    default_autotest,
>          "Report":  None,
>      },
> +    {
> +        "Name":    "Devargs autotest",
> +        "Command": "devargs_autotest",
> +        "Func":    default_autotest,
> +        "Report":  None,
> +    },
>      #
>      # Please always make sure that ring_perf is the last test!
>      #
> diff --git a/app/test/meson.build b/app/test/meson.build
> index f144d8b8ed6..de8540f6119 100644
> --- a/app/test/meson.build
> +++ b/app/test/meson.build
> @@ -42,6 +42,7 @@ test_sources = files(
>          'test_cryptodev_security_pdcp.c',
>          'test_cycles.c',
>          'test_debug.c',
> +        'test_devargs.c',
>          'test_distributor.c',
>          'test_distributor_perf.c',
>          'test_eal_flags.c',
> @@ -201,6 +202,7 @@ fast_tests = [
>          ['common_autotest', true],
>          ['cpuflags_autotest', true],
>          ['debug_autotest', true],
> +        ['devargs_autotest', true],
>          ['eal_flags_c_opt_autotest', false],
>          ['eal_flags_main_opt_autotest', false],
>          ['eal_flags_n_opt_autotest', false],
> diff --git a/app/test/test_devargs.c b/app/test/test_devargs.c
> new file mode 100644
> index 00000000000..8a173368347
> --- /dev/null
> +++ b/app/test/test_devargs.c
> @@ -0,0 +1,147 @@
> +/* SPDX-License-Identifier: BSD-3-Clause
> + * Copyright (c) 2021 NVIDIA Corporation & Affiliates
> + */
> +
> +#include <stdlib.h>
> +#include <stdio.h>
> +#include <string.h>
> +
> +#include <rte_common.h>
> +#include <rte_devargs.h>
> +#include <rte_kvargs.h>
> +
> +#include "test.h"
> +
> +/* Check layer arguments. */
> +static int
> +test_args(const char *devargs, const char *layer, const char *args, 
> const int n)
> +{
> +	struct rte_kvargs *kvlist;
> +
> +	if (n == 0) {
> +		if (args != NULL && strlen(args) > 0) {
> +			printf("rte_devargs_parse(%s) %s args parsed (not expected)\n",
> +			       devargs, layer);
> +			return -1;
> +		} else {
> +			return 0;
> +		}
> +	}
> +	if (args == NULL) {
> +		printf("rte_devargs_parse(%s) %s args not parsed\n",
> +		       devargs, layer);
> +		return -1;
> +	}
> +	kvlist = rte_kvargs_parse(args, NULL);
> +	if (kvlist == NULL) {
> +		printf("rte_devargs_parse(%s) %s_str: %s not parsed\n",
> +		       devargs, layer, args);
> +		return -1;
> +	}
> +	if ((int)kvlist->count != n) {
> +		printf("rte_devargs_parse(%s) %s_str: %s kv number %u, not %d\n",
> +		       devargs, layer, args, kvlist->count, n);
> +		return -1;
> +	}
> +	return 0;
> +}
> +
> +/* Test several valid cases */
> +static int
> +test_valid_devargs(void)
> +{
> +	static const struct {
> +		const char *devargs;
> +		int bus_kv;
> +		int class_kv;
> +		int driver_kv;
> +	} list[] = {
> +		/* Global devargs syntax: */
> +		{ "bus=pci", 1, 0, 0 },
> +		{ "class=eth", 0, 1, 0 },
> +		{ "bus=pci,addr=1:2.3/class=eth/driver=abc,k0=v0", 2, 1, 2 },
> +		{ "bus=vdev,name=/dev/file/name/class=eth", 2, 1, 0 },
> +		/* Legacy devargs syntax: */
> +		{ "1:2.3", 0, 0, 0 },
> +		{ "pci:1:2.3,k0=v0", 0, 0, 1 },
> +		{ "net_virtio_user0,iface=test,path=/dev/vhost-net,queues=1",
> +		  0, 0, 3 },

I would add here cases to verify that edge-case are properly parsed such as:

+               { "bus=vdev,name=/class/bus/path/class=eth", 2, 1, 0 },
[...]
+               { "net_virtio_user0,iface=test,path=/class/bus/,queues=1",
+                 0, 0, 3 },

To check the /class or /bus parts cannot throw off the parser (it does not currently).

Additionally, paths with multiple slashes are correct. Maybe add:

+               { "bus=vdev,name=///dblslsh/class=eth", 2, 1, 0 },

as well.

I tested all those cases without issue, it seems the parser is ok.


A second point is that the test only verifies that the numbers of kv were found.
I think this is not robust enough. Instead, I think the 'expect' part of the test
should describe exactly what each layers should be within the devargs (which bus,
class and driver are expected to be found, and the associated string).

Right now the parser could mangle part of a key-value list within the string,
recognize each layers and give incorrect strings to each layer parser.

It might be too much? I don't know. But it seems it could help ensure no error
is introduced at some point by future changes.

-- 
Gaetan Rivet