[02/12] net: add function to pretty print IPv4

  This function accepts an uint32_t representation of an IP address and
produces a string representation stored in a char * buffer. Realavent
unit tests also included.

Signed-off-by: Ronan Randles <ronan.randles@intel.com>
---
 app/test/test_net.c | 26 ++++++++++++++++++++++++++
 lib/net/rte_ip.c    | 15 +++++++++++++++
 lib/net/rte_ip.h    | 20 ++++++++++++++++++++
 lib/net/version.map |  1 +
 4 files changed, 62 insertions(+)
  

On Tue, 14 Dec 2021 14:12:32 +0000
Ronan Randles <ronan.randles@intel.com> wrote:

> This function accepts an uint32_t representation of an IP address and
> produces a string representation stored in a char * buffer. Realavent
> unit tests also included.
> 
> Signed-off-by: Ronan Randles <ronan.randles@intel.com>

Do we really have to reinvent getnameinfo()?
Is this for Windows?
  

> From: Ronan Randles [mailto:ronan.randles@intel.com]
> Sent: Tuesday, 14 December 2021 15.13
> 
> This function accepts an uint32_t representation of an IP address and
> produces a string representation stored in a char * buffer. Realavent
> unit tests also included.
> 
> 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 188054fda4..e46f0b41ba 100644
> --- a/lib/net/rte_ip.h
> +++ b/lib/net/rte_ip.h
> @@ -444,6 +444,26 @@ __rte_experimental
>  int32_t
>  rte_ip_parse_addr(const char *src_ip, uint32_t *output_addr);
> 
> +
> +/**
> + * Print IP address from 32 bit int into char * buffer.
> + *
> + * @param ip_addr
> + *   ip address to be printed.
> + * @param buffer
> + *   The buffer the string will be saved into.
> + * @param buffer_size
> + *   size of buffer to be used.
> + *
> + * @retval 0
> + *   Success.
> + * @retval -1
> + *   Failure due to invalid input arguments.
> + */
> +__rte_experimental
> +int32_t
> +rte_ip_print_addr(uint32_t ip_addr, char *buffer, uint32_t
> buffer_size);
> +

In continuation of my email reply about the IPv4 parse function...

I have a few suggestions to the IPv4 print function too:

The return value should be the number of characters written to the output string, and still -1 on error. With this modification, you could use the return type ssize_t instead of int32_t.

Furthermore, I would prefer having the parameters in the same order as snprintf(): char *str, size_t size, const uint32_t ip_addr. Please also notice the suggested changed type for the size, and the const added to the ip_addr.

-Morten
  

> From: Stephen Hemminger [mailto:stephen@networkplumber.org]
> Sent: Tuesday, 14 December 2021 17.09
> 
> On Tue, 14 Dec 2021 14:12:32 +0000
> Ronan Randles <ronan.randles@intel.com> wrote:
> 
> > This function accepts an uint32_t representation of an IP address and
> > produces a string representation stored in a char * buffer. Realavent
> > unit tests also included.
> >
> > Signed-off-by: Ronan Randles <ronan.randles@intel.com>
> 
> Do we really have to reinvent getnameinfo()?
> Is this for Windows?

For general inspiration:

We have an in-house to/from string library, where our xx_to_str() functions return char * and can take a NULL pointer as the buffer parameter to make it use a buffer from an small cyclic pool of string buffers in the library.

Probably not good from an academic standpoint, and not good for a generic library, but we know how many of these strings are being used simultaneously in our applications, so the pool is not overrun in reality. And it makes the code using these functions much shorter and readable.

-Morten
  

