From mboxrd@z Thu Jan 1 00:00:00 1970 X-Msuck: nntp://news.gmane.org/gmane.linux.lib.musl.general/5663 Path: news.gmane.org!not-for-mail From: Rich Felker Newsgroups: gmane.linux.lib.musl.general Subject: Reviewing if_nameindex and getifaddrs patch Date: Sun, 27 Jul 2014 20:49:06 -0400 Message-ID: <20140728004906.GA14406@brightrain.aerifal.cx> 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 1406508571 13799 80.91.229.3 (28 Jul 2014 00:49:31 GMT) X-Complaints-To: usenet@ger.gmane.org NNTP-Posting-Date: Mon, 28 Jul 2014 00:49:31 +0000 (UTC) To: musl@lists.openwall.com Original-X-From: musl-return-5668-gllmg-musl=m.gmane.org@lists.openwall.com Mon Jul 28 02:49:24 2014 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 1XBZ7y-0006Q6-U6 for gllmg-musl@plane.gmane.org; Mon, 28 Jul 2014 02:49:23 +0200 Original-Received: (qmail 7566 invoked by uid 550); 28 Jul 2014 00:49:20 -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 7549 invoked from network); 28 Jul 2014 00:49:19 -0000 Content-Disposition: inline User-Agent: Mutt/1.5.21 (2010-09-15) Original-Sender: Rich Felker Xref: news.gmane.org gmane.linux.lib.musl.general:5663 Archived-At: In regards to: http://git.alpinelinux.org/cgit/aports/plain/main/musl/1002-reimplement-if_nameindex-and-getifaddrs-using-netlin.patch?id=3227b4ad816f850f655b6f44dc497926cb2cdcd1 I don't see any remaining major issues except that it would obviously be nice if this could be smaller. A few minor details: For __netlink.h, I'd been asking whether everything in this file is used. I did some tests just commenting things out and only found a few items that could be removed: //#define NLM_F_MULTI 2 //#define NLM_F_ACK 4 //#define NLM_F_ATOMIC 0x400 //#define NLMSG_NOOP 0x1 //#define NLMSG_OVERRUN 0x4 //#define RTM_NEWADDR 20 I don't think it makes sense to selectively omit such a small set of lines, so I'm fine with just leaving the unneeded ones. One thing that seems rather out-of-place is: #define IFADDRS_HASH_SIZE 64 This seems to be an implementation detail of the two functions using netlink and not part of the netlink API (either the kernel's API or musl's internal __rtnetlink_enumerate API for it). So I think it would make sense to move this into the files that use it. In general, static functions need not/should not have __-prefixed names. It's not really a problem but it makes it less obvious that they're static. It would be nice to change these and perhaps give them slightly more descriptive names (although admittedly that hasn't been done elsewhere in musl) just so they make sense in a debugger, etc.) I really don't understand the 'hash' logic for getifaddrs yet, but the function seems to work. Some general description of what data the callback receives and what it's doing with it could be helpful for reviewing this part of the patch. Finally, I've been trying to reduce the unnecessary usage of __-prefixed filenames for implementation-internal purposes, except when they implement a function whose name is __-prefixed. So perhaps netlink.h and netlink.c, or netlink.h and __rtnetlink_enumerate.c, would be better names for these files (I tend to prefer netlink.c I think). For if_nameindex, we discussed the 'hash' buckets on IRC and whether they are necessary. I noted that it would be nice if there were a way to build the result array directly in the callback, but this is difficult without breaking up the allocation into lots of small pieces since there are strings that need to be located outside of the array, and "relocating" them on realloc requires nontrivial code. So I'm not sure if there's anything to improve here, but it's an idea someone else could look at. Rich