[v3] app/testpmd: fix support of hex string parser for flow API
Checks
Commit Message
There is need for users to set configuration of HEX number for RSS
key. The key byte should be pass down as hex number not as char
string. This patch enable cmdline flow parse HEX number,
in order to not using string which pass ASIC number.
Fixes: f4d623f96119 ("app/testpmd: fix missing RSS fields in flow action")
Cc: stable@dpdk.org
Signed-off-by: Wei Zhao <wei.zhao1@intel.com>
Tested-by: Peng Yuan <yuan.peng@intel.com>
---
v2:
fix bug of string length limite.
v3:
use isxdigit to check char string
---
app/test-pmd/cmdline_flow.c | 145 +++++++++++++++++++++++++++++++++++++++++++-
1 file changed, 144 insertions(+), 1 deletion(-)
Comments
On 3/22/2019 3:15 AM, Wei Zhao wrote:
> There is need for users to set configuration of HEX number for RSS
> key. The key byte should be pass down as hex number not as char
> string. This patch enable cmdline flow parse HEX number,
> in order to not using string which pass ASIC number.
>
> Fixes: f4d623f96119 ("app/testpmd: fix missing RSS fields in flow action")
> Cc: stable@dpdk.org
>
> Signed-off-by: Wei Zhao <wei.zhao1@intel.com>
> Tested-by: Peng Yuan <yuan.peng@intel.com>
<...>
> @@ -4475,6 +4486,138 @@ parse_string(struct context *ctx, const struct token *token,
> return -1;
> }
>
> +static uint32_t
> +get_hex_val(char c)
> +{
> + switch (c) {
> + case '0': case '1': case '2': case '3': case '4': case '5':
> + case '6': case '7': case '8': case '9':
> + return c - '0';
> + case 'A': case 'B': case 'C': case 'D': case 'E': case 'F':
> + return c - 'A' + 10;
> + case 'a': case 'b': case 'c': case 'd': case 'e': case 'f':
> + return c - 'a' + 10;
> + default:
> + return 0;
> + }
> +}
> +
> +static int
> +parse_hex_string(const char *src, uint8_t *dst, uint32_t *size)
> +{
> + const char *c;
> + uint32_t i;
> +
> + /* Check input parameters */
> + if ((src == NULL) ||
> + (dst == NULL) ||
> + (size == NULL) ||
> + (*size == 0))
> + return -1;
> + if ((*size & 1) != 0)
> + return -1;
> +
> + for (c = src, i = 0; i < *size; c++, i++) {
> + if (isxdigit(*c))
> + continue;
> + else
> + return -1;
> + }
> +
> + *size = *size / 2;
> +
> + /* Convert chars to bytes */
> + for (i = 0; i < *size; i++)
> + dst[i] = get_hex_val(src[2 * i]) * 16 +
> + get_hex_val(src[2 * i + 1]);
> +
> + return 0;
> +}
I can see this has been discussed already but what would you think updating the
'parse_hex_string' something like following, it is less code to maintain:
static int
parse_hex_string(const char *src, uint8_t *dst, uint32_t *size)
{
int len;
int i
for (i = 0, len = 0; i < *size; i += 2) {
char tmp[3];
snprintf(tmp, 3, src + i);
dst[len++] = strtoul(tmp, NULL, 16);
}
dst[len] = 0;
*size = len;
return 0;
}
(indeed with better error checking on strtoul ;) )
<...>
> + /* Output buffer is not necessarily NUL-terminated. */
> + memcpy(buf, hex_tmp, hexlen);
> + memset((uint8_t *)buf + len, 0x00, size - hexlen);
Can't this overflow the 'buf'? since "len = 2 * hexlen"
I guess intention is "buf + hexlen"
Hi,
> -----Original Message-----
> From: Yigit, Ferruh
> Sent: Friday, March 22, 2019 10:56 PM
> To: Zhao1, Wei <wei.zhao1@intel.com>; dev@dpdk.org
> Cc: stable@dpdk.org; stephen@networkplumber.org; Ananyev, Konstantin
> <konstantin.ananyev@intel.com>
> Subject: Re: [dpdk-stable] [PATCH v3] app/testpmd: fix support of hex string
> parser for flow API
>
> On 3/22/2019 3:15 AM, Wei Zhao wrote:
> > There is need for users to set configuration of HEX number for RSS
> > key. The key byte should be pass down as hex number not as char
> > string. This patch enable cmdline flow parse HEX number, in order to
> > not using string which pass ASIC number.
> >
> > Fixes: f4d623f96119 ("app/testpmd: fix missing RSS fields in flow
> > action")
> > Cc: stable@dpdk.org
> >
> > Signed-off-by: Wei Zhao <wei.zhao1@intel.com>
> > Tested-by: Peng Yuan <yuan.peng@intel.com>
>
> <...>
>
> > @@ -4475,6 +4486,138 @@ parse_string(struct context *ctx, const struct
> token *token,
> > return -1;
> > }
> >
> > +static uint32_t
> > +get_hex_val(char c)
> > +{
> > + switch (c) {
> > + case '0': case '1': case '2': case '3': case '4': case '5':
> > + case '6': case '7': case '8': case '9':
> > + return c - '0';
> > + case 'A': case 'B': case 'C': case 'D': case 'E': case 'F':
> > + return c - 'A' + 10;
> > + case 'a': case 'b': case 'c': case 'd': case 'e': case 'f':
> > + return c - 'a' + 10;
> > + default:
> > + return 0;
> > + }
> > +}
> > +
> > +static int
> > +parse_hex_string(const char *src, uint8_t *dst, uint32_t *size) {
> > + const char *c;
> > + uint32_t i;
> > +
> > + /* Check input parameters */
> > + if ((src == NULL) ||
> > + (dst == NULL) ||
> > + (size == NULL) ||
> > + (*size == 0))
> > + return -1;
> > + if ((*size & 1) != 0)
> > + return -1;
> > +
> > + for (c = src, i = 0; i < *size; c++, i++) {
> > + if (isxdigit(*c))
> > + continue;
> > + else
> > + return -1;
> > + }
> > +
> > + *size = *size / 2;
> > +
> > + /* Convert chars to bytes */
> > + for (i = 0; i < *size; i++)
> > + dst[i] = get_hex_val(src[2 * i]) * 16 +
> > + get_hex_val(src[2 * i + 1]);
> > +
> > + return 0;
> > +}
>
> I can see this has been discussed already but what would you think updating
> the 'parse_hex_string' something like following, it is less code to maintain:
>
> static int
> parse_hex_string(const char *src, uint8_t *dst, uint32_t *size) {
> int len;
> int i
> for (i = 0, len = 0; i < *size; i += 2) {
> char tmp[3];
> snprintf(tmp, 3, src + i);
> dst[len++] = strtoul(tmp, NULL, 16);
> }
> dst[len] = 0;
> *size = len;
> return 0;
> }
>
> (indeed with better error checking on strtoul ;) )
Why delete these check from parse_hex_string()?
/* Check input parameters */
if ((src == NULL) ||
(dst == NULL) ||
(size == NULL) ||
(*size == 0))
return -1;
if ((*size & 1) != 0)
return -1;
for (c = src, i = 0; i < *size; c++, i++) {
if (isxdigit(*c))
continue;
else
return -1;
}
>
> <...>
>
> > + /* Output buffer is not necessarily NUL-terminated. */
> > + memcpy(buf, hex_tmp, hexlen);
> > + memset((uint8_t *)buf + len, 0x00, size - hexlen);
>
> Can't this overflow the 'buf'? since "len = 2 * hexlen"
> I guess intention is "buf + hexlen"
On 3/25/2019 3:39 AM, Zhao1, Wei wrote:
> Hi,
>
>> -----Original Message-----
>> From: Yigit, Ferruh
>> Sent: Friday, March 22, 2019 10:56 PM
>> To: Zhao1, Wei <wei.zhao1@intel.com>; dev@dpdk.org
>> Cc: stable@dpdk.org; stephen@networkplumber.org; Ananyev, Konstantin
>> <konstantin.ananyev@intel.com>
>> Subject: Re: [dpdk-stable] [PATCH v3] app/testpmd: fix support of hex string
>> parser for flow API
>>
>> On 3/22/2019 3:15 AM, Wei Zhao wrote:
>>> There is need for users to set configuration of HEX number for RSS
>>> key. The key byte should be pass down as hex number not as char
>>> string. This patch enable cmdline flow parse HEX number, in order to
>>> not using string which pass ASIC number.
>>>
>>> Fixes: f4d623f96119 ("app/testpmd: fix missing RSS fields in flow
>>> action")
>>> Cc: stable@dpdk.org
>>>
>>> Signed-off-by: Wei Zhao <wei.zhao1@intel.com>
>>> Tested-by: Peng Yuan <yuan.peng@intel.com>
>>
>> <...>
>>
>>> @@ -4475,6 +4486,138 @@ parse_string(struct context *ctx, const struct
>> token *token,
>>> return -1;
>>> }
>>>
>>> +static uint32_t
>>> +get_hex_val(char c)
>>> +{
>>> + switch (c) {
>>> + case '0': case '1': case '2': case '3': case '4': case '5':
>>> + case '6': case '7': case '8': case '9':
>>> + return c - '0';
>>> + case 'A': case 'B': case 'C': case 'D': case 'E': case 'F':
>>> + return c - 'A' + 10;
>>> + case 'a': case 'b': case 'c': case 'd': case 'e': case 'f':
>>> + return c - 'a' + 10;
>>> + default:
>>> + return 0;
>>> + }
>>> +}
>>> +
>>> +static int
>>> +parse_hex_string(const char *src, uint8_t *dst, uint32_t *size) {
>>> + const char *c;
>>> + uint32_t i;
>>> +
>>> + /* Check input parameters */
>>> + if ((src == NULL) ||
>>> + (dst == NULL) ||
>>> + (size == NULL) ||
>>> + (*size == 0))
>>> + return -1;
>>> + if ((*size & 1) != 0)
>>> + return -1;
>>> +
>>> + for (c = src, i = 0; i < *size; c++, i++) {
>>> + if (isxdigit(*c))
>>> + continue;
>>> + else
>>> + return -1;
>>> + }
>>> +
>>> + *size = *size / 2;
>>> +
>>> + /* Convert chars to bytes */
>>> + for (i = 0; i < *size; i++)
>>> + dst[i] = get_hex_val(src[2 * i]) * 16 +
>>> + get_hex_val(src[2 * i + 1]);
>>> +
>>> + return 0;
>>> +}
>>
>> I can see this has been discussed already but what would you think updating
>> the 'parse_hex_string' something like following, it is less code to maintain:
>>
>> static int
>> parse_hex_string(const char *src, uint8_t *dst, uint32_t *size) {
>> int len;
>> int i
>> for (i = 0, len = 0; i < *size; i += 2) {
>> char tmp[3];
>> snprintf(tmp, 3, src + i);
>> dst[len++] = strtoul(tmp, NULL, 16);
>> }
>> dst[len] = 0;
>> *size = len;
>> return 0;
>> }
>>
>> (indeed with better error checking on strtoul ;) )
>
>
> Why delete these check from parse_hex_string()?
The point is using 'strtoul' instead of your functions, so that you won't need
'get_hex_val()' at all, or won't need 'isxdigit()' because 'strtoul' will check
it, won't need size should be multiply of two restriction '(*size & 1)' because
of implementation change. Probably you will need NULL checks, but again point is
why not using 'strtoul' instead of writing your version of it?
>
> /* Check input parameters */
> if ((src == NULL) ||
> (dst == NULL) ||
> (size == NULL) ||
> (*size == 0))
> return -1;
> if ((*size & 1) != 0)
> return -1;
> for (c = src, i = 0; i < *size; c++, i++) {
> if (isxdigit(*c))
> continue;
> else
> return -1;
> }
>
>
>
>>
>> <...>
>>
>>> + /* Output buffer is not necessarily NUL-terminated. */
>>> + memcpy(buf, hex_tmp, hexlen);
>>> + memset((uint8_t *)buf + len, 0x00, size - hexlen);
>>
>> Can't this overflow the 'buf'? since "len = 2 * hexlen"
>> I guess intention is "buf + hexlen"
Hi，Ferruh
> -----Original Message-----
> From: Yigit, Ferruh
> Sent: Monday, March 25, 2019 4:46 PM
> To: Zhao1, Wei <wei.zhao1@intel.com>; dev@dpdk.org
> Cc: stable@dpdk.org; stephen@networkplumber.org; Ananyev, Konstantin
> <konstantin.ananyev@intel.com>
> Subject: Re: [dpdk-stable] [PATCH v3] app/testpmd: fix support of hex string
> parser for flow API
>
> On 3/25/2019 3:39 AM, Zhao1, Wei wrote:
> > Hi,
> >
> >> -----Original Message-----
> >> From: Yigit, Ferruh
> >> Sent: Friday, March 22, 2019 10:56 PM
> >> To: Zhao1, Wei <wei.zhao1@intel.com>; dev@dpdk.org
> >> Cc: stable@dpdk.org; stephen@networkplumber.org; Ananyev,
> Konstantin
> >> <konstantin.ananyev@intel.com>
> >> Subject: Re: [dpdk-stable] [PATCH v3] app/testpmd: fix support of hex
> >> string parser for flow API
> >>
> >> On 3/22/2019 3:15 AM, Wei Zhao wrote:
> >>> There is need for users to set configuration of HEX number for RSS
> >>> key. The key byte should be pass down as hex number not as char
> >>> string. This patch enable cmdline flow parse HEX number, in order to
> >>> not using string which pass ASIC number.
> >>>
> >>> Fixes: f4d623f96119 ("app/testpmd: fix missing RSS fields in flow
> >>> action")
> >>> Cc: stable@dpdk.org
> >>>
> >>> Signed-off-by: Wei Zhao <wei.zhao1@intel.com>
> >>> Tested-by: Peng Yuan <yuan.peng@intel.com>
> >>
> >> <...>
> >>
> >>> @@ -4475,6 +4486,138 @@ parse_string(struct context *ctx, const
> >>> struct
> >> token *token,
> >>> return -1;
> >>> }
> >>>
> >>> +static uint32_t
> >>> +get_hex_val(char c)
> >>> +{
> >>> + switch (c) {
> >>> + case '0': case '1': case '2': case '3': case '4': case '5':
> >>> + case '6': case '7': case '8': case '9':
> >>> + return c - '0';
> >>> + case 'A': case 'B': case 'C': case 'D': case 'E': case 'F':
> >>> + return c - 'A' + 10;
> >>> + case 'a': case 'b': case 'c': case 'd': case 'e': case 'f':
> >>> + return c - 'a' + 10;
> >>> + default:
> >>> + return 0;
> >>> + }
> >>> +}
> >>> +
> >>> +static int
> >>> +parse_hex_string(const char *src, uint8_t *dst, uint32_t *size) {
> >>> + const char *c;
> >>> + uint32_t i;
> >>> +
> >>> + /* Check input parameters */
> >>> + if ((src == NULL) ||
> >>> + (dst == NULL) ||
> >>> + (size == NULL) ||
> >>> + (*size == 0))
> >>> + return -1;
> >>> + if ((*size & 1) != 0)
> >>> + return -1;
> >>> +
> >>> + for (c = src, i = 0; i < *size; c++, i++) {
> >>> + if (isxdigit(*c))
> >>> + continue;
> >>> + else
> >>> + return -1;
> >>> + }
> >>> +
> >>> + *size = *size / 2;
> >>> +
> >>> + /* Convert chars to bytes */
> >>> + for (i = 0; i < *size; i++)
> >>> + dst[i] = get_hex_val(src[2 * i]) * 16 +
> >>> + get_hex_val(src[2 * i + 1]);
> >>> +
> >>> + return 0;
> >>> +}
> >>
> >> I can see this has been discussed already but what would you think
> >> updating the 'parse_hex_string' something like following, it is less code to
> maintain:
> >>
> >> static int
> >> parse_hex_string(const char *src, uint8_t *dst, uint32_t *size) {
> >> int len;
> >> int i
> >> for (i = 0, len = 0; i < *size; i += 2) {
> >> char tmp[3];
> >> snprintf(tmp, 3, src + i);
> >> dst[len++] = strtoul(tmp, NULL, 16);
> >> }
> >> dst[len] = 0;
> >> *size = len;
> >> return 0;
> >> }
> >>
> >> (indeed with better error checking on strtoul ;) )
> >
> >
> > Why delete these check from parse_hex_string()?
>
> The point is using 'strtoul' instead of your functions, so that you won't need
> 'get_hex_val()' at all, or won't need 'isxdigit()' because 'strtoul' will check it,
> won't need size should be multiply of two restriction '(*size & 1)' because of
> implementation change. Probably you will need NULL checks, but again point
> is why not using 'strtoul' instead of writing your version of it?
Yes, we can use 'strtoul' , but my point is that I think we need these check code even if we use the code 'strtoul' .
isxdigit(*c)) is need because *c may be sring "0xrgh" which is not hex.
If we use strtoul will return 0 for that ,we can not distinguish between error or input is zero.
'(*size & 1) can be delete, I agree.
/* Check input parameters */
if ((src == NULL) ||
(dst == NULL) ||
(size == NULL) ||
(*size == 0))
return -1;
for (c = src, i = 0; i < *size; c++, i++) {
if (isxdigit(*c))
continue;
else
return -1;
}
> >
> > /* Check input parameters */
> > if ((src == NULL) ||
> > (dst == NULL) ||
> > (size == NULL) ||
> > (*size == 0))
> > return -1;
> > if ((*size & 1) != 0)
> > return -1;
> > for (c = src, i = 0; i < *size; c++, i++) {
> > if (isxdigit(*c))
> > continue;
> > else
> > return -1;
> > }
> >
> >
> >
> >>
> >> <...>
> >>
> >>> + /* Output buffer is not necessarily NUL-terminated. */
> >>> + memcpy(buf, hex_tmp, hexlen);
> >>> + memset((uint8_t *)buf + len, 0x00, size - hexlen);
> >>
> >> Can't this overflow the 'buf'? since "len = 2 * hexlen"
> >> I guess intention is "buf + hexlen"
On 3/25/2019 9:25 AM, Zhao1, Wei wrote:
> Hi，Ferruh
>
>> -----Original Message-----
>> From: Yigit, Ferruh
>> Sent: Monday, March 25, 2019 4:46 PM
>> To: Zhao1, Wei <wei.zhao1@intel.com>; dev@dpdk.org
>> Cc: stable@dpdk.org; stephen@networkplumber.org; Ananyev, Konstantin
>> <konstantin.ananyev@intel.com>
>> Subject: Re: [dpdk-stable] [PATCH v3] app/testpmd: fix support of hex string
>> parser for flow API
>>
>> On 3/25/2019 3:39 AM, Zhao1, Wei wrote:
>>> Hi,
>>>
>>>> -----Original Message-----
>>>> From: Yigit, Ferruh
>>>> Sent: Friday, March 22, 2019 10:56 PM
>>>> To: Zhao1, Wei <wei.zhao1@intel.com>; dev@dpdk.org
>>>> Cc: stable@dpdk.org; stephen@networkplumber.org; Ananyev,
>> Konstantin
>>>> <konstantin.ananyev@intel.com>
>>>> Subject: Re: [dpdk-stable] [PATCH v3] app/testpmd: fix support of hex
>>>> string parser for flow API
>>>>
>>>> On 3/22/2019 3:15 AM, Wei Zhao wrote:
>>>>> There is need for users to set configuration of HEX number for RSS
>>>>> key. The key byte should be pass down as hex number not as char
>>>>> string. This patch enable cmdline flow parse HEX number, in order to
>>>>> not using string which pass ASIC number.
>>>>>
>>>>> Fixes: f4d623f96119 ("app/testpmd: fix missing RSS fields in flow
>>>>> action")
>>>>> Cc: stable@dpdk.org
>>>>>
>>>>> Signed-off-by: Wei Zhao <wei.zhao1@intel.com>
>>>>> Tested-by: Peng Yuan <yuan.peng@intel.com>
>>>>
>>>> <...>
>>>>
>>>>> @@ -4475,6 +4486,138 @@ parse_string(struct context *ctx, const
>>>>> struct
>>>> token *token,
>>>>> return -1;
>>>>> }
>>>>>
>>>>> +static uint32_t
>>>>> +get_hex_val(char c)
>>>>> +{
>>>>> + switch (c) {
>>>>> + case '0': case '1': case '2': case '3': case '4': case '5':
>>>>> + case '6': case '7': case '8': case '9':
>>>>> + return c - '0';
>>>>> + case 'A': case 'B': case 'C': case 'D': case 'E': case 'F':
>>>>> + return c - 'A' + 10;
>>>>> + case 'a': case 'b': case 'c': case 'd': case 'e': case 'f':
>>>>> + return c - 'a' + 10;
>>>>> + default:
>>>>> + return 0;
>>>>> + }
>>>>> +}
>>>>> +
>>>>> +static int
>>>>> +parse_hex_string(const char *src, uint8_t *dst, uint32_t *size) {
>>>>> + const char *c;
>>>>> + uint32_t i;
>>>>> +
>>>>> + /* Check input parameters */
>>>>> + if ((src == NULL) ||
>>>>> + (dst == NULL) ||
>>>>> + (size == NULL) ||
>>>>> + (*size == 0))
>>>>> + return -1;
>>>>> + if ((*size & 1) != 0)
>>>>> + return -1;
>>>>> +
>>>>> + for (c = src, i = 0; i < *size; c++, i++) {
>>>>> + if (isxdigit(*c))
>>>>> + continue;
>>>>> + else
>>>>> + return -1;
>>>>> + }
>>>>> +
>>>>> + *size = *size / 2;
>>>>> +
>>>>> + /* Convert chars to bytes */
>>>>> + for (i = 0; i < *size; i++)
>>>>> + dst[i] = get_hex_val(src[2 * i]) * 16 +
>>>>> + get_hex_val(src[2 * i + 1]);
>>>>> +
>>>>> + return 0;
>>>>> +}
>>>>
>>>> I can see this has been discussed already but what would you think
>>>> updating the 'parse_hex_string' something like following, it is less code to
>> maintain:
>>>>
>>>> static int
>>>> parse_hex_string(const char *src, uint8_t *dst, uint32_t *size) {
>>>> int len;
>>>> int i
>>>> for (i = 0, len = 0; i < *size; i += 2) {
>>>> char tmp[3];
>>>> snprintf(tmp, 3, src + i);
>>>> dst[len++] = strtoul(tmp, NULL, 16);
>>>> }
>>>> dst[len] = 0;
>>>> *size = len;
>>>> return 0;
>>>> }
>>>>
>>>> (indeed with better error checking on strtoul ;) )
>>>
>>>
>>> Why delete these check from parse_hex_string()?
>>
>> The point is using 'strtoul' instead of your functions, so that you won't need
>> 'get_hex_val()' at all, or won't need 'isxdigit()' because 'strtoul' will check it,
>> won't need size should be multiply of two restriction '(*size & 1)' because of
>> implementation change. Probably you will need NULL checks, but again point
>> is why not using 'strtoul' instead of writing your version of it?
>
> Yes, we can use 'strtoul' , but my point is that I think we need these check code even if we use the code 'strtoul' .
> isxdigit(*c)) is need because *c may be sring "0xrgh" which is not hex.
> If we use strtoul will return 0 for that ,we can not distinguish between error or input is zero.
> '(*size & 1) can be delete, I agree.
>
> /* Check input parameters */
> if ((src == NULL) ||
> (dst == NULL) ||
> (size == NULL) ||
> (*size == 0))
> return -1;
> for (c = src, i = 0; i < *size; c++, i++) {
> if (isxdigit(*c))
> continue;
> else
> return -1;
> }
>
Please feel free to keep/add whatever check is required, I wasn't suggesting the
final implementation.
'strtol' can detect invalid chars, by providing non-NULL to second argument, so
'isxdigit()' is not required but more checks needed.
Overall you previously mentioned 'strtol' can't be used, but it can be used and
I believe it is better way to go, but I am asking what do you think about it?
Checks and implementation details can be handled.
>
>>>
>>> /* Check input parameters */
>>> if ((src == NULL) ||
>>> (dst == NULL) ||
>>> (size == NULL) ||
>>> (*size == 0))
>>> return -1;
>>> if ((*size & 1) != 0)
>>> return -1;
>>> for (c = src, i = 0; i < *size; c++, i++) {
>>> if (isxdigit(*c))
>>> continue;
>>> else
>>> return -1;
>>> }
>>>
>>>
>>>
>>>>
>>>> <...>
>>>>
>>>>> + /* Output buffer is not necessarily NUL-terminated. */
>>>>> + memcpy(buf, hex_tmp, hexlen);
>>>>> + memset((uint8_t *)buf + len, 0x00, size - hexlen);
>>>>
>>>> Can't this overflow the 'buf'? since "len = 2 * hexlen"
>>>> I guess intention is "buf + hexlen"
>
Hi，Ferruh
> -----Original Message-----
> From: Yigit, Ferruh
> Sent: Monday, March 25, 2019 5:36 PM
> To: Zhao1, Wei <wei.zhao1@intel.com>; dev@dpdk.org
> Cc: stable@dpdk.org; stephen@networkplumber.org; Ananyev, Konstantin
> <konstantin.ananyev@intel.com>
> Subject: Re: [dpdk-stable] [PATCH v3] app/testpmd: fix support of hex string
> parser for flow API
>
> On 3/25/2019 9:25 AM, Zhao1, Wei wrote:
> > Hi，Ferruh
> >
> >> -----Original Message-----
> >> From: Yigit, Ferruh
> >> Sent: Monday, March 25, 2019 4:46 PM
> >> To: Zhao1, Wei <wei.zhao1@intel.com>; dev@dpdk.org
> >> Cc: stable@dpdk.org; stephen@networkplumber.org; Ananyev,
> Konstantin
> >> <konstantin.ananyev@intel.com>
> >> Subject: Re: [dpdk-stable] [PATCH v3] app/testpmd: fix support of hex
> >> string parser for flow API
> >>
> >> On 3/25/2019 3:39 AM, Zhao1, Wei wrote:
> >>> Hi,
> >>>
> >>>> -----Original Message-----
> >>>> From: Yigit, Ferruh
> >>>> Sent: Friday, March 22, 2019 10:56 PM
> >>>> To: Zhao1, Wei <wei.zhao1@intel.com>; dev@dpdk.org
> >>>> Cc: stable@dpdk.org; stephen@networkplumber.org; Ananyev,
> >> Konstantin
> >>>> <konstantin.ananyev@intel.com>
> >>>> Subject: Re: [dpdk-stable] [PATCH v3] app/testpmd: fix support of
> >>>> hex string parser for flow API
> >>>>
> >>>> On 3/22/2019 3:15 AM, Wei Zhao wrote:
> >>>>> There is need for users to set configuration of HEX number for RSS
> >>>>> key. The key byte should be pass down as hex number not as char
> >>>>> string. This patch enable cmdline flow parse HEX number, in order
> >>>>> to not using string which pass ASIC number.
> >>>>>
> >>>>> Fixes: f4d623f96119 ("app/testpmd: fix missing RSS fields in flow
> >>>>> action")
> >>>>> Cc: stable@dpdk.org
> >>>>>
> >>>>> Signed-off-by: Wei Zhao <wei.zhao1@intel.com>
> >>>>> Tested-by: Peng Yuan <yuan.peng@intel.com>
> >>>>
> >>>> <...>
> >>>>
> >>>>> @@ -4475,6 +4486,138 @@ parse_string(struct context *ctx, const
> >>>>> struct
> >>>> token *token,
> >>>>> return -1;
> >>>>> }
> >>>>>
> >>>>> +static uint32_t
> >>>>> +get_hex_val(char c)
> >>>>> +{
> >>>>> + switch (c) {
> >>>>> + case '0': case '1': case '2': case '3': case '4': case '5':
> >>>>> + case '6': case '7': case '8': case '9':
> >>>>> + return c - '0';
> >>>>> + case 'A': case 'B': case 'C': case 'D': case 'E': case 'F':
> >>>>> + return c - 'A' + 10;
> >>>>> + case 'a': case 'b': case 'c': case 'd': case 'e': case 'f':
> >>>>> + return c - 'a' + 10;
> >>>>> + default:
> >>>>> + return 0;
> >>>>> + }
> >>>>> +}
> >>>>> +
> >>>>> +static int
> >>>>> +parse_hex_string(const char *src, uint8_t *dst, uint32_t *size) {
> >>>>> + const char *c;
> >>>>> + uint32_t i;
> >>>>> +
> >>>>> + /* Check input parameters */
> >>>>> + if ((src == NULL) ||
> >>>>> + (dst == NULL) ||
> >>>>> + (size == NULL) ||
> >>>>> + (*size == 0))
> >>>>> + return -1;
> >>>>> + if ((*size & 1) != 0)
> >>>>> + return -1;
> >>>>> +
> >>>>> + for (c = src, i = 0; i < *size; c++, i++) {
> >>>>> + if (isxdigit(*c))
> >>>>> + continue;
> >>>>> + else
> >>>>> + return -1;
> >>>>> + }
> >>>>> +
> >>>>> + *size = *size / 2;
> >>>>> +
> >>>>> + /* Convert chars to bytes */
> >>>>> + for (i = 0; i < *size; i++)
> >>>>> + dst[i] = get_hex_val(src[2 * i]) * 16 +
> >>>>> + get_hex_val(src[2 * i + 1]);
> >>>>> +
> >>>>> + return 0;
> >>>>> +}
> >>>>
> >>>> I can see this has been discussed already but what would you think
> >>>> updating the 'parse_hex_string' something like following, it is
> >>>> less code to
> >> maintain:
> >>>>
> >>>> static int
> >>>> parse_hex_string(const char *src, uint8_t *dst, uint32_t *size) {
> >>>> int len;
> >>>> int i
> >>>> for (i = 0, len = 0; i < *size; i += 2) {
> >>>> char tmp[3];
> >>>> snprintf(tmp, 3, src + i);
> >>>> dst[len++] = strtoul(tmp, NULL, 16);
> >>>> }
> >>>> dst[len] = 0;
> >>>> *size = len;
> >>>> return 0;
> >>>> }
> >>>>
> >>>> (indeed with better error checking on strtoul ;) )
> >>>
> >>>
> >>> Why delete these check from parse_hex_string()?
> >>
> >> The point is using 'strtoul' instead of your functions, so that you
> >> won't need 'get_hex_val()' at all, or won't need 'isxdigit()' because
> >> 'strtoul' will check it, won't need size should be multiply of two
> >> restriction '(*size & 1)' because of implementation change. Probably
> >> you will need NULL checks, but again point is why not using 'strtoul'
> instead of writing your version of it?
> >
> > Yes, we can use 'strtoul' , but my point is that I think we need these check
> code even if we use the code 'strtoul' .
> > isxdigit(*c)) is need because *c may be sring "0xrgh" which is not hex.
> > If we use strtoul will return 0 for that ,we can not distinguish between
> error or input is zero.
> > '(*size & 1) can be delete, I agree.
> >
> > /* Check input parameters */
> > if ((src == NULL) ||
> > (dst == NULL) ||
> > (size == NULL) ||
> > (*size == 0))
> > return -1;
> > for (c = src, i = 0; i < *size; c++, i++) {
> > if (isxdigit(*c))
> > continue;
> > else
> > return -1;
> > }
> >
>
> Please feel free to keep/add whatever check is required, I wasn't suggesting
> the final implementation.
>
> 'strtol' can detect invalid chars, by providing non-NULL to second argument,
> so 'isxdigit()' is not required but more checks needed.
Get , thank you.
>
> Overall you previously mentioned 'strtol' can't be used, but it can be used
> and I believe it is better way to go, but I am asking what do you think about it?
> Checks and implementation details can be handled.
Ok, update in v4
>
> >
> >>>
> >>> /* Check input parameters */
> >>> if ((src == NULL) ||
> >>> (dst == NULL) ||
> >>> (size == NULL) ||
> >>> (*size == 0))
> >>> return -1;
> >>> if ((*size & 1) != 0)
> >>> return -1;
> >>> for (c = src, i = 0; i < *size; c++, i++) {
> >>> if (isxdigit(*c))
> >>> continue;
> >>> else
> >>> return -1;
> >>> }
> >>>
> >>>
> >>>
> >>>>
> >>>> <...>
> >>>>
> >>>>> + /* Output buffer is not necessarily NUL-terminated. */
> >>>>> + memcpy(buf, hex_tmp, hexlen);
> >>>>> + memset((uint8_t *)buf + len, 0x00, size - hexlen);
> >>>>
> >>>> Can't this overflow the 'buf'? since "len = 2 * hexlen"
> >>>> I guess intention is "buf + hexlen"
> >
Hi, Ferruh
> -----Original Message-----
> From: Yigit, Ferruh
> Sent: Friday, March 22, 2019 10:56 PM
> To: Zhao1, Wei <wei.zhao1@intel.com>; dev@dpdk.org
> Cc: stable@dpdk.org; stephen@networkplumber.org; Ananyev, Konstantin
> <konstantin.ananyev@intel.com>
> Subject: Re: [dpdk-stable] [PATCH v3] app/testpmd: fix support of hex string
> parser for flow API
>
> On 3/22/2019 3:15 AM, Wei Zhao wrote:
> > There is need for users to set configuration of HEX number for RSS
> > key. The key byte should be pass down as hex number not as char
> > string. This patch enable cmdline flow parse HEX number, in order to
> > not using string which pass ASIC number.
> >
> > Fixes: f4d623f96119 ("app/testpmd: fix missing RSS fields in flow
> > action")
> > Cc: stable@dpdk.org
> >
> > Signed-off-by: Wei Zhao <wei.zhao1@intel.com>
> > Tested-by: Peng Yuan <yuan.peng@intel.com>
>
> <...>
>
> > @@ -4475,6 +4486,138 @@ parse_string(struct context *ctx, const struct
> token *token,
> > return -1;
> > }
> >
> > +static uint32_t
> > +get_hex_val(char c)
> > +{
> > + switch (c) {
> > + case '0': case '1': case '2': case '3': case '4': case '5':
> > + case '6': case '7': case '8': case '9':
> > + return c - '0';
> > + case 'A': case 'B': case 'C': case 'D': case 'E': case 'F':
> > + return c - 'A' + 10;
> > + case 'a': case 'b': case 'c': case 'd': case 'e': case 'f':
> > + return c - 'a' + 10;
> > + default:
> > + return 0;
> > + }
> > +}
> > +
> > +static int
> > +parse_hex_string(const char *src, uint8_t *dst, uint32_t *size) {
> > + const char *c;
> > + uint32_t i;
> > +
> > + /* Check input parameters */
> > + if ((src == NULL) ||
> > + (dst == NULL) ||
> > + (size == NULL) ||
> > + (*size == 0))
> > + return -1;
> > + if ((*size & 1) != 0)
> > + return -1;
> > +
> > + for (c = src, i = 0; i < *size; c++, i++) {
> > + if (isxdigit(*c))
> > + continue;
> > + else
> > + return -1;
> > + }
> > +
> > + *size = *size / 2;
> > +
> > + /* Convert chars to bytes */
> > + for (i = 0; i < *size; i++)
> > + dst[i] = get_hex_val(src[2 * i]) * 16 +
> > + get_hex_val(src[2 * i + 1]);
> > +
> > + return 0;
> > +}
>
> I can see this has been discussed already but what would you think updating
> the 'parse_hex_string' something like following, it is less code to maintain:
>
> static int
> parse_hex_string(const char *src, uint8_t *dst, uint32_t *size) {
> int len;
> int i
> for (i = 0, len = 0; i < *size; i += 2) {
> char tmp[3];
> snprintf(tmp, 3, src + i);
> dst[len++] = strtoul(tmp, NULL, 16);
> }
> dst[len] = 0;
> *size = len;
> return 0;
> }
>
> (indeed with better error checking on strtoul ;) )
>
> <...>
I have change code style as your guide as bellow,
but strtoul() seems do not work, it return c with a none-null every time.
you can have a try yourself.
static int
parse_hex_string(const char *src, uint8_t *dst, uint32_t *size)
{
char *c = NULL;
uint32_t i, len;
char tmp[3];
/* Check input parameters */
if ((src == NULL) ||
(dst == NULL) ||
(size == NULL) ||
(*size == 0))
return -1;
/* Convert chars to bytes */
for (i = 0, len = 0; i < *size; i += 2) {
snprintf(tmp, 3, "%s" ,src + i);
dst[len++] = strtoul(tmp, &c, 16);
if(c)
return -1;
}
dst[len] = 0;
*size = len;
return 0;
}
>
> > + /* Output buffer is not necessarily NUL-terminated. */
> > + memcpy(buf, hex_tmp, hexlen);
> > + memset((uint8_t *)buf + len, 0x00, size - hexlen);
>
> Can't this overflow the 'buf'? since "len = 2 * hexlen"
> I guess intention is "buf + hexlen"
Hi, Ferruh
> -----Original Message-----
> From: Zhao1, Wei
> Sent: Tuesday, April 9, 2019 3:38 PM
> To: Yigit, Ferruh <ferruh.yigit@intel.com>; dev@dpdk.org
> Cc: stable@dpdk.org; stephen@networkplumber.org; Ananyev, Konstantin
> <konstantin.ananyev@intel.com>
> Subject: RE: [dpdk-stable] [PATCH v3] app/testpmd: fix support of hex string
> parser for flow API
>
> Hi, Ferruh
>
> > -----Original Message-----
> > From: Yigit, Ferruh
> > Sent: Friday, March 22, 2019 10:56 PM
> > To: Zhao1, Wei <wei.zhao1@intel.com>; dev@dpdk.org
> > Cc: stable@dpdk.org; stephen@networkplumber.org; Ananyev, Konstantin
> > <konstantin.ananyev@intel.com>
> > Subject: Re: [dpdk-stable] [PATCH v3] app/testpmd: fix support of hex
> > string parser for flow API
> >
> > On 3/22/2019 3:15 AM, Wei Zhao wrote:
> > > There is need for users to set configuration of HEX number for RSS
> > > key. The key byte should be pass down as hex number not as char
> > > string. This patch enable cmdline flow parse HEX number, in order to
> > > not using string which pass ASIC number.
> > >
> > > Fixes: f4d623f96119 ("app/testpmd: fix missing RSS fields in flow
> > > action")
> > > Cc: stable@dpdk.org
> > >
> > > Signed-off-by: Wei Zhao <wei.zhao1@intel.com>
> > > Tested-by: Peng Yuan <yuan.peng@intel.com>
> >
> > <...>
> >
> > > @@ -4475,6 +4486,138 @@ parse_string(struct context *ctx, const
> > > struct
> > token *token,
> > > return -1;
> > > }
> > >
> > > +static uint32_t
> > > +get_hex_val(char c)
> > > +{
> > > + switch (c) {
> > > + case '0': case '1': case '2': case '3': case '4': case '5':
> > > + case '6': case '7': case '8': case '9':
> > > + return c - '0';
> > > + case 'A': case 'B': case 'C': case 'D': case 'E': case 'F':
> > > + return c - 'A' + 10;
> > > + case 'a': case 'b': case 'c': case 'd': case 'e': case 'f':
> > > + return c - 'a' + 10;
> > > + default:
> > > + return 0;
> > > + }
> > > +}
> > > +
> > > +static int
> > > +parse_hex_string(const char *src, uint8_t *dst, uint32_t *size) {
> > > + const char *c;
> > > + uint32_t i;
> > > +
> > > + /* Check input parameters */
> > > + if ((src == NULL) ||
> > > + (dst == NULL) ||
> > > + (size == NULL) ||
> > > + (*size == 0))
> > > + return -1;
> > > + if ((*size & 1) != 0)
> > > + return -1;
> > > +
> > > + for (c = src, i = 0; i < *size; c++, i++) {
> > > + if (isxdigit(*c))
> > > + continue;
> > > + else
> > > + return -1;
> > > + }
> > > +
> > > + *size = *size / 2;
> > > +
> > > + /* Convert chars to bytes */
> > > + for (i = 0; i < *size; i++)
> > > + dst[i] = get_hex_val(src[2 * i]) * 16 +
> > > + get_hex_val(src[2 * i + 1]);
> > > +
> > > + return 0;
> > > +}
> >
> > I can see this has been discussed already but what would you think
> > updating the 'parse_hex_string' something like following, it is less code to
> maintain:
> >
> > static int
> > parse_hex_string(const char *src, uint8_t *dst, uint32_t *size) {
> > int len;
> > int i
> > for (i = 0, len = 0; i < *size; i += 2) {
> > char tmp[3];
> > snprintf(tmp, 3, src + i);
> > dst[len++] = strtoul(tmp, NULL, 16);
> > }
> > dst[len] = 0;
> > *size = len;
> > return 0;
> > }
> >
> > (indeed with better error checking on strtoul ;) )
> >
> > <...>
>
>
>
> I have change code style as your guide as bellow, but strtoul() seems do not
> work, it return c with a none-null every time.
> you can have a try yourself.
>
>
> static int
> parse_hex_string(const char *src, uint8_t *dst, uint32_t *size)
> {
> char *c = NULL;
> uint32_t i, len;
> char tmp[3];
>
> /* Check input parameters */
> if ((src == NULL) ||
> (dst == NULL) ||
> (size == NULL) ||
> (*size == 0))
> return -1;
>
> /* Convert chars to bytes */
> for (i = 0, len = 0; i < *size; i += 2) {
> snprintf(tmp, 3, "%s" ,src + i);
> dst[len++] = strtoul(tmp, &c, 16);
> if(c)
> return -1;
> }
> dst[len] = 0;
> *size = len;
>
> return 0;
> }
>
As this code with strtoul do not work, I suggest we use v3 of get_hex_val(), do you think so?
> >
> > > + /* Output buffer is not necessarily NUL-terminated. */
> > > + memcpy(buf, hex_tmp, hexlen);
> > > + memset((uint8_t *)buf + len, 0x00, size - hexlen);
> >
> > Can't this overflow the 'buf'? since "len = 2 * hexlen"
> > I guess intention is "buf + hexlen"
Hi, Ferruh
> -----Original Message-----
> From: Yigit, Ferruh
> Sent: Friday, March 22, 2019 10:56 PM
> To: Zhao1, Wei <wei.zhao1@intel.com>; dev@dpdk.org
> Cc: stable@dpdk.org; stephen@networkplumber.org; Ananyev, Konstantin
> <konstantin.ananyev@intel.com>
> Subject: Re: [dpdk-stable] [PATCH v3] app/testpmd: fix support of hex string
> parser for flow API
>
> On 3/22/2019 3:15 AM, Wei Zhao wrote:
> > There is need for users to set configuration of HEX number for RSS
> > key. The key byte should be pass down as hex number not as char
> > string. This patch enable cmdline flow parse HEX number, in order to
> > not using string which pass ASIC number.
> >
> > Fixes: f4d623f96119 ("app/testpmd: fix missing RSS fields in flow
> > action")
> > Cc: stable@dpdk.org
> >
> > Signed-off-by: Wei Zhao <wei.zhao1@intel.com>
> > Tested-by: Peng Yuan <yuan.peng@intel.com>
>
> <...>
>
> > @@ -4475,6 +4486,138 @@ parse_string(struct context *ctx, const struct
> token *token,
> > return -1;
> > }
> >
> > +static uint32_t
> > +get_hex_val(char c)
> > +{
> > + switch (c) {
> > + case '0': case '1': case '2': case '3': case '4': case '5':
> > + case '6': case '7': case '8': case '9':
> > + return c - '0';
> > + case 'A': case 'B': case 'C': case 'D': case 'E': case 'F':
> > + return c - 'A' + 10;
> > + case 'a': case 'b': case 'c': case 'd': case 'e': case 'f':
> > + return c - 'a' + 10;
> > + default:
> > + return 0;
> > + }
> > +}
> > +
> > +static int
> > +parse_hex_string(const char *src, uint8_t *dst, uint32_t *size) {
> > + const char *c;
> > + uint32_t i;
> > +
> > + /* Check input parameters */
> > + if ((src == NULL) ||
> > + (dst == NULL) ||
> > + (size == NULL) ||
> > + (*size == 0))
> > + return -1;
> > + if ((*size & 1) != 0)
> > + return -1;
> > +
> > + for (c = src, i = 0; i < *size; c++, i++) {
> > + if (isxdigit(*c))
> > + continue;
> > + else
> > + return -1;
> > + }
> > +
> > + *size = *size / 2;
> > +
> > + /* Convert chars to bytes */
> > + for (i = 0; i < *size; i++)
> > + dst[i] = get_hex_val(src[2 * i]) * 16 +
> > + get_hex_val(src[2 * i + 1]);
> > +
> > + return 0;
> > +}
>
> I can see this has been discussed already but what would you think updating
> the 'parse_hex_string' something like following, it is less code to maintain:
>
> static int
> parse_hex_string(const char *src, uint8_t *dst, uint32_t *size) {
> int len;
> int i
> for (i = 0, len = 0; i < *size; i += 2) {
> char tmp[3];
> snprintf(tmp, 3, src + i);
> dst[len++] = strtoul(tmp, NULL, 16);
> }
> dst[len] = 0;
> *size = len;
> return 0;
> }
>
> (indeed with better error checking on strtoul ;) )
>
> <...>
>
> > + /* Output buffer is not necessarily NUL-terminated. */
> > + memcpy(buf, hex_tmp, hexlen);
> > + memset((uint8_t *)buf + len, 0x00, size - hexlen);
>
> Can't this overflow the 'buf'? since "len = 2 * hexlen"
> I guess intention is "buf + hexlen"
Yes, you are right, update in v4
On 4/9/2019 8:38 AM, Zhao1, Wei wrote:
> Hi, Ferruh
>
>> -----Original Message-----
>> From: Yigit, Ferruh
>> Sent: Friday, March 22, 2019 10:56 PM
>> To: Zhao1, Wei <wei.zhao1@intel.com>; dev@dpdk.org
>> Cc: stable@dpdk.org; stephen@networkplumber.org; Ananyev, Konstantin
>> <konstantin.ananyev@intel.com>
>> Subject: Re: [dpdk-stable] [PATCH v3] app/testpmd: fix support of hex string
>> parser for flow API
>>
>> On 3/22/2019 3:15 AM, Wei Zhao wrote:
>>> There is need for users to set configuration of HEX number for RSS
>>> key. The key byte should be pass down as hex number not as char
>>> string. This patch enable cmdline flow parse HEX number, in order to
>>> not using string which pass ASIC number.
>>>
>>> Fixes: f4d623f96119 ("app/testpmd: fix missing RSS fields in flow
>>> action")
>>> Cc: stable@dpdk.org
>>>
>>> Signed-off-by: Wei Zhao <wei.zhao1@intel.com>
>>> Tested-by: Peng Yuan <yuan.peng@intel.com>
>>
>> <...>
>>
>>> @@ -4475,6 +4486,138 @@ parse_string(struct context *ctx, const struct
>> token *token,
>>> return -1;
>>> }
>>>
>>> +static uint32_t
>>> +get_hex_val(char c)
>>> +{
>>> + switch (c) {
>>> + case '0': case '1': case '2': case '3': case '4': case '5':
>>> + case '6': case '7': case '8': case '9':
>>> + return c - '0';
>>> + case 'A': case 'B': case 'C': case 'D': case 'E': case 'F':
>>> + return c - 'A' + 10;
>>> + case 'a': case 'b': case 'c': case 'd': case 'e': case 'f':
>>> + return c - 'a' + 10;
>>> + default:
>>> + return 0;
>>> + }
>>> +}
>>> +
>>> +static int
>>> +parse_hex_string(const char *src, uint8_t *dst, uint32_t *size) {
>>> + const char *c;
>>> + uint32_t i;
>>> +
>>> + /* Check input parameters */
>>> + if ((src == NULL) ||
>>> + (dst == NULL) ||
>>> + (size == NULL) ||
>>> + (*size == 0))
>>> + return -1;
>>> + if ((*size & 1) != 0)
>>> + return -1;
>>> +
>>> + for (c = src, i = 0; i < *size; c++, i++) {
>>> + if (isxdigit(*c))
>>> + continue;
>>> + else
>>> + return -1;
>>> + }
>>> +
>>> + *size = *size / 2;
>>> +
>>> + /* Convert chars to bytes */
>>> + for (i = 0; i < *size; i++)
>>> + dst[i] = get_hex_val(src[2 * i]) * 16 +
>>> + get_hex_val(src[2 * i + 1]);
>>> +
>>> + return 0;
>>> +}
>>
>> I can see this has been discussed already but what would you think updating
>> the 'parse_hex_string' something like following, it is less code to maintain:
>>
>> static int
>> parse_hex_string(const char *src, uint8_t *dst, uint32_t *size) {
>> int len;
>> int i
>> for (i = 0, len = 0; i < *size; i += 2) {
>> char tmp[3];
>> snprintf(tmp, 3, src + i);
>> dst[len++] = strtoul(tmp, NULL, 16);
>> }
>> dst[len] = 0;
>> *size = len;
>> return 0;
>> }
>>
>> (indeed with better error checking on strtoul ;) )
>>
>> <...>
>
>
>
> I have change code style as your guide as bellow,
> but strtoul() seems do not work, it return c with a none-null every time.
> you can have a try yourself.
Hi Wei,
Yes it return 'c' none-null, which is OK/expected.
According man page:
"
If endptr is not NULL, strtol() stores the address of the first invalid
character in *endptr. If there were no digits at all, strtol() stores the
original value of nptr in *endptr (and returns 0). In particular, if *nptr is
not '\0' but **endptr is '\0' on return, the entire string is valid.
"
First invalid char in below is 'tmp[2]', which is '0', so 'strtol' returns
'&tmp[2]', this is aligned with what described above.
It says, "if *nptr is not '\0' but **endptr is '\0' on return, the entire string
is valid.", beware it says "**endptr is '\0'", so following check should work:
if (*c != 0) {
len--;
dst[len] = 0;
*size = len;
return -1;
}
>
>
> static int
> parse_hex_string(const char *src, uint8_t *dst, uint32_t *size)
> {
> char *c = NULL;
> uint32_t i, len;
> char tmp[3];
>
> /* Check input parameters */
> if ((src == NULL) ||
> (dst == NULL) ||
> (size == NULL) ||
> (*size == 0))
> return -1;
>
> /* Convert chars to bytes */
> for (i = 0, len = 0; i < *size; i += 2) {
> snprintf(tmp, 3, "%s" ,src + i);
> dst[len++] = strtoul(tmp, &c, 16);
> if(c)
> return -1;
> }
> dst[len] = 0;
> *size = len;
>
> return 0;
> }
>
>>
>>> + /* Output buffer is not necessarily NUL-terminated. */
>>> + memcpy(buf, hex_tmp, hexlen);
>>> + memset((uint8_t *)buf + len, 0x00, size - hexlen);
>>
>> Can't this overflow the 'buf'? since "len = 2 * hexlen"
>> I guess intention is "buf + hexlen"
Hi, Ferruh
> -----Original Message-----
> From: Yigit, Ferruh
> Sent: Tuesday, April 9, 2019 4:45 PM
> To: Zhao1, Wei <wei.zhao1@intel.com>; dev@dpdk.org
> Cc: stable@dpdk.org; stephen@networkplumber.org; Ananyev, Konstantin
> <konstantin.ananyev@intel.com>
> Subject: Re: [dpdk-stable] [PATCH v3] app/testpmd: fix support of hex string
> parser for flow API
>
> On 4/9/2019 8:38 AM, Zhao1, Wei wrote:
> > Hi, Ferruh
> >
> >> -----Original Message-----
> >> From: Yigit, Ferruh
> >> Sent: Friday, March 22, 2019 10:56 PM
> >> To: Zhao1, Wei <wei.zhao1@intel.com>; dev@dpdk.org
> >> Cc: stable@dpdk.org; stephen@networkplumber.org; Ananyev,
> Konstantin
> >> <konstantin.ananyev@intel.com>
> >> Subject: Re: [dpdk-stable] [PATCH v3] app/testpmd: fix support of hex
> >> string parser for flow API
> >>
> >> On 3/22/2019 3:15 AM, Wei Zhao wrote:
> >>> There is need for users to set configuration of HEX number for RSS
> >>> key. The key byte should be pass down as hex number not as char
> >>> string. This patch enable cmdline flow parse HEX number, in order to
> >>> not using string which pass ASIC number.
> >>>
> >>> Fixes: f4d623f96119 ("app/testpmd: fix missing RSS fields in flow
> >>> action")
> >>> Cc: stable@dpdk.org
> >>>
> >>> Signed-off-by: Wei Zhao <wei.zhao1@intel.com>
> >>> Tested-by: Peng Yuan <yuan.peng@intel.com>
> >>
> >> <...>
> >>
> >>> @@ -4475,6 +4486,138 @@ parse_string(struct context *ctx, const
> >>> struct
> >> token *token,
> >>> return -1;
> >>> }
> >>>
> >>> +static uint32_t
> >>> +get_hex_val(char c)
> >>> +{
> >>> + switch (c) {
> >>> + case '0': case '1': case '2': case '3': case '4': case '5':
> >>> + case '6': case '7': case '8': case '9':
> >>> + return c - '0';
> >>> + case 'A': case 'B': case 'C': case 'D': case 'E': case 'F':
> >>> + return c - 'A' + 10;
> >>> + case 'a': case 'b': case 'c': case 'd': case 'e': case 'f':
> >>> + return c - 'a' + 10;
> >>> + default:
> >>> + return 0;
> >>> + }
> >>> +}
> >>> +
> >>> +static int
> >>> +parse_hex_string(const char *src, uint8_t *dst, uint32_t *size) {
> >>> + const char *c;
> >>> + uint32_t i;
> >>> +
> >>> + /* Check input parameters */
> >>> + if ((src == NULL) ||
> >>> + (dst == NULL) ||
> >>> + (size == NULL) ||
> >>> + (*size == 0))
> >>> + return -1;
> >>> + if ((*size & 1) != 0)
> >>> + return -1;
> >>> +
> >>> + for (c = src, i = 0; i < *size; c++, i++) {
> >>> + if (isxdigit(*c))
> >>> + continue;
> >>> + else
> >>> + return -1;
> >>> + }
> >>> +
> >>> + *size = *size / 2;
> >>> +
> >>> + /* Convert chars to bytes */
> >>> + for (i = 0; i < *size; i++)
> >>> + dst[i] = get_hex_val(src[2 * i]) * 16 +
> >>> + get_hex_val(src[2 * i + 1]);
> >>> +
> >>> + return 0;
> >>> +}
> >>
> >> I can see this has been discussed already but what would you think
> >> updating the 'parse_hex_string' something like following, it is less code to
> maintain:
> >>
> >> static int
> >> parse_hex_string(const char *src, uint8_t *dst, uint32_t *size) {
> >> int len;
> >> int i
> >> for (i = 0, len = 0; i < *size; i += 2) {
> >> char tmp[3];
> >> snprintf(tmp, 3, src + i);
> >> dst[len++] = strtoul(tmp, NULL, 16);
> >> }
> >> dst[len] = 0;
> >> *size = len;
> >> return 0;
> >> }
> >>
> >> (indeed with better error checking on strtoul ;) )
> >>
> >> <...>
> >
> >
> >
> > I have change code style as your guide as bellow, but strtoul() seems
> > do not work, it return c with a none-null every time.
> > you can have a try yourself.
>
> Hi Wei,
>
> Yes it return 'c' none-null, which is OK/expected.
>
> According man page:
> "
> If endptr is not NULL, strtol() stores the address of the first invalid character
> in *endptr. If there were no digits at all, strtol() stores the original value of
> nptr in *endptr (and returns 0). In particular, if *nptr is not '\0' but **endptr
> is '\0' on return, the entire string is valid.
> "
>
> First invalid char in below is 'tmp[2]', which is '0', so 'strtol' returns '&tmp[2]',
> this is aligned with what described above.
>
> It says, "if *nptr is not '\0' but **endptr is '\0' on return, the entire string is
> valid.", beware it says "**endptr is '\0'", so following check should work:
>
> if (*c != 0) {
> len--;
> dst[len] = 0;
> *size = len;
> return -1;
> }
Yes, you are right, it keep address of 'tmp[2]' in c, update in v4 for check code .
>
> >
> >
> > static int
> > parse_hex_string(const char *src, uint8_t *dst, uint32_t *size) {
> > char *c = NULL;
> > uint32_t i, len;
> > char tmp[3];
> >
> > /* Check input parameters */
> > if ((src == NULL) ||
> > (dst == NULL) ||
> > (size == NULL) ||
> > (*size == 0))
> > return -1;
> >
> > /* Convert chars to bytes */
> > for (i = 0, len = 0; i < *size; i += 2) {
> > snprintf(tmp, 3, "%s" ,src + i);
> > dst[len++] = strtoul(tmp, &c, 16);
> > if(c)
> > return -1;
> > }
> > dst[len] = 0;
> > *size = len;
> >
> > return 0;
> > }
> >
> >>
> >>> + /* Output buffer is not necessarily NUL-terminated. */
> >>> + memcpy(buf, hex_tmp, hexlen);
> >>> + memset((uint8_t *)buf + len, 0x00, size - hexlen);
> >>
> >> Can't this overflow the 'buf'? since "len = 2 * hexlen"
> >> I guess intention is "buf + hexlen"
@@ -35,6 +35,7 @@ enum index {
PREFIX,
BOOLEAN,
STRING,
+ HEX,
MAC_ADDR,
IPV4_ADDR,
IPV6_ADDR,
@@ -1122,6 +1123,9 @@ static int parse_boolean(struct context *, const struct token *,
static int parse_string(struct context *, const struct token *,
const char *, unsigned int,
void *, unsigned int);
+static int parse_hex(struct context *ctx, const struct token *token,
+ const char *str, unsigned int len,
+ void *buf, unsigned int size);
static int parse_mac_addr(struct context *, const struct token *,
const char *, unsigned int,
void *, unsigned int);
@@ -1198,6 +1202,13 @@ static const struct token token_list[] = {
.call = parse_string,
.comp = comp_none,
},
+ [HEX] = {
+ .name = "{hex}",
+ .type = "HEX",
+ .help = "fixed string",
+ .call = parse_hex,
+ .comp = comp_none,
+ },
[MAC_ADDR] = {
.name = "{MAC address}",
.type = "MAC-48",
@@ -2306,7 +2317,7 @@ static const struct token token_list[] = {
[ACTION_RSS_KEY] = {
.name = "key",
.help = "RSS hash key",
- .next = NEXT(action_rss, NEXT_ENTRY(STRING)),
+ .next = NEXT(action_rss, NEXT_ENTRY(HEX)),
.args = ARGS(ARGS_ENTRY_ARB(0, 0),
ARGS_ENTRY_ARB
(offsetof(struct action_rss_data, conf) +
@@ -4475,6 +4486,138 @@ parse_string(struct context *ctx, const struct token *token,
return -1;
}
+static uint32_t
+get_hex_val(char c)
+{
+ switch (c) {
+ case '0': case '1': case '2': case '3': case '4': case '5':
+ case '6': case '7': case '8': case '9':
+ return c - '0';
+ case 'A': case 'B': case 'C': case 'D': case 'E': case 'F':
+ return c - 'A' + 10;
+ case 'a': case 'b': case 'c': case 'd': case 'e': case 'f':
+ return c - 'a' + 10;
+ default:
+ return 0;
+ }
+}
+
+static int
+parse_hex_string(const char *src, uint8_t *dst, uint32_t *size)
+{
+ const char *c;
+ uint32_t i;
+
+ /* Check input parameters */
+ if ((src == NULL) ||
+ (dst == NULL) ||
+ (size == NULL) ||
+ (*size == 0))
+ return -1;
+ if ((*size & 1) != 0)
+ return -1;
+
+ for (c = src, i = 0; i < *size; c++, i++) {
+ if (isxdigit(*c))
+ continue;
+ else
+ return -1;
+ }
+
+ *size = *size / 2;
+
+ /* Convert chars to bytes */
+ for (i = 0; i < *size; i++)
+ dst[i] = get_hex_val(src[2 * i]) * 16 +
+ get_hex_val(src[2 * i + 1]);
+
+ return 0;
+}
+
+static int
+parse_hex(struct context *ctx, const struct token *token,
+ const char *str, unsigned int len,
+ void *buf, unsigned int size)
+{
+ const struct arg *arg_data = pop_args(ctx);
+ const struct arg *arg_len = pop_args(ctx);
+ const struct arg *arg_addr = pop_args(ctx);
+ char tmp[16]; /* Ought to be enough. */
+ int ret;
+ unsigned int hexlen = len;
+ unsigned int length = 256;
+ uint8_t hex_tmp[length];
+
+ /* Arguments are expected. */
+ if (!arg_data)
+ return -1;
+ if (!arg_len) {
+ push_args(ctx, arg_data);
+ return -1;
+ }
+ if (!arg_addr) {
+ push_args(ctx, arg_len);
+ push_args(ctx, arg_data);
+ return -1;
+ }
+ size = arg_data->size;
+ /* Bit-mask fill is not supported. */
+ if (arg_data->mask)
+ goto error;
+ if (!ctx->object)
+ return len;
+
+ /* translate bytes string to array. */
+ if (str[0] == '0' && ((str[1] == 'x') ||
+ (str[1] == 'X'))) {
+ str += 2;
+ hexlen -= 2;
+ }
+ if (hexlen > length)
+ return -1;
+ ret = parse_hex_string(str, hex_tmp, &hexlen);
+ if (ret < 0)
+ goto error;
+ /* Let parse_int() fill length information first. */
+ ret = snprintf(tmp, sizeof(tmp), "%u", hexlen);
+ if (ret < 0)
+ goto error;
+ push_args(ctx, arg_len);
+ ret = parse_int(ctx, token, tmp, ret, NULL, 0);
+ if (ret < 0) {
+ pop_args(ctx);
+ goto error;
+ }
+ buf = (uint8_t *)ctx->object + arg_data->offset;
+ /* Output buffer is not necessarily NUL-terminated. */
+ memcpy(buf, hex_tmp, hexlen);
+ memset((uint8_t *)buf + len, 0x00, size - hexlen);
+ if (ctx->objmask)
+ memset((uint8_t *)ctx->objmask + arg_data->offset,
+ 0xff, hexlen);
+ /* Save address if requested. */
+ if (arg_addr->size) {
+ memcpy((uint8_t *)ctx->object + arg_addr->offset,
+ (void *[]){
+ (uint8_t *)ctx->object + arg_data->offset
+ },
+ arg_addr->size);
+ if (ctx->objmask)
+ memcpy((uint8_t *)ctx->objmask + arg_addr->offset,
+ (void *[]){
+ (uint8_t *)ctx->objmask + arg_data->offset
+ },
+ arg_addr->size);
+ }
+ return len;
+error:
+ push_args(ctx, arg_addr);
+ push_args(ctx, arg_len);
+ push_args(ctx, arg_data);
+ return -1;
+
+}
+
/**
* Parse a MAC address.
*