Re: [dpdk-dev] [PATCH v2] lib/cmdline: init CLI parsing memory

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

Hi Xueming,

Sorry for the late response.

On Tue, Dec 26, 2017 at 12:57:41PM +0000, Xueming(Steven) Li wrote:
> HI Olivier,
> 
> By reading p1 comments carefully, looks like the pointer to result buffer issue
> not resolved by result copy. How about this:
> 
> @@ -263,6 +263,7 @@
>  #ifdef RTE_LIBRTE_CMDLINE_DEBUG
>  	char debug_buf[BUFSIZ];
>  #endif
> +	char *result_buf = result.buf;
>  
>  	if (!cl || !buf)
>  		return CMDLINE_PARSE_BAD_ARGS;
> @@ -312,16 +313,13 @@
>  		debug_printf("INST %d\n", inst_num);
>  
>  		/* fully parsed */
> -		tok = match_inst(inst, buf, 0, tmp_result.buf,
> -				 sizeof(tmp_result.buf));
> +		tok = match_inst(inst, buf, 0, result_buf, sizeof(result.buf));

If we don't use tmp_result, it is maybe better to also replace
sizeof(result.buf) by CMDLINE_PARSE_RESULT_BUFSIZE

>  
>  		if (tok > 0) /* we matched at least one token */
>  			err = CMDLINE_PARSE_BAD_ARGS;
>  
>  		else if (!tok) {
>  			debug_printf("INST fully parsed\n");
> -			memcpy(&result, &tmp_result,
> -			       sizeof(result));
>  			/* skip spaces */
>  			while (isblank2(*curbuf)) {
>  				curbuf++;
> @@ -332,6 +330,7 @@
>  				if (!f) {
>  					memcpy(&f, &inst->f, sizeof(f));
>  					memcpy(&data, &inst->data, sizeof(data));
> +					result_buf = tmp_result.buf;
>  				}
>  				else {
>  					/* more than 1 inst matches */
> 


I guess the issue you are talking about is the one described in
"cmdline: fix dynamic tokens parsing" in my previous description?

I think this patch is ok, and is probably better than the initial
suggestion, because it avoids to copy the buffer. However, I don't
understand why the previous patch was wrong, can you detail?

Thanks
Olivier