From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <olivier.matz@6wind.com>
Received: from mail.droids-corp.org (zoll.droids-corp.org [94.23.50.67])
 by dpdk.org (Postfix) with ESMTP id EF4001B167
 for <dev@dpdk.org>; Tue, 16 Jan 2018 13:45:52 +0100 (CET)
Received: from lfbn-lil-1-110-231.w90-45.abo.wanadoo.fr ([90.45.197.231]
 helo=droids-corp.org)
 by mail.droids-corp.org with esmtpsa (TLS1.0:RSA_AES_256_CBC_SHA1:256)
 (Exim 4.89) (envelope-from <olivier.matz@6wind.com>)
 id 1ebQcl-0008Rn-7J; Tue, 16 Jan 2018 13:45:56 +0100
Received: by droids-corp.org (sSMTP sendmail emulation);
 Tue, 16 Jan 2018 13:45:49 +0100
Date: Tue, 16 Jan 2018 13:45:49 +0100
From: Olivier Matz <olivier.matz@6wind.com>
To: "Xueming(Steven) Li" <xuemingl@mellanox.com>
Cc: Adrien Mazarguil <adrien.mazarguil@6wind.com>,
 "dev@dpdk.org" <dev@dpdk.org>
Message-ID: <20180116124549.32w7g5jmhl633m2v@platinum>
References: <20171115155402.9967-1-xuemingl@mellanox.com>
 <20171209153923.19958-1-xuemingl@mellanox.com>
 <20171214153543.2d2ydissujk55cng@platinum>
 <VI1PR0502MB4079CAE045609269F59D3B84AC060@VI1PR0502MB4079.eurprd05.prod.outlook.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <VI1PR0502MB4079CAE045609269F59D3B84AC060@VI1PR0502MB4079.eurprd05.prod.outlook.com>
User-Agent: NeoMutt/20170113 (1.7.2)
Subject: Re: [dpdk-dev] [PATCH v2] lib/cmdline: init CLI parsing memory
X-BeenThere: dev@dpdk.org
X-Mailman-Version: 2.1.15
Precedence: list
List-Id: DPDK patches and discussions <dev.dpdk.org>
List-Unsubscribe: <https://dpdk.org/ml/options/dev>,
 <mailto:dev-request@dpdk.org?subject=unsubscribe>
List-Archive: <http://dpdk.org/ml/archives/dev/>
List-Post: <mailto:dev@dpdk.org>
List-Help: <mailto:dev-request@dpdk.org?subject=help>
List-Subscribe: <https://dpdk.org/ml/listinfo/dev>,
 <mailto:dev-request@dpdk.org?subject=subscribe>
X-List-Received-Date: Tue, 16 Jan 2018 12:45:53 -0000

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