Re: [PATCH] app/testpmd: fix auto completion for indirect list action

[Ferruh Yigit <ferruh.yigit@amd.com>]

On 3/18/2024 9:21 AM, Shani Peretz wrote:
> In the process of auto completion of a command in testpmd,
> the parser splits the command into tokens, where each token
> represents an argument and defines a parsing function.
> The parsing function of the indirect_list action argument was returning
> before having the opportunity to handle the argument.
> 

Hi Shani,

I can see a few other handles follows the updated logic, but to
understand more, was the problematic part following:
```
	if (!action)
		return -1;
```

If so why 'action' can be NULL and why need to continue for this case,
can you please help me understand?

Also even if 'action' is NULL, function will return output of
'parse_int()', is this expected?


Thanks,
ferruh

> The fix ensures that the function appropriately handles
> the argument before finishing.
> 
> Fixes: 72a3dec7126f ("ethdev: add indirect flow list action")
> 
> Signed-off-by: Shani Peretz <shperetz@nvidia.com>
> ---
>  app/test-pmd/cmdline_flow.c | 46 ++++++++++++++++++++-----------------
>  1 file changed, 25 insertions(+), 21 deletions(-)
> 
> diff --git a/app/test-pmd/cmdline_flow.c b/app/test-pmd/cmdline_flow.c
> index fd6c51f72d..60ee9337cf 100644
> --- a/app/test-pmd/cmdline_flow.c
> +++ b/app/test-pmd/cmdline_flow.c
> @@ -7839,11 +7839,13 @@ static const struct token token_list[] = {
>  		.type = "UNSIGNED",
>  		.help = "unsigned integer value",
>  		.call = parse_indlst_id2ptr,
> +		.comp = comp_none,
>  	},
>  	[INDIRECT_LIST_ACTION_ID2PTR_CONF] = {
>  		.type = "UNSIGNED",
>  		.help = "unsigned integer value",
>  		.call = parse_indlst_id2ptr,
> +		.comp = comp_none,
>  	},
>  	[ACTION_SHARED_INDIRECT] = {
>  		.name = "shared_indirect",
> @@ -11912,34 +11914,36 @@ parse_indlst_id2ptr(struct context *ctx, const struct token *token,
>  	uint32_t id;
>  	int ret;
>  
> -	if (!action)
> -		return -1;
>  	ctx->objdata = 0;
>  	ctx->object = &id;
>  	ctx->objmask = NULL;
>  	ret = parse_int(ctx, token, str, len, ctx->object, sizeof(id));
> +	ctx->object = action;
>  	if (ret != (int)len)
>  		return ret;
> -	ctx->object = action;
> -	action_conf = (void *)(uintptr_t)action->conf;
> -	action_conf->conf = NULL;
> -	switch (ctx->curr) {
> -	case INDIRECT_LIST_ACTION_ID2PTR_HANDLE:
> -	action_conf->handle = (typeof(action_conf->handle))
> -				port_action_handle_get_by_id(ctx->port, id);
> -		if (!action_conf->handle) {
> -			printf("no indirect list handle for id %u\n", id);
> -			return -1;
> +
> +	/* set handle and conf */
> +	if (action) {
> +		action_conf = (void *)(uintptr_t)action->conf;
> +		action_conf->conf = NULL;
> +		switch (ctx->curr) {
> +		case INDIRECT_LIST_ACTION_ID2PTR_HANDLE:
> +		action_conf->handle = (typeof(action_conf->handle))
> +					port_action_handle_get_by_id(ctx->port, id);
> +			if (!action_conf->handle) {
> +				printf("no indirect list handle for id %u\n", id);
> +				return -1;
> +			}
> +			break;
> +		case INDIRECT_LIST_ACTION_ID2PTR_CONF:
> +			indlst_conf = indirect_action_list_conf_get(id);
> +			if (!indlst_conf)
> +				return -1;
> +			action_conf->conf = (const void **)indlst_conf->conf;
> +			break;
> +		default:
> +			break;
>  		}
> -		break;
> -	case INDIRECT_LIST_ACTION_ID2PTR_CONF:
> -		indlst_conf = indirect_action_list_conf_get(id);
> -		if (!indlst_conf)
> -			return -1;
> -		action_conf->conf = (const void **)indlst_conf->conf;
> -		break;
> -	default:
> -		break;
>  	}
>  	return ret;
>  }