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

[Thomas Monjalon <thomas@monjalon.net>]

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.

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