Re: [dpdk-dev] [PATCH v2 1/7] cmdline: make implementation opaque

[Olivier Matz <olivier.matz@6wind.com>]

Hi Dmitry,

On Fri, Jul 31, 2020 at 12:06:45AM +0300, Dmitry Kozlyuk wrote:
> struct cmdline exposes platform-specific members it contains, most
> notably struct termios that is only available on Unix. Make the
> structure opaque.
> 
> Remove tests checking struct cmdline content as meaningless.
> 
> Add cmdline_get_rdline() to access history buffer.
> The new function is currently used only in tests.
> 
> Signed-off-by: Dmitry Kozlyuk <dmitry.kozliuk@gmail.com>

First, please forgive me for the very late feedback. It is all the more
problematic because I think this patch introduces an ABI breakage, that
should have been announced.

Making cmdline struct opaque clearly goes in the right direction, as
it will prevent future ABI breakage.

In my opinion, we could accept the patch for 20.11, knowing it reduce
the risk of future ABI breakage, and that cmdline is not a core
component of DPDK. However it has to be discussed and accepted by other
people.

Else, the patch would be delayed for 21.11. From what I see from the
other patches, it looks possible to keep cmdline struct public without
breaking the existing ABI by adding #ifdef CONFIG_RTE_EXEC_ENV_WINDOWS,
is it correct?

One minor comment below:

> --- a/app/test/test_cmdline_lib.c
> +++ b/app/test/test_cmdline_lib.c
> @@ -46,22 +46,29 @@ complete_buffer(__rte_unused struct rdline *rdl,
>  static int
>  test_cmdline_parse_fns(void)
>  {
> -	struct cmdline cl;
> +	struct cmdline *cl;
> +	cmdline_parse_ctx_t ctx;
>  	int i = 0;
>  	char dst[CMDLINE_TEST_BUFSIZE];
>  
> +	cl = cmdline_new(&ctx, "prompt", -1, -1);
> +	if (cl == NULL) {
> +		printf("Error: cannot create cmdline to test parse fns!\n");
> +		return -1;
> +	}
> +
>  	if (cmdline_parse(NULL, "buffer") >= 0)
>  		goto error;
> -	if (cmdline_parse(&cl, NULL) >= 0)
> +	if (cmdline_parse(cl, NULL) >= 0)
>  		goto error;
>  
>  	if (cmdline_complete(NULL, "buffer", &i, dst, sizeof(dst)) >= 0)
>  		goto error;
> -	if (cmdline_complete(&cl, NULL, &i, dst, sizeof(dst)) >= 0)
> +	if (cmdline_complete(cl, NULL, &i, dst, sizeof(dst)) >= 0)
>  		goto error;
> -	if (cmdline_complete(&cl, "buffer", NULL, dst, sizeof(dst)) >= 0)
> +	if (cmdline_complete(cl, "buffer", NULL, dst, sizeof(dst)) >= 0)
>  		goto error;
> -	if (cmdline_complete(&cl, "buffer", &i, NULL, sizeof(dst)) >= 0)
> +	if (cmdline_complete(cl, "buffer", &i, NULL, sizeof(dst)) >= 0)
>  		goto error;
>  
>  	return 0;
> @@ -166,11 +173,11 @@ static int
>  test_cmdline_fns(void)
>  {
>  	cmdline_parse_ctx_t ctx;
> -	struct cmdline cl, *tmp;
> +	struct cmdline *cl;
>  
>  	memset(&ctx, 0, sizeof(ctx));
> -	tmp = cmdline_new(&ctx, "test", -1, -1);
> -	if (tmp == NULL)
> +	cl = cmdline_new(&ctx, "test", -1, -1);
> +	if (cl == NULL)
>  		goto error;
>  
>  	if (cmdline_new(NULL, "prompt", 0, 0) != NULL)
> @@ -179,7 +186,7 @@ test_cmdline_fns(void)
>  		goto error;
>  	if (cmdline_in(NULL, "buffer", CMDLINE_TEST_BUFSIZE) >= 0)
>  		goto error;
> -	if (cmdline_in(&cl, NULL, CMDLINE_TEST_BUFSIZE) >= 0)
> +	if (cmdline_in(cl, NULL, CMDLINE_TEST_BUFSIZE) >= 0)
>  		goto error;
>  	if (cmdline_write_char(NULL, 0) >= 0)
>  		goto error;
> @@ -188,31 +195,14 @@ test_cmdline_fns(void)
>  	cmdline_set_prompt(NULL, "prompt");
>  	cmdline_free(NULL);
>  	cmdline_printf(NULL, "format");
> -	/* this should fail as stream handles are invalid */
> -	cmdline_printf(tmp, "format");
>  	cmdline_interact(NULL);
>  	cmdline_quit(NULL);
>  
> -	/* check if void calls change anything when they should fail */
> -	cl = *tmp;
> -
> -	cmdline_printf(&cl, NULL);
> -	if (memcmp(&cl, tmp, sizeof(cl))) goto mismatch;
> -	cmdline_set_prompt(&cl, NULL);
> -	if (memcmp(&cl, tmp, sizeof(cl))) goto mismatch;
> -	cmdline_in(&cl, NULL, CMDLINE_TEST_BUFSIZE);
> -	if (memcmp(&cl, tmp, sizeof(cl))) goto mismatch;
> -
> -	cmdline_free(tmp);
> -
>  	return 0;
>  
>  error:
>  	printf("Error: function accepted null parameter!\n");
>  	return -1;
> -mismatch:
> -	printf("Error: data changed!\n");
> -	return -1;

cmdline_free(cl) is missing.

before your patch, cmdline_free(tmp) was already missing in error case
by the way.


Thanks,
Olivier