Re: [patch] add ether_(aton, ntoa)

[Rich Felker <dalias@aerifal.cx>]

On Sun, Apr 14, 2013 at 04:27:08PM -0500, Strake wrote:
> >From 0166471873b3aa801ac4e6301350b08d65e8f24f Mon Sep 17 00:00:00 2001
> From: Strake <strake888@gmail.com>
> Date: Sun, 14 Apr 2013 12:09:30 -0500
> Subject: [PATCH] add ether_(aton, ntoa)

Thanks. I should have mentioned, there's actually an old patch for
this on the list, but if I remember right somebody flamed the
contributor and drove him off... :(

We should look and compare the two approaches to see which is better,
or possibly merge ideas from both.

> +static struct ether_addr a;
> +
> +struct ether_addr *ether_aton_r (const char *x, struct ether_addr *p_a) {
> +	for (int ii = 0; ii < 6; ii++) {
> +		unsigned long int n;
> +		if (ii != 0) {
> +			if (x[0] != ':') return 0; /* bad format */
> +			else x++;
> +		}
> +		n = strtoul (x, &x, 16);
> +		if (n > 0xFF) return 0; /* bad byte */
> +		(*p_a).ether_addr_octet[ii] = n;
> +	}
> +	if (x[0] != 0) return 0; /* bad format */
> +	return p_a;
> +}
> +
> +struct ether_addr *ether_aton (const char *x) {
> +	return ether_aton_r (x, &a);
> +}

I would put the static object inside the function that uses it rather
than outside, but this is purely a stylistic consideration. Also, it
_might_ be better to separate the functions with static storage out so
that programs using the _r interfaces don't pull in extra .bss, but
it's a tradeoff -- if we do this for every ugly nonstandard function,
we just end up making the .a file a lot larger and making build time a
lot slower. So I think I'm okay with the way you did it; we might even
want to go the other direction and put all 4 functions in one file.

> +char *ether_ntoa_r (const struct ether_addr *p_a, char *x) {
> +	char *y;
> +	y = x;
> +	for (int ii = 0; ii < 6; ii++) {
> +		x += sprintf (x, ii == 0 ? "%0hhX" : ":%0hhX", (*p_a).ether_addr_octet[ii]);

The hh is unnecessary here. The argument has type int (due to default
promotions) so just "%X" is fine. Also, as far as I can tell, the 0
was not doing anything since no width was specified. If your intent is
to always have 2 hex digits, "%.2X" is the right format to use.

Could you compare and see if it generates smaller code if we use a
single snprintf/sscanf to implement these functions rather than a
loop? I'm not sure which is better, but since they're not widely used,
my main interest is keeping them small. Also, how are these functions
documented to handle malformed input, and are you matching the
documentation on that?

Rich