From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mails.dpdk.org (mails.dpdk.org [217.70.189.124]) by inbox.dpdk.org (Postfix) with ESMTP id 32EE2A0C41; Tue, 19 Oct 2021 17:07:45 +0200 (CEST) Received: from [217.70.189.124] (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 1B49D41164; Tue, 19 Oct 2021 17:07:45 +0200 (CEST) Received: from out3-smtp.messagingengine.com (out3-smtp.messagingengine.com [66.111.4.27]) by mails.dpdk.org (Postfix) with ESMTP id C46284111D for ; Tue, 19 Oct 2021 17:07:43 +0200 (CEST) Received: from compute2.internal (compute2.nyi.internal [10.202.2.42]) by mailout.nyi.internal (Postfix) with ESMTP id 15F935C01E4; Tue, 19 Oct 2021 11:07:43 -0400 (EDT) Received: from imap48 ([10.202.2.98]) by compute2.internal (MEProxy); Tue, 19 Oct 2021 11:07:43 -0400 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=u256.net; h= mime-version:message-id:in-reply-to:references:date:from:to:cc :subject:content-type; s=fm3; bh=cJaG3FQPzGk7zOrfrGeh0/FA8zx2WM+ HYzXMYo1hoQk=; b=D8b2iZeyV3D/CGdAt/x2KhTIwiD7rPMz8A/ddryteSa1pbZ r6kM3TxbQnW5PIpRS+snSwvvu41Rfsre+738/qVrta/OaL9XXSGvbdOC/3JUIUgD qr5q9Q4BC7CJEQ9cF0AHwz9w52yYnFf4J2BToVPBdSrjP1iKP70HQj11iLe3/vGP mUXOoWxt0RhWvTX9afPsFVshIFy21Nz+3GxVubBTu0RLdAEURaCpS8cEy0ufbt2m tE3qIBfSt9/rdqE6qJ95ttjxusNn9CMQ6U9KAPXSi3yN3Lz+olyPsQFRMIlgghBI 0L55k9DfNeDhLEf2Dmv/kEcaw/2LG4uHDnjsuqg== DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d= messagingengine.com; h=cc:content-type:date:from:in-reply-to :message-id:mime-version:references:subject:to:x-me-proxy :x-me-proxy:x-me-sender:x-me-sender:x-sasl-enc; s=fm1; bh=cJaG3F QPzGk7zOrfrGeh0/FA8zx2WM+HYzXMYo1hoQk=; b=dIAZiL1YNzE+/IpOuRER5H QpWUayPwbMLB5bcs0SXOhcz1h78KD+bUacTjrFhtMIv2EXAQuKvBd0kkY1DlalBi ry0J3+D7E38i2W1qqQQGcwdkTIW98r2XGCIFa6sY+b51NZkKJS5lBmIwuU4ZI+a4 YZUmtoh/QfctVvlanCIBxxDq0m8rr48xfgHUlaF7Nz7eHkpM5zVEEblrVkDz75xS BFMzzWKfHWO2I2FIsHxEGre3s2utA+acoeO0fDOfHuqLH8QPHBhT6k2NusXLXuSp 3PZZoN+KhXHSMwUvjYiiyookliMl+vQmb5MoNRswWxUxkZBsdsOLMbQ14kPlo6qw == X-ME-Sender: X-ME-Proxy-Cause: gggruggvucftvghtrhhoucdtuddrgedvtddrvddvvddgkedtucetufdoteggodetrfdotf fvucfrrhhofhhilhgvmecuhfgrshhtofgrihhlpdfqfgfvpdfurfetoffkrfgpnffqhgen uceurghilhhouhhtmecufedttdenucesvcftvggtihhpihgvnhhtshculddquddttddmne cujfgurhepofgfggfkjghffffhvffutgesthdtredtreerjeenucfhrhhomhepifgrtoht rghnpgftihhvvghtuceoghhrihhvvgesuhdvheeirdhnvghtqeenucggtffrrghtthgvrh hnpefguefhffelleduueehueetueefgedtieevudduhfegveetuedtleeuffeuheekfeen ucevlhhushhtvghrufhiiigvpedtnecurfgrrhgrmhepmhgrihhlfhhrohhmpehgrhhivh gvsehuvdehiedrnhgvth X-ME-Proxy: Received: by mailuser.nyi.internal (Postfix, from userid 501) id A922121E006E; Tue, 19 Oct 2021 11:07:42 -0400 (EDT) X-Mailer: MessagingEngine.com Webmail Interface User-Agent: Cyrus-JMAP/3.5.0-alpha0-1369-gd055fb5e7c-fm-20211018.002-gd055fb5e Mime-Version: 1.0 Message-Id: In-Reply-To: <20211005155435.279043-4-xuemingl@nvidia.com> References: <20211005123012.264727-1-xuemingl@nvidia.com> <20211005155435.279043-1-xuemingl@nvidia.com> <20211005155435.279043-4-xuemingl@nvidia.com> Date: Tue, 19 Oct 2021 17:07:22 +0200 From: =?UTF-8?Q?Ga=C3=ABtan_Rivet?= To: "Xueming(Steven) Li" , dev@dpdk.org Cc: "Thomas Monjalon" , "David Marchand" Content-Type: text/plain Subject: Re: [dpdk-dev] [PATCH v1 3/3] test/devargs: add devargs test cases X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" On Tue, Oct 5, 2021, at 17:54, Xueming Li wrote: > Initial version to test Global devargs syntax. > > Signed-off-by: Xueming Li 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 > +#include > +#include > + > +#include > +#include > +#include > + > +#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