> -----Original Message-----
> From: Morten Brørup <mb@smartsharesystems.com>
> Sent: Tuesday, December 14, 2021 5:31 PM
> To: Randles, Ronan <ronan.randles@intel.com>; dev@dpdk.org
> Cc: Van Haaren, Harry <harry.van.haaren@intel.com>
> Subject: RE: [PATCH 02/12] net: add function to pretty print IPv4
> 
> > From: Ronan Randles [mailto:ronan.randles@intel.com]
> > Sent: Tuesday, 14 December 2021 15.13
> >
> > This function accepts an uint32_t representation of an IP address and
> > produces a string representation stored in a char * buffer. Realavent
> > unit tests also included.
> >
> > 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 188054fda4..e46f0b41ba 100644
> > --- a/lib/net/rte_ip.h
> > +++ b/lib/net/rte_ip.h
> > @@ -444,6 +444,26 @@ __rte_experimental
> >  int32_t
> >  rte_ip_parse_addr(const char *src_ip, uint32_t *output_addr);
> >
> > +
> > +/**
> > + * Print IP address from 32 bit int into char * buffer.
> > + *
> > + * @param ip_addr
> > + *   ip address to be printed.
> > + * @param buffer
> > + *   The buffer the string will be saved into.
> > + * @param buffer_size
> > + *   size of buffer to be used.
> > + *
> > + * @retval 0
> > + *   Success.
> > + * @retval -1
> > + *   Failure due to invalid input arguments.
> > + */
> > +__rte_experimental
> > +int32_t
> > +rte_ip_print_addr(uint32_t ip_addr, char *buffer, uint32_t
> > buffer_size);
> > +
> 
> In continuation of my email reply about the IPv4 parse function...
> 
> I have a few suggestions to the IPv4 print function too:
> 
> The return value should be the number of characters written to the output string, and still -1 on error. With this modification, you could
> use the return type ssize_t instead of int32_t.
> 
> Furthermore, I would prefer having the parameters in the same order as snprintf(): char *str, size_t size, const uint32_t ip_addr. Please
> also notice the suggested changed type for the size, and the const added to the ip_addr.
> 
Honestly, I don't understand why we need to introduce such functions
inside DPDK at all.
What's wrong with existing standard ones: inet_ntop() and inet_pton()?
  

On Wed, 15 Dec 2021 01:06:14 +0000
"Ananyev, Konstantin" <konstantin.ananyev@intel.com> wrote:

> > -----Original Message-----
> > From: Morten Brørup <mb@smartsharesystems.com>
> > Sent: Tuesday, December 14, 2021 5:31 PM
> > To: Randles, Ronan <ronan.randles@intel.com>; dev@dpdk.org
> > Cc: Van Haaren, Harry <harry.van.haaren@intel.com>
> > Subject: RE: [PATCH 02/12] net: add function to pretty print IPv4
> >   
> > > From: Ronan Randles [mailto:ronan.randles@intel.com]
> > > Sent: Tuesday, 14 December 2021 15.13
> > >
> > > This function accepts an uint32_t representation of an IP address and
> > > produces a string representation stored in a char * buffer. Realavent
> > > unit tests also included.
> > >
> > > 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 188054fda4..e46f0b41ba 100644
> > > --- a/lib/net/rte_ip.h
> > > +++ b/lib/net/rte_ip.h
> > > @@ -444,6 +444,26 @@ __rte_experimental
> > >  int32_t
> > >  rte_ip_parse_addr(const char *src_ip, uint32_t *output_addr);
> > >
> > > +
> > > +/**
> > > + * Print IP address from 32 bit int into char * buffer.
> > > + *
> > > + * @param ip_addr
> > > + *   ip address to be printed.
> > > + * @param buffer
> > > + *   The buffer the string will be saved into.
> > > + * @param buffer_size
> > > + *   size of buffer to be used.
> > > + *
> > > + * @retval 0
> > > + *   Success.
> > > + * @retval -1
> > > + *   Failure due to invalid input arguments.
> > > + */
> > > +__rte_experimental
> > > +int32_t
> > > +rte_ip_print_addr(uint32_t ip_addr, char *buffer, uint32_t
> > > buffer_size);
> > > +  
> > 
> > In continuation of my email reply about the IPv4 parse function...
> > 
> > I have a few suggestions to the IPv4 print function too:
> > 
> > The return value should be the number of characters written to the output string, and still -1 on error. With this modification, you could
> > use the return type ssize_t instead of int32_t.
> > 
> > Furthermore, I would prefer having the parameters in the same order as snprintf(): char *str, size_t size, const uint32_t ip_addr. Please
> > also notice the suggested changed type for the size, and the const added to the ip_addr.
> >   
> Honestly, I don't understand why we need to introduce such functions
> inside DPDK at all.
> What's wrong with existing standard ones: inet_ntop() and inet_pton()?

