DPDK patches and discussions
 help / color / mirror / Atom feed
* [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  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

* 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

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).