Re: [RESEND #2] Re: [PATCH 08/12] parse: Add helpers for parsing IP addresses

[David Gibson <david@gibson.dropbear.id.au>]

[-- Attachment #1: Type: text/plain, Size: 11280 bytes --]

On Wed, Jul 01, 2026 at 02:11:52AM +0200, Stefano Brivio wrote:
> *Actually* resending to jmaloy@redhat.com, the original email had:
> 
> On Wed, 1 Jul 2026 02:07:25 +0200
> Stefano Brivio <sbrivio@redhat.com> wrote:
> 
> > On Fri, 26 Jun 2026 17:09:59 +1000
> > David Gibson <david@gibson.dropbear.id.au> wrote:
> > 
> > > parse_ipv[46]() are wrappers around inet_pton() that are more
> > > convenient for use when the IP is part of a longer string, rather than
> > > the entire string.  parse_inany() replaces inany_pton() which again
> > > will become more convenient for strings including IPs that aren't just
> > > an IP.
> > > 
> > > For now we only have some simple and sometimes awkward use cases,
> > > we'll replace these with more natural uses in future.
> > > 
> > > Cc: Jon Maloy <jamloy@redhat.com>
> 
> ...jamloy@redhat.com instead.

Yeah, caught that just to late.  Fixed.

> 
> > > 
> > > Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> > > ---
> > >  Makefile   |  1 +
> > >  conf.c     |  6 ++--
> > >  fwd_rule.c | 21 ++++---------
> > >  inany.c    | 24 ++------------
> > >  inany.h    |  1 -
> > >  parse.c    | 91 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
> > >  parse.h    |  4 +++
> > >  7 files changed, 109 insertions(+), 39 deletions(-)
> > > 
> > > diff --git a/Makefile b/Makefile
> > > index 85d279c0..e2b22ddf 100644
> > > --- a/Makefile
> > > +++ b/Makefile
> > > @@ -227,4 +227,5 @@ pesto.cppcheck: CPPCHECK_FLAGS += --suppress=unusedFunction:inany.c
> > >  pesto.cppcheck: CPPCHECK_FLAGS += --suppress=unusedFunction:ip.h
> > >  pesto.cppcheck: CPPCHECK_FLAGS += --suppress=unusedFunction:serialise.c
> > >  pesto.cppcheck: CPPCHECK_FLAGS += --suppress=staticFunction:fwd_rule.c
> > > +pesto.cppcheck: CPPCHECK_FLAGS += --suppress=staticFunction:parse.c
> > >  pesto.cppcheck: $(PESTO_SRCS) $(PESTO_HEADERS) seccomp_pesto.h
> > > diff --git a/conf.c b/conf.c
> > > index bd357117..2d1895d4 100644
> > > --- a/conf.c
> > > +++ b/conf.c
> > > @@ -357,7 +357,7 @@ static uint8_t conf_ip4_prefix(const char *arg)
> > >  	struct in_addr mask;
> > >  	unsigned long len;
> > >  
> > > -	if (inet_pton(AF_INET, arg, &mask)) {
> > > +	if (parse_ipv4(&p, &mask) && parse_eoi(p)) {
> > >  		in_addr_t hmask = ntohl(mask.s_addr);
> > >  		len = __builtin_popcount(hmask);
> > >  		if ((hmask << len) == 0)
> > > @@ -1577,7 +1577,9 @@ void conf(struct ctx *c, int argc, char **argv)
> > >  			if (addr_has_prefix_len && prefix_len_from_opt)
> > >  				die("Redundant prefix length specification");
> > >  
> > > -			if (!addr_has_prefix_len && !inany_pton(optarg, &addr))
> > > +			p = optarg;
> > > +			if (!addr_has_prefix_len &&
> > > +			    !(parse_inany(&p, &addr) && parse_eoi(p)))  
> > 
> > parse_port_range() reports error if the input isn't NULL-terminated,
> > but these new functions don't, and in all the usages at least
> > introduced in this patch, you always need to couple that with a &&
> > parse_eoi(p) (or && !*p).
> > 
> > Is there a reason for this difference?
> > 
> > >  				die("Invalid address: %s", optarg);
> > >  
> > >  			if (prefix_len_from_opt && inany_v4(&addr))
> > > diff --git a/fwd_rule.c b/fwd_rule.c
> > > index 0b85b17e..6d7ec2c5 100644
> > > --- a/fwd_rule.c
> > > +++ b/fwd_rule.c
> > > @@ -595,6 +595,8 @@ void fwd_rule_parse(char optname, bool del, const char *optarg,
> > >  	strncpy(buf, optarg, sizeof(buf) - 1);
> > >  
> > >  	if ((spec = strchr(buf, '/'))) {
> > > +		const char *p = buf;
> > > +
> > >  		*spec = 0;
> > >  		spec++;
> > >  
> > > @@ -616,22 +618,11 @@ void fwd_rule_parse(char optname, bool del, const char *optarg,
> > >  			}
> > >  		}
> > >  
> > > -		if (ifname == buf + 1) {	/* Interface without address */
> > > +		if (ifname == buf + 1 || /* Interface without address */
> > > +		    !strcmp(buf, "*")) /* Explicit wildcard address */
> > >  			addr = NULL;
> > > -		} else {
> > > -			char *p = buf;
> > > -
> > > -			/* Allow square brackets for IPv4 too for convenience */
> > > -			if (*p == '[' && p[strlen(p) - 1] == ']') {
> > > -				p[strlen(p) - 1] = '\0';
> > > -				p++;
> > > -			}
> > > -
> > > -			if (strcmp(p, "*") == 0)
> > > -				addr = NULL;
> > > -			else if (!inany_pton(p, &addr_buf))
> > > -				die("Bad forwarding address '%s'", p);
> > > -		}
> > > +		else if (!parse_inany(&p, &addr_buf) && parse_eoi(p))
> > > +			die("Bad forwarding address '%s'", buf);
> > >  	} else {
> > >  		spec = buf;
> > >  
> > > diff --git a/inany.c b/inany.c
> > > index 23faf3ff..154f08b5 100644
> > > --- a/inany.c
> > > +++ b/inany.c
> > > @@ -27,6 +27,7 @@
> > >  #include "ip.h"
> > >  #include "inany.h"
> > >  #include "fwd.h"
> > > +#include "parse.h"
> > >  
> > >  const union inany_addr inany_loopback4 = INANY_INIT4(IN4ADDR_LOOPBACK_INIT);
> > >  const union inany_addr inany_any4 = INANY_INIT4(IN4ADDR_ANY_INIT);
> > > @@ -70,26 +71,6 @@ const char *inany_ntop(const union inany_addr *src, char *dst, socklen_t size)
> > >  	return inet_ntop(AF_INET6, &src->a6, dst, size);
> > >  }
> > >  
> > > -/** inany_pton - Parse an IPv[46] address from text format
> > > - * @src:	IPv[46] address
> > > - * @dst:	output buffer, filled with parsed address
> > > - *
> > > - * Return: on success, 1, if no parseable address is found, 0
> > > - */
> > > -int inany_pton(const char *src, union inany_addr *dst)
> > > -{
> > > -	if (inet_pton(AF_INET, src, &dst->v4mapped.a4)) {
> > > -		memset(&dst->v4mapped.zero, 0, sizeof(dst->v4mapped.zero));
> > > -		memset(&dst->v4mapped.one, 0xff, sizeof(dst->v4mapped.one));
> > > -		return 1;
> > > -	}
> > > -
> > > -	if (inet_pton(AF_INET6, src, &dst->a6))
> > > -		return 1;
> > > -
> > > -	return 0;
> > > -}
> > > -
> > >  /**
> > >   * inany_prefix_pton() - Parse an IPv[46] address with prefix length
> > >   * @src:	IPv[46] address and prefix length string in CIDR format
> > > @@ -104,6 +85,7 @@ int inany_prefix_pton(const char *src, union inany_addr *dst,
> > >  	char astr[INANY_ADDRSTRLEN] = { 0 };
> > >  	size_t alen = strcspn(src, "/");
> > >  	const char *pstr = &src[alen + 1];
> > > +	const char *p = astr;
> > >  	unsigned long plen;
> > >  	char *end;
> > >  
> > > @@ -129,7 +111,7 @@ int inany_prefix_pton(const char *src, union inany_addr *dst,
> > >  		return 1;
> > >  	}
> > >  
> > > -	if (inany_pton(astr, dst)) {
> > > +	if (parse_inany(&p, dst) && parse_eoi(p)) {
> > >  		if (plen > 32)
> > >  			return 0;
> > >  		*prefix_len = plen + 96;
> > > diff --git a/inany.h b/inany.h
> > > index 6bf3ecaa..93d98368 100644
> > > --- a/inany.h
> > > +++ b/inany.h
> > > @@ -303,7 +303,6 @@ static inline int inany_from_sockaddr(union inany_addr *dst, in_port_t *port,
> > >  
> > >  bool inany_matches(const union inany_addr *a, const union inany_addr *b);
> > >  const char *inany_ntop(const union inany_addr *src, char *dst, socklen_t size);
> > > -int inany_pton(const char *src, union inany_addr *dst);
> > >  int inany_prefix_pton(const char *src, union inany_addr *dst,
> > >  		      uint8_t *prefix_len);
> > >  
> > > diff --git a/parse.c b/parse.c
> > > index bd091fde..0349c5dc 100644
> > > --- a/parse.c
> > > +++ b/parse.c
> > > @@ -16,9 +16,12 @@
> > >  #include <stddef.h>
> > >  #include <stdlib.h>
> > >  #include <string.h>
> > > +#include <limits.h>
> > > +#include <arpa/inet.h>
> > >  
> > >  #include "common.h"
> > >  #include "parse.h"
> > > +#include "inany.h"
> > >  
> > >  /**
> > >   * DOC: Theory of Operation
> > > @@ -110,3 +113,91 @@ bool parse_port_range(const char **cursor, struct port_range *range)
> > >  	*cursor = p;
> > >  	return true;
> > >  }
> > > +
> > > +/**
> > > + * parse_ipv4() - Parse an IPv4 address from a string
> > > + * @abuf:	On success, updated with parsed address
> > > + */
> > > +bool parse_ipv4(const char **cursor, struct in_addr *abuf)
> > > +{
> > > +	/* Brackets are not typical on IPv4, but allow for consistency */
> > > +	const char *p = *cursor;
> > > +	bool bracket = parse_literal(&p, "[");
> > > +	char buf[INET_ADDRSTRLEN];
> > > +	struct in_addr addr;
> > > +	size_t len;
> > > +
> > > +	if (bracket)
> > > +		len = strcspn(p, "]");
> > > +	else
> > > +		len = strspn(p, "0123456789.");
> > > +
> > > +	if (len >= sizeof(buf))
> > > +		return false;
> > > +	memcpy(buf, p, len);
> > > +	buf[len] = '\0';
> > > +	p += len;
> > > +
> > > +	if (!inet_pton(AF_INET, buf, &addr))
> > > +		return false;
> > > +
> > > +	if (bracket && !parse_literal(&p, "]"))
> > > +		return false;
> > > +
> > > +	*cursor = p;
> > > +	*abuf = addr;
> > > +	return true;
> > > +}
> > > +
> > > +/**
> > > + * parse_ipv6() - Parse an IPv6 address from a string
> > > + * @abuf:	On success, updated with parsed address
> > > + */
> > > +static bool parse_ipv6(const char **cursor, struct in6_addr *abuf)
> > > +{
> > > +	const char *p = *cursor;
> > > +	bool bracket = parse_literal(&p, "[");
> > > +	char buf[INET6_ADDRSTRLEN];
> > > +	struct in6_addr addr;
> > > +	size_t len;
> > > +
> > > +	if (bracket)
> > > +		len = strcspn(p, "]");
> > > +	else
> > > +		len = strspn(p, "0123456789aAbBcCdDeEfF:.");
> > > +
> > > +	if (len >= sizeof(buf))
> > > +		return false;
> > > +	memcpy(buf, p, len);
> > > +	buf[len] = '\0';
> > > +	p += len;
> > > +
> > > +	if (!inet_pton(AF_INET6, buf, &addr))
> > > +		return false;
> > > +
> > > +	if (bracket && !parse_literal(&p, "]"))
> > > +		return false;
> > > +
> > > +	*cursor = p;
> > > +	*abuf = addr;
> > > +	return true;
> > > +}
> > > +
> > > +/**
> > > + * parse_inany() - Parse an IPv4 or IPv6 address from a string
> > > + * @addr:	On success, updated with parsed address
> > > + */
> > > +bool parse_inany(const char **cursor, union inany_addr *addr)
> > > +{
> > > +	struct in_addr a4;
> > > +
> > > +	if (parse_ipv6(cursor, &addr->a6))
> > > +		return true;
> > > +
> > > +	if (parse_ipv4(cursor, &a4)) {
> > > +		*addr = inany_from_v4(a4);
> > > +		return true;
> > > +	}
> > > +
> > > +	return false;
> > > +}
> > > diff --git a/parse.h b/parse.h
> > > index 155b3995..2820a065 100644
> > > --- a/parse.h
> > > +++ b/parse.h
> > > @@ -9,6 +9,8 @@
> > >  #include <stdbool.h>
> > >  #include <netinet/in.h>
> > >  
> > > +union inany_addr;
> > > +
> > >  /**
> > >   * port_range() - Represents a non-empty range of ports
> > >   * @first:	First port number in the range
> > > @@ -24,5 +26,7 @@ bool parse_literal(const char **cursor, const char *lit);
> > >  bool parse_eoi(const char *cursor);
> > >  bool parse_unsigned(const char **cursor, int base, unsigned long *valp);
> > >  bool parse_port_range(const char **cursor, struct port_range *range);
> > > +bool parse_ipv4(const char **cursor, struct in_addr *abuf);
> > > +bool parse_inany(const char **cursor, union inany_addr *addr);
> > >  
> > >  #endif /* _PARSE_H */  
> 
> -- 
> Stefano
> 

-- 
David Gibson (he or they)	| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you, not the other way
				| around.
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]