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

[Bruce Richardson <bruce.richardson@intel.com>]

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.