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

["Xueming(Steven) Li" <xuemingl@nvidia.com>]

On Tue, 2021-10-19 at 17:07 +0200, Gaëtan Rivet wrote:
> 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.
> 

Good suggestion, will added them in v2, thanks!