[PATCH v6 1/3] cmdline: use C standard library as number parser

[Anatoly Burakov <anatoly.burakov@intel.com>]

Remove custom number parser and use C standard library instead. In order to
keep compatibility with earlier versions of the parser, we have to take
into account a couple of quirks:

- We did not consider "negative" numbers to be valid for anything other
  than base-10 numbers, whereas C standard library does. Adjust the tests
  to match the new behavior.
- We did not consider numbers such as "+4" to be valid, whereas C
  standard library does. Adjust the tests to match the new behavior.
- C standard library's strtoull does not do range checks on negative
  numbers, so we have to parse knowingly-negative numbers as signed.
- C standard library does not support binary numbers, so we keep around the
  relevant parts of the custom parser in place to support binary numbers.

Signed-off-by: Anatoly Burakov <anatoly.burakov@intel.com>
---

Notes:
    v5 -> v6:
    - Allowed more negative numbers (such as negative octals or hex)
    - Updated unit tests to check new cases
    - Small refactoring of code to reduce amount of noise
    - More verbose debug output
    
    v4 -> v5:
    - Added this commit

 app/test/test_cmdline_num.c     |  19 +-
 lib/cmdline/cmdline_parse_num.c | 364 ++++++++++++++++----------------
 2 files changed, 195 insertions(+), 188 deletions(-)

