diff mbox series

[01/12] net: add string to IPv4 parse function

Message ID 20211214141242.3383831-2-ronan.randles@intel.com (mailing list archive)
State Not Applicable, archived
Delegated to: Thomas Monjalon
Headers show
Series add packet generator library and example app | expand

Checks

Context Check Description
ci/checkpatch success coding style OK

Commit Message

Ronan Randles Dec. 14, 2021, 2:12 p.m. UTC
Added function that accepts ip string as a parameter and returns an ip
address represented by a uint32_t. Relevant unit test for this function
is also included.

Signed-off-by: Harry van Haaren <harry.van.haaren@intel.com>
Signed-off-by: Ronan Randles <ronan.randles@intel.com>
---
 app/test/meson.build |  2 ++
 app/test/test_net.c  | 61 ++++++++++++++++++++++++++++++++++++++++++++
 lib/net/meson.build  |  1 +
 lib/net/rte_ip.c     | 43 +++++++++++++++++++++++++++++++
 lib/net/rte_ip.h     | 18 +++++++++++++
 lib/net/version.map  |  8 ++++++
 6 files changed, 133 insertions(+)
 create mode 100644 app/test/test_net.c
 create mode 100644 lib/net/rte_ip.c

Comments

Morten Brørup Dec. 14, 2021, 5:31 p.m. UTC | #1
> From: Ronan Randles [mailto:ronan.randles@intel.com]
> Sent: Tuesday, 14 December 2021 15.13
> 
> Added function that accepts ip string as a parameter and returns an ip
> address represented by a uint32_t. Relevant unit test for this function
> is also included.
> 
> Signed-off-by: Harry van Haaren <harry.van.haaren@intel.com>
> Signed-off-by: Ronan Randles <ronan.randles@intel.com>
> ---

[snip]

> diff --git a/lib/net/rte_ip.h b/lib/net/rte_ip.h
> index c575250852..188054fda4 100644
> --- a/lib/net/rte_ip.h
> +++ b/lib/net/rte_ip.h
> @@ -426,6 +426,24 @@ rte_ipv4_udptcp_cksum_verify(const struct
> rte_ipv4_hdr *ipv4_hdr,
>  	return 0;
>  }
> 
> +/**
> + * IP address parser.
> + *
> + * @param src_ip
> + *   The IP address to be parsed.
> + * @param output_addr
> + *   The array in which the parsed digits will be saved.
> + *
> + * @retval 0
> + *   Success.
> + * @retval -1
> + *   Failure due to invalid input arguments.
> + */
> +
> +__rte_experimental
> +int32_t
> +rte_ip_parse_addr(const char *src_ip, uint32_t *output_addr);
> +

Good initiative!

This should set a precedent for to/from string functions, so be careful about names and calling conventions.

I have a few suggestions:

The function should take a parameter to tell if the input string must be zero-terminated or not. This is highly useful for parsing subnet strings (e.g. "192.0.2.0/24") and IP range strings (e.g. "192.0.2.2-192.0.2.253").

The return value should be the number of characters read from the input string, and still -1 on error. With this modification, also consider using the return type ssize_t instead of int32_t.

-Morten
Bruce Richardson Dec. 15, 2021, 9:27 a.m. UTC | #2
On Tue, Dec 14, 2021 at 06:31:06PM +0100, Morten Brørup wrote:
> > From: Ronan Randles [mailto:ronan.randles@intel.com]
> > Sent: Tuesday, 14 December 2021 15.13
> > 
> > Added function that accepts ip string as a parameter and returns an ip
> > address represented by a uint32_t. Relevant unit test for this function
> > is also included.
> > 
> > Signed-off-by: Harry van Haaren <harry.van.haaren@intel.com>
> > Signed-off-by: Ronan Randles <ronan.randles@intel.com>
> > ---
> 
> [snip]
> 
> > diff --git a/lib/net/rte_ip.h b/lib/net/rte_ip.h
> > index c575250852..188054fda4 100644
> > --- a/lib/net/rte_ip.h
> > +++ b/lib/net/rte_ip.h
> > @@ -426,6 +426,24 @@ rte_ipv4_udptcp_cksum_verify(const struct
> > rte_ipv4_hdr *ipv4_hdr,
> >  	return 0;
> >  }
> > 
> > +/**
> > + * IP address parser.
> > + *
> > + * @param src_ip
> > + *   The IP address to be parsed.
> > + * @param output_addr
> > + *   The array in which the parsed digits will be saved.
> > + *
> > + * @retval 0
> > + *   Success.
> > + * @retval -1
> > + *   Failure due to invalid input arguments.
> > + */
> > +
> > +__rte_experimental
> > +int32_t
> > +rte_ip_parse_addr(const char *src_ip, uint32_t *output_addr);
> > +
> 
> Good initiative!
> 
> This should set a precedent for to/from string functions, so be careful about names and calling conventions.
> 
> I have a few suggestions:
> 
> The function should take a parameter to tell if the input string must be zero-terminated or not. This is highly useful for parsing subnet strings (e.g. "192.0.2.0/24") and IP range strings (e.g. "192.0.2.2-192.0.2.253").
> 
> The return value should be the number of characters read from the input string, and still -1 on error. With this modification, also consider using the return type ssize_t instead of int32_t.
> 
Interesting point on the "must be zero-terminated" parameter. However, if
the return value is the number of characters read, then the user can check
for null-termination very easily after the call, and not need the
parameter. Therefore, I would suggest having the function always assume
chars after the address and let the user check for null if needed
afterwards, to keep function simpler.

