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.zh...@intel.com>; dev@dpdk.org >> Cc: sta...@dpdk.org; step...@networkplumber.org; Ananyev, Konstantin >> <konstantin.anan...@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: sta...@dpdk.org >>> >>> Signed-off-by: Wei Zhao <wei.zh...@intel.com> >>> Tested-by: Peng Yuan <yuan.p...@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"