Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 2 (+2188 -842) |
9531 Want netstat -u to show PIDs associated with sockets
Review Request #2311 — Created Sept. 18, 2019 and submitted
Information | |
---|---|
citrus | |
illumos-gate | |
master | |
9531, 11707, 11708 | |
32b7e5d... | |
Reviewers | |
general | |
9531 Want netstat -u to show PIDs associated with sockets
11707 provide 64-bit libdhcpagent
11708 netstat should be smatch and CERRWARN clean
Also available to view/review in github at https://github.com/citrus-it/illumos-omnios/commits/ig_9531_netstat-u
-
-
usr/src/cmd/cmd-inet/usr.bin/netstat/Makefile (Diff revision 2) Why do we need Makefile.cmd.64 here now? I don't see this discussed in any of the bugs. It'd help to understand if this is related to adding -u support or if this was done as just a separate thing while working on netstat.
-
usr/src/cmd/cmd-inet/usr.bin/netstat/netstat.c (Diff revision 2) Can you use the sys/ccompile.h \_\_NORETURN macro? Also is there a reason this isn't static? Now that netstat is only in a single file, not sure why it'd need to be external. I assume it's not worth trying to unify fail and fatal?
-
usr/src/cmd/cmd-inet/usr.bin/netstat/netstat.c (Diff revision 2) Maybe write a mini-theory statement about what it's for, why it exists, how it works, etc.? For example, if you had tried an AVL tree here but found it too slow, that'd be useful to record as that might be a natural question someone has (note this isn't suggesting that the hashing should be rewritten with just an AVL tree).
-
usr/src/cmd/cmd-inet/usr.bin/netstat/netstat.c (Diff revision 2) I'd recommend commenting where these values came from. Even if you just picked them arbitrarily.
-
usr/src/cmd/cmd-inet/usr.bin/netstat/netstat.c (Diff revision 2) It'd be helpful to have a comment that explains why you're checking for the ph_type and ph_family to be zero here.
-
usr/src/cmd/cmd-inet/usr.bin/netstat/netstat.c (Diff revision 2) Mind putting parens around this multi-line if expression? Same at the one a few below.
-
usr/src/cmd/cmd-inet/usr.bin/netstat/netstat.c (Diff revision 2) I think it's clearer if you have an unknown sentinel that's actually outside of function scope, as in it's its own top-level value. I think that makes it clearer about its purpose and why it exists.
-
usr/src/cmd/cmd-inet/usr.bin/netstat/netstat.c (Diff revision 2) Please put a void in a function that takes no arguments to make it clear it takes no arguments. Otherwise, sometimes a compiler will think it is a K&R style declaration.
-
usr/src/cmd/cmd-inet/usr.bin/netstat/netstat.c (Diff revision 2) Since you've opted into c99, you can scope these in the for loop variable declarations and limit their visibility. (I would suggest limiting local variables to their minimum required scope in general)
-
usr/src/cmd/cmd-inet/usr.bin/netstat/netstat.c (Diff revision 2) When using indexes elsewhere you use unsigned integers. Given that this is iterating all of the indexes, probably it should be unsigned to be consistent (also there can never be a negative index).
-
usr/src/cmd/cmd-inet/usr.bin/netstat/netstat.c (Diff revision 2) In all of the definitions I have, pid is always a signed type. So I think you need %ld or %d depending on bitness.
-
usr/src/cmd/cmd-inet/usr.bin/netstat/netstat.c (Diff revision 2) I would make these arguments const if you can. Though I know the proc handle probalby can't.
-
usr/src/cmd/cmd-inet/usr.bin/netstat/netstat.c (Diff revision 2) Should ip or arp be in the list? I don't recall if they're TLI or not.
-
usr/src/cmd/cmd-inet/usr.bin/netstat/netstat.c (Diff revision 2) Is there a reason this memory allocation failure isn't considered fatal?
-
usr/src/cmd/cmd-inet/usr.bin/netstat/netstat.c (Diff revision 2) I assume we don't care about truncation at all here for users?
-
usr/src/cmd/cmd-inet/usr.bin/netstat/netstat.c (Diff revision 2) Why is this in units of longs? Can you expand the comment there? Does a sockaddr_storage not do the trick?
-
usr/src/cmd/cmd-inet/usr.bin/netstat/netstat.c (Diff revision 2) Please put parens around multi-line continuation for this if statement.
-
-
usr/src/cmd/cmd-inet/usr.bin/netstat/netstat.c (Diff revision 2) Any particular reason we're ignoring the return value here? If this fails, that probably indicates something has gone rather wrong, right?
-
usr/src/cmd/cmd-inet/usr.bin/netstat/netstat.c (Diff revision 2) See earlier notes about signedness of i and scoping of ph/ph2.
-
usr/src/cmd/cmd-inet/usr.bin/netstat/netstat.c (Diff revision 2) See earlier notes about signedness of i and scoping of ph/ph2.
-
usr/src/cmd/cmd-inet/usr.bin/netstat/netstat.c (Diff revision 2) If you're cleaning these up, the casts shouldn't be necessary, fwiw.
-
usr/src/cmd/cmd-inet/usr.bin/netstat/netstat.c (Diff revision 2) I would add a comment above each of these blocks explaining why they're done this way and not the old way so someone doesn't later come along and do the opposite of what you've done.
-
usr/src/cmd/cmd-inet/usr.bin/netstat/netstat.c (Diff revision 2) Isn't a major always a 32-bit value? I'm not sure we should be using PRIx64 for it here.
-
usr/src/cmd/cmd-inet/usr.bin/netstat/netstat.c (Diff revision 2) Aren't sie_flags defined as a uint64_t and therefore %x is incorrect?
-
usr/src/cmd/cmd-inet/usr.bin/netstat/netstat.c (Diff revision 2) Based on this, I assume you're trying to make sure that transport_count can't overflow. However, signed overflow is undefined behavior. Therefore you probably want to make this an unsigned value or make sure when you're adding to it it doesn't overflow before you actually do the arithmetic.
-
usr/src/cmd/cmd-inet/usr.bin/netstat/netstat.c (Diff revision 2) Why are we comparing mib_id here to EXPER_SOCK_INFO and not to mib_id like when we counted the number of elements? If these are different, can't we end up overflowing?
-
usr/src/cmd/cmd-inet/usr.bin/netstat/netstat.c (Diff revision 2) I suspect you're tyring to make sure that the length and offsets don't overflow the amount of data we have right? In that case the arithmetic should be done on uintptr_t's I think.
-
usr/src/cmd/cmd-inet/usr.bin/netstat/netstat.c (Diff revision 2) In this section and looping we currently seem to have an assumption that the number of attrs and the number of socket info structures will be identical. Why is that? I can see that there are cases in the kernel where we'll not generate it if we can't allocate memory. How do we ensure that these are always the same. In particular, cases like at 5532, 5542. What makes sure they're actually in sync?
-
usr/src/cmd/cmd-inet/usr.bin/netstat/netstat.c (Diff revision 2) Why are the unsigned gauges being converted to signed values? Doesn't this risk truncation?
-
usr/src/cmd/cmd-inet/usr.bin/netstat/netstat.c (Diff revision 2) Just to make sure the Xflag really is just an internal debugging flag that you use to dump internal state and that's why it's been added all over the place, right?
-
usr/src/cmd/cmd-inet/usr.bin/netstat/netstat.c (Diff revision 2) This has the same question as the TCP bits do on iptr/aptr.
-
usr/src/cmd/cmd-inet/usr.bin/netstat/netstat.c (Diff revision 2) How did you determine that this was sufficient size? Is it ok for us to truncate this if we're wrong? Any reason not to just use asprintf so we don't have to guess?
-
-
-
usr/src/cmd/cmd-inet/usr.bin/netstat/netstat.c (Diff revision 2) Looks like signed/unsigned mismatch. I think ks_ndata is unsigned, but you're comparing to a signed value (i).
-
usr/src/lib/libdhcpagent/common/dhcpagent_ipc.h (Diff revision 2) Isn't a time_t signed. If so, shouldn't this be an int32_t? Also, since this is private, we could put it in now as an int64_t so that way 64-bit stuff isn't being truncated. We shouldn't assume a library will always be 64-bit.
-
usr/src/man/man1m/netstat.1m (Diff revision 2) Should this mention that they might be truncated? Can we talk about the behavior a user should expect for multiple processes that have the same fd open. As in they should expect it to be an exhaustive list (given that it's a moving snapshot) or not.
-
usr/src/uts/common/fs/sockfs/socksubr.c (Diff revision 2) Comment needs to explain _why_ it must be done before.
-
usr/src/uts/common/fs/sockfs/socksubr.c (Diff revision 2) Since we're rewriting these should we verify that we don't overflow the buffers with snprintf now instead?
-
usr/src/uts/common/inet/ip/ipclassifier.c (Diff revision 2) Since this is only ever used in loops where we're appending them to mib2 data, rather than kmem_alloc/freeing it every iteration, should we just ask callers to pass in the struct and we'll fill it out. That way a single stack allocation can be done and we don't have to keep going back and forth in kmem? Doesn't matter either way per se.
-
usr/src/uts/common/inet/ip/ipclassifier.c (Diff revision 2) Presumably we need to actually hold the conn_lock while checking this. Are callers required to have done so? If so, should we be asserting that?
-
usr/src/uts/common/inet/ip/ipclassifier.c (Diff revision 2) You probably want KM_NOSLEEP | KM_NORMALPRI.
-
usr/src/uts/common/inet/tcp/tcp_stats.c (Diff revision 2) Conventionally the != is on the previous line.
-
usr/src/uts/common/sys/socketvar.h (Diff revision 2) I would make sure to double check 32-bit / 64-bit alignment on the updated version of the structure.
-
usr/src/uts/common/sys/strsubr.h (Diff revision 2) This comment would be more helpful if it explained why it was needed. In this case looking at spec_clone() doesn't really make it clearer as to why this member is useful.
Change Summary:
Updates following rm's review.
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 3 (+2379 -939) |
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 4 (+2415 -940) |
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 5 (+2425 -941) |
-
Thanks for the follow ups here and working on this! I have a few follow ups on the changed bits.
-
usr/src/cmd/cmd-inet/usr.bin/netstat/netstat.c (Diff revisions 2 - 5) I see you've moved the fatal in place of the fail macro. Should this also have the __NORETURN attribute applied?
-
usr/src/cmd/cmd-inet/usr.bin/netstat/netstat.c (Diff revisions 2 - 5) Because we've changed SENDQ and RECVQ to an int64_t to deal with overflow, this should be `"%6" PRIu64` or similar, I believe. This is also true of the RECVQ bit.
-
usr/src/cmd/cmd-inet/usr.bin/netstat/netstat.c (Diff revisions 2 - 5) Mind putting braces around this new multi-line if?
-
usr/src/cmd/cmd-inet/usr.bin/netstat/netstat.c (Diff revisions 2 - 5) I'm not sure if this is from the original netstat (it shows as new between a diff of two things), but surely if format is null I imagine we should just call exit and not continue?
-
usr/src/man/man1m/netstat.1m (Diff revisions 2 - 5) When I see snapshot, I think that we're going to be able to gather this information atomically. How about the following: 'While the system gathers this information, the processes associated with a given endpoint may change. If such a change occurs, it may not be reflected in the output.'
-
usr/src/uts/common/inet/ip/ipclassifier.c (Diff revisions 2 - 5) Why is it OK to drop the mutex here? What ensures that this data, like the upcalls, can't be changed out from under you? Should we be putting a reference on the conn_t or does our caller make sure that it has one? I think that'll make sure that if someone is calling ip_quiesce_conn() that we won't have our upcalls and other data torn out from under us. It's not clear to me if this is ultimately being called from a ipcl_walk() call or not.
-
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 6 (+2448 -945) |
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 7 (+2450 -945) |
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 8 (+2450 -945) |
-
-
usr/src/uts/common/sys/socketvar.h (Diff revision 8) Why not uint64_t here too? (as you did some other places)
for all of:
char si_son_straddr[ADRSTRLEN];
char si_lvn_straddr[ADRSTRLEN];
char si_fvn_straddr[ADRSTRLEN];
-
In general, everything looks good. Thanks for all the work here.
-
usr/src/cmd/cmd-inet/usr.bin/netstat/netstat.c (Diff revisions 5 - 8) Shouldn't this kind of warning go to stderr?
-
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 9 (+2450 -945) |