* [dpdk-dev] CLI parsing issue
@ 2017-04-20 8:36 Lu, Wenzhuo
2017-04-20 8:54 ` Olivier MATZ
0 siblings, 1 reply; 6+ messages in thread
From: Lu, Wenzhuo @ 2017-04-20 8:36 UTC (permalink / raw)
To: Olivier Matz; +Cc: dev
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++;
Best regards
Wenzhuo Lu
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [dpdk-dev] CLI parsing issue 2017-04-20 8:36 [dpdk-dev] CLI parsing issue Lu, Wenzhuo @ 2017-04-20 8:54 ` Olivier MATZ 2017-04-21 1:17 ` Lu, Wenzhuo 0 siblings, 1 reply; 6+ messages in thread From: Olivier MATZ @ 2017-04-20 8:54 UTC (permalink / raw) To: Lu, Wenzhuo; +Cc: dev 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [dpdk-dev] CLI parsing issue 2017-04-20 8:54 ` Olivier MATZ @ 2017-04-21 1:17 ` Lu, Wenzhuo 2017-04-24 1:49 ` Lu, Wenzhuo 0 siblings, 1 reply; 6+ messages in thread From: Lu, Wenzhuo @ 2017-04-21 1:17 UTC (permalink / raw) To: Olivier MATZ; +Cc: dev 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [dpdk-dev] CLI parsing issue 2017-04-21 1:17 ` Lu, Wenzhuo @ 2017-04-24 1:49 ` Lu, Wenzhuo 2017-04-24 10:11 ` Olivier Matz 0 siblings, 1 reply; 6+ messages in thread From: Lu, Wenzhuo @ 2017-04-24 1:49 UTC (permalink / raw) To: 'Olivier MATZ'; +Cc: 'dev@dpdk.org' 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? ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [dpdk-dev] CLI parsing issue 2017-04-24 1:49 ` Lu, Wenzhuo @ 2017-04-24 10:11 ` Olivier Matz 2017-04-25 1:16 ` Lu, Wenzhuo 0 siblings, 1 reply; 6+ messages in thread From: Olivier Matz @ 2017-04-24 10:11 UTC (permalink / raw) To: Lu, Wenzhuo; +Cc: 'dev@dpdk.org' 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [dpdk-dev] CLI parsing issue 2017-04-24 10:11 ` Olivier Matz @ 2017-04-25 1:16 ` Lu, Wenzhuo 0 siblings, 0 replies; 6+ messages in thread From: Lu, Wenzhuo @ 2017-04-25 1:16 UTC (permalink / raw) To: Olivier Matz; +Cc: 'dev@dpdk.org' 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2017-04-25 1:16 UTC | newest] Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2017-04-20 8:36 [dpdk-dev] CLI parsing issue Lu, Wenzhuo 2017-04-20 8:54 ` Olivier MATZ 2017-04-21 1:17 ` Lu, Wenzhuo 2017-04-24 1:49 ` Lu, Wenzhuo 2017-04-24 10:11 ` Olivier Matz 2017-04-25 1:16 ` Lu, Wenzhuo
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).