[PATCH v2 05/61] net/null: use kvargs numeric helpers

[Stephen Hemminger]

The three device argument handlers each convert with strtoul() and
check neither errno nor the end pointer, so "size=foo" is silently
taken as zero and "size=99999999999999999999" as ULONG_MAX truncated
to unsigned int. The checks for UINT_MAX that follow only catch the
truncated overflow case on a 32 bit build.

A zero packet size was accepted, and the value is later cast to the
16 bit mbuf data length, so anything above 65535 was silently
truncated. Convert with an explicit 1..UINT16_MAX range instead.

The copy and no-rx arguments are booleans, so store them as bool and
use rte_kvargs_handle_bool. This also means they now accept the word
forms such as "copy=on", not only 0 and 1.

While here, set an error code when copy and no-rx are both given.
The error was logged but the zero return from the last successful
rte_kvargs_process() was returned, so probe reported success without
ever creating the device.

The booleans use rte_kvargs_process_opt(), so that a bare key with no
value enables the option.

Signed-off-by: Stephen Hemminger <stephen at networkplumber.org>
---
 doc/guides/nics/null.rst        |  2 +-
 drivers/net/null/rte_eth_null.c | 78 ++++++++++-----------------------
 2 files changed, 24 insertions(+), 56 deletions(-)

diff --git a/doc/guides/nics/null.rst b/doc/guides/nics/null.rst
index c0e3199102..fd8d685d83 100644
--- a/doc/guides/nics/null.rst
+++ b/doc/guides/nics/null.rst
@@ -30,7 +30,7 @@ Runtime Configuration
 
 - ``size`` [optional, default=64 bytes]
 
- Custom packet length value to use.r
+ Custom packet length value to use, from 1 to 65535.
  If ``copy`` is enabled, this is the length of copy operation.
 
 .. code-block:: console
diff --git a/drivers/net/null/rte_eth_null.c b/drivers/net/null/rte_eth_null.c
index a469825cb6..6f3b27bd78 100644
--- a/drivers/net/null/rte_eth_null.c
+++ b/drivers/net/null/rte_eth_null.c
@@ -3,7 +3,8 @@
  *  All rights reserved.
  */
 
-#include <stdlib.h>
+#include <stdbool.h>
+#include <stdint.h>
 
 #include <rte_mbuf.h>
 #include <ethdev_driver.h>
@@ -19,8 +20,8 @@
 #define ETH_NULL_PACKET_NO_RX_ARG	"no-rx"
 
 static unsigned int default_packet_size = 64;
-static unsigned int default_packet_copy;
-static unsigned int default_no_rx;
+static bool default_packet_copy;
+static bool default_no_rx;
 
 static const char *valid_arguments[] = {
 	ETH_NULL_PACKET_SIZE_ARG,
@@ -45,15 +46,15 @@ struct null_queue {
 };
 
 struct pmd_options {
-	unsigned int packet_copy;
+	bool packet_copy;
 	unsigned int packet_size;
-	unsigned int no_rx;
+	bool no_rx;
 };
 
 struct pmd_internals {
 	unsigned int packet_size;
-	unsigned int packet_copy;
-	unsigned int no_rx;
+	bool packet_copy;
+	bool no_rx;
 	uint16_t port_id;
 
 	struct null_queue rx_null_queues[RTE_MAX_QUEUES_PER_PORT];
@@ -606,55 +607,22 @@ eth_dev_null_create(struct rte_vdev_device *dev, struct pmd_options *args)
 	return 0;
 }
 
-static inline int
-get_packet_size_arg(const char *key __rte_unused,
-		const char *value, void *extra_args)
-{
-	const char *a = value;
-	unsigned int *packet_size = extra_args;
-
-	if ((value == NULL) || (extra_args == NULL))
-		return -EINVAL;
-
-	*packet_size = (unsigned int)strtoul(a, NULL, 0);
-	if (*packet_size == UINT_MAX)
-		return -1;
-
-	return 0;
-}
-
-static inline int
-get_packet_copy_arg(const char *key __rte_unused,
-		const char *value, void *extra_args)
-{
-	const char *a = value;
-	unsigned int *packet_copy = extra_args;
-
-	if ((value == NULL) || (extra_args == NULL))
-		return -EINVAL;
-
-	*packet_copy = (unsigned int)strtoul(a, NULL, 0);
-	if (*packet_copy == UINT_MAX)
-		return -1;
-
-	return 0;
-}
-
+/* Packet size is stored in a 16 bit mbuf data length, and must not be zero. */
 static int
-get_packet_no_rx_arg(const char *key __rte_unused,
+get_packet_size_arg(const char *key __rte_unused,
 		const char *value, void *extra_args)
 {
-	const char *a = value;
-	unsigned int no_rx;
+	uint64_t packet_size;
+	int ret;
 
-	if (value == NULL || extra_args == NULL)
+	if (extra_args == NULL)
 		return -EINVAL;
 
-	no_rx = (unsigned int)strtoul(a, NULL, 0);
-	if (no_rx != 0 && no_rx != 1)
-		return -1;
+	ret = rte_kvargs_to_uint(value, 1, UINT16_MAX, &packet_size);
+	if (ret < 0)
+		return ret;
 
-	*(unsigned int *)extra_args = no_rx;
+	*(unsigned int *)extra_args = packet_size;
 	return 0;
 }
 
@@ -714,16 +682,15 @@ rte_pmd_null_probe(struct rte_vdev_device *dev)
 		if (ret < 0)
 			goto free_kvlist;
 
-
-		ret = rte_kvargs_process(kvlist,
+		ret = rte_kvargs_process_opt(kvlist,
 				ETH_NULL_PACKET_COPY_ARG,
-				&get_packet_copy_arg, &args.packet_copy);
+				rte_kvargs_handle_bool, &args.packet_copy);
 		if (ret < 0)
 			goto free_kvlist;
 
-		ret = rte_kvargs_process(kvlist,
+		ret = rte_kvargs_process_opt(kvlist,
 				ETH_NULL_PACKET_NO_RX_ARG,
-				&get_packet_no_rx_arg, &args.no_rx);
+				rte_kvargs_handle_bool, &args.no_rx);
 		if (ret < 0)
 			goto free_kvlist;
 
@@ -732,11 +699,12 @@ rte_pmd_null_probe(struct rte_vdev_device *dev)
 				"Both %s and %s arguments at the same time not supported",
 				ETH_NULL_PACKET_COPY_ARG,
 				ETH_NULL_PACKET_NO_RX_ARG);
+			ret = -EINVAL;
 			goto free_kvlist;
 		}
 	}
 
-	PMD_LOG(INFO, "Configure pmd_null: packet size is %d, "
+	PMD_LOG(INFO, "Configure pmd_null: packet size is %u, "
 			"packet copy is %s", args.packet_size,
 			args.packet_copy ? "enabled" : "disabled");
 
-- 
2.53.0