[PATCH 1/4] app/testpmd: fix stats-period option check

[PATCH 1/4] app/testpmd: fix stats-period option check

[David Marchand]

Rather than silently ignore an invalid value, raise an error for
stats-period user input.

Fixes: cfea1f3048d1 ("app/testpmd: print statistics periodically")
Cc: stable@dpdk.org

Signed-off-by: David Marchand <david.marchand@redhat.com>
---
 app/test-pmd/parameters.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/app/test-pmd/parameters.c b/app/test-pmd/parameters.c
index 11b0cce577..d715750bb8 100644
--- a/app/test-pmd/parameters.c
+++ b/app/test-pmd/parameters.c
@@ -776,7 +776,7 @@ launch_args_parse(int argc, char** argv)
 				n = strtoul(optarg, &end, 10);
 				if ((optarg[0] == '\0') || (end == NULL) ||
 						(*end != '\0'))
-					break;
+					rte_exit(EXIT_FAILURE, "Invalid stats-period value\n");
 
 				stats_period = n;
 				break;
-- 
2.44.0

Re: [PATCH 1/4] app/testpmd: fix stats-period option check

[Ferruh Yigit]

On 3/8/2024 2:48 PM, David Marchand wrote:
> Rather than silently ignore an invalid value, raise an error for
> stats-period user input.
> 
> Fixes: cfea1f3048d1 ("app/testpmd: print statistics periodically")
> Cc: stable@dpdk.org
> 
> Signed-off-by: David Marchand <david.marchand@redhat.com>
>

Acked-by: Ferruh Yigit <ferruh.yigit@amd.com>

[PATCH 2/4] app/testpmd: fix burst option parsing

[David Marchand]

rte_eth_dev_info_get() is not supposed to fail for a valid port_id, but
for the theoretical case when it would fail, raise an error rather than
skip subsequent options.

Fixes: 6f51deb903b2 ("app/testpmd: check status of getting ethdev info")
Cc: stable@dpdk.org

Signed-off-by: David Marchand <david.marchand@redhat.com>
---
 app/test-pmd/parameters.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/app/test-pmd/parameters.c b/app/test-pmd/parameters.c
index d715750bb8..8c21744009 100644
--- a/app/test-pmd/parameters.c
+++ b/app/test-pmd/parameters.c
@@ -1128,9 +1128,9 @@ launch_args_parse(int argc, char** argv)
 								0,
 								&dev_info);
 					if (ret != 0)
-						return;
-
-					rec_nb_pkts = dev_info
+						rec_nb_pkts = 0;
+					else
+						rec_nb_pkts = dev_info
 						.default_rxportconf.burst_size;
 
 					if (rec_nb_pkts == 0)
-- 
2.44.0

Re: [PATCH 2/4] app/testpmd: fix burst option parsing

[Ferruh Yigit]

On 3/8/2024 2:48 PM, David Marchand wrote:
> rte_eth_dev_info_get() is not supposed to fail for a valid port_id, but
> for the theoretical case when it would fail, raise an error rather than
> skip subsequent options.
> 
> Fixes: 6f51deb903b2 ("app/testpmd: check status of getting ethdev info")
> Cc: stable@dpdk.org
> 
> Signed-off-by: David Marchand <david.marchand@redhat.com>
> ---
>  app/test-pmd/parameters.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/app/test-pmd/parameters.c b/app/test-pmd/parameters.c
> index d715750bb8..8c21744009 100644
> --- a/app/test-pmd/parameters.c
> +++ b/app/test-pmd/parameters.c
> @@ -1128,9 +1128,9 @@ launch_args_parse(int argc, char** argv)
>  								0,
>  								&dev_info);
>  					if (ret != 0)
> -						return;
> -
> -					rec_nb_pkts = dev_info
> +						rec_nb_pkts = 0;
> +					else
> +						rec_nb_pkts = dev_info
>  						.default_rxportconf.burst_size;
>  
>  					if (rec_nb_pkts == 0)

'eth_dev_info_get_print_err()' already fail, but it may not be very
clear to the user,
OK to print a failure log, but setting 'rec_nb_pkts = 0;' as above also
will generate an error message that also may be confusing to the user.

What about print an explicit error message for the
'eth_dev_info_get_print_err()' failed case?

Re: [PATCH 2/4] app/testpmd: fix burst option parsing

[David Marchand]