Agreed, I see no added value in reinventing here
  

> From: Stephen Hemminger [mailto:stephen@networkplumber.org]
> Sent: Wednesday, 15 December 2021 04.21
> 
> On Wed, 15 Dec 2021 01:06:14 +0000
> "Ananyev, Konstantin" <konstantin.ananyev@intel.com> wrote:
> 
> > > -----Original Message-----
> > > From: Morten Brørup <mb@smartsharesystems.com>
> > > Sent: Tuesday, December 14, 2021 5:31 PM
> > > To: Randles, Ronan <ronan.randles@intel.com>; dev@dpdk.org
> > > Cc: Van Haaren, Harry <harry.van.haaren@intel.com>
> > > Subject: RE: [PATCH 02/12] net: add function to pretty print IPv4
> > >
> > > > From: Ronan Randles [mailto:ronan.randles@intel.com]
> > > > Sent: Tuesday, 14 December 2021 15.13
> > > >
> > > > This function accepts an uint32_t representation of an IP address
> and
> > > > produces a string representation stored in a char * buffer.
> Realavent
> > > > unit tests also included.
> > > >
> > > > 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 188054fda4..e46f0b41ba 100644
> > > > --- a/lib/net/rte_ip.h
> > > > +++ b/lib/net/rte_ip.h
> > > > @@ -444,6 +444,26 @@ __rte_experimental
> > > >  int32_t
> > > >  rte_ip_parse_addr(const char *src_ip, uint32_t *output_addr);
> > > >
> > > > +
> > > > +/**
> > > > + * Print IP address from 32 bit int into char * buffer.
> > > > + *
> > > > + * @param ip_addr
> > > > + *   ip address to be printed.
> > > > + * @param buffer
> > > > + *   The buffer the string will be saved into.
> > > > + * @param buffer_size
> > > > + *   size of buffer to be used.
> > > > + *
> > > > + * @retval 0
> > > > + *   Success.
> > > > + * @retval -1
> > > > + *   Failure due to invalid input arguments.
> > > > + */
> > > > +__rte_experimental
> > > > +int32_t
> > > > +rte_ip_print_addr(uint32_t ip_addr, char *buffer, uint32_t
> > > > buffer_size);
> > > > +
> > >
> > > In continuation of my email reply about the IPv4 parse function...
> > >
> > > I have a few suggestions to the IPv4 print function too:
> > >
> > > The return value should be the number of characters written to the
> output string, and still -1 on error. With this modification, you could
> > > use the return type ssize_t instead of int32_t.
> > >
> > > Furthermore, I would prefer having the parameters in the same order
> as snprintf(): char *str, size_t size, const uint32_t ip_addr. Please
> > > also notice the suggested changed type for the size, and the const
> added to the ip_addr.
> > >
> > Honestly, I don't understand why we need to introduce such functions
> > inside DPDK at all.
> > What's wrong with existing standard ones: inet_ntop() and
> inet_pton()?
> 
> Agreed, I see no added value in reinventing here

I think that DPDK functions for converting all sorts of types to/from strings would be useful; not only IP addresses, but also MAC addresses, TCP/UDP port numbers and VLAN IDs.

If you don't like IP address string conversion functions in the net library, DPDK could have a string conversions library. That library could expose a multitude of APIs for the same purpose, so the application can use the API that best fits each application use.
  

