[dpdk-dev] CLI parsing issue
Checks
Commit Message
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,
Best regards
Wenzhuo Lu
Comments
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?
Thanks,
Olivier
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'.
>
>
> Thanks,
> Olivier
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?
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
Hi Olivier,
> -----Original Message-----
> From: Olivier Matz [mailto:olivier.matz@6wind.com]
> Sent: Monday, April 24, 2017 6:12 PM
> To: Lu, Wenzhuo
> Cc: 'dev@dpdk.org'
> Subject: Re: CLI parsing issue
>
> 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
>
Thanks for confirming that. Will send the patch soon :)
>
> Regards,
> Olivier
@@ -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++;