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 9BBAFA0543; Tue, 24 May 2022 19:45:15 +0200 (CEST) Received: from [217.70.189.124] (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 4395E400EF; Tue, 24 May 2022 19:45:15 +0200 (CEST) Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.129.124]) by mails.dpdk.org (Postfix) with ESMTP id CE67D400D6 for ; Tue, 24 May 2022 19:45:08 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1653414308; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: in-reply-to:in-reply-to:references:references; bh=RA+4YHwb6sMcBCJFAKGIbwIi78Jj6KgG/zKt/7CRP+o=; b=cRMDsrW2OC22hk7Q5R/RXiZKNxB2UX9GcY/AZjecAJE4SO8C03a3w9OqCJ16gJPasDw6AW /tv5bonO/3nXmYhRLWvReWzZY4JF18DQIGOeyIQPWmOBzlH+h/gKTHh2T3IZs+x35+9Dwy Z5cdsBtXGqCjrSme+poyR32//HRvIaE= Received: from mail-lf1-f70.google.com (mail-lf1-f70.google.com [209.85.167.70]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id us-mta-673-jsN-g-LyP5GvmjDzqK98Ww-1; Tue, 24 May 2022 13:45:07 -0400 X-MC-Unique: jsN-g-LyP5GvmjDzqK98Ww-1 Received: by mail-lf1-f70.google.com with SMTP id w38-20020a0565120b2600b00477b08c1730so9436146lfu.13 for ; Tue, 24 May 2022 10:45:06 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=RA+4YHwb6sMcBCJFAKGIbwIi78Jj6KgG/zKt/7CRP+o=; b=mh13B6OgxHwQBo6CuYiXcd66e4GSGk/sliU53OvcPYB6PVTjQ3B/fJwxfwzu3DeSiC Ed1lmKUDejBX1Akw10ljB0KE81v1kNDk1x705LJwMjSJsmAlVg1dRHbXToOvwCkHzu3r smcTjfPLlot2pcwvKn5ImleDhZqxihMHz9tjpXDLRNwtNKOXlgnbJ4tE5bi96TaHGJhi WJ7HMGBQgAdbmS+wQRAeMlQG1In7AJ6CK8h7UFY3R6o5AIp/CgaPzPn2lSPa4u9bK+GQ Lg4cDyFVQEsUe/L4WWH7+LDsJj1o/j2n/l4GcCdxoLv6JTLqE3MaA11hhsISEIxlB3xi qgbQ== X-Gm-Message-State: AOAM532tkTh6nJTXNy7OF3hDQjP8j9MU8wH/3c+VMOhjTfXpc0Zwv2Kr U47c03h1SdkO//NKZZGKb93rZzSsJl6trpr/s7t+9UN+y6ymEiu48u0iF4LFbSqniUVqth1lWix bY47Bg8ALr7ZBY/CVwtY= X-Received: by 2002:a05:6512:3183:b0:473:dffc:18ac with SMTP id i3-20020a056512318300b00473dffc18acmr19749409lfe.217.1653414303033; Tue, 24 May 2022 10:45:03 -0700 (PDT) X-Google-Smtp-Source: ABdhPJyDp2ZhLDnHUjMyTPLzSdFRyX3kat3iEhzbNdW1Z5qrLXOqUpFOJQTXAootrjdumnwQywisDP03PntBhJcoDW0= X-Received: by 2002:a05:6512:3183:b0:473:dffc:18ac with SMTP id i3-20020a056512318300b00473dffc18acmr19749386lfe.217.1653414302780; Tue, 24 May 2022 10:45:02 -0700 (PDT) MIME-Version: 1.0 References: <20220513075718.18674-1-david.marchand@redhat.com> <20220523071031.1868862-1-david.marchand@redhat.com> <20220523071031.1868862-3-david.marchand@redhat.com> <2203012.AOvM4ru3NT@thomas> In-Reply-To: <2203012.AOvM4ru3NT@thomas> From: David Marchand Date: Tue, 24 May 2022 19:44:51 +0200 Message-ID: Subject: Re: [PATCH 2/6] app/testpmd: register driver specific commands To: Thomas Monjalon Cc: dev , Andrew Rybchenko , Ferruh Yigit , Xiaoyun Li , Aman Singh , Yuying Zhang , Bruce Richardson , spiked@nvidia.com Authentication-Results: relay.mimecast.com; auth=pass smtp.auth=CUSA124A263 smtp.mailfrom=dmarchan@redhat.com X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset="UTF-8" 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 On Tue, May 24, 2022 at 7:21 PM Thomas Monjalon wrote: > > 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. Yes, it will be fixed in next revision. > > [...] > > +# Driver specific sources include some testpmd headers. > > Suggested reword: > Driver-specific commands are located in driver directories. At the moment, it's only about testpmd commands. Maybe in the future we can have other specific code in those driver sources. That's why I preferred a generic term. > > > +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? It can probably be removed. > > > +#define TESTPMD_ADD_DRIVER_COMMANDS(c) \ > > Why not TESTPMD_ADD_COMMANDS? I forgot to align both the function and macro. Please note though that for now, the registered commands through this API get displayed in the "Driver specific" usage section. So maybe the API should state it is about driver specific commands. WDYT? > > > +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? We don't want to push a driver dependency to testpmd if there is nothing to add. > > [...] > > --- 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. I'm okay with the approach. I'll prepare a new revision with only the API part, then and respin the drivers updates later. -- David Marchand