Re: [dpdk-dev] CLI parsing issue

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

Hi Wenzhuo,

On Mon, 24 Apr 2017 01:49:38 +0000, "Lu, Wenzhuo" <wenzhuo.lu@intel.com> wrote:
> 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.matz@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.lu@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.lu@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? 

I checked in detail, and you're right there is a bug here. Your proposed
patch looks good, you can send it. Thank you for reporting it!

You can add in the log:
Fixes: af75078fece3 ("first public release")
CC: stable@dpdk.org


Regards,
Olivier