> > From: Stephen Hemminger [mailto:stephen@networkplumber.org]
> > Sent: Wednesday, 15 December 2021 04.21
> >
> > On Wed, 15 Dec 2021 01:06:14 +0000
> > "Ananyev, Konstantin" <konstantin.ananyev@intel.com> wrote:
> >
> > > > -----Original Message-----
> > > > From: Morten Brørup <mb@smartsharesystems.com>
> > > > Sent: Tuesday, December 14, 2021 5:31 PM
> > > > To: Randles, Ronan <ronan.randles@intel.com>; dev@dpdk.org
> > > > Cc: Van Haaren, Harry <harry.van.haaren@intel.com>
> > > > Subject: RE: [PATCH 02/12] net: add function to pretty print IPv4
> > > >
> > > > > From: Ronan Randles [mailto:ronan.randles@intel.com]
> > > > > Sent: Tuesday, 14 December 2021 15.13
> > > > >
> > > > > This function accepts an uint32_t representation of an IP address
> > and
> > > > > produces a string representation stored in a char * buffer.
> > Realavent
> > > > > unit tests also included.
> > > > >
> > > > > 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 188054fda4..e46f0b41ba 100644
> > > > > --- a/lib/net/rte_ip.h
> > > > > +++ b/lib/net/rte_ip.h
> > > > > @@ -444,6 +444,26 @@ __rte_experimental
> > > > >  int32_t
> > > > >  rte_ip_parse_addr(const char *src_ip, uint32_t *output_addr);
> > > > >
> > > > > +
> > > > > +/**
> > > > > + * Print IP address from 32 bit int into char * buffer.
> > > > > + *
> > > > > + * @param ip_addr
> > > > > + *   ip address to be printed.
> > > > > + * @param buffer
> > > > > + *   The buffer the string will be saved into.
> > > > > + * @param buffer_size
> > > > > + *   size of buffer to be used.
> > > > > + *
> > > > > + * @retval 0
> > > > > + *   Success.
> > > > > + * @retval -1
> > > > > + *   Failure due to invalid input arguments.
> > > > > + */
> > > > > +__rte_experimental
> > > > > +int32_t
> > > > > +rte_ip_print_addr(uint32_t ip_addr, char *buffer, uint32_t
> > > > > buffer_size);
> > > > > +
> > > >
> > > > In continuation of my email reply about the IPv4 parse function...
> > > >
> > > > I have a few suggestions to the IPv4 print function too:
> > > >
> > > > The return value should be the number of characters written to the
> > output string, and still -1 on error. With this modification, you could
> > > > use the return type ssize_t instead of int32_t.
> > > >
> > > > Furthermore, I would prefer having the parameters in the same order
> > as snprintf(): char *str, size_t size, const uint32_t ip_addr. Please
> > > > also notice the suggested changed type for the size, and the const
> > added to the ip_addr.
> > > >
> > > Honestly, I don't understand why we need to introduce such functions
> > > inside DPDK at all.
> > > What's wrong with existing standard ones: inet_ntop() and
> > inet_pton()?
> >
> > Agreed, I see no added value in reinventing here
> 
> I think that DPDK functions for converting all sorts of types to/from strings would be useful; not only IP addresses, but also MAC addresses,
> TCP/UDP port numbers and VLAN IDs.

For MACs we already have:
rte_ether_format_addr()/rte_ether_unformat_addr()

> 
> If you don't like IP address string conversion functions in the net library, DPDK could have a string conversions library. That library could
> expose a multitude of APIs for the same purpose, so the application can use the API that best fits each application use.

I don’t mind to add new functions into net lib, if they are useful ones.
But for that particular case, I just don't see what is the reason to
develop and maintain our own functions while existing analogues:
- are well known, widely adopted and field proven
- do provide the same or even more comprehensive functionality
  

