* [dpdk-dev] [PATCH] testpmd: Fix port validation code of "port stop all" command @ 2015-03-05 7:30 Tetsuya Mukawa 2015-03-05 13:33 ` Qiu, Michael 2015-03-06 13:44 ` De Lara Guarch, Pablo 0 siblings, 2 replies; 9+ messages in thread From: Tetsuya Mukawa @ 2015-03-05 7:30 UTC (permalink / raw) To: dev When "port stop all" is executed, the command doesn't work as it should because of wrong port validation. The patch fixes this issue. Reported-by: Pablo de Lara <pablo.de.lara.guarch@intel.com> Signed-off-by: Tetsuya Mukawa <mukawa@igel.co.jp> --- app/test-pmd/testpmd.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c index 61291be..bb65342 100644 --- a/app/test-pmd/testpmd.c +++ b/app/test-pmd/testpmd.c @@ -1484,7 +1484,7 @@ stop_port(portid_t pid) printf("Stopping ports...\n"); FOREACH_PORT(pi, ports) { - if (!port_id_is_invalid(pid, DISABLED_WARN) && pid != pi) + if (pid != pi && pid != (portid_t)RTE_PORT_ALL) continue; port = &ports[pi]; -- 1.9.1 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [dpdk-dev] [PATCH] testpmd: Fix port validation code of "port stop all" command 2015-03-05 7:30 [dpdk-dev] [PATCH] testpmd: Fix port validation code of "port stop all" command Tetsuya Mukawa @ 2015-03-05 13:33 ` Qiu, Michael 2015-03-05 13:38 ` Qiu, Michael 2015-03-06 13:53 ` De Lara Guarch, Pablo 2015-03-06 13:44 ` De Lara Guarch, Pablo 1 sibling, 2 replies; 9+ messages in thread From: Qiu, Michael @ 2015-03-05 13:33 UTC (permalink / raw) To: Tetsuya Mukawa, dev Hi, Tetsuya and Pablo This is not a full fix, I have generate the full fix patch two days ago, See below: diff --git a/app/test-pmd/config.c b/app/test-pmd/config.c index 49be819..ec53923 100644 --- a/app/test-pmd/config.c +++ b/app/test-pmd/config.c @@ -384,6 +384,9 @@ port_infos_display(portid_t port_id) int port_id_is_invalid(portid_t port_id, enum print_warning warning) { + if (port_id == (portid_t)RTE_PORT_ALL) + return 0; + if (ports[port_id].enabled) return 0; diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c index e556b4c..1c4c651 100644 --- a/app/test-pmd/testpmd.c +++ b/app/test-pmd/testpmd.c @@ -1326,6 +1326,9 @@ start_port(portid_t pid) return -1; } + if (port_id_is_invalid(pid, ENABLED_WARN)) + return 0; + if (init_fwd_streams() < 0) { printf("Fail from init_fwd_streams()\n"); return -1; @@ -1482,10 +1485,14 @@ stop_port(portid_t pid) dcb_test = 0; dcb_config = 0; } + + if (port_id_is_invalid(pid, ENABLED_WARN)) + return; + printf("Stopping ports...\n"); FOREACH_PORT(pi, ports) { - if (!port_id_is_invalid(pid, DISABLED_WARN) && pid != pi) + if (pid != pi && pid != (portid_t)RTE_PORT_ALL) continue; port = &ports[pi]; @@ -1517,10 +1524,13 @@ close_port(portid_t pid) return; } + if (port_id_is_invalid(pid, ENABLED_WARN)) + return; + printf("Closing ports...\n"); FOREACH_PORT(pi, ports) { - if (!port_id_is_invalid(pid, DISABLED_WARN) && pid != pi) + if (pid != pi && pid != (portid_t)RTE_PORT_ALL) continue; port = &ports[pi]; -- 1.9.3 Thanks, Michael On 3/5/2015 3:31 PM, Tetsuya Mukawa wrote: > When "port stop all" is executed, the command doesn't work as it should > because of wrong port validation. The patch fixes this issue. > > Reported-by: Pablo de Lara <pablo.de.lara.guarch@intel.com> > Signed-off-by: Tetsuya Mukawa <mukawa@igel.co.jp> > --- > app/test-pmd/testpmd.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c > index 61291be..bb65342 100644 > --- a/app/test-pmd/testpmd.c > +++ b/app/test-pmd/testpmd.c > @@ -1484,7 +1484,7 @@ stop_port(portid_t pid) > printf("Stopping ports...\n"); > > FOREACH_PORT(pi, ports) { > - if (!port_id_is_invalid(pid, DISABLED_WARN) && pid != pi) > + if (pid != pi && pid != (portid_t)RTE_PORT_ALL) > continue; > > port = &ports[pi]; ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [dpdk-dev] [PATCH] testpmd: Fix port validation code of "port stop all" command 2015-03-05 13:33 ` Qiu, Michael @ 2015-03-05 13:38 ` Qiu, Michael 2015-03-06 13:53 ` De Lara Guarch, Pablo 1 sibling, 0 replies; 9+ messages in thread From: Qiu, Michael @ 2015-03-05 13:38 UTC (permalink / raw) To: Tetsuya Mukawa, dev On 3/5/2015 9:33 PM, Qiu, Michael wrote: > Hi, Tetsuya and Pablo > This is not a full fix, I have generate the full fix patch two days ago, > See below: > > diff --git a/app/test-pmd/config.c b/app/test-pmd/config.c > index 49be819..ec53923 100644 > --- a/app/test-pmd/config.c > +++ b/app/test-pmd/config.c > @@ -384,6 +384,9 @@ port_infos_display(portid_t port_id) > int > port_id_is_invalid(portid_t port_id, enum print_warning warning) > { > + if (port_id == (portid_t)RTE_PORT_ALL) > + return 0; > + > if (ports[port_id].enabled) > return 0; > > diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c > index e556b4c..1c4c651 100644 > --- a/app/test-pmd/testpmd.c > +++ b/app/test-pmd/testpmd.c > @@ -1326,6 +1326,9 @@ start_port(portid_t pid) > return -1; > } > > + if (port_id_is_invalid(pid, ENABLED_WARN)) > + return 0; > + > if (init_fwd_streams() < 0) { > printf("Fail from init_fwd_streams()\n"); > return -1; > @@ -1482,10 +1485,14 @@ stop_port(portid_t pid) > dcb_test = 0; > dcb_config = 0; > } > + > + if (port_id_is_invalid(pid, ENABLED_WARN)) > + return; > + > printf("Stopping ports...\n"); > > FOREACH_PORT(pi, ports) { > - if (!port_id_is_invalid(pid, DISABLED_WARN) && pid != pi) > + if (pid != pi && pid != (portid_t)RTE_PORT_ALL) > continue; > > port = &ports[pi]; > @@ -1517,10 +1524,13 @@ close_port(portid_t pid) > return; > } > > + if (port_id_is_invalid(pid, ENABLED_WARN)) > + return; > + > printf("Closing ports...\n"); > > FOREACH_PORT(pi, ports) { > - if (!port_id_is_invalid(pid, DISABLED_WARN) && pid != pi) > + if (pid != pi && pid != (portid_t)RTE_PORT_ALL) > continue; > > port = &ports[pi]; This diff is based on: [PATCH] app/test-pmd: Fix log issue without nic binded Thanks, Michael ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [dpdk-dev] [PATCH] testpmd: Fix port validation code of "port stop all" command 2015-03-05 13:33 ` Qiu, Michael 2015-03-05 13:38 ` Qiu, Michael @ 2015-03-06 13:53 ` De Lara Guarch, Pablo 2015-03-09 2:22 ` Tetsuya Mukawa 1 sibling, 1 reply; 9+ messages in thread From: De Lara Guarch, Pablo @ 2015-03-06 13:53 UTC (permalink / raw) To: Qiu, Michael, Tetsuya Mukawa, dev Hi Michael, > -----Original Message----- > From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Qiu, Michael > Sent: Thursday, March 05, 2015 1:33 PM > To: Tetsuya Mukawa; dev@dpdk.org > Subject: Re: [dpdk-dev] [PATCH] testpmd: Fix port validation code of "port > stop all" command > > Hi, Tetsuya and Pablo > This is not a full fix, I have generate the full fix patch two days ago, Sorry I did not see this earlier. Did you upstream this patch already? I acked Tetsuya's patch, as it was simple and works, but I cannot find this one. Thanks, Pablo > See below: > > diff --git a/app/test-pmd/config.c b/app/test-pmd/config.c > index 49be819..ec53923 100644 > --- a/app/test-pmd/config.c > +++ b/app/test-pmd/config.c > @@ -384,6 +384,9 @@ port_infos_display(portid_t port_id) > int > port_id_is_invalid(portid_t port_id, enum print_warning warning) > { > + if (port_id == (portid_t)RTE_PORT_ALL) > + return 0; > + > if (ports[port_id].enabled) > return 0; > > diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c > index e556b4c..1c4c651 100644 > --- a/app/test-pmd/testpmd.c > +++ b/app/test-pmd/testpmd.c > @@ -1326,6 +1326,9 @@ start_port(portid_t pid) > return -1; > } > > + if (port_id_is_invalid(pid, ENABLED_WARN)) > + return 0; > + > if (init_fwd_streams() < 0) { > printf("Fail from init_fwd_streams()\n"); > return -1; > @@ -1482,10 +1485,14 @@ stop_port(portid_t pid) > dcb_test = 0; > dcb_config = 0; > } > + > + if (port_id_is_invalid(pid, ENABLED_WARN)) > + return; > + > printf("Stopping ports...\n"); > > FOREACH_PORT(pi, ports) { > - if (!port_id_is_invalid(pid, DISABLED_WARN) && pid != pi) > + if (pid != pi && pid != (portid_t)RTE_PORT_ALL) > continue; > > port = &ports[pi]; > @@ -1517,10 +1524,13 @@ close_port(portid_t pid) > return; > } > > + if (port_id_is_invalid(pid, ENABLED_WARN)) > + return; > + > printf("Closing ports...\n"); > > FOREACH_PORT(pi, ports) { > - if (!port_id_is_invalid(pid, DISABLED_WARN) && pid != pi) > + if (pid != pi && pid != (portid_t)RTE_PORT_ALL) > continue; > > port = &ports[pi]; > -- > 1.9.3 > > Thanks, > Michael > > On 3/5/2015 3:31 PM, Tetsuya Mukawa wrote: > > When "port stop all" is executed, the command doesn't work as it should > > because of wrong port validation. The patch fixes this issue. > > > > Reported-by: Pablo de Lara <pablo.de.lara.guarch@intel.com> > > Signed-off-by: Tetsuya Mukawa <mukawa@igel.co.jp> > > --- > > app/test-pmd/testpmd.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c > > index 61291be..bb65342 100644 > > --- a/app/test-pmd/testpmd.c > > +++ b/app/test-pmd/testpmd.c > > @@ -1484,7 +1484,7 @@ stop_port(portid_t pid) > > printf("Stopping ports...\n"); > > > > FOREACH_PORT(pi, ports) { > > - if (!port_id_is_invalid(pid, DISABLED_WARN) && pid != pi) > > + if (pid != pi && pid != (portid_t)RTE_PORT_ALL) > > continue; > > > > port = &ports[pi]; ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [dpdk-dev] [PATCH] testpmd: Fix port validation code of "port stop all" command 2015-03-06 13:53 ` De Lara Guarch, Pablo @ 2015-03-09 2:22 ` Tetsuya Mukawa 2015-03-09 3:49 ` Qiu, Michael 0 siblings, 1 reply; 9+ messages in thread From: Tetsuya Mukawa @ 2015-03-09 2:22 UTC (permalink / raw) To: De Lara Guarch, Pablo, Qiu, Michael, dev On 2015/03/06 22:53, De Lara Guarch, Pablo wrote: > Hi Michael, > >> -----Original Message----- >> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Qiu, Michael >> Sent: Thursday, March 05, 2015 1:33 PM >> To: Tetsuya Mukawa; dev@dpdk.org >> Subject: Re: [dpdk-dev] [PATCH] testpmd: Fix port validation code of "port >> stop all" command >> >> Hi, Tetsuya and Pablo >> This is not a full fix, I have generate the full fix patch two days ago, Hi Michel, I am sorry for late replying, and thanks for your work. > Sorry I did not see this earlier. Did you upstream this patch already? > I acked Tetsuya's patch, as it was simple and works, but I cannot find > this one. > > Thanks, > Pablo > >> See below: >> >> diff --git a/app/test-pmd/config.c b/app/test-pmd/config.c >> index 49be819..ec53923 100644 >> --- a/app/test-pmd/config.c >> +++ b/app/test-pmd/config.c >> @@ -384,6 +384,9 @@ port_infos_display(portid_t port_id) >> int >> port_id_is_invalid(portid_t port_id, enum print_warning warning) >> { >> + if (port_id == (portid_t)RTE_PORT_ALL) >> + return 0; >> + I am not clearly sure that we need to add above 'if statement'. >> if (ports[port_id].enabled) >> return 0; >> >> diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c >> index e556b4c..1c4c651 100644 >> --- a/app/test-pmd/testpmd.c >> +++ b/app/test-pmd/testpmd.c >> @@ -1326,6 +1326,9 @@ start_port(portid_t pid) >> return -1; >> } >> >> + if (port_id_is_invalid(pid, ENABLED_WARN)) >> + return 0; >> + Same as above. >> if (init_fwd_streams() < 0) { >> printf("Fail from init_fwd_streams()\n"); >> return -1; >> @@ -1482,10 +1485,14 @@ stop_port(portid_t pid) >> dcb_test = 0; >> dcb_config = 0; >> } >> + >> + if (port_id_is_invalid(pid, ENABLED_WARN)) >> + return; >> + Same as above. >> printf("Stopping ports...\n"); >> >> FOREACH_PORT(pi, ports) { >> - if (!port_id_is_invalid(pid, DISABLED_WARN) && pid != pi) >> + if (pid != pi && pid != (portid_t)RTE_PORT_ALL) >> continue; >> >> port = &ports[pi]; >> @@ -1517,10 +1524,13 @@ close_port(portid_t pid) >> return; >> } >> >> + if (port_id_is_invalid(pid, ENABLED_WARN)) >> + return; >> + Same as above. >> printf("Closing ports...\n"); >> >> FOREACH_PORT(pi, ports) { >> - if (!port_id_is_invalid(pid, DISABLED_WARN) && pid != pi) >> + if (pid != pi && pid != (portid_t)RTE_PORT_ALL) >> continue; >> >> port = &ports[pi]; >> -- >> 1.9.3 FOREACH_PORT() returns valid ports, so is it not enough to check like above? I am not clearly understand which case we need to add above extra if statements. Could you please describe? But I agree we cannot use my previous patch. We need to fix not only stop_port() but also close_port() like start_port(). Thanks, Tetsuya >> Thanks, >> Michael >> >> On 3/5/2015 3:31 PM, Tetsuya Mukawa wrote: >>> When "port stop all" is executed, the command doesn't work as it should >>> because of wrong port validation. The patch fixes this issue. >>> >>> Reported-by: Pablo de Lara <pablo.de.lara.guarch@intel.com> >>> Signed-off-by: Tetsuya Mukawa <mukawa@igel.co.jp> >>> --- >>> app/test-pmd/testpmd.c | 2 +- >>> 1 file changed, 1 insertion(+), 1 deletion(-) >>> >>> diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c >>> index 61291be..bb65342 100644 >>> --- a/app/test-pmd/testpmd.c >>> +++ b/app/test-pmd/testpmd.c >>> @@ -1484,7 +1484,7 @@ stop_port(portid_t pid) >>> printf("Stopping ports...\n"); >>> >>> FOREACH_PORT(pi, ports) { >>> - if (!port_id_is_invalid(pid, DISABLED_WARN) && pid != pi) >>> + if (pid != pi && pid != (portid_t)RTE_PORT_ALL) >>> continue; >>> >>> port = &ports[pi]; ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [dpdk-dev] [PATCH] testpmd: Fix port validation code of "port stop all" command 2015-03-09 2:22 ` Tetsuya Mukawa @ 2015-03-09 3:49 ` Qiu, Michael 2015-03-09 5:21 ` Tetsuya Mukawa 0 siblings, 1 reply; 9+ messages in thread From: Qiu, Michael @ 2015-03-09 3:49 UTC (permalink / raw) To: Tetsuya Mukawa, De Lara Guarch, Pablo, dev On 3/9/2015 10:22 AM, Tetsuya Mukawa wrote: > On 2015/03/06 22:53, De Lara Guarch, Pablo wrote: >> Hi Michael, >> >>> -----Original Message----- >>> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Qiu, Michael >>> Sent: Thursday, March 05, 2015 1:33 PM >>> To: Tetsuya Mukawa; dev@dpdk.org >>> Subject: Re: [dpdk-dev] [PATCH] testpmd: Fix port validation code of "port >>> stop all" command >>> >>> Hi, Tetsuya and Pablo >>> This is not a full fix, I have generate the full fix patch two days ago, > Hi Michel, > > I am sorry for late replying, and thanks for your work. > >> Sorry I did not see this earlier. Did you upstream this patch already? >> I acked Tetsuya's patch, as it was simple and works, but I cannot find >> this one. >> >> Thanks, >> Pablo >> >>> See below: >>> >>> diff --git a/app/test-pmd/config.c b/app/test-pmd/config.c >>> index 49be819..ec53923 100644 >>> --- a/app/test-pmd/config.c >>> +++ b/app/test-pmd/config.c >>> @@ -384,6 +384,9 @@ port_infos_display(portid_t port_id) >>> int >>> port_id_is_invalid(portid_t port_id, enum print_warning warning) >>> { >>> + if (port_id == (portid_t)RTE_PORT_ALL) >>> + return 0; >>> + > I am not clearly sure that we need to add above 'if statement'. Because some times RTE_PORT_ALL will pass to port start/stop/close, but the check will be invalid. Actually, we should see it as valid, then all port valid check will work for start/stop/close action > >>> if (ports[port_id].enabled) >>> return 0; >>> >>> diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c >>> index e556b4c..1c4c651 100644 >>> --- a/app/test-pmd/testpmd.c >>> +++ b/app/test-pmd/testpmd.c >>> @@ -1326,6 +1326,9 @@ start_port(portid_t pid) >>> return -1; >>> } >>> >>> + if (port_id_is_invalid(pid, ENABLED_WARN)) >>> + return 0; >>> + > Same as above. > >>> if (init_fwd_streams() < 0) { >>> printf("Fail from init_fwd_streams()\n"); >>> return -1; >>> @@ -1482,10 +1485,14 @@ stop_port(portid_t pid) >>> dcb_test = 0; >>> dcb_config = 0; >>> } >>> + >>> + if (port_id_is_invalid(pid, ENABLED_WARN)) >>> + return; >>> + > Same as above. > >>> printf("Stopping ports...\n"); >>> >>> FOREACH_PORT(pi, ports) { >>> - if (!port_id_is_invalid(pid, DISABLED_WARN) && pid != pi) >>> + if (pid != pi && pid != (portid_t)RTE_PORT_ALL) >>> continue; >>> >>> port = &ports[pi]; >>> @@ -1517,10 +1524,13 @@ close_port(portid_t pid) >>> return; >>> } >>> >>> + if (port_id_is_invalid(pid, ENABLED_WARN)) >>> + return; >>> + > Same as above. > >>> printf("Closing ports...\n"); >>> >>> FOREACH_PORT(pi, ports) { >>> - if (!port_id_is_invalid(pid, DISABLED_WARN) && pid != pi) >>> + if (pid != pi && pid != (portid_t)RTE_PORT_ALL) >>> continue; >>> >>> port = &ports[pi]; >>> -- >>> 1.9.3 > FOREACH_PORT() returns valid ports, so is it not enough to check like above? > I am not clearly understand which case we need to add above extra if > statements. > Could you please describe? Yes, just consider this situation, the valid port number are [0, 1], but you try to to stop prot number 2, what will happen? Noting will be show, at least we need an error log. So it must be check. Thanks, Michael > But I agree we cannot use my previous patch. > We need to fix not only stop_port() but also close_port() like start_port(). > > Thanks, > Tetsuya > >>> Thanks, >>> Michael >>> >>> On 3/5/2015 3:31 PM, Tetsuya Mukawa wrote: >>>> When "port stop all" is executed, the command doesn't work as it should >>>> because of wrong port validation. The patch fixes this issue. >>>> >>>> Reported-by: Pablo de Lara <pablo.de.lara.guarch@intel.com> >>>> Signed-off-by: Tetsuya Mukawa <mukawa@igel.co.jp> >>>> --- >>>> app/test-pmd/testpmd.c | 2 +- >>>> 1 file changed, 1 insertion(+), 1 deletion(-) >>>> >>>> diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c >>>> index 61291be..bb65342 100644 >>>> --- a/app/test-pmd/testpmd.c >>>> +++ b/app/test-pmd/testpmd.c >>>> @@ -1484,7 +1484,7 @@ stop_port(portid_t pid) >>>> printf("Stopping ports...\n"); >>>> >>>> FOREACH_PORT(pi, ports) { >>>> - if (!port_id_is_invalid(pid, DISABLED_WARN) && pid != pi) >>>> + if (pid != pi && pid != (portid_t)RTE_PORT_ALL) >>>> continue; >>>> >>>> port = &ports[pi]; > > ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [dpdk-dev] [PATCH] testpmd: Fix port validation code of "port stop all" command 2015-03-09 3:49 ` Qiu, Michael @ 2015-03-09 5:21 ` Tetsuya Mukawa 2015-03-09 6:01 ` Qiu, Michael 0 siblings, 1 reply; 9+ messages in thread From: Tetsuya Mukawa @ 2015-03-09 5:21 UTC (permalink / raw) To: Qiu, Michael, De Lara Guarch, Pablo, dev On 2015/03/09 12:49, Qiu, Michael wrote: > On 3/9/2015 10:22 AM, Tetsuya Mukawa wrote: >> On 2015/03/06 22:53, De Lara Guarch, Pablo wrote: >>> Hi Michael, >>> >>>> -----Original Message----- >>>> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Qiu, Michael >>>> Sent: Thursday, March 05, 2015 1:33 PM >>>> To: Tetsuya Mukawa; dev@dpdk.org >>>> Subject: Re: [dpdk-dev] [PATCH] testpmd: Fix port validation code of "port >>>> stop all" command >>>> >>>> Hi, Tetsuya and Pablo >>>> This is not a full fix, I have generate the full fix patch two days ago, >> Hi Michel, >> >> I am sorry for late replying, and thanks for your work. >> >>> Sorry I did not see this earlier. Did you upstream this patch already? >>> I acked Tetsuya's patch, as it was simple and works, but I cannot find >>> this one. >>> >>> Thanks, >>> Pablo >>> >>>> See below: >>>> >>>> diff --git a/app/test-pmd/config.c b/app/test-pmd/config.c >>>> index 49be819..ec53923 100644 >>>> --- a/app/test-pmd/config.c >>>> +++ b/app/test-pmd/config.c >>>> @@ -384,6 +384,9 @@ port_infos_display(portid_t port_id) >>>> int >>>> port_id_is_invalid(portid_t port_id, enum print_warning warning) >>>> { >>>> + if (port_id == (portid_t)RTE_PORT_ALL) >>>> + return 0; >>>> + >> I am not clearly sure that we need to add above 'if statement'. > Because some times RTE_PORT_ALL will pass to port start/stop/close, but > the check will be invalid. > > Actually, we should see it as valid, then all port valid check will work > for start/stop/close action > >>>> if (ports[port_id].enabled) >>>> return 0; >>>> >>>> diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c >>>> index e556b4c..1c4c651 100644 >>>> --- a/app/test-pmd/testpmd.c >>>> +++ b/app/test-pmd/testpmd.c >>>> @@ -1326,6 +1326,9 @@ start_port(portid_t pid) >>>> return -1; >>>> } >>>> >>>> + if (port_id_is_invalid(pid, ENABLED_WARN)) >>>> + return 0; >>>> + >> Same as above. >> >>>> if (init_fwd_streams() < 0) { >>>> printf("Fail from init_fwd_streams()\n"); >>>> return -1; >>>> @@ -1482,10 +1485,14 @@ stop_port(portid_t pid) >>>> dcb_test = 0; >>>> dcb_config = 0; >>>> } >>>> + >>>> + if (port_id_is_invalid(pid, ENABLED_WARN)) >>>> + return; >>>> + >> Same as above. >> >>>> printf("Stopping ports...\n"); >>>> >>>> FOREACH_PORT(pi, ports) { >>>> - if (!port_id_is_invalid(pid, DISABLED_WARN) && pid != pi) >>>> + if (pid != pi && pid != (portid_t)RTE_PORT_ALL) >>>> continue; >>>> >>>> port = &ports[pi]; >>>> @@ -1517,10 +1524,13 @@ close_port(portid_t pid) >>>> return; >>>> } >>>> >>>> + if (port_id_is_invalid(pid, ENABLED_WARN)) >>>> + return; >>>> + >> Same as above. >> >>>> printf("Closing ports...\n"); >>>> >>>> FOREACH_PORT(pi, ports) { >>>> - if (!port_id_is_invalid(pid, DISABLED_WARN) && pid != pi) >>>> + if (pid != pi && pid != (portid_t)RTE_PORT_ALL) >>>> continue; >>>> >>>> port = &ports[pi]; >>>> -- >>>> 1.9.3 >> FOREACH_PORT() returns valid ports, so is it not enough to check like above? >> I am not clearly understand which case we need to add above extra if >> statements. >> Could you please describe? > Yes, just consider this situation, the valid port number are [0, 1], > but you try to to stop prot number 2, what will happen? > > Noting will be show, at least we need an error log. > > So it must be check. Hi Michael, Thanks, I've understood it. Have you already submitted it as patch? I could not find it in patchwork. I will send an ack to your patch. Thanks, Tetsuya > Thanks, > Michael >> But I agree we cannot use my previous patch. >> We need to fix not only stop_port() but also close_port() like start_port(). >> >> Thanks, >> Tetsuya >> >>>> Thanks, >>>> Michael >>>> >>>> On 3/5/2015 3:31 PM, Tetsuya Mukawa wrote: >>>>> When "port stop all" is executed, the command doesn't work as it should >>>>> because of wrong port validation. The patch fixes this issue. >>>>> >>>>> Reported-by: Pablo de Lara <pablo.de.lara.guarch@intel.com> >>>>> Signed-off-by: Tetsuya Mukawa <mukawa@igel.co.jp> >>>>> --- >>>>> app/test-pmd/testpmd.c | 2 +- >>>>> 1 file changed, 1 insertion(+), 1 deletion(-) >>>>> >>>>> diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c >>>>> index 61291be..bb65342 100644 >>>>> --- a/app/test-pmd/testpmd.c >>>>> +++ b/app/test-pmd/testpmd.c >>>>> @@ -1484,7 +1484,7 @@ stop_port(portid_t pid) >>>>> printf("Stopping ports...\n"); >>>>> >>>>> FOREACH_PORT(pi, ports) { >>>>> - if (!port_id_is_invalid(pid, DISABLED_WARN) && pid != pi) >>>>> + if (pid != pi && pid != (portid_t)RTE_PORT_ALL) >>>>> continue; >>>>> >>>>> port = &ports[pi]; >> ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [dpdk-dev] [PATCH] testpmd: Fix port validation code of "port stop all" command 2015-03-09 5:21 ` Tetsuya Mukawa @ 2015-03-09 6:01 ` Qiu, Michael 0 siblings, 0 replies; 9+ messages in thread From: Qiu, Michael @ 2015-03-09 6:01 UTC (permalink / raw) To: Tetsuya Mukawa, De Lara Guarch, Pablo, dev On 3/9/2015 1:21 PM, Tetsuya Mukawa wrote: > On 2015/03/09 12:49, Qiu, Michael wrote: >> On 3/9/2015 10:22 AM, Tetsuya Mukawa wrote: >>> On 2015/03/06 22:53, De Lara Guarch, Pablo wrote: >>>> Hi Michael, >>>> >>>>> -----Original Message----- >>>>> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Qiu, Michael >>>>> Sent: Thursday, March 05, 2015 1:33 PM >>>>> To: Tetsuya Mukawa; dev@dpdk.org >>>>> Subject: Re: [dpdk-dev] [PATCH] testpmd: Fix port validation code of "port >>>>> stop all" command >>>>> >>>>> Hi, Tetsuya and Pablo >>>>> This is not a full fix, I have generate the full fix patch two days ago, >>> Hi Michel, >>> >>> I am sorry for late replying, and thanks for your work. >>> >>>> Sorry I did not see this earlier. Did you upstream this patch already? >>>> I acked Tetsuya's patch, as it was simple and works, but I cannot find >>>> this one. >>>> >>>> Thanks, >>>> Pablo >>>> >>>>> See below: >>>>> >>>>> diff --git a/app/test-pmd/config.c b/app/test-pmd/config.c >>>>> index 49be819..ec53923 100644 >>>>> --- a/app/test-pmd/config.c >>>>> +++ b/app/test-pmd/config.c >>>>> @@ -384,6 +384,9 @@ port_infos_display(portid_t port_id) >>>>> int >>>>> port_id_is_invalid(portid_t port_id, enum print_warning warning) >>>>> { >>>>> + if (port_id == (portid_t)RTE_PORT_ALL) >>>>> + return 0; >>>>> + >>> I am not clearly sure that we need to add above 'if statement'. >> Because some times RTE_PORT_ALL will pass to port start/stop/close, but >> the check will be invalid. >> >> Actually, we should see it as valid, then all port valid check will work >> for start/stop/close action >> >>>>> if (ports[port_id].enabled) >>>>> return 0; >>>>> >>>>> diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c >>>>> index e556b4c..1c4c651 100644 >>>>> --- a/app/test-pmd/testpmd.c >>>>> +++ b/app/test-pmd/testpmd.c >>>>> @@ -1326,6 +1326,9 @@ start_port(portid_t pid) >>>>> return -1; >>>>> } >>>>> >>>>> + if (port_id_is_invalid(pid, ENABLED_WARN)) >>>>> + return 0; >>>>> + >>> Same as above. >>> >>>>> if (init_fwd_streams() < 0) { >>>>> printf("Fail from init_fwd_streams()\n"); >>>>> return -1; >>>>> @@ -1482,10 +1485,14 @@ stop_port(portid_t pid) >>>>> dcb_test = 0; >>>>> dcb_config = 0; >>>>> } >>>>> + >>>>> + if (port_id_is_invalid(pid, ENABLED_WARN)) >>>>> + return; >>>>> + >>> Same as above. >>> >>>>> printf("Stopping ports...\n"); >>>>> >>>>> FOREACH_PORT(pi, ports) { >>>>> - if (!port_id_is_invalid(pid, DISABLED_WARN) && pid != pi) >>>>> + if (pid != pi && pid != (portid_t)RTE_PORT_ALL) >>>>> continue; >>>>> >>>>> port = &ports[pi]; >>>>> @@ -1517,10 +1524,13 @@ close_port(portid_t pid) >>>>> return; >>>>> } >>>>> >>>>> + if (port_id_is_invalid(pid, ENABLED_WARN)) >>>>> + return; >>>>> + >>> Same as above. >>> >>>>> printf("Closing ports...\n"); >>>>> >>>>> FOREACH_PORT(pi, ports) { >>>>> - if (!port_id_is_invalid(pid, DISABLED_WARN) && pid != pi) >>>>> + if (pid != pi && pid != (portid_t)RTE_PORT_ALL) >>>>> continue; >>>>> >>>>> port = &ports[pi]; >>>>> -- >>>>> 1.9.3 >>> FOREACH_PORT() returns valid ports, so is it not enough to check like above? >>> I am not clearly understand which case we need to add above extra if >>> statements. >>> Could you please describe? >> Yes, just consider this situation, the valid port number are [0, 1], >> but you try to to stop prot number 2, what will happen? >> >> Noting will be show, at least we need an error log. >> >> So it must be check. > Hi Michael, > > Thanks, I've understood it. > Have you already submitted it as patch? > I could not find it in patchwork. > I will send an ack to your patch. I have not send yet, I will send out now and add will add you in cc list. Thanks, Michael > Thanks, > Tetsuya > >> Thanks, >> Michael >>> But I agree we cannot use my previous patch. >>> We need to fix not only stop_port() but also close_port() like start_port(). >>> >>> Thanks, >>> Tetsuya >>> >>>>> Thanks, >>>>> Michael >>>>> >>>>> On 3/5/2015 3:31 PM, Tetsuya Mukawa wrote: >>>>>> When "port stop all" is executed, the command doesn't work as it should >>>>>> because of wrong port validation. The patch fixes this issue. >>>>>> >>>>>> Reported-by: Pablo de Lara <pablo.de.lara.guarch@intel.com> >>>>>> Signed-off-by: Tetsuya Mukawa <mukawa@igel.co.jp> >>>>>> --- >>>>>> app/test-pmd/testpmd.c | 2 +- >>>>>> 1 file changed, 1 insertion(+), 1 deletion(-) >>>>>> >>>>>> diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c >>>>>> index 61291be..bb65342 100644 >>>>>> --- a/app/test-pmd/testpmd.c >>>>>> +++ b/app/test-pmd/testpmd.c >>>>>> @@ -1484,7 +1484,7 @@ stop_port(portid_t pid) >>>>>> printf("Stopping ports...\n"); >>>>>> >>>>>> FOREACH_PORT(pi, ports) { >>>>>> - if (!port_id_is_invalid(pid, DISABLED_WARN) && pid != pi) >>>>>> + if (pid != pi && pid != (portid_t)RTE_PORT_ALL) >>>>>> continue; >>>>>> >>>>>> port = &ports[pi]; > > ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [dpdk-dev] [PATCH] testpmd: Fix port validation code of "port stop all" command 2015-03-05 7:30 [dpdk-dev] [PATCH] testpmd: Fix port validation code of "port stop all" command Tetsuya Mukawa 2015-03-05 13:33 ` Qiu, Michael @ 2015-03-06 13:44 ` De Lara Guarch, Pablo 1 sibling, 0 replies; 9+ messages in thread From: De Lara Guarch, Pablo @ 2015-03-06 13:44 UTC (permalink / raw) To: Tetsuya Mukawa, dev > -----Original Message----- > From: Tetsuya Mukawa [mailto:mukawa@igel.co.jp] > Sent: Thursday, March 05, 2015 7:30 AM > To: dev@dpdk.org > Cc: De Lara Guarch, Pablo; Tetsuya Mukawa > Subject: [PATCH] testpmd: Fix port validation code of "port stop all" > command > > When "port stop all" is executed, the command doesn't work as it should > because of wrong port validation. The patch fixes this issue. > > Reported-by: Pablo de Lara <pablo.de.lara.guarch@intel.com> > Signed-off-by: Tetsuya Mukawa <mukawa@igel.co.jp> > --- > app/test-pmd/testpmd.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c > index 61291be..bb65342 100644 > --- a/app/test-pmd/testpmd.c > +++ b/app/test-pmd/testpmd.c > @@ -1484,7 +1484,7 @@ stop_port(portid_t pid) > printf("Stopping ports...\n"); > > FOREACH_PORT(pi, ports) { > - if (!port_id_is_invalid(pid, DISABLED_WARN) && pid != pi) > + if (pid != pi && pid != (portid_t)RTE_PORT_ALL) > continue; > > port = &ports[pi]; > -- > 1.9.1 Acked-by: Pablo de Lara <pablo.de.lara.guarch@intel.com> ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2015-03-09 6:01 UTC | newest] Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2015-03-05 7:30 [dpdk-dev] [PATCH] testpmd: Fix port validation code of "port stop all" command Tetsuya Mukawa 2015-03-05 13:33 ` Qiu, Michael 2015-03-05 13:38 ` Qiu, Michael 2015-03-06 13:53 ` De Lara Guarch, Pablo 2015-03-09 2:22 ` Tetsuya Mukawa 2015-03-09 3:49 ` Qiu, Michael 2015-03-09 5:21 ` Tetsuya Mukawa 2015-03-09 6:01 ` Qiu, Michael 2015-03-06 13:44 ` De Lara Guarch, Pablo
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).