On Tue, Mar 12, 2024 at 5:47 PM Ferruh Yigit <ferruh.yigit@amd.com> wrote:
>
> On 3/8/2024 2:48 PM, David Marchand wrote:
> > rte_eth_dev_info_get() is not supposed to fail for a valid port_id, but
> > for the theoretical case when it would fail, raise an error rather than
> > skip subsequent options.
> >
> > Fixes: 6f51deb903b2 ("app/testpmd: check status of getting ethdev info")
> > Cc: stable@dpdk.org
> >
> > Signed-off-by: David Marchand <david.marchand@redhat.com>
> > ---
> >  app/test-pmd/parameters.c | 6 +++---
> >  1 file changed, 3 insertions(+), 3 deletions(-)
> >
> > diff --git a/app/test-pmd/parameters.c b/app/test-pmd/parameters.c
> > index d715750bb8..8c21744009 100644
> > --- a/app/test-pmd/parameters.c
> > +++ b/app/test-pmd/parameters.c
> > @@ -1128,9 +1128,9 @@ launch_args_parse(int argc, char** argv)
> >                                                               0,
> >                                                               &dev_info);
> >                                       if (ret != 0)
> > -                                             return;
> > -
> > -                                     rec_nb_pkts = dev_info
> > +                                             rec_nb_pkts = 0;
> > +                                     else
> > +                                             rec_nb_pkts = dev_info
> >                                               .default_rxportconf.burst_size;
> >
> >                                       if (rec_nb_pkts == 0)
>
> 'eth_dev_info_get_print_err()' already fail, but it may not be very
> clear to the user,
> OK to print a failure log, but setting 'rec_nb_pkts = 0;' as above also
> will generate an error message that also may be confusing to the user.
>
> What about print an explicit error message for the
> 'eth_dev_info_get_print_err()' failed case?