15/12/2021 14:06, Ananyev, Konstantin:
> 
> > > From: Stephen Hemminger [mailto:stephen@networkplumber.org]
> > > Sent: Wednesday, 15 December 2021 04.21
> > >
> > > On Wed, 15 Dec 2021 01:06:14 +0000
> > > "Ananyev, Konstantin" <konstantin.ananyev@intel.com> wrote:
> > > > > > --- a/lib/net/rte_ip.h
> > > > > > +++ b/lib/net/rte_ip.h
> > > > > > @@ -444,6 +444,26 @@ __rte_experimental
> > > > > >  int32_t
> > > > > >  rte_ip_parse_addr(const char *src_ip, uint32_t *output_addr);
> > > > > >
> > > > > > +
> > > > > > +/**
> > > > > > + * Print IP address from 32 bit int into char * buffer.
> > > > > > + *
> > > > > > + * @param ip_addr
> > > > > > + *   ip address to be printed.
> > > > > > + * @param buffer
> > > > > > + *   The buffer the string will be saved into.
> > > > > > + * @param buffer_size
> > > > > > + *   size of buffer to be used.
> > > > > > + *
> > > > > > + * @retval 0
> > > > > > + *   Success.
> > > > > > + * @retval -1
> > > > > > + *   Failure due to invalid input arguments.
> > > > > > + */
> > > > > > +__rte_experimental
> > > > > > +int32_t
> > > > > > +rte_ip_print_addr(uint32_t ip_addr, char *buffer, uint32_t
> > > > > > buffer_size);
> > > > > > +
> > > > >
> > > > > In continuation of my email reply about the IPv4 parse function...
> > > > >
> > > > > I have a few suggestions to the IPv4 print function too:
> > > > >
> > > > > The return value should be the number of characters written to the
> > > output string, and still -1 on error. With this modification, you could
> > > > > use the return type ssize_t instead of int32_t.
> > > > >
> > > > > Furthermore, I would prefer having the parameters in the same order
> > > as snprintf(): char *str, size_t size, const uint32_t ip_addr. Please
> > > > > also notice the suggested changed type for the size, and the const
> > > added to the ip_addr.
> > > > >
> > > > Honestly, I don't understand why we need to introduce such functions
> > > > inside DPDK at all.
> > > > What's wrong with existing standard ones: inet_ntop() and
> > > inet_pton()?
> > >
> > > Agreed, I see no added value in reinventing here
> > 
> > I think that DPDK functions for converting all sorts of types to/from strings would be useful; not only IP addresses, but also MAC addresses,
> > TCP/UDP port numbers and VLAN IDs.
> 
> For MACs we already have:
> rte_ether_format_addr()/rte_ether_unformat_addr()
> 
> > 
> > If you don't like IP address string conversion functions in the net library, DPDK could have a string conversions library. That library could
> > expose a multitude of APIs for the same purpose, so the application can use the API that best fits each application use.
> 
> I don’t mind to add new functions into net lib, if they are useful ones.
> But for that particular case, I just don't see what is the reason to
> develop and maintain our own functions while existing analogues:
> - are well known, widely adopted and field proven
> - do provide the same or even more comprehensive functionality

+1
Waiting for an answer from the authors. One month silence so far.
  

