3729 getifaddrs must learn to stop worrying and love the other address families
Review Request #318 - Created Jan. 4, 2017 and updated
Information | |
---|---|
Sebastian Wiedenroth | |
illumos-gate | |
Reviewers | |
general | |
Presently getifaddrs enumerates network interface addresses only from AF_INET and AF_INET6.
This is contrary to the expectations of a small set of software (e.g. dhcpcd, olsrd) which expects this function to deal also in AF_LINK.With this change getifaddrs() also returns AF_LINK entries, supplying the MAC address in 'ifa_addr' and a reasonably filled 'if_data_t' in 'ifa_data'.
This should be sufficient for most applications that are interested in things like the MAC address or the MTU.
Booted OI with this change, ran a small tool that prints the getifaddrs() details.
Ran the test tool with and without the environment variable GETIFADDRS_MAY_RETURN_AF_LINK=1 set.
Rebuild the test tool and retried.Ran with libumem and ::findleaks on the test tool (that also calls freeifaddrs()) in all four combinations.
-
usr/src/man/man3socket/getifaddrs.3socket (Diff revision 1) -
I'd still remove "BUGS", but mention under notes that AF_INET, AF_INET6, and AF_LINK are returned, and that any other address families (not that there are any I can think of currently) are not returned. This way, we insure proper setting of expectations.
Description: |
|
---|
-
usr/src/lib/libsocket/inet/getifaddrs.c (Diff revision 2) -
Probably "was not" and "door allocated"?
-
usr/src/lib/libsocket/inet/getifaddrs.c (Diff revision 2) -
Where this number comes from?
-
usr/src/lib/libsocket/inet/getifaddrs.c (Diff revision 2) -
How about the following given that socket() calls really shouldn't fail, so we won't get in that situation too often? Just trying to reduce repeating stuff:
if ((sock4 = socket(AF_INET, SOCK_DGRAM, 0)) < 0 || (sock6 = socket(AF_INET6, SOCK_DGRAM, 0)) < 0 || (door_fd = open(DLMGMT_DOOR, O_RDONLY)) < 0 || (dld_fd = open(DLD_CONTROL_DEV, O_RDWR)) < 0) { err = errno; close(sock4); close(sock6); close(door_fd); errno = err; return (-1); }
-
usr/src/lib/libsocket/inet/getifaddrs.c (Diff revision 2) -
Use
for (;;)
instead. -
usr/src/lib/libsocket/inet/getifaddrs.c (Diff revision 2) -
Remove extra empty line.
-
usr/src/lib/libsocket/inet/getifaddrs.c (Diff revision 2) -
Move declarations to the start of the code block.
-
usr/src/lib/libsocket/inet/getifaddrs.c (Diff revision 2) -
What if we have both v4 and v6 plumbed? Do we even want this information for AF_LINK?
This looks good to me, but you really should have Dan review this.
In general, I think this looks good. However, there are a bunch of nits and the like below.
-
usr/src/lib/libsocket/inet/getifaddrs.c (Diff revision 3) -
This should probably be static.
-
usr/src/lib/libsocket/inet/getifaddrs.c (Diff revision 3) -
It'd be helpful to describe the error semantics here. Notably that -1 / errno is used if the door call fails and that otherwise a positive error value is returned. Honestly, it may more sense to distinguish between the success of the door call and instead have a separate argument to take the door return value semantically to avoid the overloading.
-
usr/src/lib/libsocket/inet/getifaddrs.c (Diff revision 3) -
Same static note as earlier.
-
usr/src/lib/libsocket/inet/getifaddrs.c (Diff revision 3) -
Is there a reason this is cast to void?
-
usr/src/lib/libsocket/inet/getifaddrs.c (Diff revision 3) -
Same static note as earlier.
-
usr/src/lib/libsocket/inet/getifaddrs.c (Diff revision 3) -
As these are private structures unlike the door_arg_t, I recommend always running a bzero first. This way if the structure changes and someone forgets to update this, we're less likely to send stack garbage.
-
usr/src/lib/libsocket/inet/getifaddrs.c (Diff revision 3) -
If you're going to return a boolean, why not make this return a boolean_t?
-
usr/src/lib/libsocket/inet/getifaddrs.c (Diff revision 3) -
Same static note as earlier.
-
usr/src/lib/libsocket/inet/getifaddrs.c (Diff revision 3) -
As these are private structures unlike the door_arg_t, I recommend always running a bzero first. This way if the structure changes and someone forgets to update this, we're less likely to send stack garbage.
-
usr/src/lib/libsocket/inet/getifaddrs.c (Diff revision 3) -
If you're going to return a boolean, why not make this return a boolean_t?
-
usr/src/lib/libsocket/inet/getifaddrs.c (Diff revision 3) -
The cast should be unnecessary.
-
usr/src/lib/libsocket/inet/getifaddrs.c (Diff revision 3) -
While both of these structures are the same size, I'm not sure if this is safe or not. Consider the case where a bug causes dlmgmtd not to null terminate the resulting array. In that case we'd end up not null terminating lifr_name. Do consumers of this structure assume that the name is always null terminated? Based on what I see in the manual they do. We also may not want to bake the assumption that they're the same into this.
-
usr/src/lib/libsocket/inet/getifaddrs.c (Diff revision 3) -
So the size of the address is probably less than the size of the sockaddr_storage. Should we make sure to zero the allocation to make sure we're not leaking arbitrary data into the buffer?
-
usr/src/lib/libsocket/inet/getifaddrs.c (Diff revision 3) -
Do other implementations prefer the IPv4 index over the IPv6?
-
usr/src/lib/libsocket/inet/getifaddrs.c (Diff revision 4) -
Sorry, I missed this earlier. But this could clobber errno potentially.
-
usr/src/lib/libsocket/inet/getifaddrs.c (Diff revision 4) -
In this failure case, how is errno being set?
-
usr/src/lib/libsocket/inet/getifaddrs.c (Diff revision 4) -
Same errno comment here.
-
usr/src/lib/libsocket/inet/getifaddrs.c (Diff revision 4) -
I would just sanity check this by actually checking for overflow and if it does, just continue or skip this bit of information.
Ship It!
-
usr/src/lib/libsocket/inet/getifaddrs.c (Diff revision 5) -
Tiny cstyle nit. A two-or-more-line if clause should have braces for its code, even if it's one line.
The above cstyle nit is just that... a nit. Otherwise, I'm happy.
Change Summary:
added new symbol to work around libuv bug
Testing Done: |
|
|||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Diff: |
Revision 6 (+357 -34) |
-
usr/src/lib/libsocket/inet/getifaddrs.c (Diff revisions 5 - 6) -
We currently don't use stdbool.h and use our own boolean_t instead in illumos-gate (
boolean_t
predates stdbool.h and I think no one's wanted to try to convert everything nor leave things in a mixed state). -
usr/src/lib/libsocket/inet/getifaddrs.c (Diff revisions 5 - 6) -
Use
B_FALSE
-
usr/src/lib/libsocket/inet/getifaddrs.c (Diff revisions 5 - 6) -
and
B_TRUE
here -
usr/src/lib/libsocket/inet/getifaddrs.c (Diff revisions 5 - 6) -
B_TRUE
Ship It!
-
usr/src/lib/libsocket/inet/getifaddrs.c (Diff revisions 5 - 7) -
Why remove this line?
Ship It!
-
usr/src/lib/libsocket/common/mapfile-vers (Diff revision 7) -
Do we really need to make it global???