Re: [PATCH 02/22] util: Helper for formatting MAC addresses

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

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

On Sun, Aug 18, 2024 at 05:44:55PM +0200, Stefano Brivio wrote:
> On Fri, 16 Aug 2024 15:39:43 +1000
> David Gibson <david@gibson.dropbear.id.au> wrote:
> 
> > There are a couple of places where we somewhat messily open code formatting
> > an Ethernet like MAC address for display.  Add an eth_ntop() helper for
> > this.
> > 
> > Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> > ---
> >  conf.c |  7 +++----
> >  dhcp.c |  5 ++---
> >  util.c | 19 +++++++++++++++++++
> >  util.h |  3 +++
> >  4 files changed, 27 insertions(+), 7 deletions(-)
> > 
> > diff --git a/conf.c b/conf.c
> > index ed097bdc..830f91a6 100644
> > --- a/conf.c
> > +++ b/conf.c
> > @@ -921,7 +921,8 @@ pasta_opts:
> >   */
> >  static void conf_print(const struct ctx *c)
> >  {
> > -	char buf4[INET_ADDRSTRLEN], buf6[INET6_ADDRSTRLEN], ifn[IFNAMSIZ];
> > +	char buf4[INET_ADDRSTRLEN], buf6[INET6_ADDRSTRLEN];
> > +	char bufmac[ETH_ADDRSTRLEN], ifn[IFNAMSIZ];
> >  	int i;
> >  
> >  	info("Template interface: %s%s%s%s%s",
> > @@ -955,9 +956,7 @@ static void conf_print(const struct ctx *c)
> >  		info("Namespace interface: %s", c->pasta_ifn);
> >  
> >  	info("MAC:");
> > -	info("    host: %02x:%02x:%02x:%02x:%02x:%02x",
> > -	     c->mac[0], c->mac[1], c->mac[2],
> > -	     c->mac[3], c->mac[4], c->mac[5]);
> > +	info("    host: %s", eth_ntop(c->mac, bufmac, sizeof(bufmac)));
> >  
> >  	if (c->ifi4) {
> >  		if (!c->no_dhcp) {
> > diff --git a/dhcp.c b/dhcp.c
> > index aa9f59da..acc5b03e 100644
> > --- a/dhcp.c
> > +++ b/dhcp.c
> > @@ -276,6 +276,7 @@ static void opt_set_dns_search(const struct ctx *c, size_t max_len)
> >  int dhcp(const struct ctx *c, const struct pool *p)
> >  {
> >  	size_t mlen, dlen, offset = 0, opt_len, opt_off = 0;
> > +	char macstr[ETH_ADDRSTRLEN];
> >  	const struct ethhdr *eh;
> >  	const struct iphdr *iph;
> >  	const struct udphdr *uh;
> > @@ -340,9 +341,7 @@ int dhcp(const struct ctx *c, const struct pool *p)
> >  		return -1;
> >  	}
> >  
> > -	info("    from %02x:%02x:%02x:%02x:%02x:%02x",
> > -	     m->chaddr[0], m->chaddr[1], m->chaddr[2],
> > -	     m->chaddr[3], m->chaddr[4], m->chaddr[5]);
> > +	info("    from %s", eth_ntop(m->chaddr, macstr, sizeof(macstr)));
> >  
> >  	m->yiaddr = c->ip4.addr;
> >  	mask.s_addr = htonl(0xffffffff << (32 - c->ip4.prefix_len));
> > diff --git a/util.c b/util.c
> > index 0b414045..892358b1 100644
> > --- a/util.c
> > +++ b/util.c
> > @@ -676,6 +676,25 @@ const char *sockaddr_ntop(const void *sa, char *dst, socklen_t size)
> >  	return dst;
> >  }
> >  
> > +/** eth_ntop() - Convert an Ethernet MAC address to text format
> > + * @mac:	MAC address
> > + * @dst:	output buffer, minimum ETH_ADDRSTRLEN bytes
> > + * @size:	size of buffer at @dst
> 
> Nit: s/output/Output, s/size/Size

Fixed.

> > + *
> > + * Return: On success, a non-null pointer to @dst, NULL on failure
> > + */
> > +const char *eth_ntop(const unsigned char *mac, char *dst, size_t size)
> > +{
> > +	int len;
> > +
> > +	len = snprintf(dst, size, "%02x:%02x:%02x:%02x:%02x:%02x",
> > +		       mac[0], mac[1], mac[2], mac[3], mac[4], mac[5]);
> > +	if (len < 0 || (size_t)len >= size)
> > +		return NULL;
> > +
> > +	return dst;
> > +}
> > +
> >  /** str_ee_origin() - Convert socket extended error origin to a string
> >   * @ee:		Socket extended error structure
> >   *
> > diff --git a/util.h b/util.h
> > index cb4d181c..c1748074 100644
> > --- a/util.h
> > +++ b/util.h
> > @@ -215,9 +215,12 @@ static inline const char *af_name(sa_family_t af)
> >  
> >  #define SOCKADDR_STRLEN		MAX(SOCKADDR_INET_STRLEN, SOCKADDR_INET6_STRLEN)
> >  
> > +#define ETH_ADDRSTRLEN	(ETH_ALEN * 3)
> 
> The fact that this includes two digits plus separator for all non-last
> octets of a MAC address, and two digits plus NULL terminator for the
> last octet, looks a bit subtle to me.
> 
> Defining this as sizeof("00:11:22:33:44:55") wouldn't scream
> "off-by-one" as much, to me. Not a strong preference.

Yeah, that makes sense.  Done.

-- 
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 --]