diff mbox

[dpdk-dev] testpmd: Fix port validation code of "port stop all" command

Message ID 533710CFB86FA344BFBF2D6802E60286CEEEC5@SHSMSX101.ccr.corp.intel.com (mailing list archive)
State Not Applicable, archived
Headers show

Commit Message

Michael Qiu March 5, 2015, 1:33 p.m. UTC
Hi, Tetsuya and Pablo
This is not a full fix, I have generate the full fix patch two days ago,
See below:

Comments

Michael Qiu March 5, 2015, 1:38 p.m. UTC | #1
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
Pablo de Lara March 6, 2015, 1:53 p.m. UTC | #2
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];
Tetsuya Mukawa March 9, 2015, 2:22 a.m. UTC | #3
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];
Michael Qiu March 9, 2015, 3:49 a.m. UTC | #4
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];
>
>
Tetsuya Mukawa March 9, 2015, 5:21 a.m. UTC | #5
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];
>>
Michael Qiu March 9, 2015, 6:01 a.m. UTC | #6
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];
>
>
diff mbox

Patch

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];