[v4,3/3] test/devargs: add devargs test cases
Checks
Commit Message
Initial version to test global devargs syntax.
Signed-off-by: Xueming Li <xuemingl@nvidia.com>
---
app/test/meson.build | 5 ++
app/test/test_devargs.c | 184 ++++++++++++++++++++++++++++++++++++++++
2 files changed, 189 insertions(+)
create mode 100644 app/test/test_devargs.c
Comments
On Wed, Oct 20, 2021, at 13:12, Xueming Li wrote:
> Initial version to test global devargs syntax.
>
> Signed-off-by: Xueming Li <xuemingl@nvidia.com>
> ---
> app/test/meson.build | 5 ++
> app/test/test_devargs.c | 184 ++++++++++++++++++++++++++++++++++++++++
> 2 files changed, 189 insertions(+)
> create mode 100644 app/test/test_devargs.c
>
> diff --git a/app/test/meson.build b/app/test/meson.build
> index a16374b7a10..c4b0241010d 100644
> --- a/app/test/meson.build
> +++ b/app/test/meson.build
> @@ -399,6 +399,11 @@ if dpdk_conf.has('RTE_NET_RING')
> fast_tests += [['latencystats_autotest', true]]
> fast_tests += [['pdump_autotest', true]]
> endif
> +if dpdk_conf.has('RTE_NET_VIRTIO')
> + test_deps += 'net_virtio'
> + test_sources += 'test_devargs.c'
> + fast_tests += [['devargs_autotest', true]]
> +endif
Hi Steven,
Thanks for adding new use-cases and expanding the expect.
The dep check for the test can be improved I think.
When the build is shared, not all built drivers will be available, even if part of the meson config. It might generate false negative when running your test.
Additionally, even if net_virtio is not built, a subset of your test remains valid.
Finally, net_virtio might not be generic enough as a driver. A simpler one could be used, such as net_ring maybe.
Instead of checking the dependencies in the meson file, you could detect support in preamble
of the test:
bool pci_bus_avail = (rte_bus_find_by_name("pci") != NULL);
bool vdev_bus_avail = (rte_bus_find_by_name("vdev") != NULL);
bool eth_class_avail = [...]
Then during the test case, depending on the expect part of list[i],
you can skip the case if its deps were not found. In that case, am INFO or DEBUG level
message could be printed to say that the test was skipped.
On Wed, 2021-10-20 at 13:55 +0200, Gaëtan Rivet wrote:
> On Wed, Oct 20, 2021, at 13:12, Xueming Li wrote:
> > Initial version to test global devargs syntax.
> >
> > Signed-off-by: Xueming Li <xuemingl@nvidia.com>
> > ---
> > app/test/meson.build | 5 ++
> > app/test/test_devargs.c | 184 ++++++++++++++++++++++++++++++++++++++++
> > 2 files changed, 189 insertions(+)
> > create mode 100644 app/test/test_devargs.c
> >
> > diff --git a/app/test/meson.build b/app/test/meson.build
> > index a16374b7a10..c4b0241010d 100644
> > --- a/app/test/meson.build
> > +++ b/app/test/meson.build
> > @@ -399,6 +399,11 @@ if dpdk_conf.has('RTE_NET_RING')
> > fast_tests += [['latencystats_autotest', true]]
> > fast_tests += [['pdump_autotest', true]]
> > endif
> > +if dpdk_conf.has('RTE_NET_VIRTIO')
> > + test_deps += 'net_virtio'
> > + test_sources += 'test_devargs.c'
> > + fast_tests += [['devargs_autotest', true]]
> > +endif
>
> Hi Steven,
>
> Thanks for adding new use-cases and expanding the expect.
> The dep check for the test can be improved I think.
>
> When the build is shared, not all built drivers will be available, even if part of the meson config. It might generate false negative when running your test.
>
> Additionally, even if net_virtio is not built, a subset of your test remains valid.
>
> Finally, net_virtio might not be generic enough as a driver. A simpler one could be used, such as net_ring maybe.
>
> Instead of checking the dependencies in the meson file, you could detect support in preamble
> of the test:
>
> bool pci_bus_avail = (rte_bus_find_by_name("pci") != NULL);
> bool vdev_bus_avail = (rte_bus_find_by_name("vdev") != NULL);
> bool eth_class_avail = [...]
>
> Then during the test case, depending on the expect part of list[i],
> you can skip the case if its deps were not found. In that case, am INFO or DEBUG level
> message could be printed to say that the test was skipped.
>
For vdev supported drivers, currently no API to find it. Macro worked
for me:
#ifdef RTE_NET_VIRTIO
{ "net_virtio_user0,iface=test,path=/dev/vhost-
net,queues=1",
0, 0, 3, "vdev", "net_virtio_user0", NULL },
{
"net_virtio_user0,iface=test,path=/class/bus/,queues=1",
0, 0, 3, "vdev", "net_virtio_user0", NULL },
#endif
PCI, vdev and eth is enabled by default, seems we don't need any flag.
But macros can be used as well to be safe, how do you think?
On Wed, Oct 20, 2021, at 15:53, Xueming(Steven) Li wrote:
> On Wed, 2021-10-20 at 13:55 +0200, Gaëtan Rivet wrote:
>> On Wed, Oct 20, 2021, at 13:12, Xueming Li wrote:
>> > Initial version to test global devargs syntax.
>> >
>> > Signed-off-by: Xueming Li <xuemingl@nvidia.com>
>> > ---
>> > app/test/meson.build | 5 ++
>> > app/test/test_devargs.c | 184 ++++++++++++++++++++++++++++++++++++++++
>> > 2 files changed, 189 insertions(+)
>> > create mode 100644 app/test/test_devargs.c
>> >
>> > diff --git a/app/test/meson.build b/app/test/meson.build
>> > index a16374b7a10..c4b0241010d 100644
>> > --- a/app/test/meson.build
>> > +++ b/app/test/meson.build
>> > @@ -399,6 +399,11 @@ if dpdk_conf.has('RTE_NET_RING')
>> > fast_tests += [['latencystats_autotest', true]]
>> > fast_tests += [['pdump_autotest', true]]
>> > endif
>> > +if dpdk_conf.has('RTE_NET_VIRTIO')
>> > + test_deps += 'net_virtio'
>> > + test_sources += 'test_devargs.c'
>> > + fast_tests += [['devargs_autotest', true]]
>> > +endif
>>
>> Hi Steven,
>>
>> Thanks for adding new use-cases and expanding the expect.
>> The dep check for the test can be improved I think.
>>
>> When the build is shared, not all built drivers will be available, even if part of the meson config. It might generate false negative when running your test.
>>
>> Additionally, even if net_virtio is not built, a subset of your test remains valid.
>>
>> Finally, net_virtio might not be generic enough as a driver. A simpler one could be used, such as net_ring maybe.
>>
>> Instead of checking the dependencies in the meson file, you could detect support in preamble
>> of the test:
>>
>> bool pci_bus_avail = (rte_bus_find_by_name("pci") != NULL);
>> bool vdev_bus_avail = (rte_bus_find_by_name("vdev") != NULL);
>> bool eth_class_avail = [...]
>>
>> Then during the test case, depending on the expect part of list[i],
>> you can skip the case if its deps were not found. In that case, am INFO or DEBUG level
>> message could be printed to say that the test was skipped.
>>
>
> For vdev supported drivers, currently no API to find it. Macro worked
> for me:
>
> #ifdef RTE_NET_VIRTIO
> { "net_virtio_user0,iface=test,path=/dev/vhost-
> net,queues=1",
> 0, 0, 3, "vdev", "net_virtio_user0", NULL },
> {
> "net_virtio_user0,iface=test,path=/class/bus/,queues=1",
> 0, 0, 3, "vdev", "net_virtio_user0", NULL },
> #endif
>
> PCI, vdev and eth is enabled by default, seems we don't need any flag.
> But macros can be used as well to be safe, how do you think?
With the macro you will go back to the same issue the meson-based dep check has:
the macro will be set, but it's possible the relevant .so won't be loaded when
executing the test.
For Busses and classes, similarly is there a chance that even though they are built,
they won't be loaded at runtime and make the test result incorrect?
What do you think about switching from net_virtio to net_ring instead otherwise?
It seems a lighter, utility driver that might be present even in minimal builds.
It is used in EAL tests, I think it makes more sense than net_virtio for unit tests.
The virtio-specific path-based parameters are irrelevant there, the actual
driver probe won't be executed anyway so any parameter looking like a path will
test properly.
Checkout app/test/test_eal_flags.c for examples of net_ring dummy devargs.
On Wed, Oct 20, 2021 at 04:22:54PM +0200, Gaëtan Rivet wrote:
>
>
> On Wed, Oct 20, 2021, at 15:53, Xueming(Steven) Li wrote:
> > On Wed, 2021-10-20 at 13:55 +0200, Gaëtan Rivet wrote:
> >> On Wed, Oct 20, 2021, at 13:12, Xueming Li wrote:
> >> > Initial version to test global devargs syntax.
> >> >
> >> > Signed-off-by: Xueming Li <xuemingl@nvidia.com>
> >> > ---
> >> > app/test/meson.build | 5 ++
> >> > app/test/test_devargs.c | 184 ++++++++++++++++++++++++++++++++++++++++
> >> > 2 files changed, 189 insertions(+)
> >> > create mode 100644 app/test/test_devargs.c
> >> >
> >> > diff --git a/app/test/meson.build b/app/test/meson.build
> >> > index a16374b7a10..c4b0241010d 100644
> >> > --- a/app/test/meson.build
> >> > +++ b/app/test/meson.build
> >> > @@ -399,6 +399,11 @@ if dpdk_conf.has('RTE_NET_RING')
> >> > fast_tests += [['latencystats_autotest', true]]
> >> > fast_tests += [['pdump_autotest', true]]
> >> > endif
> >> > +if dpdk_conf.has('RTE_NET_VIRTIO')
> >> > + test_deps += 'net_virtio'
> >> > + test_sources += 'test_devargs.c'
> >> > + fast_tests += [['devargs_autotest', true]]
> >> > +endif
> >>
> >> Hi Steven,
> >>
> >> Thanks for adding new use-cases and expanding the expect.
> >> The dep check for the test can be improved I think.
> >>
> >> When the build is shared, not all built drivers will be available, even if part of the meson config. It might generate false negative when running your test.
> >>
> >> Additionally, even if net_virtio is not built, a subset of your test remains valid.
> >>
> >> Finally, net_virtio might not be generic enough as a driver. A simpler one could be used, such as net_ring maybe.
> >>
> >> Instead of checking the dependencies in the meson file, you could detect support in preamble
> >> of the test:
> >>
> >> bool pci_bus_avail = (rte_bus_find_by_name("pci") != NULL);
> >> bool vdev_bus_avail = (rte_bus_find_by_name("vdev") != NULL);
> >> bool eth_class_avail = [...]
> >>
> >> Then during the test case, depending on the expect part of list[i],
> >> you can skip the case if its deps were not found. In that case, am INFO or DEBUG level
> >> message could be printed to say that the test was skipped.
> >>
> >
> > For vdev supported drivers, currently no API to find it. Macro worked
> > for me:
> >
> > #ifdef RTE_NET_VIRTIO
> > { "net_virtio_user0,iface=test,path=/dev/vhost-
> > net,queues=1",
> > 0, 0, 3, "vdev", "net_virtio_user0", NULL },
> > {
> > "net_virtio_user0,iface=test,path=/class/bus/,queues=1",
> > 0, 0, 3, "vdev", "net_virtio_user0", NULL },
> > #endif
> >
> > PCI, vdev and eth is enabled by default, seems we don't need any flag.
> > But macros can be used as well to be safe, how do you think?
>
> With the macro you will go back to the same issue the meson-based dep check has:
> the macro will be set, but it's possible the relevant .so won't be loaded when
> executing the test.
>
> For Busses and classes, similarly is there a chance that even though they are built,
> they won't be loaded at runtime and make the test result incorrect?
>
> What do you think about switching from net_virtio to net_ring instead otherwise?
> It seems a lighter, utility driver that might be present even in minimal builds.
> It is used in EAL tests, I think it makes more sense than net_virtio for unit tests.
>
> The virtio-specific path-based parameters are irrelevant there, the actual
> driver probe won't be executed anyway so any parameter looking like a path will
> test properly.
>
> Checkout app/test/test_eal_flags.c for examples of net_ring dummy devargs.
>
I'd also recommend making use of the "TEST_SKIPPED" return value rather
than erroring out if a necessary dependency can't be found.
@@ -399,6 +399,11 @@ if dpdk_conf.has('RTE_NET_RING')
fast_tests += [['latencystats_autotest', true]]
fast_tests += [['pdump_autotest', true]]
endif
+if dpdk_conf.has('RTE_NET_VIRTIO')
+ test_deps += 'net_virtio'
+ test_sources += 'test_devargs.c'
+ fast_tests += [['devargs_autotest', true]]
+endif
if dpdk_conf.has('RTE_LIB_POWER')
test_deps += 'power'
new file mode 100644
@@ -0,0 +1,184 @@
+/* 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 <rte_bus.h>
+#include <rte_class.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;
+ const char *bus;
+ const char *name;
+ const char *class;
+ } list[] = {
+ /* Global devargs syntax: */
+ { "bus=pci",
+ 1, 0, 0, "pci", NULL, NULL},
+ { "class=eth",
+ 0, 1, 0, NULL, NULL, "eth" },
+ { "bus=pci,addr=1:2.3/class=eth/driver=abc,k0=v0",
+ 2, 1, 2, "pci", "0000:01:02.3", "eth" },
+ { "bus=vdev,name=/dev/file/name/class=eth",
+ 2, 1, 0, "vdev", "/dev/file/name", "eth" },
+ { "bus=vdev,name=/class/bus/path/class=eth",
+ 2, 1, 0, "vdev", "/class/bus/path", "eth" },
+ { "bus=vdev,name=///dblslsh/class=eth",
+ 2, 1, 0, "vdev", "///dblslsh", "eth" },
+ /* Legacy devargs syntax: */
+ { "1:2.3", 0, 0, 0,
+ "pci", "1:2.3", NULL },
+ { "pci:1:2.3,k0=v0",
+ 0, 0, 1, "pci", "1:2.3", NULL },
+ { "net_virtio_user0,iface=test,path=/dev/vhost-net,queues=1",
+ 0, 0, 3, "vdev", "net_virtio_user0", NULL },
+ { "net_virtio_user0,iface=test,path=/class/bus/,queues=1",
+ 0, 0, 3, "vdev", "net_virtio_user0", NULL },
+ };
+ struct rte_devargs da;
+ uint32_t i;
+ int ret;
+ int fail = 0;
+
+ for (i = 0; i < RTE_DIM(list); i++) {
+ memset(&da, 0, sizeof(da));
+ ret = rte_devargs_parse(&da, list[i].devargs);
+ if (ret < 0) {
+ printf("rte_devargs_parse(%s) returned %d (but should not)\n",
+ list[i].devargs, ret);
+ goto fail;
+ }
+ if ((list[i].bus_kv > 0 || list[i].bus != NULL) &&
+ da.bus == NULL) {
+ printf("rte_devargs_parse(%s) bus not parsed\n",
+ list[i].devargs);
+ goto fail;
+ }
+ if (test_args(list[i].devargs, "bus", da.bus_str,
+ list[i].bus_kv) != 0)
+ goto fail;
+ if (list[i].bus != NULL &&
+ strcmp(da.bus->name, list[i].bus) != 0) {
+ printf("rte_devargs_parse(%s) bus name (%s) not expected (%s)\n",
+ list[i].devargs, da.bus->name, list[i].bus);
+ goto fail;
+ }
+ if ((list[i].class_kv > 0 || list[i].class != NULL) &&
+ da.cls == NULL) {
+ printf("rte_devargs_parse(%s) class not parsed\n",
+ list[i].devargs);
+ goto fail;
+ }
+ if (test_args(list[i].devargs, "class", da.cls_str,
+ list[i].class_kv) != 0)
+ goto fail;
+ if (list[i].class != NULL &&
+ strcmp(da.cls->name, list[i].class) != 0) {
+ printf("rte_devargs_parse(%s) class name (%s) not expected (%s)\n",
+ list[i].devargs, da.cls->name, list[i].class);
+ goto fail;
+ }
+ if (test_args(list[i].devargs, "driver", da.drv_str,
+ list[i].driver_kv) != 0)
+ goto fail;
+ if (list[i].name != NULL &&
+ strcmp(da.name, list[i].name) != 0) {
+ printf("rte_devargs_parse(%s) device name (%s) not expected (%s)\n",
+ list[i].devargs, da.name, list[i].name);
+ goto fail;
+ }
+ goto cleanup;
+fail:
+ fail = -1;
+cleanup:
+ rte_devargs_reset(&da);
+ }
+ return fail;
+}
+
+/* Test several invalid cases */
+static int
+test_invalid_devargs(void)
+{
+ static const char * const list[] = {
+ "bus=wrong-bus",
+ "class=wrong-class"};
+ struct rte_devargs da;
+ uint32_t i;
+ int ret;
+ int fail = 0;
+
+ for (i = 0; i < RTE_DIM(list); i++) {
+ ret = rte_devargs_parse(&da, list[i]);
+ if (ret >= 0) {
+ printf("rte_devargs_parse(%s) returned %d (but should not)\n",
+ list[i], ret);
+ fail = ret;
+ }
+ rte_devargs_reset(&da);
+ }
+ return fail;
+}
+
+static int
+test_devargs(void)
+{
+ printf("== test valid case ==\n");
+ if (test_valid_devargs() < 0)
+ return -1;
+ printf("== test invalid case ==\n");
+ if (test_invalid_devargs() < 0)
+ return -1;
+ return 0;
+}
+
+REGISTER_TEST_COMMAND(devargs_autotest, test_devargs);