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