[dpdk-stable] [PATCH] app/testpmd: fix support of hex string parser for flow API

[dpdk-stable] [PATCH] app/testpmd: fix support of hex string parser for flow API

[Wei Zhao]

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>
---
 app/test-pmd/cmdline_flow.c | 141 +++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 140 insertions(+), 1 deletion(-)

diff --git a/app/test-pmd/cmdline_flow.c b/app/test-pmd/cmdline_flow.c
index 36659a6..2445731 100644
--- a/app/test-pmd/cmdline_flow.c
+++ b/app/test-pmd/cmdline_flow.c
@@ -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",
@@ -1544,7 +1555,7 @@ static const struct token token_list[] = {
 		.name = "pattern",
 		.help = "byte string to look for",
 		.next = NEXT(item_raw,
-			     NEXT_ENTRY(STRING),
+			     NEXT_ENTRY(HEX),
 			     NEXT_ENTRY(ITEM_PARAM_IS,
 					ITEM_PARAM_SPEC,
 					ITEM_PARAM_MASK)),
@@ -4475,6 +4486,134 @@ 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 ((((*c) >= '0') && ((*c) <= '9')) ||
+			(((*c) >= 'A') && ((*c) <= 'F')) ||
+			(((*c) >= 'a') && ((*c) <= 'f')))
+			continue;
+
+		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;
+	uint8_t hex_tmp[16];
+
+	/* 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 || size < len)
+		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;
+	}
+	parse_hex_string(str, hex_tmp, &hexlen);
+	/* 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.
  *
-- 
2.7.5

Re: [dpdk-stable] [dpdk-dev] [PATCH] app/testpmd: fix support of hex string parser for flow API

[Ananyev, Konstantin]

Hi Wei,

> 
> 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>
> ---
>  app/test-pmd/cmdline_flow.c | 141 +++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 140 insertions(+), 1 deletion(-)
> 
> diff --git a/app/test-pmd/cmdline_flow.c b/app/test-pmd/cmdline_flow.c
> index 36659a6..2445731 100644
> --- a/app/test-pmd/cmdline_flow.c
> +++ b/app/test-pmd/cmdline_flow.c
> @@ -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",
> @@ -1544,7 +1555,7 @@ static const struct token token_list[] = {
>  		.name = "pattern",
>  		.help = "byte string to look for",
>  		.next = NEXT(item_raw,
> -			     NEXT_ENTRY(STRING),
> +			     NEXT_ENTRY(HEX),
>  			     NEXT_ENTRY(ITEM_PARAM_IS,
>  					ITEM_PARAM_SPEC,
>  					ITEM_PARAM_MASK)),
> @@ -4475,6 +4486,134 @@ 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 ((((*c) >= '0') && ((*c) <= '9')) ||
> +			(((*c) >= 'A') && ((*c) <= 'F')) ||
> +			(((*c) >= 'a') && ((*c) <= 'f')))
> +			continue;
> +
> +		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;
> +}

What's wrong with strtoul() and friends?
Why you need to write your own version?
Konstantin

> +
> +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;
> +	uint8_t hex_tmp[16];
> +
> +	/* 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 || size < len)
> +		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;
> +	}
> +	parse_hex_string(str, hex_tmp, &hexlen);
> +	/* 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.
>   *
> --
> 2.7.5

Re: [dpdk-stable] [dpdk-dev] [PATCH] app/testpmd: fix support of hex string parser for flow API

[Zhao1, Wei]

Hi,  Konstantin

> -----Original Message-----
> From: Ananyev, Konstantin
> Sent: Friday, March 15, 2019 5:45 PM
> To: Zhao1, Wei <wei.zhao1@intel.com>; dev@dpdk.org
> Cc: stable@dpdk.org; Zhang, Qi Z <qi.z.zhang@intel.com>;
> adrien.mazarguil@6wind.com; Zhao1, Wei <wei.zhao1@intel.com>
> Subject: RE: [dpdk-dev] [PATCH] app/testpmd: fix support of hex string
> parser for flow API
> 
> Hi Wei,
> 
> >
> > 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>
> > ---
> >  app/test-pmd/cmdline_flow.c | 141
> > +++++++++++++++++++++++++++++++++++++++++++-
> >  1 file changed, 140 insertions(+), 1 deletion(-)
> >
> > diff --git a/app/test-pmd/cmdline_flow.c b/app/test-pmd/cmdline_flow.c
> > index 36659a6..2445731 100644
> > --- a/app/test-pmd/cmdline_flow.c
> > +++ b/app/test-pmd/cmdline_flow.c
> > @@ -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",
> > @@ -1544,7 +1555,7 @@ static const struct token token_list[] = {
> >  		.name = "pattern",
> >  		.help = "byte string to look for",
> >  		.next = NEXT(item_raw,
> > -			     NEXT_ENTRY(STRING),
> > +			     NEXT_ENTRY(HEX),
> >  			     NEXT_ENTRY(ITEM_PARAM_IS,
> >  					ITEM_PARAM_SPEC,
> >  					ITEM_PARAM_MASK)),
> > @@ -4475,6 +4486,134 @@ 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 ((((*c) >= '0') && ((*c) <= '9')) ||
> > +			(((*c) >= 'A') && ((*c) <= 'F')) ||
> > +			(((*c) >= 'a') && ((*c) <= 'f')))
> > +			continue;
> > +
> > +		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;
> > +}
> 
> What's wrong with strtoul() and friends?
> Why you need to write your own version?
> Konstantin
> 

Good idea, I WILL replace get_hex_val() with strtoul() in v2.

> > +
> > +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;
> > +	uint8_t hex_tmp[16];
> > +
> > +	/* 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 || size < len)
> > +		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;
> > +	}
> > +	parse_hex_string(str, hex_tmp, &hexlen);
> > +	/* 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.
> >   *
> > --
> > 2.7.5

[dpdk-stable] [PATCH v2] app/testpmd: fix support of hex string parser for flow API

[Wei Zhao]

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>

---

v2:
fix bug of string length limite.
---
 app/test-pmd/cmdline_flow.c | 146 +++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 145 insertions(+), 1 deletion(-)

diff --git a/app/test-pmd/cmdline_flow.c b/app/test-pmd/cmdline_flow.c
index 36659a6..7d263c6 100644
--- a/app/test-pmd/cmdline_flow.c
+++ b/app/test-pmd/cmdline_flow.c
@@ -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,139 @@ 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 ((((*c) >= '0') && ((*c) <= '9')) ||
+			(((*c) >= 'A') && ((*c) <= 'F')) ||
+			(((*c) >= 'a') && ((*c) <= 'f')))
+			continue;
+
+		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.
  *
-- 
2.7.5

Re: [dpdk-stable] [dpdk-dev] [PATCH] app/testpmd: fix support of hex string parser for flow API

[Zhao1, Wei]

Hi,  Konstantin

> -----Original Message-----
> From: Ananyev, Konstantin
> Sent: Friday, March 15, 2019 5:45 PM
> To: Zhao1, Wei <wei.zhao1@intel.com>; dev@dpdk.org
> Cc: stable@dpdk.org; Zhang, Qi Z <qi.z.zhang@intel.com>;
> adrien.mazarguil@6wind.com; Zhao1, Wei <wei.zhao1@intel.com>
> Subject: RE: [dpdk-dev] [PATCH] app/testpmd: fix support of hex string
> parser for flow API
> 
> Hi Wei,
> 
> >
> > 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>
> > ---
> >  app/test-pmd/cmdline_flow.c | 141
> > +++++++++++++++++++++++++++++++++++++++++++-
> >  1 file changed, 140 insertions(+), 1 deletion(-)
> >
> > diff --git a/app/test-pmd/cmdline_flow.c b/app/test-pmd/cmdline_flow.c
> > index 36659a6..2445731 100644
> > --- a/app/test-pmd/cmdline_flow.c
> > +++ b/app/test-pmd/cmdline_flow.c
> > @@ -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",
> > @@ -1544,7 +1555,7 @@ static const struct token token_list[] = {
> >  		.name = "pattern",
> >  		.help = "byte string to look for",
> >  		.next = NEXT(item_raw,
> > -			     NEXT_ENTRY(STRING),
> > +			     NEXT_ENTRY(HEX),
> >  			     NEXT_ENTRY(ITEM_PARAM_IS,
> >  					ITEM_PARAM_SPEC,
> >  					ITEM_PARAM_MASK)),
> > @@ -4475,6 +4486,134 @@ 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 ((((*c) >= '0') && ((*c) <= '9')) ||
> > +			(((*c) >= 'A') && ((*c) <= 'F')) ||
> > +			(((*c) >= 'a') && ((*c) <= 'f')))
> > +			continue;
> > +
> > +		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;
> > +}
> 
> What's wrong with strtoul() and friends?
> Why you need to write your own version?
> Konstantin


I have try to use strtoul(), but it do not work, because 
I want to pars char byte on bye one, not the long string by one time, and also 
RSS KEY can be as long as 100 byte long for i40e,   but strtoul() return only 64 bit long.
app\test-eventdev and examples\ip_pipeline , drivers\net\softnic also use get_hex_val(), so I will it also.


> 
> > +
> > +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;
> > +	uint8_t hex_tmp[16];
> > +
> > +	/* 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 || size < len)
> > +		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;
> > +	}
> > +	parse_hex_string(str, hex_tmp, &hexlen);
> > +	/* 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.
> >   *
> > --
> > 2.7.5

Re: [dpdk-stable] [dpdk-dev] [PATCH] app/testpmd: fix support of hex string parser for flow API

[Stephen Hemminger]

On Fri, 15 Mar 2019 16:43:55 +0800
Wei Zhao <wei.zhao1@intel.com> wrote:

> +	/* 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 ((((*c) >= '0') && ((*c) <= '9')) ||
> +			(((*c) >= 'A') && ((*c) <= 'F')) ||
> +			(((*c) >= 'a') && ((*c) <= 'f')))
> +			continue;
> +

Excessive number of parens here. You don't need them around (*c)

Why did you not use isxdigit() instead?

Re: [dpdk-stable] [PATCH v2] app/testpmd: fix support of hex string parser for flow API

[Zhao1, Wei]

Send on behalf of pengyuan
Tested-by: Peng Yuan <yuan.peng@intel.com>




- Tested Branch: master
- Tested Commit: 239912fa798e6e671072ca7ff987afd74c1e506c
- OS: 4.13.9-300.fc27.x86_64
- GCC: gcc (GCC) 7.3.1 20180303 (Red Hat 7.3.1-5)
- CPU: Intel(R) Xeon(R) CPU E5-2699 v4 @ 2.20GHz
- NIC: Intel Corporation Device Fortville [8086:1583]
- Default x86_64-native-linuxapp-gcc configuration
- Prerequisites:
- Total 1 case1, 1 passed, 0 failed

- Case steps:
1. Bind the pf port to dpdk driver:
./usertools/dpdk-devbind.py -b igb_uio 05:00.0 05:00.1
2. start testpmd:
./x86_64-native-linuxapp-gcc/app/testpmd -c 1ffff -n 4 - -i --nb-cores=8 --rxq=4 --txq=4 --port-topology=chained

3.    testpmd> flow create 0 ingress pattern end actions rss types ipv4-udp end key 1234567890123456789012345678901234567890FFFFFFFFFFFF1234567890123456789012345678901234567890FFFFFFFFFFFF / end
    testpmd> show port 0 rss-hash key
    RSS functions:
     ipv4-udp udp
    RSS key:
    1234567890123456789012345678901234567890FFFFFFFFFFFF1234567890123456789012345678901234567890FFFFFFFFFFFF

52bytes key can be set successfully.

> -----Original Message-----
> From: Zhao1, Wei
> Sent: Monday, March 18, 2019 4:16 PM
> To: dev@dpdk.org
> Cc: stable@dpdk.org; Zhang, Qi Z <qi.z.zhang@intel.com>; Lu, Wenzhuo
> <wenzhuo.lu@intel.com>; Ananyev, Konstantin
> <konstantin.ananyev@intel.com>; Zhao1, Wei <wei.zhao1@intel.com>
> Subject: [PATCH v2] app/testpmd: fix support of hex string parser for flow
> API
> 
> 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>
> 
> ---
> 
> v2:
> fix bug of string length limite.
> ---
>  app/test-pmd/cmdline_flow.c | 146
> +++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 145 insertions(+), 1 deletion(-)
> 
> diff --git a/app/test-pmd/cmdline_flow.c b/app/test-pmd/cmdline_flow.c
> index 36659a6..7d263c6 100644
> --- a/app/test-pmd/cmdline_flow.c
> +++ b/app/test-pmd/cmdline_flow.c
> @@ -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,139 @@ 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 ((((*c) >= '0') && ((*c) <= '9')) ||
> +			(((*c) >= 'A') && ((*c) <= 'F')) ||
> +			(((*c) >= 'a') && ((*c) <= 'f')))
> +			continue;
> +
> +		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.
>   *
> --
> 2.7.5

Re: [dpdk-stable] [dpdk-dev] [PATCH] app/testpmd: fix support of hex string parser for flow API

[Ananyev, Konstantin]

Hi Wei,

> >
> > >
> > > 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>
> > > ---
> > >  app/test-pmd/cmdline_flow.c | 141
> > > +++++++++++++++++++++++++++++++++++++++++++-
> > >  1 file changed, 140 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/app/test-pmd/cmdline_flow.c b/app/test-pmd/cmdline_flow.c
> > > index 36659a6..2445731 100644
> > > --- a/app/test-pmd/cmdline_flow.c
> > > +++ b/app/test-pmd/cmdline_flow.c
> > > @@ -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",
> > > @@ -1544,7 +1555,7 @@ static const struct token token_list[] = {
> > >  		.name = "pattern",
> > >  		.help = "byte string to look for",
> > >  		.next = NEXT(item_raw,
> > > -			     NEXT_ENTRY(STRING),
> > > +			     NEXT_ENTRY(HEX),
> > >  			     NEXT_ENTRY(ITEM_PARAM_IS,
> > >  					ITEM_PARAM_SPEC,
> > >  					ITEM_PARAM_MASK)),
> > > @@ -4475,6 +4486,134 @@ 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 ((((*c) >= '0') && ((*c) <= '9')) ||
> > > +			(((*c) >= 'A') && ((*c) <= 'F')) ||
> > > +			(((*c) >= 'a') && ((*c) <= 'f')))
> > > +			continue;
> > > +
> > > +		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;
> > > +}
> >
> > What's wrong with strtoul() and friends?
> > Why you need to write your own version?
> > Konstantin
> 
> 
> I have try to use strtoul(), but it do not work, because
> I want to pars char byte on bye one, not the long string by one time, and also
> RSS KEY can be as long as 100 byte long for i40e,   but strtoul() return only 64 bit long.
> app\test-eventdev and examples\ip_pipeline , drivers\net\softnic also use get_hex_val(), so I will it also.

Ok, thanks for explanation.
Probably at least you can use isxdigit(), isdigit, toupper(), etc. as Stephen already suggested.
Konstantin

> 
> 
> >
> > > +
> > > +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;
> > > +	uint8_t hex_tmp[16];
> > > +
> > > +	/* 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 || size < len)
> > > +		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;
> > > +	}
> > > +	parse_hex_string(str, hex_tmp, &hexlen);
> > > +	/* 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.
> > >   *
> > > --
> > > 2.7.5

Re: [dpdk-stable] [dpdk-dev] [PATCH] app/testpmd: fix support of hex string parser for flow API

[Zhao1, Wei]

Hi,  Stephen Hemminger

> -----Original Message-----
> From: Stephen Hemminger [mailto:stephen@networkplumber.org]
> Sent: Monday, March 18, 2019 11:27 PM
> To: Zhao1, Wei <wei.zhao1@intel.com>
> Cc: dev@dpdk.org; stable@dpdk.org; Zhang, Qi Z <qi.z.zhang@intel.com>;
> adrien.mazarguil@6wind.com
> Subject: Re: [dpdk-dev] [PATCH] app/testpmd: fix support of hex string
> parser for flow API
> 
> On Fri, 15 Mar 2019 16:43:55 +0800
> Wei Zhao <wei.zhao1@intel.com> wrote:
> 
> > +	/* 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 ((((*c) >= '0') && ((*c) <= '9')) ||
> > +			(((*c) >= 'A') && ((*c) <= 'F')) ||
> > +			(((*c) >= 'a') && ((*c) <= 'f')))
> > +			continue;
> > +
> 
> Excessive number of parens here. You don't need them around (*c)
> 
> Why did you not use isxdigit() instead?

Ok, I will have a try of isxdigit() and update in v2.

Re: [dpdk-stable] [dpdk-dev] [PATCH] app/testpmd: fix support of hex string parser for flow API

[Zhao1, Wei]

Hi, Konstantin

> -----Original Message-----
> From: Ananyev, Konstantin
> Sent: Thursday, March 21, 2019 10:04 PM
> To: Zhao1, Wei <wei.zhao1@intel.com>; dev@dpdk.org
> Cc: stable@dpdk.org; Zhang, Qi Z <qi.z.zhang@intel.com>;
> adrien.mazarguil@6wind.com
> Subject: RE: [dpdk-dev] [PATCH] app/testpmd: fix support of hex string
> parser for flow API
> 
> 
> Hi Wei,
> 
> > >
> > > >
> > > > 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>
> > > > ---
> > > >  app/test-pmd/cmdline_flow.c | 141
> > > > +++++++++++++++++++++++++++++++++++++++++++-
> > > >  1 file changed, 140 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/app/test-pmd/cmdline_flow.c
> > > > b/app/test-pmd/cmdline_flow.c index 36659a6..2445731 100644
> > > > --- a/app/test-pmd/cmdline_flow.c
> > > > +++ b/app/test-pmd/cmdline_flow.c
> > > > @@ -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",
> > > > @@ -1544,7 +1555,7 @@ static const struct token token_list[] = {
> > > >  		.name = "pattern",
> > > >  		.help = "byte string to look for",
> > > >  		.next = NEXT(item_raw,
> > > > -			     NEXT_ENTRY(STRING),
> > > > +			     NEXT_ENTRY(HEX),
> > > >  			     NEXT_ENTRY(ITEM_PARAM_IS,
> > > >  					ITEM_PARAM_SPEC,
> > > >  					ITEM_PARAM_MASK)),
> > > > @@ -4475,6 +4486,134 @@ 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 ((((*c) >= '0') && ((*c) <= '9')) ||
> > > > +			(((*c) >= 'A') && ((*c) <= 'F')) ||
> > > > +			(((*c) >= 'a') && ((*c) <= 'f')))
> > > > +			continue;
> > > > +
> > > > +		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;
> > > > +}
> > >
> > > What's wrong with strtoul() and friends?
> > > Why you need to write your own version?
> > > Konstantin


> >
> >
> > I have try to use strtoul(), but it do not work, because I want to
> > pars char byte on bye one, not the long string by one time, and also
> > RSS KEY can be as long as 100 byte long for i40e,   but strtoul() return only 64
> bit long.
> > app\test-eventdev and examples\ip_pipeline , drivers\net\softnic also use
> get_hex_val(), so I will it also.
> 
> Ok, thanks for explanation.
> Probably at least you can use isxdigit(), isdigit, toupper(), etc. as Stephen
> already suggested.
> Konstantin

Ok, I will have a try of isxdigit() and update in v2.

> 
> >
> >
> > >
> > > > +
> > > > +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;
> > > > +	uint8_t hex_tmp[16];
> > > > +
> > > > +	/* 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 || size < len)
> > > > +		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;
> > > > +	}
> > > > +	parse_hex_string(str, hex_tmp, &hexlen);
> > > > +	/* 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.
> > > >   *
> > > > --
> > > > 2.7.5

[dpdk-stable] [PATCH v3] app/testpmd: fix support of hex string parser for flow API

[Wei Zhao]

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

diff --git a/app/test-pmd/cmdline_flow.c b/app/test-pmd/cmdline_flow.c
index 36659a6..4bc033c 100644
--- a/app/test-pmd/cmdline_flow.c
+++ b/app/test-pmd/cmdline_flow.c
@@ -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.
  *
-- 
2.7.5

Re: [dpdk-stable] [PATCH v3] app/testpmd: fix support of hex string parser for flow API

[Ferruh Yigit]

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"

Re: [dpdk-stable] [PATCH v3] app/testpmd: fix support of hex string parser for flow API

[Zhao1, Wei]

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"

Re: [dpdk-stable] [PATCH v3] app/testpmd: fix support of hex string parser for flow API

[Ferruh Yigit]

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"

Re: [dpdk-stable] [PATCH v3] app/testpmd: fix support of hex string parser for flow API

[Zhao1, Wei]

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"

Re: [dpdk-stable] [PATCH v3] app/testpmd: fix support of hex string parser for flow API

[Ferruh Yigit]

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"
> 

Re: [dpdk-stable] [PATCH v3] app/testpmd: fix support of hex string parser for flow API

[Zhao1, Wei]

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"
> >

Re: [dpdk-stable] [PATCH v3] app/testpmd: fix support of hex string parser for flow API

[Zhao1, Wei]

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"

Re: [dpdk-stable] [PATCH v3] app/testpmd: fix support of hex string parser for flow API

[Zhao1, Wei]

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"

Re: [dpdk-stable] [PATCH v3] app/testpmd: fix support of hex string parser for flow API

[Zhao1, Wei]

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

[dpdk-stable] [PATCH v4] app/testpmd: fix support of hex string parser for flow API

[Wei Zhao]

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

v4:
fix bug in set memory to zero and change code for translating
string to hex
---
 app/test-pmd/cmdline_flow.c | 128 +++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 127 insertions(+), 1 deletion(-)

diff --git a/app/test-pmd/cmdline_flow.c b/app/test-pmd/cmdline_flow.c
index 1c83bc9..54ff175 100644
--- a/app/test-pmd/cmdline_flow.c
+++ b/app/test-pmd/cmdline_flow.c
@@ -36,6 +36,7 @@ enum index {
 	PREFIX,
 	BOOLEAN,
 	STRING,
+	HEX,
 	MAC_ADDR,
 	IPV4_ADDR,
 	IPV6_ADDR,
@@ -1123,6 +1124,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);
@@ -1199,6 +1203,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",
@@ -2307,7 +2318,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) +
@@ -4476,6 +4487,121 @@ parse_string(struct context *ctx, const struct token *token,
 	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 != 0) {
+			len--;
+			dst[len] = 0;
+			*size = len;
+			return -1;
+		}
+	}
+	dst[len] = 0;
+	*size = len;
+
+	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 + hexlen, 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.
  *
-- 
2.7.5

Re: [dpdk-stable] [PATCH v3] app/testpmd: fix support of hex string parser for flow API

[Ferruh Yigit]

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"

Re: [dpdk-stable] [PATCH v3] app/testpmd: fix support of hex string parser for flow API

[Zhao1, Wei]

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"

Re: [dpdk-stable] [PATCH v4] app/testpmd: fix support of hex string parser for flow API

[Ferruh Yigit]

On 4/9/2019 9:41 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>

Reviewed-by: Ferruh Yigit <ferruh.yigit@intel.com>

Applied to dpdk-next-net/master, thanks.