[patch] add ether_(aton, ntoa)

[patch] add ether_(aton, ntoa)

[Strake]

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)

---
 include/netinet/ether.h  | 14 ++++++++++++++
 src/network/ether_aton.c | 23 +++++++++++++++++++++++
 src/network/ether_ntoa.c | 17 +++++++++++++++++
 3 files changed, 54 insertions(+)
 create mode 100644 include/netinet/ether.h
 create mode 100644 src/network/ether_aton.c
 create mode 100644 src/network/ether_ntoa.c

diff --git a/include/netinet/ether.h b/include/netinet/ether.h
new file mode 100644
index 0000000..c5179d5
--- /dev/null
+++ b/include/netinet/ether.h
@@ -0,0 +1,14 @@
+#ifndef _NETINET_ETHER_H
+#define _NETINET_ETHER_H
+
+#include <netinet/if_ether.h>
+
+char *ether_ntoa (const struct ether_addr *);
+
+struct ether_addr *ether_aton (const char *);
+
+char *ether_ntoa_r (const struct ether_addr *, char *);
+
+struct ether_addr *ether_aton_r (const char *, struct ether_addr *);
+
+#endif
diff --git a/src/network/ether_aton.c b/src/network/ether_aton.c
new file mode 100644
index 0000000..4270872
--- /dev/null
+++ b/src/network/ether_aton.c
@@ -0,0 +1,23 @@
+#include <stdlib.h>
+#include <netinet/ether.h>
+
+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);
+}
diff --git a/src/network/ether_ntoa.c b/src/network/ether_ntoa.c
new file mode 100644
index 0000000..5aaf0da
--- /dev/null
+++ b/src/network/ether_ntoa.c
@@ -0,0 +1,17 @@
+#include <stdio.h>
+#include <netinet/ether.h>
+
+static char x[18];
+
+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]);
+	}
+	return y;
+}
+
+char *ether_ntoa (const struct ether_addr *p_a) {
+	return ether_ntoa_r (p_a, x);
+}

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

[Rich Felker]

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

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

[Strake]

On 14/04/2013, Rich Felker <dalias@aerifal.cx> wrote:
> I would put the static object inside the function that uses it rather
> than outside, but this is purely a stylistic consideration.

Yeah, I just like to define closure-bound free variables without the
closure scope itself, and automatics within.

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

I would've done, but I wished to match how inet_* are organized.

>> +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.

Thanks. New patch follows message.

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

ntoa: same size
aton: mine is smaller

> Also, how are these functions
> documented to handle malformed input, and are you matching the
> documentation on that?

Linux docs say that ether_aton return 0 on malformed input, as mine does.
ether_ntoa is infallible.

Cheers,
Strake

From ad5ab07c6ec15b8b98717750888947634c2fa039 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)

---
 include/netinet/ether.h  | 14 ++++++++++++++
 src/network/ether_aton.c | 23 +++++++++++++++++++++++
 src/network/ether_ntoa.c | 17 +++++++++++++++++
 3 files changed, 54 insertions(+)
 create mode 100644 include/netinet/ether.h
 create mode 100644 src/network/ether_aton.c
 create mode 100644 src/network/ether_ntoa.c

diff --git a/include/netinet/ether.h b/include/netinet/ether.h
new file mode 100644
index 0000000..c5179d5
--- /dev/null
+++ b/include/netinet/ether.h
@@ -0,0 +1,14 @@
+#ifndef _NETINET_ETHER_H
+#define _NETINET_ETHER_H
+
+#include <netinet/if_ether.h>
+
+char *ether_ntoa (const struct ether_addr *);
+
+struct ether_addr *ether_aton (const char *);
+
+char *ether_ntoa_r (const struct ether_addr *, char *);
+
+struct ether_addr *ether_aton_r (const char *, struct ether_addr *);
+
+#endif
diff --git a/src/network/ether_aton.c b/src/network/ether_aton.c
new file mode 100644
index 0000000..4270872
--- /dev/null
+++ b/src/network/ether_aton.c
@@ -0,0 +1,23 @@
+#include <stdlib.h>
+#include <netinet/ether.h>
+
+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);
+}
diff --git a/src/network/ether_ntoa.c b/src/network/ether_ntoa.c
new file mode 100644
index 0000000..468ac48
--- /dev/null
+++ b/src/network/ether_ntoa.c
@@ -0,0 +1,17 @@
+#include <stdio.h>
+#include <netinet/ether.h>
+
+static char x[18];
+
+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 ? "%.2X" : ":%.2X", (*p_a).ether_addr_octet[ii]);
+	}
+	return y;
+}
+
+char *ether_ntoa (const struct ether_addr *p_a) {
+	return ether_ntoa_r (p_a, x);
+}

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