rte_exit(EXIT_FAILURE, "Failed to retrieve device info, this is
probably a driver bug. "
        "To workaround this issue, please provide a value between 1
and %d\n", MAX_PKT_BURST);

Does it work for you?


-- 
David Marchand

Re: [PATCH 2/4] app/testpmd: fix burst option parsing

[Ferruh Yigit]

On 3/13/2024 7:24 AM, David Marchand wrote:
> On Tue, Mar 12, 2024 at 5:47 PM Ferruh Yigit <ferruh.yigit@amd.com> wrote:
>>
>> On 3/8/2024 2:48 PM, David Marchand wrote:
>>> rte_eth_dev_info_get() is not supposed to fail for a valid port_id, but
>>> for the theoretical case when it would fail, raise an error rather than
>>> skip subsequent options.
>>>
>>> Fixes: 6f51deb903b2 ("app/testpmd: check status of getting ethdev info")
>>> Cc: stable@dpdk.org
>>>
>>> Signed-off-by: David Marchand <david.marchand@redhat.com>
>>> ---
>>>  app/test-pmd/parameters.c | 6 +++---
>>>  1 file changed, 3 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/app/test-pmd/parameters.c b/app/test-pmd/parameters.c
>>> index d715750bb8..8c21744009 100644
>>> --- a/app/test-pmd/parameters.c
>>> +++ b/app/test-pmd/parameters.c
>>> @@ -1128,9 +1128,9 @@ launch_args_parse(int argc, char** argv)
>>>                                                               0,
>>>                                                               &dev_info);
>>>                                       if (ret != 0)
>>> -                                             return;
>>> -
>>> -                                     rec_nb_pkts = dev_info
>>> +                                             rec_nb_pkts = 0;
>>> +                                     else
>>> +                                             rec_nb_pkts = dev_info
>>>                                               .default_rxportconf.burst_size;
>>>
>>>                                       if (rec_nb_pkts == 0)
>>
>> 'eth_dev_info_get_print_err()' already fail, but it may not be very
>> clear to the user,
>> OK to print a failure log, but setting 'rec_nb_pkts = 0;' as above also
>> will generate an error message that also may be confusing to the user.
>>
>> What about print an explicit error message for the
>> 'eth_dev_info_get_print_err()' failed case?
> 
> rte_exit(EXIT_FAILURE, "Failed to retrieve device info, this is
> probably a driver bug. "
>         "To workaround this issue, please provide a value between 1
> and %d\n", MAX_PKT_BURST);
> 
> Does it work for you?
> 
> 

'eth_dev_info_get_print_err()' already logs error about getting device
info, but driver recommended 'burst' setting failed information is missing.

What about more direct,
"Failed to get driver recommended burst size, please provide a value
between 1 and MAX_PKT_BURST"

Re: [PATCH 2/4] app/testpmd: fix burst option parsing

[David Marchand]

On Wed, Mar 13, 2024 at 11:37 AM Ferruh Yigit <ferruh.yigit@amd.com> wrote:
>
> On 3/13/2024 7:24 AM, David Marchand wrote:
> > On Tue, Mar 12, 2024 at 5:47 PM Ferruh Yigit <ferruh.yigit@amd.com> wrote:
> >>
> >> On 3/8/2024 2:48 PM, David Marchand wrote:
> >>> rte_eth_dev_info_get() is not supposed to fail for a valid port_id, but
> >>> for the theoretical case when it would fail, raise an error rather than
> >>> skip subsequent options.
> >>>
> >>> Fixes: 6f51deb903b2 ("app/testpmd: check status of getting ethdev info")
> >>> Cc: stable@dpdk.org
> >>>
> >>> Signed-off-by: David Marchand <david.marchand@redhat.com>
> >>> ---
> >>>  app/test-pmd/parameters.c | 6 +++---
> >>>  1 file changed, 3 insertions(+), 3 deletions(-)
> >>>
> >>> diff --git a/app/test-pmd/parameters.c b/app/test-pmd/parameters.c
> >>> index d715750bb8..8c21744009 100644
> >>> --- a/app/test-pmd/parameters.c
> >>> +++ b/app/test-pmd/parameters.c
> >>> @@ -1128,9 +1128,9 @@ launch_args_parse(int argc, char** argv)
> >>>                                                               0,
> >>>                                                               &dev_info);
> >>>                                       if (ret != 0)
> >>> -                                             return;
> >>> -
> >>> -                                     rec_nb_pkts = dev_info
> >>> +                                             rec_nb_pkts = 0;
> >>> +                                     else
> >>> +                                             rec_nb_pkts = dev_info
> >>>                                               .default_rxportconf.burst_size;
> >>>
> >>>                                       if (rec_nb_pkts == 0)
> >>
> >> 'eth_dev_info_get_print_err()' already fail, but it may not be very
> >> clear to the user,
> >> OK to print a failure log, but setting 'rec_nb_pkts = 0;' as above also
> >> will generate an error message that also may be confusing to the user.
> >>
> >> What about print an explicit error message for the
> >> 'eth_dev_info_get_print_err()' failed case?
> >
> > rte_exit(EXIT_FAILURE, "Failed to retrieve device info, this is
> > probably a driver bug. "
> >         "To workaround this issue, please provide a value between 1
> > and %d\n", MAX_PKT_BURST);
> >
> > Does it work for you?
> >
> >
>
> 'eth_dev_info_get_print_err()' already logs error about getting device
> info, but driver recommended 'burst' setting failed information is missing.
>
> What about more direct,
> "Failed to get driver recommended burst size, please provide a value
> between 1 and MAX_PKT_BURST"

Which is really close to the existing log message.

                                        if (rec_nb_pkts == 0)
                                                rte_exit(EXIT_FAILURE,
                                                        "PMD does not
recommend a burst size. "
                                                        "Provided
value must be between "
                                                        "1 and %d\n",
MAX_PKT_BURST);

I am unconvinced, but if you think strongly for this, I won't debate more.


-- 
David Marchand

Re: [PATCH 2/4] app/testpmd: fix burst option parsing

[Ferruh Yigit]

On 3/13/2024 11:13 AM, David Marchand wrote:
> On Wed, Mar 13, 2024 at 11:37 AM Ferruh Yigit <ferruh.yigit@amd.com> wrote:
>>
>> On 3/13/2024 7:24 AM, David Marchand wrote:
>>> On Tue, Mar 12, 2024 at 5:47 PM Ferruh Yigit <ferruh.yigit@amd.com> wrote:
>>>>
>>>> On 3/8/2024 2:48 PM, David Marchand wrote:
>>>>> rte_eth_dev_info_get() is not supposed to fail for a valid port_id, but
>>>>> for the theoretical case when it would fail, raise an error rather than
>>>>> skip subsequent options.
>>>>>
>>>>> Fixes: 6f51deb903b2 ("app/testpmd: check status of getting ethdev info")
>>>>> Cc: stable@dpdk.org
>>>>>
>>>>> Signed-off-by: David Marchand <david.marchand@redhat.com>
>>>>> ---
>>>>>  app/test-pmd/parameters.c | 6 +++---
>>>>>  1 file changed, 3 insertions(+), 3 deletions(-)
>>>>>
>>>>> diff --git a/app/test-pmd/parameters.c b/app/test-pmd/parameters.c
>>>>> index d715750bb8..8c21744009 100644
>>>>> --- a/app/test-pmd/parameters.c
>>>>> +++ b/app/test-pmd/parameters.c
>>>>> @@ -1128,9 +1128,9 @@ launch_args_parse(int argc, char** argv)
>>>>>                                                               0,
>>>>>                                                               &dev_info);
>>>>>                                       if (ret != 0)
>>>>> -                                             return;
>>>>> -
>>>>> -                                     rec_nb_pkts = dev_info
>>>>> +                                             rec_nb_pkts = 0;
>>>>> +                                     else
>>>>> +                                             rec_nb_pkts = dev_info
>>>>>                                               .default_rxportconf.burst_size;
>>>>>
>>>>>                                       if (rec_nb_pkts == 0)
>>>>
>>>> 'eth_dev_info_get_print_err()' already fail, but it may not be very
>>>> clear to the user,
>>>> OK to print a failure log, but setting 'rec_nb_pkts = 0;' as above also
>>>> will generate an error message that also may be confusing to the user.
>>>>
>>>> What about print an explicit error message for the
>>>> 'eth_dev_info_get_print_err()' failed case?
>>>
>>> rte_exit(EXIT_FAILURE, "Failed to retrieve device info, this is
>>> probably a driver bug. "
>>>         "To workaround this issue, please provide a value between 1
>>> and %d\n", MAX_PKT_BURST);
>>>
>>> Does it work for you?
>>>
>>>
>>
>> 'eth_dev_info_get_print_err()' already logs error about getting device
>> info, but driver recommended 'burst' setting failed information is missing.
>>
>> What about more direct,
>> "Failed to get driver recommended burst size, please provide a value
>> between 1 and MAX_PKT_BURST"
> 
> Which is really close to the existing log message.
> 
>                                         if (rec_nb_pkts == 0)
>                                                 rte_exit(EXIT_FAILURE,
>                                                         "PMD does not
> recommend a burst size. "
>                                                         "Provided
> value must be between "
>                                                         "1 and %d\n",
> MAX_PKT_BURST);
> 
> I am unconvinced, but if you think strongly for this, I won't debate more.
> 
> 

Yes it is close, and I don't have strong opinion on this,

But existing message says "PMD does not recommend a burst size.", I
think this may be confusing for user.

If I see that message I would either debug relevant driver code or
communicate with driver maintainer for it. But that is not the case,
just failed to get recommended burst size and problem is in testpmd.

Anyway, my concern is existing message can be misleading for user.

Re: [PATCH 2/4] app/testpmd: fix burst option parsing

[Stephen Hemminger]

On Wed, 13 Mar 2024 12:09:01 +0000
Ferruh Yigit <ferruh.yigit@amd.com> wrote:

> > Which is really close to the existing log message.
> > 
> >                                         if (rec_nb_pkts == 0)
> >                                                 rte_exit(EXIT_FAILURE,
> >                                                         "PMD does not
> > recommend a burst size. "
> >                                                         "Provided
> > value must be between "
> >                                                         "1 and %d\n",
> > MAX_PKT_BURST);
> > 
> > I am unconvinced, but if you think strongly for this, I won't debate more.
> > 
> >   
> 
> Yes it is close, and I don't have strong opinion on this,
> 
> But existing message says "PMD does not recommend a burst size.", I
> think this may be confusing for user.
> 
> If I see that message I would either debug relevant driver code or
> communicate with driver maintainer for it. But that is not the case,
> just failed to get recommended burst size and problem is in testpmd.
> 
> Anyway, my concern is existing message can be misleading for user.

Maybe shorter:
 	"PMD does not provide required burst size\n"

[PATCH v2 1/6] app/testpmd: fix stats-period option check

[David Marchand]

Rather than silently ignore an invalid value, raise an error for
stats-period user input.

Fixes: cfea1f3048d1 ("app/testpmd: print statistics periodically")
Cc: stable@dpdk.org

Signed-off-by: David Marchand <david.marchand@redhat.com>
Acked-by: Ferruh Yigit <ferruh.yigit@amd.com>
---
 app/test-pmd/parameters.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/app/test-pmd/parameters.c b/app/test-pmd/parameters.c
index 11b0cce577..d715750bb8 100644
--- a/app/test-pmd/parameters.c
+++ b/app/test-pmd/parameters.c
@@ -776,7 +776,7 @@ launch_args_parse(int argc, char** argv)
 				n = strtoul(optarg, &end, 10);
 				if ((optarg[0] == '\0') || (end == NULL) ||
 						(*end != '\0'))
-					break;
+					rte_exit(EXIT_FAILURE, "Invalid stats-period value\n");
 
 				stats_period = n;
 				break;
-- 
2.44.0

[PATCH v2 2/6] app/testpmd: fix burst option parsing

[David Marchand]

rte_eth_dev_info_get() is not supposed to fail for a valid port_id, but
for the theoretical case when it would fail, raise an error rather than
skip subsequent options.

Fixes: 6f51deb903b2 ("app/testpmd: check status of getting ethdev info")
Cc: stable@dpdk.org

Signed-off-by: David Marchand <david.marchand@redhat.com>
---
Changes since v1:
- switched to a dedicated error message,

---
 app/test-pmd/parameters.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/app/test-pmd/parameters.c b/app/test-pmd/parameters.c
index d715750bb8..3414a0d38c 100644
--- a/app/test-pmd/parameters.c
+++ b/app/test-pmd/parameters.c
@@ -1128,7 +1128,9 @@ launch_args_parse(int argc, char** argv)
 								0,
 								&dev_info);
 					if (ret != 0)
