[<prev] [next>] [thread-next>] [day] [month] [year] [list]
Message-ID: <20140728004906.GA14406@brightrain.aerifal.cx>
Date: Sun, 27 Jul 2014 20:49:06 -0400
From: Rich Felker <dalias@...c.org>
To: musl@...ts.openwall.com
Subject: Reviewing if_nameindex and getifaddrs patch
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
Powered by blists - more mailing lists
Confused about mailing lists and their use?
Read about mailing lists on Wikipedia
and check out these
guidelines on proper formatting of your messages.