[Rich Felker]

On Sun, Apr 14, 2013 at 11:10:55PM -0500, Strake wrote:
> +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);

&x has the wrong type; strtoul requires char **, not const char **.
A separate temp var is required for the output, as in.

	char *y;
	n = strtoul (x, &y, 16);
	x = y;

or similar. As far as I know the naive cast one could make to "fix"
the type mismatch is not valid; it would be an aliasing violation.

> +		if (n > 0xFF) return 0; /* bad byte */
> +		(*p_a).ether_addr_octet[ii] = n;

Is there a reason you're using (*p_a). rather than p_a-> ?

Also, I'm not sure if this is worth changing or not, but usually we
avoid writing to output pointers except on success. For standard
interfaces this is a conformance issue, but for these it probably
doesn't matter.

> +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 ? "%.2X" : ":%.2X", (*p_a).ether_addr_octet[ii]);

And again, why not p_a-> ?

Rich

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

[Strake]

On 19/04/2013, Rich Felker <dalias@aerifal.cx> wrote:
> On Sun, Apr 14, 2013 at 11:10:55PM -0500, Strake wrote:
>> +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);
>
> &x has the wrong type; strtoul requires char **, not const char **.
> A separate temp var is required for the output, as in.
>
> 	char *y;
> 	n = strtoul (x, &y, 16);
> 	x = y;
>
> or similar. As far as I know the naive cast one could make to "fix"
> the type mismatch is not valid; it would be an aliasing violation.

What bad could happen?

>> +		if (n > 0xFF) return 0; /* bad byte */
>> +		(*p_a).ether_addr_octet[ii] = n;
>
> Is there a reason you're using (*p_a). rather than p_a-> ?

No sane one.

> Also, I'm not sure if this is worth changing or not, but usually we
> avoid writing to output pointers except on success. For standard
> interfaces this is a conformance issue, but for these it probably
> doesn't matter.

Well, we may as well do it properly.

From 97d977299521a0ae7cc23deed16dabfd22363248 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)

---
 include/netinet/ether.h  | 14 ++++++++++++++
 src/network/ether_aton.c | 26 ++++++++++++++++++++++++++
 src/network/ether_ntoa.c | 17 +++++++++++++++++
 3 files changed, 57 insertions(+)
 create mode 100644 include/netinet/ether.h
 create mode 100644 src/network/ether_aton.c
 create mode 100644 src/network/ether_ntoa.c

diff --git a/include/netinet/ether.h b/include/netinet/ether.h
new file mode 100644
index 0000000..c5179d5
--- /dev/null
+++ b/include/netinet/ether.h
@@ -0,0 +1,14 @@
+#ifndef _NETINET_ETHER_H
+#define _NETINET_ETHER_H
+
+#include <netinet/if_ether.h>
+
+char *ether_ntoa (const struct ether_addr *);
+
+struct ether_addr *ether_aton (const char *);
+
+char *ether_ntoa_r (const struct ether_addr *, char *);
+
+struct ether_addr *ether_aton_r (const char *, struct ether_addr *);
+
+#endif
diff --git a/src/network/ether_aton.c b/src/network/ether_aton.c
new file mode 100644
index 0000000..2690cc5
--- /dev/null
+++ b/src/network/ether_aton.c
@@ -0,0 +1,26 @@
+#include <stdlib.h>
+#include <string.h>
+#include <netinet/ether.h>
+
+static struct ether_addr a;
+
+struct ether_addr *ether_aton_r (const char *x, struct ether_addr *p_a) {
+	struct ether_addr 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 */
+		a.ether_addr_octet[ii] = n;
+	}
+	if (x[0] != 0) return 0; /* bad format */
+	memmove (p_a, &a, sizeof (struct ether_addr));
+	return p_a;
+}
+
+struct ether_addr *ether_aton (const char *x) {
+	return ether_aton_r (x, &a);
+}
diff --git a/src/network/ether_ntoa.c b/src/network/ether_ntoa.c
new file mode 100644
index 0000000..bcc773a
--- /dev/null
+++ b/src/network/ether_ntoa.c
@@ -0,0 +1,17 @@
+#include <stdio.h>
+#include <netinet/ether.h>
+
+static char x[18];
+
+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 ? "%.2X" : ":%.2X", p_a -> ether_addr_octet[ii]);
+	}
+	return y;
+}
+
+char *ether_ntoa (const struct ether_addr *p_a) {
+	return ether_ntoa_r (p_a, x);
+}
-- 
1.7.11.1

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

