Re: [PATCH 2/6] app/testpmd: register driver specific commands

[David Marchand <david.marchand@redhat.com>]

On Tue, May 24, 2022 at 7:21 PM Thomas Monjalon <thomas@monjalon.net> 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 <david.marchand@redhat.com>
> > ---
> [...]
> > +     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