Hi Olivier,
> -----Original Message----- > From: Lu, Wenzhuo > Sent: Friday, April 21, 2017 9:18 AM > To: Olivier MATZ > Cc: dev@dpdk.org > Subject: RE: CLI parsing issue > > Hi Olivier, > > > -----Original Message----- > > From: Olivier MATZ [mailto:olivier.m...@6wind.com] > > Sent: Thursday, April 20, 2017 4:55 PM > > To: Lu, Wenzhuo > > Cc: dev@dpdk.org > > Subject: Re: CLI parsing issue > > > > Hi Wenzhuo, > > > > On Thu, 20 Apr 2017 08:36:38 +0000, "Lu, Wenzhuo" > > <wenzhuo...@intel.com> > > wrote: > > > Hi Olivier, > > > I met a problem thar the parsing result of CLI is wrong. > > > I checked this function, cmdline_parse. It will check the CLI > > > instances one by one. Even if an instance is matched, the parsing > > > will not stop for ambiguous check. Seems the following check may > > > change the parsing result of the previous one, > > > /* fully parsed */ > > > tok = match_inst(inst, buf, 0, > > > result.buf, sizeof(result.buf), > > > > > > &dyn_tokens); > > > > > > > > > Is it better to use a temporary validate for match_inst and only > > > store the result when it matches, so the previous result has no > > > chance to be changed? Like bellow, > > > > > > > > > diff --git a/lib/librte_cmdline/cmdline_parse.c > > > b/lib/librte_cmdline/cmdline_parse.c > > > index 763c286..663efd1 100644 > > > --- a/lib/librte_cmdline/cmdline_parse.c > > > +++ b/lib/librte_cmdline/cmdline_parse.c > > > @@ -259,6 +259,7 @@ > > > char buf[CMDLINE_PARSE_RESULT_BUFSIZE]; > > > long double align; /* strong alignment constraint for buf > > > */ > > > } result; > > > + char tmp_buf[CMDLINE_PARSE_RESULT_BUFSIZE]; > > > cmdline_parse_token_hdr_t > > *dyn_tokens[CMDLINE_PARSE_DYNAMIC_TOKENS]; > > > void (*f)(void *, struct cmdline *, void *) = NULL; > > > void *data = NULL; > > > @@ -321,7 +322,7 @@ > > > debug_printf("INST %d\n", inst_num); > > > > > > /* fully parsed */ > > > - tok = match_inst(inst, buf, 0, result.buf, > > > sizeof(result.buf), > > > + tok = match_inst(inst, buf, 0, tmp_buf, > > > + sizeof(tmp_buf), > > > &dyn_tokens); > > > > > > if (tok > 0) /* we matched at least one token */ @@ > > > -329,6 +330,8 @@ > > > > > > else if (!tok) { > > > debug_printf("INST fully parsed\n"); > > > + memcpy(result.buf, tmp_buf, > > > + CMDLINE_PARSE_RESULT_BUFSIZE); > > > /* skip spaces */ > > > while (isblank2(*curbuf)) { > > > curbuf++; > > > > > > > > > > At first glance, I think your patch doesn't hurt. Do you have an > > example code that triggers the issue? > Sorry, I didn't show you the issue we met. > It's easy to reproduce on 17.05 RC1. > "testpmd> set tx loopback 0 on > Invalid port 116" > Whatever the input port id is, it's taken as 116 after parsing the CLI. > > Interesting, this issue is triggered by this patch, after I added a new CLI, > the > "set tx loopback ..." is not working. > > commit 22e6545fd02cab42332acd716b8921dd0aab3ad9 > Author: Wenzhuo Lu <wenzhuo...@intel.com> > Date: Fri Feb 24 11:24:35 2017 +0800 > > app/testpmd: set TC strict link priority mode > > I checked the implement of CLI parsing. > The implementation is going through all the instances in main_ctx to see > which instance can match the string we typed. > If typing "set tx loopback 0 on", it matches cmd_set_tx_loopback, and the > parsing result is, > $2 = {set = "set\000tx loopback 0 off \n", '\000' <repeats 104 times>, tx = > "tx\000loopback 0 off \n", '\000' <repeats 108 times>, > loopback = "loopback\000\060 off \n", '\000' <repeats 111 times>, > port_id = 0 '\000', > on_off = "off\000\n", '\000' <repeats 122 times>} > > Till now, everything is fine. > Then the parsing is not stopped, it's going on to check if the string can > match > any of the left instances. When checking cmd_strict_link_prio, although it > doesn't match, the parsing result is changed to, > $1 = {set = "set\000tx loopback 0 off \n", '\000' <repeats 104 times>, tx = > "tx\000loopback 0 off \n", '\000' <repeats 108 times>, > loopback = "loopback\000\060 off \n", '\000' <repeats 111 times>, > port_id = 116 't', > on_off = "x\000loopback 0 off \n", '\000' <repeats 109 times>} > > You see, now the port id and on_off both are wrong. Port_id points to char > 't' of "tx loopback ...". So it's always 116, the ASCII of 't'. Any news? Shall I send a patch?
