Netstat needs to be 64-bit so that it can Pgrab() 64-bit and 32-bit processes in order to extract the socket information.

OK, we should make that clear somewhere. I think that's a pretty easy nuance to miss. I guess we should do this assuming $(BUILD64) is set?

I've added a comment.

I don't see anything else under usr/src/cmd/ that builds only a 64-bit binary predicating on $(BUILD64). It seems to be used for dual-arch components.

I can add it but then a 32-bit netstat binary would be built if with BUILD64=# and it would only show socket information for 32-bit processes.
Let me know your preference.

My preference would be to do this if BUILD64 was set to #. If BUILD64 was set to # then there wouldn't be 64-bit components, so it wouldn't be a problem.

Ok.

No reason, I've changed it to use that. I'm leaving both functions in place for now.
It is not static because there is a fail() prototype in usr/src/cmd/stat/common/statcommon.h which is included by this, and that prototype is not static.

Wait, if it's coming from another header, then why are we redeclaring it here at all?

I added it so that I could mark it as noreturn to satisfy gcc/smatch once I removed the warning suppressions.

I'll see if it can be done in the header instead.

I've added some notes.

I don't actually have to since I'm checking that it's a character device I can just go straight to checking the major number.

ph_type and ph_family will always be zero for a TLI socket.
It was just belt and braces - now removed.

I haven't seen any, and this list is copied from pfiles.c

https://github.com/illumos/illumos-gate/blob/master/usr/src/cmd/ptools/pfiles/pfiles.c#L289

I think because I consider the whole exercise of trying to find process information best effort, given it's a merge of two separately taken snapshots etc.

For this to fail though, a fatal error is probably in order.

OK, I guess that's a reasonable perspective. We should probably make sure it's clear in the manual page it's best effort then.

Tweaked the manual a tad.

I was thinking about this a bit more and now there's no way for me as a user to know this occurred. I would consider issuing a warning when this occurs, still printing everything that was gathered, but exiting non-zero to make it clear that an internal error occurred.

I made it fatal in the last revision. If we're that short of memory, exiting seems like a valid option.

No, I don't think we care. The code allows up to 128 characters (I couldn't find a definitive limit for usernames anywhere).

This was copied verbatim from pfiles.c

https://github.com/illumos/illumos-gate/blob/master/usr/src/cmd/ptools/pfiles/pfiles.c#L766
To be honest, we only need sa_family which is the first element of struct sockaddr, so I could just use a plain struct sockaddr - I'll give it a try.

I meant iff (https://en.wikipedia.org/wiki/If_and_only_if ) but I can change or expand it if it is not commonly understood.

I do know the abbreviation, but wasn't sure. I would expand it as otherwise to a lot of folks I think it'll look like a typo.

Done.

Just the best-effort thing again. It can only fail here with EINTR.

These (reverse argument order) were to satisfy smatch. I'll drop the casts too.

Done - you can imagine how many inconsistencies there were in the format strings before I did this...

It's actually copied straight from the extant gather_attrs().

It should be just checking that it is not 0. I'll fix gather_attrs() too.

I was looking at the new version and there is still an overflow potential, but I guess it's not the most realistic? Not sure if it's worth worrying about or not.

It's probably not realistic, but I added overflow detection for transport_count and switched calloc() to recallocarray().

It's because the data is a mixture of tables. We look through for the tables relating to the protocol that is being looked at and count up how many connections there are before allocating the info array, then look through the list again for the extra information tables. 
The data from the kernel will never cause an overflow here. The EXPER_SOCK_INFO tables contain socket information indexed by the position of the corresponding connection in the mib_id table.
However, this looks like a good place for an assertion; I'll add one.

It's a pattern from the extant gather_attrs() again but yes, it looks like it should be uintptr_t.

The attrs and socket info structures are additional information for each TCP connection. Both attrs and info are indexed with the TCP connection offset in the MIB so they are the same size as each other and the same size as the number of connections. However the information may not be present at all, or not present for a particular connection.

OK. I would strongly suggest improving the comment to make it clear about the design there. It was kind of non-obvious to me from the kernel code.

Done.

Yes - Xflag was pre-existing, I just cleaned up the existing output and added more.

You're right, I'll fix and move to int64_t. I didn't assume the library would always be 32-bit, but I did assume that the cleanup would happen once somebody moves dhcpagent et al. to 64-bit. netstat is currently the only 64-bit consumer.

Yes, better, fixed.

Callers don't have the lock, but they do hold a reference on connp, acquired through ipcl_get_next_conn()

I don't think we need to hold a lock - the calling function reads values from connp without a lock (tcp_snmp_get() for example).

Yes, but it's not doing any upcalls. Can you make sure that as long as you have a hold, CONN_CLOSING can't be set and therefore it's safe to perform the upcall due to having the hold (or CONN_CLOSING being set isn't a requirement for making the upcall)?

You're right, it needs to acquire the mutex, fixed.

Thanks for catching this.

Well not originally, because fatal() has an early return if format is NULL, it is not a noreturn function. You spotted this in another comment. I've fixed and added the __NORETURN tag.

done - PRId64

Done

It is from the original, but I agree and have fixed it (as well as marking fatal() as __NORETURN now that it is)

Much better, taken verbatim, thanks.

I don't have a complete answer to this (help would be appreciated).
We do have a reference on the conn_t as this function is always being called from a loop that uses ipcl_get_next_conn()
The reason for the CONN_CLOSING check, according to the original netstat change notes, is to handle sockets that pop off the sockmod. In this case, the sonode associated with the connection can be freed but a dangling reference left in the conn_t upper_handle. I checked other instances where socket upcalls are used, and none of them seem to check for this or worry about a race between checking and making the upcall. Some upcalls are made while holding the mutex, others are not.
It might be possible to hold the conn lock while making the upcall - I will investigate.

OK. So, in this case, based on my reading, the upcalls are supposed to be valid while we hold a reference. So I don't think it can go away until after we finish the call to ipcl_get_next_conn(). So I don't think it makes sense to hold the conn lock during the upcall in this case. I guess what's there is probably fine. I would probably add a comment about all of this above there so someone else doesn't get confused in the future.

Indeed.. no idea how that happened!

BTW, some may find this link helpful:

https://github.com/illumos/illumos-gate/compare/master...citrus-it:ig_9531_netstat-u

This is part of the change that survives from Mohamed's review request a few years ago.
Before this change, there was a struct k_sockinfo that wrapped struct sockinfo and added these additional strings to data returned from the kstat request.

Since struct sockinfo is declared as a private interface, Mohamed just moved them as-is directly into struct sockinfo before adding the new fields needed for netstat -u:

-#define    ADRSTRLEN (2 * sizeof (void *) + 1)
-/*
- * kernel structure for passing the sockinfo data back up to the user.
- * the strings array allows us to convert AF_UNIX addresses into strings
- * with a common method regardless of which n-bit kernel we're running.
- */
-struct k_sockinfo {
-   struct sockinfo ks_si;
-   char        ks_straddr[3][ADRSTRLEN];
-};


So I haven't changed any types here, which bit could uint64_t be used for?

Maybe I'm confused.  I thought those were kernel memeory addresses.  Are they network addresses?  AF_UNIX?

No, you aren't confused, I was. They're string representations of 64-bit kernel memory addresses, built using snprintf("%p")

Since netstat seems to be the only in-gate consumer of the sockfs:sock_unix_list kstat, and it's a private interface that I'm nodifying anyway, yes I could change these to uint64_t.

I think it's fine to keep them as strings for the ISA independence that it provides for now as per the original comment, fwiw.

It also allows easier testing with the previous netstat binary which I kept around for the purpose.