> -----Original Message-----
> From: Thomas Monjalon <thomas@monjalon.net>
> Sent: Wednesday, January 19, 2022 2:24 PM
> To: Morten Brørup <mb@smartsharesystems.com>; Stephen Hemminger
> <stephen@networkplumber.org>; Randles, Ronan <ronan.randles@intel.com>;
> Van Haaren, Harry <harry.van.haaren@intel.com>; Ananyev, Konstantin
> <konstantin.ananyev@intel.com>
> Cc: dev@dpdk.org
> Subject: Re: [PATCH 02/12] net: add function to pretty print IPv4
> 
> 15/12/2021 14:06, Ananyev, Konstantin:
> >
> > > > From: Stephen Hemminger [mailto:stephen@networkplumber.org]
> > > > Sent: Wednesday, 15 December 2021 04.21
> > > >
> > > > On Wed, 15 Dec 2021 01:06:14 +0000
> > > > "Ananyev, Konstantin" <konstantin.ananyev@intel.com> wrote:
> > > > > > > --- a/lib/net/rte_ip.h
> > > > > > > +++ b/lib/net/rte_ip.h
> > > > > > > @@ -444,6 +444,26 @@ __rte_experimental
> > > > > > >  int32_t
> > > > > > >  rte_ip_parse_addr(const char *src_ip, uint32_t *output_addr);
> > > > > > >
> > > > > > > +
> > > > > > > +/**
> > > > > > > + * Print IP address from 32 bit int into char * buffer.
> > > > > > > + *
> > > > > > > + * @param ip_addr
> > > > > > > + *   ip address to be printed.
> > > > > > > + * @param buffer
> > > > > > > + *   The buffer the string will be saved into.
> > > > > > > + * @param buffer_size
> > > > > > > + *   size of buffer to be used.
> > > > > > > + *
> > > > > > > + * @retval 0
> > > > > > > + *   Success.
> > > > > > > + * @retval -1
> > > > > > > + *   Failure due to invalid input arguments.
> > > > > > > + */
> > > > > > > +__rte_experimental
> > > > > > > +int32_t
> > > > > > > +rte_ip_print_addr(uint32_t ip_addr, char *buffer, uint32_t
> > > > > > > buffer_size);
> > > > > > > +
> > > > > >
> > > > > > In continuation of my email reply about the IPv4 parse function...
> > > > > >
> > > > > > I have a few suggestions to the IPv4 print function too:
> > > > > >
> > > > > > The return value should be the number of characters written to the
> > > > output string, and still -1 on error. With this modification, you could
> > > > > > use the return type ssize_t instead of int32_t.
> > > > > >
> > > > > > Furthermore, I would prefer having the parameters in the same order
> > > > as snprintf(): char *str, size_t size, const uint32_t ip_addr. Please
> > > > > > also notice the suggested changed type for the size, and the const
> > > > added to the ip_addr.
> > > > > >
> > > > > Honestly, I don't understand why we need to introduce such functions
> > > > > inside DPDK at all.
> > > > > What's wrong with existing standard ones: inet_ntop() and
> > > > inet_pton()?
> > > >
> > > > Agreed, I see no added value in reinventing here
> > >
> > > I think that DPDK functions for converting all sorts of types to/from strings
> would be useful; not only IP addresses, but also MAC addresses,
> > > TCP/UDP port numbers and VLAN IDs.
> >
> > For MACs we already have:
> > rte_ether_format_addr()/rte_ether_unformat_addr()
> >
> > >
> > > If you don't like IP address string conversion functions in the net library, DPDK
> could have a string conversions library. That library could
> > > expose a multitude of APIs for the same purpose, so the application can use
> the API that best fits each application use.
> >
> > I don’t mind to add new functions into net lib, if they are useful ones.
> > But for that particular case, I just don't see what is the reason to
> > develop and maintain our own functions while existing analogues:
> > - are well known, widely adopted and field proven
> > - do provide the same or even more comprehensive functionality
> 
> +1
> Waiting for an answer from the authors. One month silence so far.

Hi All,

Ronan and I are working on a V2 patchset, and hope to share it in the next days.

Personally I think there is value in DPDK having various functionality "built in", and
if that functionality is available for e.g. Ether, I see no reason why IPv4 or other
protocols shouldn't have that functionality available. If I was a newcomer to DPDK,
I would find it difficult to understand why I can format/unformat Ether with DPDK,
but had to use Linux/Windows functions to format/unformat IP/UDP.

I liked Morten's feedback around real-world usage, however we have not got around
to implementing that functionality in the V2 patchset. We intended to propose that
change in signature/extra-functionality being added to V3.

I believe the gen library was discussed briefly at last tech-board call (unfortunately I was not
available to participate). Perhaps a quick chat/discussion at the next tech-board would be helpful
to identify community stance on the library? At that point the V2 should be available too.

Regards, -Harry