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.march...@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.
[...]
> +# 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