[Szabolcs Nagy]

* Strake <strake888@gmail.com> [2013-05-05 09:02:04 -0500]:
> On 19/04/2013, Rich Felker <dalias@aerifal.cx> wrote:
> > &x has the wrong type; strtoul requires char **, not const char **.
> > A separate temp var is required for the output, as in.
> >
> > 	char *y;
> > 	n = strtoul (x, &y, 16);
> > 	x = y;
> >
> > or similar. As far as I know the naive cast one could make to "fix"
> > the type mismatch is not valid; it would be an aliasing violation.
> 
> What bad could happen?


the naive cast is strtoul(x, (char**)&x, 16)

(const char **) and (char **) are not compatible types
and they are not required to have the same representation
and alignment

so the conversion itself may invoke undefined behaviour

but even if the representation and alignment is the same
an object with effective type (const char*) cannot be
accessed through a (char*) lvalue expression within strtoul

so the aliasing rules are violated as well
(a compiler may reorder the loads from x and the stores
to *(char**)&x arbitarily)

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

[Rich Felker]

On Sun, May 05, 2013 at 06:24:19PM +0200, Szabolcs Nagy wrote:
> * Strake <strake888@gmail.com> [2013-05-05 09:02:04 -0500]:
> > On 19/04/2013, Rich Felker <dalias@aerifal.cx> wrote:
> > > &x has the wrong type; strtoul requires char **, not const char **.
> > > A separate temp var is required for the output, as in.
> > >
> > > 	char *y;
> > > 	n = strtoul (x, &y, 16);
> > > 	x = y;
> > >
> > > or similar. As far as I know the naive cast one could make to "fix"
> > > the type mismatch is not valid; it would be an aliasing violation.
> > 
> > What bad could happen?
> 
> 
> the naive cast is strtoul(x, (char**)&x, 16)
> 
> (const char **) and (char **) are not compatible types
> and they are not required to have the same representation
> and alignment
> 
> so the conversion itself may invoke undefined behaviour

I'm not so worried about this. Assuming that all pointers have the
same size and representation is a reasonable modern assumption, and
whether they do is implementation-defined, not undefined. Still it may
be nice for higher-level code which could be reused on obscure targets
outside of musl to avoid such assumptions when possible.

Perhaps it would be nice to come up with a systematic way of
documenting which code in musl depends on implementation-defined
characteristics, since there was recently demand for this in another
thread..

> but even if the representation and alignment is the same
> an object with effective type (const char*) cannot be
> accessed through a (char*) lvalue expression within strtoul
> 
> so the aliasing rules are violated as well
> (a compiler may reorder the loads from x and the stores
> to *(char**)&x arbitarily)

Indeed this is the real issue, but it's fairly hard to hit. If the
compiler can only see the code in the caller and not inside strtoul,
it can't do any reordering, because it must assume the callee could
convert the pointer back to the right type and perform legal writes
through it. The aliasing violation only has pratical consequences if
you assume optimization can take place across translation units, e.g.
LTO. In any case, it's policy not to do this kind of aliasing
violation, especially when the identical result can easily be achieved
using a temp variable of the right type.

Rich

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

[Strake]

