usr/src/man/man3socket/getifaddrs.3socket (Diff revision 1)

Show all issues

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.

1. 

   Sebastian Wiedenroth May 26, 2017, 2:05 a.m.

   I've updated the man page for the new version. Is this what you had in mind?

usr/src/lib/libsocket/inet/getifaddrs.c (Diff revision 2)

Show all issues

Probably "was not" and "door allocated"?

1. 

   Sebastian Wiedenroth May 26, 2017, 3:01 a.m.

   this was taken from dladm_door_call() in usr/src/lib/libdladm/common/libdlmgmt.c
   
   I'll fix this here, should I also update the original source?

2. 

   Yuri Pankov May 26, 2017, 3:24 a.m.

   If you wish to, I didn't want to mark this as issue, just as hint.

usr/src/lib/libsocket/inet/getifaddrs.c (Diff revision 2)

Show all issues

Where this number comes from?

1. 

   Sebastian Wiedenroth May 26, 2017, 3:01 a.m.

   From looking at dladm_walk_macaddr() in usr/src/lib/libdladm/common/libdllink.c

2. 

   Yuri Pankov May 26, 2017, 3:24 a.m.

   It's strange that we don't have a define for this.

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);
}

1. 

   Sebastian Wiedenroth May 26, 2017, 3:01 a.m.

   that looks indeed much better. I wasn't sure if it was safe to call close() like that. I guess I'll need to make sure the sockets are initialized to something invalid like -1?

2. 

   Yuri Pankov May 26, 2017, 3:24 a.m.

   Yeah, hopefully others won't find this bad/ugly :-) and even if libsocket is not linted, let's cast the return values of close() to (void), showing that we really don't care about its return value.

usr/src/lib/libsocket/inet/getifaddrs.c (Diff revision 2)

Show all issues

Use for (;;) instead.

1. 

   Vitaliy Gusev May 26, 2017, 7:15 p.m.

   Yuri, why for(;;) is better  than while (1) ?

2. 

   Yuri Pankov May 26, 2017, 7:23 p.m.

   Even if we don't lint libsocket, just to be consistent with other parts of the code where lint would whine about using constant condition. And a lot of style guides I did check (from time to time) do say "Forever loops are done with for's, not while's.".

3. 

   Vitaliy Gusev May 26, 2017, 8 p.m.

   Never heard about that. It looks as overhead styles :) Consisetency is good reason. Thanks.

usr/src/lib/libsocket/inet/getifaddrs.c (Diff revision 2)

Show all issues

Remove extra empty line.

usr/src/lib/libsocket/inet/getifaddrs.c (Diff revision 2)

Show all issues

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?

1. 

   Sebastian Wiedenroth May 26, 2017, 3:01 a.m.

   It does not matter if we check with v4 or v6 because the information should be the same.
   
   The interface index is useful to match an ip address to a mac address.

2. 

   Yuri Pankov May 26, 2017, 3:24 a.m.

   My point is that if we can't rely on this information to be available always, it's not that useful.

3. 

   Sebastian Wiedenroth May 26, 2017, 6:30 p.m.

   In the case where we have an ip address and want to match it to a mac adress it is available, so it is there when we need it.
   But I've looked a bit closer at this again and indeed the metric is NOT guaranteed to be the same. So I'll adjust it to not provide it for AF_LINK entries.

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)

Show all issues

This should probably be static.

usr/src/lib/libsocket/inet/getifaddrs.c (Diff revision 3)

Show all issues

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.

1. 

   Sebastian Wiedenroth May 27, 2017, 3:56 p.m.

   As there is already rbuf I've changed the return values around a bit. I hope this makes things cleaner. Let me know if this is what you had in mind and if I can improve things further.

2. 

   Robert Mustacchi May 27, 2017, 6:34 p.m.

   Sure, this works as a way to do this, thanks!

usr/src/lib/libsocket/inet/getifaddrs.c (Diff revision 3)

Show all issues

Same static note as earlier.

usr/src/lib/libsocket/inet/getifaddrs.c (Diff revision 3)

Show all issues

Is there a reason this is cast to void?

1. 

   Sebastian Wiedenroth May 27, 2017, 3:56 p.m.

   I ran lint manually and got:
   
   warning: function returns value which is always ignored: munmap (E_FUNC_RET_ALWAYS_IGNOR2)
   Not sure how I can handle that better or if it should be ignored. Happy to drop the cast if that is the preferred style.

2. 

   Robert Mustacchi May 27, 2017, 6:34 p.m.

   In asking this, I more so meant why are we ignoring the return value. If munmap returns EINVAL what does that mean? Does that mean there was a programmer error from us or that something is really wrong with the system?

3. 

   Sebastian Wiedenroth May 27, 2017, 9:40 p.m.

   quote from door_call(3C):
   If the results of a door invocation exceed the size of the buffer
   specified by rsize, the system automatically allocates  a new buffer in
   the caller's address space  and updates the rbuf and rsize members to
   reflect this location. In this case, the caller is  responsible for
   reclaiming this area using munmap(rbuf, rsize) when the buffer is no
   longer required.  See munmap(2).
   
   
   So if something is wrong the data we have came from the door_call().
   
   I don't think any input from us should cause door_call() to return a buffer that we'd have to cleanup but can't.
   I've looked through grok and nearly all munmap calls have their return values casted away.
   
   There are a few in testcases that have asserts and a few others that actually do some handling like printing a message.