-						return;
+						rte_exit(EXIT_FAILURE, "Failed to get driver "
+							"recommended burst size, please provide a "
+							"value between 1 and %d\n", MAX_PKT_BURST);
 
 					rec_nb_pkts = dev_info
 						.default_rxportconf.burst_size;
-- 
2.44.0

Re: [PATCH v2 2/6] app/testpmd: fix burst option parsing

[Ferruh Yigit]

On 3/14/2024 9:17 AM, David Marchand wrote:
> rte_eth_dev_info_get() is not supposed to fail for a valid port_id, but
> for the theoretical case when it would fail, raise an error rather than
> skip subsequent options.
> 
> Fixes: 6f51deb903b2 ("app/testpmd: check status of getting ethdev info")
> Cc: stable@dpdk.org
> 
> Signed-off-by: David Marchand <david.marchand@redhat.com>
>

Acked-by: Ferruh Yigit <ferruh.yigit@amd.com>

[PATCH v2 3/6] app/testpmd: fix error message for invalid option

[David Marchand]

"""
The variable optind is the index of the next element to be processed in
argv.  The system initializes this value to 1.  The caller can reset it
to 1 to restart scanning of the same argv, or when scanning a new
argument vector.
"""

Hence, if an invalid option is passed through testpmd cmdline, getopt
returns '?' and increments optind to the next index in argv for a
subsequent call.
The message should log the previous index.