On 05/05/2013, Rich Felker <dalias@aerifal.cx> wrote:
> On Sun, May 05, 2013 at 06:24:19PM +0200, Szabolcs Nagy wrote:
>> but even if the representation and alignment is the same
>> an object with effective type (const char*) cannot be
>> accessed through a (char*) lvalue expression within strtoul
>>
>> so the aliasing rules are violated as well
>> (a compiler may reorder the loads from x and the stores
>> to *(char**)&x arbitarily)
>
> Indeed this is the real issue, but it's fairly hard to hit. If the
> compiler can only see the code in the caller and not inside strtoul,
> it can't do any reordering, because it must assume the callee could
> convert the pointer back to the right type and perform legal writes
> through it. The aliasing violation only has pratical consequences if
> you assume optimization can take place across translation units, e.g.
> LTO. In any case, it's policy not to do this kind of aliasing
> violation, especially when the identical result can easily be achieved
> using a temp variable of the right type.

All very esoteric. This is properly the compiler's worry, not mine.
Alas, lossage finds safety in standardization. But what would I know?
I'm not a committee.

From 3f301831e3e42ce23d5d3c0f3721232d66b93114 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)

---
 include/netinet/ether.h  | 14 ++++++++++++++
 src/network/ether_aton.c | 30 ++++++++++++++++++++++++++++++
 src/network/ether_ntoa.c | 17 +++++++++++++++++
 3 files changed, 61 insertions(+)
 create mode 100644 include/netinet/ether.h
 create mode 100644 src/network/ether_aton.c
 create mode 100644 src/network/ether_ntoa.c

diff --git a/include/netinet/ether.h b/include/netinet/ether.h
new file mode 100644
index 0000000..c5179d5
--- /dev/null
+++ b/include/netinet/ether.h
@@ -0,0 +1,14 @@
+#ifndef _NETINET_ETHER_H
+#define _NETINET_ETHER_H
+
+#include <netinet/if_ether.h>
+
+char *ether_ntoa (const struct ether_addr *);
+
+struct ether_addr *ether_aton (const char *);
+
+char *ether_ntoa_r (const struct ether_addr *, char *);
+
+struct ether_addr *ether_aton_r (const char *, struct ether_addr *);
+
+#endif
diff --git a/src/network/ether_aton.c b/src/network/ether_aton.c
new file mode 100644
index 0000000..55f56ec
--- /dev/null
+++ b/src/network/ether_aton.c
@@ -0,0 +1,30 @@
+#include <stdlib.h>
+#include <string.h>
+#include <netinet/ether.h>
+
+static struct ether_addr a;
+
+struct ether_addr *ether_aton_r (const char *x, struct ether_addr *p_a) {
+	struct ether_addr a;
+	for (int ii = 0; ii < 6; ii++) {
+		unsigned long int n;
+		if (ii != 0) {
+			if (x[0] != ':') return 0; /* bad format */
+			else x++;
+		}
+		{
+			char *y;
+			n = strtoul (x, &y, 16);
+			x = y;
+		}
+		if (n > 0xFF) return 0; /* bad byte */
+		a.ether_addr_octet[ii] = n;
+	}
+	if (x[0] != 0) return 0; /* bad format */
+	memmove (p_a, &a, sizeof (struct ether_addr));
+	return p_a;
+}
+
+struct ether_addr *ether_aton (const char *x) {
+	return ether_aton_r (x, &a);
+}
diff --git a/src/network/ether_ntoa.c b/src/network/ether_ntoa.c
new file mode 100644
index 0000000..bcc773a
--- /dev/null
+++ b/src/network/ether_ntoa.c
@@ -0,0 +1,17 @@
+#include <stdio.h>
+#include <netinet/ether.h>
+
+static char x[18];
+
+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 ? "%.2X" : ":%.2X", p_a -> ether_addr_octet[ii]);
+	}
+	return y;
+}
+
+char *ether_ntoa (const struct ether_addr *p_a) {
+	return ether_ntoa_r (p_a, x);
+}
-- 
1.7.11.1

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

[Rich Felker]

On Sun, Apr 14, 2013 at 11:10:55PM -0500, Strake wrote:
> > 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.
> 
> ntoa: same size
> aton: mine is smaller