4. 

   Robert Mustacchi May 28, 2017, 7:30 p.m.

   OK, fair enough. Whenver I see things that may return errors cast to void, I always want to ask. I think you're right that hopefully door_call will put something right there. If it did fail, I'm not sure if we'd want to drive on, but the much more damaging case of unmapping the wrong thing we can't tell anyways so this is probably fine.

usr/src/lib/libsocket/inet/getifaddrs.c (Diff revision 3)

Show all issues

Same static note as earlier.

usr/src/lib/libsocket/inet/getifaddrs.c (Diff revision 3)

Show all issues

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)

Show all issues

If you're going to return a boolean, why not make this return a boolean_t?

1. 

   Sebastian Wiedenroth May 27, 2017, 3:56 p.m.

   I've changed the return paths a bit now to explicitly return -1 or 0, but could still make it boolean_t if you think that is better.

2. 

   Robert Mustacchi May 27, 2017, 6:34 p.m.

   The 0 and -1/errno pattern is fine. Though I made a note about the errno consistency bit on those. Do you want to preserve the return value from dlmgmtd as the errno in those cases? Though I know it's just discarded anyways.

3. 

   Sebastian Wiedenroth May 27, 2017, 9:40 p.m.

   The value in lr_err can actually be used directly used as errno, so this was easy. The latest version should address this.

usr/src/lib/libsocket/inet/getifaddrs.c (Diff revision 3)

Show all issues

Same static note as earlier.

usr/src/lib/libsocket/inet/getifaddrs.c (Diff revision 3)

Show all issues

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)

Show all issues

If you're going to return a boolean, why not make this return a boolean_t?

1. 

   Sebastian Wiedenroth May 27, 2017, 3:56 p.m.

   I've changed the return paths a bit now to explicitly return -1 or 0, but could still make it boolean_t if you think that is better.

2. 

   Robert Mustacchi May 27, 2017, 6:34 p.m.

   See other notes.

usr/src/lib/libsocket/inet/getifaddrs.c (Diff revision 3)

Show all issues

The cast should be unnecessary.

usr/src/lib/libsocket/inet/getifaddrs.c (Diff revision 3)

Show all issues

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.

1. 

   Sebastian Wiedenroth May 27, 2017, 3:56 p.m.

   I've changed it to strlcpy() and also moved it a bit closer to the actual ioctl call.
   Can you explain a bit more why we should not assume that they're the same?
   
   I use it to fill in the ip interface index, but during the review process this code does seem to cause a bit of confusion.
   
   While I understand that the mapping between the links and ip is a bit odd I still think the data would be useful.
   
   So I wonder how I could structure that better or if my understanding is just too wrong and that part of the code should be dropped.

2. 

   Robert Mustacchi May 27, 2017, 6:34 p.m.

   In this case I left out the operative word size. I was just saying that we didn't want to assume that the dlmgmtd size (in the door call) would always be the same as the buffer size.
   Strictly speaking the IP interface on top of a datalink can have a different from the datalink from the kernel's perspective, but that's impossible to do with ifconfig / ipadm, etc. so I'm not sure if that case matters.

3. 

   Sebastian Wiedenroth May 27, 2017, 9:40 p.m.

   ah ok, checking for overflow is now done.

usr/src/lib/libsocket/inet/getifaddrs.c (Diff revision 3)

Show all issues

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)

Show all issues

Do other implementations prefer the IPv4 index over the IPv6?

1. 

   Sebastian Wiedenroth May 27, 2017, 3:56 p.m.

   This is not much of a preference. The returned index will be the same if both IPv4 and IPv6 are on the same link. We just have to make sure we try both in case it is IPv4-only or IPv6-only.

2. 

   Robert Mustacchi May 27, 2017, 6:34 p.m.

   Ah, gotcha. I wasn't sure if they had the same index or not. Thanks!

usr/src/lib/libsocket/inet/getifaddrs.c (Diff revision 4)

Show all issues

Sorry, I missed this earlier. But this could clobber errno potentially.

usr/src/lib/libsocket/inet/getifaddrs.c (Diff revision 4)

Show all issues

In this failure case, how is errno being set?

1. 

   Sebastian Wiedenroth May 27, 2017, 9:39 p.m.

   We can use lr_err for this, done in the latest version.

usr/src/lib/libsocket/inet/getifaddrs.c (Diff revision 4)

Show all issues

Same errno comment here.

usr/src/lib/libsocket/inet/getifaddrs.c (Diff revision 4)

Show all issues

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.

usr/src/lib/libsocket/inet/getifaddrs.c (Diff revisions 5 - 6)

Show all issues

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)

Show all issues

Use B_FALSE

usr/src/lib/libsocket/inet/getifaddrs.c (Diff revisions 5 - 6)

Show all issues

and B_TRUE here

usr/src/lib/libsocket/inet/getifaddrs.c (Diff revisions 5 - 6)

Show all issues

B_TRUE