From mboxrd@z Thu Jan 1 00:00:00 1970 X-Msuck: nntp://news.gmane.org/gmane.linux.lib.musl.general/3108 Path: news.gmane.org!not-for-mail From: Rich Felker Newsgroups: gmane.linux.lib.musl.general Subject: Re: [patch] add ether_(aton, ntoa) Date: Sun, 14 Apr 2013 21:40:05 -0400 Message-ID: <20130415014005.GR20323@brightrain.aerifal.cx> References: Reply-To: musl@lists.openwall.com NNTP-Posting-Host: plane.gmane.org Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii X-Trace: ger.gmane.org 1365990016 4221 80.91.229.3 (15 Apr 2013 01:40:16 GMT) X-Complaints-To: usenet@ger.gmane.org NNTP-Posting-Date: Mon, 15 Apr 2013 01:40:16 +0000 (UTC) To: musl@lists.openwall.com Original-X-From: musl-return-3112-gllmg-musl=m.gmane.org@lists.openwall.com Mon Apr 15 03:40:20 2013 Return-path: Envelope-to: gllmg-musl@plane.gmane.org Original-Received: from mother.openwall.net ([195.42.179.200]) by plane.gmane.org with smtp (Exim 4.69) (envelope-from ) id 1URYP4-000670-EP for gllmg-musl@plane.gmane.org; Mon, 15 Apr 2013 03:40:18 +0200 Original-Received: (qmail 13825 invoked by uid 550); 15 Apr 2013 01:40:17 -0000 Mailing-List: contact musl-help@lists.openwall.com; run by ezmlm Precedence: bulk List-Post: List-Help: List-Unsubscribe: List-Subscribe: Original-Received: (qmail 13814 invoked from network); 15 Apr 2013 01:40:17 -0000 Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.5.21 (2010-09-15) Xref: news.gmane.org gmane.linux.lib.musl.general:3108 Archived-At: On Sun, Apr 14, 2013 at 04:27:08PM -0500, Strake wrote: > >From 0166471873b3aa801ac4e6301350b08d65e8f24f Mon Sep 17 00:00:00 2001 > From: Strake > 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