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'.
>
>
> Thanks,
> Olivier