diff --git a/app/test/test_cmdline_num.c b/app/test/test_cmdline_num.c
index 9276de59bd..dd70f6f824 100644
--- a/app/test/test_cmdline_num.c
+++ b/app/test/test_cmdline_num.c
@@ -139,6 +139,8 @@ const struct num_signed_str num_valid_negative_strs[] = {
 		{"-2147483648", INT32_MIN },
 		{"-2147483649", INT32_MIN - 1LL },
 		{"-9223372036854775808", INT64_MIN },
+		{"-0x8000000000000000", INT64_MIN },
+		{"-01000000000000000000000", INT64_MIN },
 };
 
 const struct num_unsigned_str num_garbage_positive_strs[] = {
@@ -175,12 +177,27 @@ const struct num_unsigned_str num_garbage_positive_strs[] = {
 
 const struct num_signed_str num_garbage_negative_strs[] = {
 		/* valid strings with garbage on the end, should still be valid */
+		/* negative numbers */
 		{"-9223372036854775808\0garbage", INT64_MIN },
 		{"-9223372036854775808\rgarbage", INT64_MIN },
 		{"-9223372036854775808\tgarbage", INT64_MIN },
 		{"-9223372036854775808\ngarbage", INT64_MIN },
 		{"-9223372036854775808#garbage", INT64_MIN },
 		{"-9223372036854775808 garbage", INT64_MIN },
+		/* negative hex */
+		{"-0x8000000000000000\0garbage", INT64_MIN },
+		{"-0x8000000000000000\rgarbage", INT64_MIN },
+		{"-0x8000000000000000\tgarbage", INT64_MIN },
+		{"-0x8000000000000000\ngarbage", INT64_MIN },
+		{"-0x8000000000000000#garbage", INT64_MIN },
+		{"-0x8000000000000000 garbage", INT64_MIN },
+		/* negative octal */
+		{"-01000000000000000000000\0garbage", INT64_MIN },
+		{"-01000000000000000000000\rgarbage", INT64_MIN },
+		{"-01000000000000000000000\tgarbage", INT64_MIN },
+		{"-01000000000000000000000\ngarbage", INT64_MIN },
+		{"-01000000000000000000000#garbage", INT64_MIN },
+		{"-01000000000000000000000 garbage", INT64_MIN },
 };
 
 const char * num_invalid_strs[] = {
@@ -197,7 +214,6 @@ const char * num_invalid_strs[] = {
 		"0b01110101017001",
 		/* false negative numbers */
 		"-12345F623",
-		"-0x1234580A",
 		"-0b0111010101",
 		/* too long (128+ chars) */
 		("0b1111000011110000111100001111000011110000111100001111000011110000"
@@ -205,7 +221,6 @@ const char * num_invalid_strs[] = {
 		"1E3",
 		"0A",
 		"-B",
-		"+4",
 		"1.23G",
 		"",
 		" ",
diff --git a/lib/cmdline/cmdline_parse_num.c b/lib/cmdline/cmdline_parse_num.c
index f21796bedb..ff2bdc28c8 100644
--- a/lib/cmdline/cmdline_parse_num.c
+++ b/lib/cmdline/cmdline_parse_num.c
@@ -4,6 +4,7 @@
  * All rights reserved.
  */
 
+#include <errno.h>
 #include <stdio.h>
 #include <stdint.h>
 #include <inttypes.h>
@@ -29,23 +30,6 @@ struct cmdline_token_ops cmdline_token_num_ops = {
 	.get_help = cmdline_get_help_num,
 };
 
-enum num_parse_state_t {
-	START,
-	DEC_NEG,
-	BIN,
-	HEX,
-
-	ERROR,
-
-	FIRST_OK, /* not used */
-	ZERO_OK,
-	HEX_OK,
-	OCTAL_OK,
-	BIN_OK,
-	DEC_NEG_OK,
-	DEC_POS_OK,
-};
-
 /* Keep it sync with enum in .h */
 static const char * num_help[] = {
 	"UINT8", "UINT16", "UINT32", "UINT64",
@@ -53,13 +37,13 @@ static const char * num_help[] = {
 };
 
 static inline int
-add_to_res(unsigned int c, uint64_t *res, unsigned int base)
+add_to_bin(unsigned int c, uint64_t *res)
 {
 	/* overflow */
-	if ((UINT64_MAX - c) / base < *res)
+	if ((UINT64_MAX - c) / 2 < *res)
 		return -1;
 
-	*res = (uint64_t) (*res * base + c);
+	*res = (uint64_t) (*res * 2 + c);
 	return 0;
 }
 
@@ -88,138 +72,119 @@ check_res_size(struct cmdline_token_num_data *nd, unsigned ressize)
 			return -1;
 		break;
 	default:
+		debug_printf("Check size: unsupported number format: %s\n",
+			num_help[nd->type]);
 		return -1;
 	}
 	return 0;
 }
 
-/* parse an int */
-RTE_EXPORT_SYMBOL(cmdline_parse_num)
-int
-cmdline_parse_num(cmdline_parse_token_hdr_t *tk, const char *srcbuf, void *res,
-	unsigned ressize)
+static int
+check_parsed_num(enum cmdline_numtype type, int neg, uint64_t uintres)
 {
-	struct cmdline_token_num_data nd;
-	enum num_parse_state_t st = START;
+	int lo_ok, hi_ok;
+
+	switch (type) {
+	case RTE_UINT8:
+		lo_ok = !neg;
+		hi_ok = uintres <= UINT8_MAX;
+		break;
+	case RTE_UINT16:
+		lo_ok = !neg;
+		hi_ok = uintres <= UINT16_MAX;
+		break;
+	case RTE_UINT32:
+		lo_ok = !neg;
+		hi_ok = uintres <= UINT32_MAX;
+		break;
+	case RTE_UINT64:
+		lo_ok = !neg;
+		hi_ok = 1; /* can't be out of range if parsed successfully */
+		break;
+	case RTE_INT8:
+		lo_ok = !neg || (int64_t)uintres >= INT8_MIN;
+		hi_ok = neg || uintres <= INT8_MAX;
+		break;
+	case RTE_INT16:
+		lo_ok = !neg || (int64_t)uintres >= INT16_MIN;
+		hi_ok = neg || uintres <= INT16_MAX;
+		break;
+	case RTE_INT32:
+		lo_ok = !neg || (int64_t)uintres >= INT32_MIN;
+		hi_ok = neg || uintres <= INT32_MAX;
+		break;
+	case RTE_INT64:
+		lo_ok = 1; /* can't be out of range if parsed successfully */
+		hi_ok = neg || uintres <= INT64_MAX;
+		break;
+	default:
+		debug_printf("Check range failed: unsupported number format: %s\n",
+				num_help[type]);
+		return -1;
+	}
+	/* check ranges */
+	if (!lo_ok || !hi_ok)
+		return -1;
+	return 0;
+}
+
+static int
+parse_num(const char *srcbuf, uint64_t *resptr)
+{
+	uint64_t uintres;
+	char *end;
+	int neg = *srcbuf == '-';
+
+	/*
+	 * strtoull does not do range checks on negative numbers, so we need to
+	 * use strtoll if we know the value we're parsing looks like a negative
+	 * one. we use base 0 for both, 0 means autodetect base.
+	 */
+	errno = 0;
+	if (neg)
+		uintres = (uint64_t)strtoll(srcbuf, &end, 0);
+	else
+		uintres = strtoull(srcbuf, &end, 0);
+
+	if (end == srcbuf || !cmdline_isendoftoken(*end) || errno == ERANGE)
+		return -1;
+
+	*resptr = uintres;
+	return end - srcbuf;
+}
+
+static int
+parse_bin(const char *srcbuf, uint64_t *res)
+{
+	uint64_t uintres = 0;
+	enum {
+		ERROR,
+		START,
+		BIN,
+		ZERO_OK,
+		BIN_OK,
+	} st = START;
 	const char * buf;
 	char c;
-	uint64_t res1 = 0;
-
-	if (!tk)
-		return -1;
-
-	if (!srcbuf || !*srcbuf)
-		return -1;
 
 	buf = srcbuf;
 	c = *buf;
-
-	memcpy(&nd, &((struct cmdline_token_num *)tk)->num_data, sizeof(nd));
-
-	/* check that we have enough room in res */
-	if (res) {
-		if (check_res_size(&nd, ressize) < 0)
-			return -1;
-	}
-
 	while (st != ERROR && c && !cmdline_isendoftoken(c)) {
 		debug_printf("%c %x -> ", c, c);
 		switch (st) {
 		case START:
-			if (c == '-') {
-				st = DEC_NEG;
-			}
-			else if (c == '0') {
+			if (c == '0') {
 				st = ZERO_OK;
 			}
-			else if (c >= '1' && c <= '9') {
-				if (add_to_res(c - '0', &res1, 10) < 0)
-					st = ERROR;
-				else
-					st = DEC_POS_OK;
-			}
-			else  {
+			else {
 				st = ERROR;
 			}
 			break;
 
 		case ZERO_OK:
-			if (c == 'x') {
-				st = HEX;
-			}
-			else if (c == 'b') {
+			if (c == 'b') {
 				st = BIN;
 			}
-			else if (c >= '0' && c <= '7') {
-				if (add_to_res(c - '0', &res1, 10) < 0)
-					st = ERROR;
-				else
-					st = OCTAL_OK;
-			}
-			else  {
-				st = ERROR;
-			}
-			break;
-
-		case DEC_NEG:
-			if (c >= '0' && c <= '9') {
-				if (add_to_res(c - '0', &res1, 10) < 0)
-					st = ERROR;
-				else
-					st = DEC_NEG_OK;
-			}
-			else {
-				st = ERROR;
-			}
-			break;
-
-		case DEC_NEG_OK:
-			if (c >= '0' && c <= '9') {
-				if (add_to_res(c - '0', &res1, 10) < 0)
-					st = ERROR;
-			}
-			else {
-				st = ERROR;
-			}
-			break;
-
-		case DEC_POS_OK:
-			if (c >= '0' && c <= '9') {
-				if (add_to_res(c - '0', &res1, 10) < 0)
-					st = ERROR;
-			}
-			else {
-				st = ERROR;
-			}
-			break;
-
-		case HEX:
-			st = HEX_OK;
-			/* fall-through */
-		case HEX_OK:
-			if (c >= '0' && c <= '9') {
-				if (add_to_res(c - '0', &res1, 16) < 0)
-					st = ERROR;
-			}
-			else if (c >= 'a' && c <= 'f') {
-				if (add_to_res(c - 'a' + 10, &res1, 16) < 0)
-					st = ERROR;
-			}
-			else if (c >= 'A' && c <= 'F') {
-				if (add_to_res(c - 'A' + 10, &res1, 16) < 0)
-					st = ERROR;
-			}
-			else {
-				st = ERROR;
-			}
-			break;
-
-
-		case OCTAL_OK:
-			if (c >= '0' && c <= '7') {
-				if (add_to_res(c - '0', &res1, 8) < 0)
-					st = ERROR;
-			}
 			else {
 				st = ERROR;
 			}
@@ -230,7 +195,7 @@ cmdline_parse_num(cmdline_parse_token_hdr_t *tk, const char *srcbuf, void *res,
 			/* fall-through */
 		case BIN_OK:
 			if (c >= '0' && c <= '1') {
-				if (add_to_res(c - '0', &res1, 2) < 0)
+				if (add_to_bin(c - '0', &uintres) < 0)
 					st = ERROR;
 			}
 			else {
@@ -239,10 +204,10 @@ cmdline_parse_num(cmdline_parse_token_hdr_t *tk, const char *srcbuf, void *res,
 			break;
 		default:
 			debug_printf("not impl ");
-
+			st = ERROR;
 		}
 
-		debug_printf("(%"PRIu64")\n", res1);
+		debug_printf("(%"PRIu64")\n", uintres);
 
 		buf ++;
 		c = *buf;
@@ -252,66 +217,93 @@ cmdline_parse_num(cmdline_parse_token_hdr_t *tk, const char *srcbuf, void *res,
 			return -1;
 	}
 
-	switch (st) {
-	case ZERO_OK:
-	case DEC_POS_OK:
-	case HEX_OK:
-	case OCTAL_OK:
-	case BIN_OK:
-		if (nd.type == RTE_INT8 && res1 <= INT8_MAX) {
-			if (res) *(int8_t *)res = (int8_t) res1;
-			return buf-srcbuf;
-		} else if (nd.type == RTE_INT16 && res1 <= INT16_MAX) {
-			if (res) *(int16_t *)res = (int16_t) res1;
-			return buf-srcbuf;
-		} else if (nd.type == RTE_INT32 && res1 <= INT32_MAX) {
-			if (res) *(int32_t *)res = (int32_t) res1;
-			return buf-srcbuf;
-		} else if (nd.type == RTE_INT64 && res1 <= INT64_MAX) {
-			if (res) *(int64_t *)res = (int64_t) res1;
-			return buf-srcbuf;
-		} else if (nd.type == RTE_UINT8 && res1 <= UINT8_MAX) {
-			if (res) *(uint8_t *)res = (uint8_t) res1;
-			return buf-srcbuf;
-		} else if (nd.type == RTE_UINT16  && res1 <= UINT16_MAX) {
-			if (res) *(uint16_t *)res = (uint16_t) res1;
-			return buf-srcbuf;
-		} else if (nd.type == RTE_UINT32 && res1 <= UINT32_MAX) {
-			if (res) *(uint32_t *)res = (uint32_t) res1;
-			return buf-srcbuf;
-		} else if (nd.type == RTE_UINT64) {
-			if (res) *(uint64_t *)res = res1;
-			return buf-srcbuf;
-		} else {
-			return -1;
-		}
-		break;
+	if (st != BIN_OK)
+		return -1;
+
+	*res = uintres;
+	return buf - srcbuf;
+}
 
-	case DEC_NEG_OK:
-		if (nd.type == RTE_INT8 &&
-				res1 <= INT8_MAX + 1) {
-			if (res) *(int8_t *)res = (int8_t) (-res1);
-			return buf-srcbuf;
-		} else if (nd.type == RTE_INT16 &&
-				res1 <= (uint16_t)INT16_MAX + 1) {
-			if (res) *(int16_t *)res = (int16_t) (-res1);
-			return buf-srcbuf;
-		} else if (nd.type == RTE_INT32 &&
-				res1 <= (uint32_t)INT32_MAX + 1) {
-			if (res) *(int32_t *)res = (int32_t) (-res1);
-			return buf-srcbuf;
-		} else if (nd.type == RTE_INT64 &&
-				res1 <= (uint64_t)INT64_MAX + 1) {
-			if (res) *(int64_t *)res = (int64_t) (-res1);
-			return buf-srcbuf;
-		} else {
-			return -1;
-		}
+static int
+write_num(enum cmdline_numtype type, void *res, uint64_t uintres)
+{
+	switch (type) {
+	case RTE_UINT8:
+		*(uint8_t *)res = (uint8_t)uintres;
+		break;
+	case RTE_UINT16:
+		*(uint16_t *)res = (uint16_t)uintres;
+		break;
+	case RTE_UINT32:
+		*(uint32_t *)res = (uint32_t)uintres;
+		break;
+	case RTE_UINT64:
+		*(uint64_t *)res = uintres;
+		break;
+	case RTE_INT8:
+		*(int8_t *)res = (int8_t)uintres;
+		break;
+	case RTE_INT16:
+		*(int16_t *)res = (int16_t)uintres;
+		break;
+	case RTE_INT32:
+		*(int32_t *)res = (int32_t)uintres;
+		break;
+	case RTE_INT64:
+		*(int64_t *)res = (int64_t)uintres;
 		break;
 	default:
-		debug_printf("error\n");
+		debug_printf("Write failed: unsupported number format: %s\n",
+				num_help[type]);
 		return -1;
 	}
+	return 0;
+}
+
+/* parse an int */
+RTE_EXPORT_SYMBOL(cmdline_parse_num)
+int
+cmdline_parse_num(cmdline_parse_token_hdr_t *tk, const char *srcbuf, void *res,
+	unsigned ressize)
+{
+	struct cmdline_token_num_data nd;
+
+	if (!tk)
+		return -1;
+
+	if (!srcbuf || !*srcbuf)
+		return -1;
+
+	memcpy(&nd, &((struct cmdline_token_num *)tk)->num_data, sizeof(nd));
+
+	/* check that we have enough room in res */
+	if (res && check_res_size(&nd, ressize) < 0)
+		return -1;
+
+	if (nd.type >= RTE_UINT8 && nd.type <= RTE_INT64) {
+		int ret, neg = *srcbuf == '-';
+		uint64_t uintres;
+
+		/* try parsing as number */
+		ret = parse_num(srcbuf, &uintres);
+
+		if (ret < 0) {
+			/* parse failed, try parsing as binary */
+			ret = parse_bin(srcbuf, &uintres);
+			if (ret < 0)
+				return -1;
+		}
+		/* check if we're within valid range */
+		if (check_parsed_num(nd.type, neg, uintres) < 0)
+			return -1;
+
+		/* parsing succeeded, write the value if necessary */
+		if (res && write_num(nd.type, res, uintres) < 0)
+			return -1;
+
+		return ret;
+	}
+	return -1;
 }
 
 
-- 
2.47.1