[v3] app/proc-info: fix port mask parse issue
Checks
Commit Message
parse_portmask return type is int,but global variable
"enabled_port_mask" type is uint32_t.so in proc_info_parse_args
function,when parse_portmask return -1,"enabled_port_mask" will
get a huge value and "if (enabled_port_mask == 0)" will never happen.
Fixes: 22561383ea17 ("app: replace dump_cfg by proc_info")
Signed-off-by: Li Han <han.li1@zte.com.cn>
---
v3:
*fix commit meassges issue
v2:
*fix typecast issue
---
app/proc-info/main.c | 9 +++------
1 file changed, 3 insertions(+), 6 deletions(-)
Comments
> -----Original Message-----
> From: Li Han [mailto:han.li1@zte.com.cn]
> Sent: Wednesday, November 7, 2018 6:10 AM
> To: Pattan, Reshma <reshma.pattan@intel.com>
> Cc: dev@dpdk.org; Li Han <han.li1@zte.com.cn>
> Subject: [PATCH v3] [dpdk-dev] app/proc-info: fix port mask parse issue
>
> parse_portmask return type is int,but global variable "enabled_port_mask" type
> is uint32_t.so in proc_info_parse_args function,when parse_portmask return -
> 1,"enabled_port_mask" will get a huge value and "if (enabled_port_mask == 0)"
> will never happen.
>
> Fixes: 22561383ea17 ("app: replace dump_cfg by proc_info")
> Signed-off-by: Li Han <han.li1@zte.com.cn>
Acked-by: Reshma Pattan <reshma.pattan@intel.com>
Hi,
07/11/2018 07:10, Li Han:
> parse_portmask return type is int,but global variable
> "enabled_port_mask" type is uint32_t.so in proc_info_parse_args
> function,when parse_portmask return -1,"enabled_port_mask" will
> get a huge value and "if (enabled_port_mask == 0)" will never happen.
>
> Fixes: 22561383ea17 ("app: replace dump_cfg by proc_info")
> Signed-off-by: Li Han <han.li1@zte.com.cn>
[...]
> -static uint32_t enabled_port_mask;
> +static uint64_t enabled_port_mask;
[...]
> -static int
> +static unsigned long
> parse_portmask(const char *portmask)
On one side, you use uint64_t, on the other side, you use unsigned long.
I don't understand the logic behind.
Hi,
below is maintainer's suggestion,and I think it's ok to do these changes?
> -----Original Message-----> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Li HanTo avoid typecast below , please use uint64_t for enabled_port_mask. So change below function return type to uint64_t> -static int> +static uint32_t> parse_portmask(const char *portmask)> {Declare pm to be of type uint64_t.> + return (uint32_t)pm;You can remove above typecast after change to uint64_t. Thanks, Reshma
------------------原始邮件------------------
发件人:ThomasMonjalon <thomas@monjalon.net>
收件人:韩丽00112882;
抄送人:dev@dpdk.org <dev@dpdk.org>;reshma.pattan@intel.com <reshma.pattan@intel.com>;
日 期 :2018年11月14日 11:10
主 题 :Re: [dpdk-dev] [PATCH v3] app/proc-info: fix port mask parse issue
Hi,
07/11/2018 07:10, Li Han:
> parse_portmask return type is int,but global variable
> "enabled_port_mask" type is uint32_t.so in proc_info_parse_args
> function,when parse_portmask return -1,"enabled_port_mask" will
> get a huge value and "if (enabled_port_mask == 0)" will never happen.
>
> Fixes: 22561383ea17 ("app: replace dump_cfg by proc_info")
> Signed-off-by: Li Han <han.li1@zte.com.cn>
[...]
> -static uint32_t enabled_port_mask;
> +static uint64_t enabled_port_mask;
[...]
> -static int
> +static unsigned long
> parse_portmask(const char *portmask)
On one side, you use uint64_t, on the other side, you use unsigned long.
I don't understand the logic behind.
On 14-Nov-18 3:10 AM, Thomas Monjalon wrote:
> Hi,
>
> 07/11/2018 07:10, Li Han:
>> parse_portmask return type is int,but global variable
>> "enabled_port_mask" type is uint32_t.so in proc_info_parse_args
>> function,when parse_portmask return -1,"enabled_port_mask" will
>> get a huge value and "if (enabled_port_mask == 0)" will never happen.
>>
>> Fixes: 22561383ea17 ("app: replace dump_cfg by proc_info")
>> Signed-off-by: Li Han <han.li1@zte.com.cn>
> [...]
>> -static uint32_t enabled_port_mask;
>> +static uint64_t enabled_port_mask;
> [...]
>> -static int
>> +static unsigned long
>> parse_portmask(const char *portmask)
>
> On one side, you use uint64_t, on the other side, you use unsigned long.
> I don't understand the logic behind.
>
Not only there's no logic, it'll actually fail on 32-bit, since 64-bit
int is unsigned long long there :)
On 07-Nov-18 6:10 AM, Li Han wrote:
> parse_portmask return type is int,but global variable
> "enabled_port_mask" type is uint32_t.so in proc_info_parse_args
> function,when parse_portmask return -1,"enabled_port_mask" will
> get a huge value and "if (enabled_port_mask == 0)" will never happen.
>
> Fixes: 22561383ea17 ("app: replace dump_cfg by proc_info")
> Signed-off-by: Li Han <han.li1@zte.com.cn>
>
> ---
> v3:
> *fix commit meassges issue
> v2:
> *fix typecast issue
> ---
> app/proc-info/main.c | 9 +++------
> 1 file changed, 3 insertions(+), 6 deletions(-)
>
> diff --git a/app/proc-info/main.c b/app/proc-info/main.c
> index c20effa..650d599 100644
> --- a/app/proc-info/main.c
> +++ b/app/proc-info/main.c
> @@ -37,7 +37,7 @@
> #define MAX_STRING_LEN 256
>
> /**< mask of enabled ports */
> -static uint32_t enabled_port_mask;
> +static uint64_t enabled_port_mask;
> /**< Enable stats. */
> static uint32_t enable_stats;
> /**< Enable xstats. */
> @@ -90,7 +90,7 @@
> /*
> * Parse the portmask provided at run time.
> */
> -static int
> +static unsigned long
> parse_portmask(const char *portmask)
> {
> char *end = NULL;
> @@ -103,12 +103,9 @@
> if ((portmask[0] == '\0') || (end == NULL) || (*end != '\0') ||
> (errno != 0)) {
> printf("%s ERROR parsing the port mask\n", __func__);
> - return -1;
> + return 0;
> }
>
> - if (pm == 0)
> - return -1;
> -
> return pm;
>
> }
>
Hi,
This fix appears wrong. If you're making the value uint64_t, you cannot
encode errors in the value. So, it's better to leave the return type as
int, return 0 or -1 on success/error, and store the parsed result in a
pointer passed to the function instead. Something like this:
static int
parse_portmask(const char *portmask, uint64_t *mask)
{
...
if (pm == 0)
return -1;
*mask = pm;
return 0;
}
On 16-Nov-18 10:15 AM, Burakov, Anatoly wrote:
> On 07-Nov-18 6:10 AM, Li Han wrote:
>> parse_portmask return type is int,but global variable
>> "enabled_port_mask" type is uint32_t.so in proc_info_parse_args
>> function,when parse_portmask return -1,"enabled_port_mask" will
>> get a huge value and "if (enabled_port_mask == 0)" will never happen.
>>
>> Fixes: 22561383ea17 ("app: replace dump_cfg by proc_info")
>> Signed-off-by: Li Han <han.li1@zte.com.cn>
>>
>> ---
>> v3:
>> *fix commit meassges issue
>> v2:
>> *fix typecast issue
>> ---
>> app/proc-info/main.c | 9 +++------
>> 1 file changed, 3 insertions(+), 6 deletions(-)
>>
>> diff --git a/app/proc-info/main.c b/app/proc-info/main.c
>> index c20effa..650d599 100644
>> --- a/app/proc-info/main.c
>> +++ b/app/proc-info/main.c
>> @@ -37,7 +37,7 @@
>> #define MAX_STRING_LEN 256
>> /**< mask of enabled ports */
>> -static uint32_t enabled_port_mask;
>> +static uint64_t enabled_port_mask;
>> /**< Enable stats. */
>> static uint32_t enable_stats;
>> /**< Enable xstats. */
>> @@ -90,7 +90,7 @@
>> /*
>> * Parse the portmask provided at run time.
>> */
>> -static int
>> +static unsigned long
>> parse_portmask(const char *portmask)
>> {
>> char *end = NULL;
>> @@ -103,12 +103,9 @@
>> if ((portmask[0] == '\0') || (end == NULL) || (*end != '\0') ||
>> (errno != 0)) {
>> printf("%s ERROR parsing the port mask\n", __func__);
>> - return -1;
>> + return 0;
>> }
>> - if (pm == 0)
>> - return -1;
>> -
>> return pm;
>> }
>>
>
> Hi,
>
> This fix appears wrong. If you're making the value uint64_t, you cannot
> encode errors in the value. So, it's better to leave the return type as
> int, return 0 or -1 on success/error, and store the parsed result in a
> pointer passed to the function instead. Something like this:
>
> static int
> parse_portmask(const char *portmask, uint64_t *mask)
> {
> ...
> if (pm == 0)
> return -1;
> *mask = pm;
> return 0;
> }
>
Also, another thing to note. Unsigned long is 32-bit on 32-bit Linux, so
if you're going to have uint64_t data, you should correct the parsing to
use strtoull() instead.
@@ -37,7 +37,7 @@
#define MAX_STRING_LEN 256
/**< mask of enabled ports */
-static uint32_t enabled_port_mask;
+static uint64_t enabled_port_mask;
/**< Enable stats. */
static uint32_t enable_stats;
/**< Enable xstats. */
@@ -90,7 +90,7 @@
/*
* Parse the portmask provided at run time.
*/
-static int
+static unsigned long
parse_portmask(const char *portmask)
{
char *end = NULL;
@@ -103,12 +103,9 @@
if ((portmask[0] == '\0') || (end == NULL) || (*end != '\0') ||
(errno != 0)) {
printf("%s ERROR parsing the port mask\n", __func__);
- return -1;
+ return 0;
}
- if (pm == 0)
- return -1;
-
return pm;
}