/Bruce
Morten Brørup Dec. 15, 2021, 9:35 a.m. UTC | #3
> From: Bruce Richardson [mailto:bruce.richardson@intel.com]
> Sent: Wednesday, 15 December 2021 10.27
> 
> On Tue, Dec 14, 2021 at 06:31:06PM +0100, Morten Brørup wrote:
> > > From: Ronan Randles [mailto:ronan.randles@intel.com]
> > > Sent: Tuesday, 14 December 2021 15.13
> > >
> > > Added function that accepts ip string as a parameter and returns an
> ip
> > > address represented by a uint32_t. Relevant unit test for this
> function
> > > is also included.
> > >
> > > Signed-off-by: Harry van Haaren <harry.van.haaren@intel.com>
> > > Signed-off-by: Ronan Randles <ronan.randles@intel.com>
> > > ---
> >
> > [snip]
> >
> > > diff --git a/lib/net/rte_ip.h b/lib/net/rte_ip.h
> > > index c575250852..188054fda4 100644
> > > --- a/lib/net/rte_ip.h
> > > +++ b/lib/net/rte_ip.h
> > > @@ -426,6 +426,24 @@ rte_ipv4_udptcp_cksum_verify(const struct
> > > rte_ipv4_hdr *ipv4_hdr,
> > >  	return 0;
> > >  }
> > >
> > > +/**
> > > + * IP address parser.
> > > + *
> > > + * @param src_ip
> > > + *   The IP address to be parsed.
> > > + * @param output_addr
> > > + *   The array in which the parsed digits will be saved.
> > > + *
> > > + * @retval 0
> > > + *   Success.
> > > + * @retval -1
> > > + *   Failure due to invalid input arguments.
> > > + */
> > > +
> > > +__rte_experimental
> > > +int32_t
> > > +rte_ip_parse_addr(const char *src_ip, uint32_t *output_addr);
> > > +
> >
> > Good initiative!
> >
> > This should set a precedent for to/from string functions, so be
> careful about names and calling conventions.
> >
> > I have a few suggestions:
> >
> > The function should take a parameter to tell if the input string must
> be zero-terminated or not. This is highly useful for parsing subnet
> strings (e.g. "192.0.2.0/24") and IP range strings (e.g. "192.0.2.2-
> 192.0.2.253").
> >
> > The return value should be the number of characters read from the
> input string, and still -1 on error. With this modification, also
> consider using the return type ssize_t instead of int32_t.
> >
> Interesting point on the "must be zero-terminated" parameter. However,
> if
> the return value is the number of characters read, then the user can
> check
> for null-termination very easily after the call, and not need the
> parameter. Therefore, I would suggest having the function always assume
> chars after the address and let the user check for null if needed
> afterwards, to keep function simpler.

That would require additional lines of code in the application. I would rather have that additional code inside the utility function. There is no need to keep the function simple.
Bruce Richardson Dec. 15, 2021, 10:11 a.m. UTC | #4
On Wed, Dec 15, 2021 at 10:35:44AM +0100, Morten Brørup wrote:
> > From: Bruce Richardson [mailto:bruce.richardson@intel.com]
> > Sent: Wednesday, 15 December 2021 10.27
> > 
> > On Tue, Dec 14, 2021 at 06:31:06PM +0100, Morten Brørup wrote:
> > > > From: Ronan Randles [mailto:ronan.randles@intel.com]
> > > > Sent: Tuesday, 14 December 2021 15.13
> > > >
> > > > Added function that accepts ip string as a parameter and returns an
> > ip
> > > > address represented by a uint32_t. Relevant unit test for this
> > function
> > > > is also included.
> > > >
> > > > Signed-off-by: Harry van Haaren <harry.van.haaren@intel.com>
> > > > Signed-off-by: Ronan Randles <ronan.randles@intel.com>
> > > > ---
> > >
> > > [snip]
> > >
> > > > diff --git a/lib/net/rte_ip.h b/lib/net/rte_ip.h
> > > > index c575250852..188054fda4 100644
> > > > --- a/lib/net/rte_ip.h
> > > > +++ b/lib/net/rte_ip.h
> > > > @@ -426,6 +426,24 @@ rte_ipv4_udptcp_cksum_verify(const struct
> > > > rte_ipv4_hdr *ipv4_hdr,
> > > >  	return 0;
> > > >  }
> > > >
> > > > +/**
> > > > + * IP address parser.
> > > > + *
> > > > + * @param src_ip
> > > > + *   The IP address to be parsed.
> > > > + * @param output_addr
> > > > + *   The array in which the parsed digits will be saved.
> > > > + *
> > > > + * @retval 0
> > > > + *   Success.
> > > > + * @retval -1
> > > > + *   Failure due to invalid input arguments.
> > > > + */
> > > > +
> > > > +__rte_experimental
> > > > +int32_t
> > > > +rte_ip_parse_addr(const char *src_ip, uint32_t *output_addr);
> > > > +
> > >
> > > Good initiative!
> > >
> > > This should set a precedent for to/from string functions, so be
> > careful about names and calling conventions.
> > >
> > > I have a few suggestions:
> > >
> > > The function should take a parameter to tell if the input string must
> > be zero-terminated or not. This is highly useful for parsing subnet
> > strings (e.g. "192.0.2.0/24") and IP range strings (e.g. "192.0.2.2-
> > 192.0.2.253").
> > >
> > > The return value should be the number of characters read from the
> > input string, and still -1 on error. With this modification, also
> > consider using the return type ssize_t instead of int32_t.
> > >
> > Interesting point on the "must be zero-terminated" parameter. However,
> > if
> > the return value is the number of characters read, then the user can
> > check
> > for null-termination very easily after the call, and not need the
> > parameter. Therefore, I would suggest having the function always assume
> > chars after the address and let the user check for null if needed
> > afterwards, to keep function simpler.
> 
> That would require additional lines of code in the application. I would rather have that additional code inside the utility function. There is no need to keep the function simple.
>
Well, it's only an extra item in the error-check conditional, but point
taken. I have no strong opinion here.
Thomas Monjalon Jan. 19, 2022, 2:20 p.m. UTC | #5
14/12/2021 15:12, Ronan Randles:
> --- a/lib/net/rte_ip.h
> +++ b/lib/net/rte_ip.h
> +/**
> + * IP address parser.
> + *
> + * @param src_ip
> + *   The IP address to be parsed.
> + * @param output_addr
> + *   The array in which the parsed digits will be saved.
> + *
> + * @retval 0
> + *   Success.
> + * @retval -1
> + *   Failure due to invalid input arguments.
> + */
> +
> +__rte_experimental
> +int32_t
> +rte_ip_parse_addr(const char *src_ip, uint32_t *output_addr);

Is it similar to inet_aton() ?
Does it support IPv6? If not, why not adding a number 4 in the name?
diff mbox series

Patch

diff --git a/app/test/meson.build b/app/test/meson.build
index 2b480adfba..4cf540fc74 100644
--- a/app/test/meson.build
+++ b/app/test/meson.build
@@ -100,6 +100,7 @@  test_sources = files(
         'test_meter.c',
         'test_mcslock.c',
         'test_mp_secondary.c',
+        'test_net.c',
         'test_per_lcore.c',
         'test_pflock.c',
         'test_pmd_perf.c',
@@ -177,6 +178,7 @@  test_deps = [
         'ipsec',
         'lpm',
         'member',
+        'net',
         'node',
         'pipeline',
         'port',
diff --git a/app/test/test_net.c b/app/test/test_net.c
new file mode 100644
index 0000000000..2cb7d3e1c9
--- /dev/null
+++ b/app/test/test_net.c
@@ -0,0 +1,61 @@ 
+/* SPDX-License-Identifier: BSD-3-Clause
+ * Copyright(c) 2021 Intel Corporation
+ */
+
+#include <stdio.h>
+#include <stdint.h>
+
+#include <rte_ip.h>
+#include <rte_common.h>
+#include "test.h"
+
+static int
+test_rte_ip_parse_addr(void)
+{
+	printf("Running IP parsing tests...\n");
+
+	struct str_ip_t {
+		const char *str;
+		uint32_t exp_output;
+		uint32_t expected_to_fail;
+	} str_ip_tests[] = {
+		{ .str = "1.2.3.4", .exp_output = RTE_IPV4(1, 2, 3, 4)},
+		{ .str = "192.168.255.255", .exp_output =
+				RTE_IPV4(192, 168, 255, 255)},
+		{ .str = "172.16.0.9", .exp_output =
+				RTE_IPV4(172, 16, 0, 9)},
+		{ .str = "1.2.3", .expected_to_fail = 1},
+		{ .str = "1.2.3.4.5", .expected_to_fail = 1},
+		{ .str = "fail.1.2.3", .expected_to_fail = 1},
+		{ .str = "", .expected_to_fail = 1},
+		{ .str = "1.2.3.fail", .expected_to_fail = 1}
+	};
+
+	uint32_t i;
+	for (i = 0; i < RTE_DIM(str_ip_tests); i++) {
+		uint32_t test_addr;
+		int32_t err = rte_ip_parse_addr(str_ip_tests[i].str,
+							&test_addr);
+		if (!test_addr) {
+			if (str_ip_tests[i].expected_to_fail != 1)
+				return -1;
+		}
+
+		if (err || test_addr != str_ip_tests[i].exp_output) {
+			if (str_ip_tests[i].expected_to_fail != 1)
+				return -1;
+		}
+	}
+
+
+	return 0;
+}
+
+static int
+test_net_tests(void)
+{
+	int ret = test_rte_ip_parse_addr();
+	return ret;
+}
+
+REGISTER_TEST_COMMAND(net_autotest, test_net_tests);
diff --git a/lib/net/meson.build b/lib/net/meson.build
index e899846578..b2577a7592 100644
--- a/lib/net/meson.build
+++ b/lib/net/meson.build
@@ -26,6 +26,7 @@  headers = files(
 sources = files(
         'rte_arp.c',
         'rte_ether.c',
+        'rte_ip.c',
         'rte_net.c',
         'rte_net_crc.c',
 )
diff --git a/lib/net/rte_ip.c b/lib/net/rte_ip.c
new file mode 100644
index 0000000000..b859dfb640
--- /dev/null
+++ b/lib/net/rte_ip.c
@@ -0,0 +1,43 @@ 
+/* SPDX-License-Identifier: BSD-3-Clause
+ * Copyright(c) 2021 Intel Corporation
+ */
+
+#include <string.h>
+#include <rte_ip.h>
+
+int32_t
+rte_ip_parse_addr(const char *src_ip, uint32_t *output_addr)
+{
+	int32_t ret = 0;
+	char *current_position;
+
+	if (src_ip == NULL)
+		return -1;
+
+	char *tok = strdup(src_ip);
+	if (tok == NULL)
+		return -1;
+
+	char *current_digit = strtok_r(tok, ".", &current_position);
+
+	*output_addr = 0;
+	uint32_t i = 0;
+	while (current_digit) {
+		uint32_t shift = ((3 - i) * 8);
+		unsigned long parsed_value = strtoul(current_digit, NULL, 0)
+								<< shift;
+
+		if (parsed_value == 0 && strcmp(current_digit, "0"))
+			break;
+
+		*output_addr |= parsed_value;
+		current_digit = strtok_r(NULL, ".", &current_position);
+		i++;
+
+	}
+	if (i != 4)
+		return -1;
+
+	free(tok);
+	return ret;
+}
diff --git a/lib/net/rte_ip.h b/lib/net/rte_ip.h
index c575250852..188054fda4 100644
--- a/lib/net/rte_ip.h
+++ b/lib/net/rte_ip.h
@@ -426,6 +426,24 @@  rte_ipv4_udptcp_cksum_verify(const struct rte_ipv4_hdr *ipv4_hdr,
 	return 0;
 }
 
+/**
+ * IP address parser.
+ *
+ * @param src_ip
+ *   The IP address to be parsed.
+ * @param output_addr
+ *   The array in which the parsed digits will be saved.
+ *
+ * @retval 0
+ *   Success.
+ * @retval -1
+ *   Failure due to invalid input arguments.
+ */
+
+__rte_experimental
+int32_t
+rte_ip_parse_addr(const char *src_ip, uint32_t *output_addr);
+
 /**
  * IPv6 Header
  */
diff --git a/lib/net/version.map b/lib/net/version.map
index 4f4330d1c4..3e5a307e48 100644
--- a/lib/net/version.map
+++ b/lib/net/version.map
@@ -12,3 +12,11 @@  DPDK_22 {
 
 	local: *;
 };
+
+EXPERIMENTAL {
+	global:
+
+	rte_net_skip_ip6_ext;
+	rte_ether_unformat_addr;
+	rte_ip_parse_addr;
+};