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 A8918A0540; Tue, 24 May 2022 19:21:41 +0200 (CEST) Received: from [217.70.189.124] (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 66BBD400EF; Tue, 24 May 2022 19:21:41 +0200 (CEST) Received: from wout3-smtp.messagingengine.com (wout3-smtp.messagingengine.com [64.147.123.19]) by mails.dpdk.org (Postfix) with ESMTP id 3A2A7400D6 for ; Tue, 24 May 2022 19:21:40 +0200 (CEST) Received: from compute3.internal (compute3.nyi.internal [10.202.2.43]) by mailout.west.internal (Postfix) with ESMTP id 07388320029B; Tue, 24 May 2022 13:21:37 -0400 (EDT) Received: from mailfrontend2 ([10.202.2.163]) by compute3.internal (MEProxy); Tue, 24 May 2022 13:21:38 -0400 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=monjalon.net; h= cc:cc:content-transfer-encoding:content-type:date:date:from:from :in-reply-to:in-reply-to:message-id:mime-version:references :reply-to:sender:subject:subject:to:to; s=fm1; t=1653412897; x= 1653499297; bh=ihig5TpI2sDdZLJV3CcS129NzsO7oK/cY+zMbxwjAm0=; b=K 2KJLn5KoDFYKu3jKZNoSMvFz6bYP1hfrDXuAhIt5gbkNTKHqqD+EQgN62Y9e2IOP LRiDPygVGoPTp5acyK8HMaM24ZNqQLb3g+JL5JpITX90k0PEJ3x8R4JMAcUazqzk q05GgV7inDz57jJuFivVpPL8lYfLiaaEfbeV1sj9zjrWkld9lwCjVsntEuEJURDv oKt/xUrf+A6mOivloVqwiX9/j3GvJsy3uajQx1fJjk06IAbUZ0gzqcoGuXsp8hku XR3J4R0xxHwe29gHmW56UbKXYfTxDqcqsC9YsvlpCeODpBK4SWbDQ3zXFTzumxYj 2Lo+1QJnEoK7KF0honkEw== DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d= messagingengine.com; h=cc:cc:content-transfer-encoding :content-type:date:date:feedback-id:feedback-id:from:from :in-reply-to:in-reply-to:message-id:mime-version:references :reply-to:sender:subject:subject:to:to:x-me-proxy:x-me-proxy :x-me-sender:x-me-sender:x-sasl-enc; s=fm1; t=1653412897; x= 1653499297; bh=ihig5TpI2sDdZLJV3CcS129NzsO7oK/cY+zMbxwjAm0=; b=q s3xRSFU+7fNYo1oTQVXeYWvpL1YjzOOdLw9JaVxz0WvaFRmqi4LDh1JwuCIezpQg OnXqlYZMdXhoCZvekjYtXFBZzLYssz/TX9FgVIxHk44BPUEFXinXoSaXn99UGI6P vbUWmKJIeSV8pdsJiMb5wFvyfTQsR4kObrIQqOLTHSW72TYsnbEXxRkn9xn40jSG 16GaNgtPNYuTxTUL6ZmqHn5UxZPAgqf6Zm25+o2nHy/y6huIvw492Pr0p0MFyeq4 +JxWx8UOHDrzSE3nRsVil6WCy7DnzAM2Wp2t6IdTibcy72H7BS9DtKXiP95JN4Fn 3ybaKjrSdjI2QhekpSVbQ== X-ME-Sender: X-ME-Received: X-ME-Proxy-Cause: gggruggvucftvghtrhhoucdtuddrgedvfedrjeefgdduudduucetufdoteggodetrfdotf fvucfrrhhofhhilhgvmecuhfgrshhtofgrihhlpdfqfgfvpdfurfetoffkrfgpnffqhgen uceurghilhhouhhtmecufedttdenucesvcftvggtihhpihgvnhhtshculddquddttddmne cujfgurhephffvvefufffkjghfggfgtgesthfuredttddtvdenucfhrhhomhepvfhhohhm rghsucfoohhnjhgrlhhonhcuoehthhhomhgrshesmhhonhhjrghlohhnrdhnvghtqeenuc ggtffrrghtthgvrhhnpedtjeeiieefhedtfffgvdelteeufeefheeujefgueetfedttdei kefgkeduhedtgfenucevlhhushhtvghrufhiiigvpedtnecurfgrrhgrmhepmhgrihhlfh hrohhmpehthhhomhgrshesmhhonhhjrghlohhnrdhnvght X-ME-Proxy: Feedback-ID: i47234305:Fastmail Received: by mail.messagingengine.com (Postfix) with ESMTPA; Tue, 24 May 2022 13:21:34 -0400 (EDT) From: Thomas Monjalon To: David Marchand Cc: dev@dpdk.org, andrew.rybchenko@oktetlabs.ru, ferruh.yigit@xilinx.com, Xiaoyun Li , Aman Singh , Yuying Zhang , Bruce Richardson , spiked@nvidia.com Subject: Re: [PATCH 2/6] app/testpmd: register driver specific commands Date: Tue, 24 May 2022 19:21:27 +0200 Message-ID: <2203012.AOvM4ru3NT@thomas> In-Reply-To: <20220523071031.1868862-3-david.marchand@redhat.com> References: <20220513075718.18674-1-david.marchand@redhat.com> <20220523071031.1868862-1-david.marchand@redhat.com> <20220523071031.1868862-3-david.marchand@redhat.com> MIME-Version: 1.0 Content-Transfer-Encoding: 7Bit Content-Type: text/plain; charset="us-ascii" 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 23/05/2022 09:10, David Marchand: > Introduce a testpmd API so that drivers can register specific commands. > > A driver can list some files to compile with testpmd, by setting them > in the testpmd_sources (driver local) meson variable. > drivers/meson.build then takes care of appending this to a global meson > variable, and adding the driver to testpmd dependency. > > Note: testpmd.h is fixed to that it is self sufficient when being > included. > > Signed-off-by: David Marchand > --- [...] > + main_ctx = NULL; > + for (i = 0; builtin_ctx[i] != NULL; i++) { > + ctx = realloc(main_ctx, (count + i + 1) * sizeof(*ctx)); > + if (ctx == NULL) > + goto err; > + main_ctx = ctx; > + main_ctx[count + i] = builtin_ctx[i]; > + } > + count += i; > + > + TAILQ_FOREACH(c, &commands_head, next) { > + for (i = 0; c->commands[i].ctx != NULL; i++) { > + ctx = realloc(main_ctx, (count + i + 1) * sizeof(*ctx)); > + if (ctx == NULL) > + goto err; > + main_ctx = ctx; > + main_ctx[count + i] = c->commands[i].ctx; > + } > + count += i; > + } > + > + /* cmdline expects a NULL terminated array */ > + ctx = realloc(main_ctx, (count + 1) * sizeof(*ctx)); > + if (ctx == NULL) > + goto err; > + main_ctx = ctx; > + main_ctx[count] = NULL; > + count += 1; As Ferruh already said, there's a lot of realloc here. [...] > +# Driver specific sources include some testpmd headers. Suggested reword: Driver-specific commands are located in driver directories. > +includes = include_directories('.') > +sources += testpmd_drivers_sources > +deps += testpmd_drivers_deps [...] > --- a/app/test-pmd/testpmd.h > +++ b/app/test-pmd/testpmd.h > +/* For registering testpmd commands. */ > +struct testpmd_commands { > + TAILQ_ENTRY(testpmd_commands) next; > + struct { > + cmdline_parse_inst_t *ctx; > + const char *help; > + } commands[]; > +}; > + > +extern void testpmd_add_commands(struct testpmd_commands *c); Why extern? > +#define TESTPMD_ADD_DRIVER_COMMANDS(c) \ Why not TESTPMD_ADD_COMMANDS? > +RTE_INIT(__##c) \ > +{ \ > + testpmd_add_commands(&c); \ > +} [...] > --- a/drivers/meson.build > +++ b/drivers/meson.build > + if testpmd_sources.length() != 0 > + testpmd_drivers_sources += testpmd_sources > + testpmd_drivers_deps += dependency_name > + endif Are you sure the check is required? What happens if adding an empty string? [...] > --- a/meson.build > +++ b/meson.build > @@ -42,6 +42,8 @@ dpdk_drivers = [] > dpdk_extra_ldflags = [] > dpdk_libs_disabled = [] > dpdk_drvs_disabled = [] > +testpmd_drivers_sources = [] > +testpmd_drivers_deps = [] It's polluting a bit the global meson namespace for testpmd but I think it's worth, I like the idea. I hope we can merge the infrastucture patches soon. We can postpone a bit the move of existing drivers because some are not so obvious. At least we should make it mandatory now for all new driver-specific commands. Thanks