Fixes: 8fad2e5ab2c5 ("app/testpmd: report invalid command line parameter")
Cc: stable@dpdk.org

Signed-off-by: David Marchand <david.marchand@redhat.com>
---
 app/test-pmd/parameters.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/app/test-pmd/parameters.c b/app/test-pmd/parameters.c
index 3414a0d38c..a4c09e2a2b 100644
--- a/app/test-pmd/parameters.c
+++ b/app/test-pmd/parameters.c
@@ -1497,7 +1497,7 @@ launch_args_parse(int argc, char** argv)
 			break;
 		default:
 			usage(argv[0]);
-			fprintf(stderr, "Invalid option: %s\n", argv[optind]);
+			fprintf(stderr, "Invalid option: %s\n", argv[optind - 1]);
 			rte_exit(EXIT_FAILURE,
 				 "Command line is incomplete or incorrect\n");
 			break;
-- 
2.44.0

Re: [PATCH v2 3/6] app/testpmd: fix error message for invalid option

[Ferruh Yigit]

On 3/14/2024 9:17 AM, David Marchand wrote:
> """
> The variable optind is the index of the next element to be processed in
> argv.  The system initializes this value to 1.  The caller can reset it
> to 1 to restart scanning of the same argv, or when scanning a new
> argument vector.
> """
> 
> Hence, if an invalid option is passed through testpmd cmdline, getopt
> returns '?' and increments optind to the next index in argv for a
> subsequent call.
> The message should log the previous index.
> 
> Fixes: 8fad2e5ab2c5 ("app/testpmd: report invalid command line parameter")
> Cc: stable@dpdk.org
> 
> Signed-off-by: David Marchand <david.marchand@redhat.com>
>

Acked-by: Ferruh Yigit <ferruh.yigit@amd.com>