This doesn't seem to match my results. I compared against the
following version of aton and it was half the size of yours:

struct ether_addr *ether_aton_r (const char *x, struct ether_addr *p_a)
{
	unsigned char *y = p_a->ether_addr_octet;
	char c;
	if (sscanf(x, "%2hhx:%2hhx:%2hhx:%2hhx:%2hhx:%2hhx%c", y,y+1,y+2,y+3,y+4,y+5,y+6,&c)!=6)
		return 0;
	return p_a;
}

Rich

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

[Strake]

On 28/04/2013, Rich Felker <dalias@aerifal.cx> wrote:
> On Sun, Apr 14, 2013 at 11:10:55PM -0500, Strake wrote:
>> > 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.
>>
>> ntoa: same size
>> aton: mine is smaller
>
> This doesn't seem to match my results. I compared against the
> following version of aton and it was half the size of yours:
>
> struct ether_addr *ether_aton_r (const char *x, struct ether_addr *p_a)
> {
> 	unsigned char *y = p_a->ether_addr_octet;
> 	char c;
> 	if (sscanf(x, "%2hhx:%2hhx:%2hhx:%2hhx:%2hhx:%2hhx%c",
> y,y+1,y+2,y+3,y+4,y+5,y+6,&c)!=6)
> 		return 0;
> 	return p_a;
> }
>
> Rich

My method:

$ cat test.c
#include <netinet/ether.h>

int main (int argc, char *argu[]) {
	struct ether_addr *p_a;
	p_a = ether_aton (argu[1]);
	return (*p_a).ether_addr_octet[0];
}

$ cc -I $MUSL/include -L $MUSL/lib -o test test.c && strip test && wc -c <test

Mine: 6200
Yours: 15504

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

[Rich Felker]

On Sun, May 05, 2013 at 09:11:12AM -0500, Strake wrote:
> >> ntoa: same size
> >> aton: mine is smaller
> >
> > This doesn't seem to match my results. I compared against the
> > following version of aton and it was half the size of yours:
> >
> > struct ether_addr *ether_aton_r (const char *x, struct ether_addr *p_a)
> > {
> > 	unsigned char *y = p_a->ether_addr_octet;
> > 	char c;
> > 	if (sscanf(x, "%2hhx:%2hhx:%2hhx:%2hhx:%2hhx:%2hhx%c",
> > y,y+1,y+2,y+3,y+4,y+5,y+6,&c)!=6)
> > 		return 0;
> > 	return p_a;
> > }
> >
> > Rich
> 
> My method:
> 
> $ cat test.c
> #include <netinet/ether.h>
> 
> int main (int argc, char *argu[]) {
> 	struct ether_addr *p_a;
> 	p_a = ether_aton (argu[1]);
> 	return (*p_a).ether_addr_octet[0];
> }
> 
> $ cc -I $MUSL/include -L $MUSL/lib -o test test.c && strip test && wc -c <test
> 
> Mine: 6200
> Yours: 15504

I see. These are two different but perfectly valid ways for looking at
size: size of the function itself versus size of the function with all
its dependencies. My approach to measuring size leads to a smaller
libc.so, whereas your approach leads to a smaller statically linked
binary when the binary does not otherwise use *scanf for anything.

This is an issue that's come up several times before when optimizing
for size, and I don't think there's one right answer for all cases.
The solution is obvious in cases where the code size without depending
on another function is minimal, but when a large function like
snprintf or sscanf is involved it's a larger trade-off.

My leaning in this case is towards making the ether_* functions as
small as possible themselves, without worrying about dependencies. My
reasoning is that, on a small system, at most one or two programs will
be using these functions anyway, and if it's Busybox (or in the
future, Toybox) then scanf is already being pulled in anyway. The main
cases where I'd lean the other way are for functions which have
usefulness in a large number of programs, some of which may be written
in light/minimalistic ways to avoid pulling in stuff like stdio.

Anyway, I welcome other comments on the issue; we don't have to make a
decision immediately but I'd like to get the ether_* stuff integrated
in one form or another soon and have it in the next release.

Rich