sockstat tcp/udp switches
Giorgos Keramidas
keramida at freebsd.org
Sat Nov 11 15:49:41 UTC 2006
On 2006-11-07 10:51, Josh Carroll <josh.carroll at psualum.com> wrote:
> Below is the patch using strsep instead. I also updated the
> gather_inet function to print a message that a protocol is not
> supported if it exists in /etc/protocols, but does not have a case in
> the gather_inet function.
>
> patch is here if you'd rather just fetch it:
>
> http://pflog.net/~floyd/sockstat.patch
This patch still has some points which are in need of a slight
modification or rearrangement, but it works fine here.
My comments, and a proposed patch to replace it (after some style fixes
and partial rewrite of the added code), are below:
> --- sockstat.c.orig Thu Jun 9 23:36:03 2005
> +++ sockstat.c Tue Nov 7 10:49:28 2006
> @@ -66,8 +66,15 @@
> static int opt_u; /* Show Unix domain sockets */
> static int opt_v; /* Verbose mode */
>
> +/* default protocols to use if no -P was defined, currently
> + tcp, udp and divert */
> +const char *default_protos[] = {"tcp", "udp", "divert", NULL};
> +
> +static int *protos; /* protocols to use */
> +
> static int *ports;
We don't use this sort of style for inline comments. This should be
something like:
% +/*
% + * Default protocols to use if no -P was defined, currently
% + * tcp, udp and divert.
% + */
% +const char *default_protos[] = {"tcp", "udp", "divert", NULL};
% +
% +static int *protos; /* protocols to use */
% +
% static int *ports;
IMHO, there's no need to repeat in the comments the protocol
list, because this means anyone updating the source definition of
default_protos[] will have to remember to update the commented
out list too. Since the default_protos[] array is static and
predefined, there is no need for special NULL termination too.
You can let the compiler find out how large it is. This is why
in the attached patch this part appears as:
% +/*
% + * Default protocols to use if no -P was defined.
% + */
% +static const char *default_protos[] = {"tcp", "udp", "divert" };
% +static size_t default_numprotos =
% + sizeof(default_protos) / sizeof(default_protos[0]);
% +
% +static int *protos; /* protocols to use */
% +static size_t numprotos; /* allocated size of protos[] */
It's also a good idea to use `size_t' for array sizes, but this
affects other parts below in the attached patch (which used `int'
to iterate through the array index).
> +
> +/* this function needs to be updated to reflect additional protocols
> + that are to be supported */
> +static int
> +get_proto_type(const char *proto)
> +{
Minor style bugs in the formatting of the comment above.
> + struct protoent *pent;
> +
> + if(strlen(proto) == 0)
> + return 0;
Pretty small style bug in whitespace.
> + pent = getprotobyname(proto);
> + if(pent == NULL) {
> + printf("Unrecognized protocol: %s\n", proto);
> + return -1;
> + } else {
> + return pent->p_proto;
> + }
Since you immediatelly return when something goes wrong, it will
save some indentation levels if parts like:
if (condition) {
...
return;
} else {
small or larger part;
return;
}
are written as:
if (condition) {
...
return;
}
small or larger part;
return;
Now it doens't matter so much, I think, but if the else-part
grows a lot, it will.
> +static void init_protos(int num)
> +{
> + int proto_count = 0;
> +
> + if(num > 0) {
> + proto_count = num;
> + } else {
> + /* find the maximum number of possible protocols */
> + while(getprotoent() != NULL)
> + proto_count++;
> + endprotoent();
> + }
> +
> + if ((protos = malloc(sizeof(int) * proto_count)) == NULL)
> + err(1, "malloc()");
> +}
I'm not sure I like the iteration of all protocol entries from
`/etc/protocols' here. This may tend to be *VERY* slow if
protocol entries are retrieved from NIS maps or something that
is not locally available at all times :-(
Maybe it's a better idea to let the protos[] array automatically
adapt to the number of protocols entries *really* entered by the
user at command line?
I'll try to work a second patch (after the attached one), which
does exactly this sort of thing, because I don't like static,
fixed limits and prefer auto-sizing when possible.
> +static int
> +parse_protos(char *protospec)
> +{
> + char **prot;
> + char *tmp = protospec;
> + int protos_defined = 0, proto_type, proto_index = 0;
> +
> + init_protos(0);
> +
> + while( (*prot = strsep(&tmp, ",")) != NULL) {
> + /* handle ,, */
> + if(strlen(*prot) > 0) {
> + proto_type = get_proto_type(*prot);
> + if(proto_type != -1) {
> + protos[proto_index++] = proto_type;
> + protos_defined++;
> + }
> + }
> + }
> + return protos_defined;
Indentation and style is kind of uncool here. Since the special
case is when strlen(*prot) == 0, we can skip these parts of the
protocol string with:
while ((*prot = strsep(&tmp, ",")) != NULL) {
if (strlen(*prot) == NULL)
continue;
...
}
The semantic use of proto_index and protos_defined is also
*exactly* the same, so they can be rolled into a _single_ index,
since they will always have the same value throughout this
function body.
This is why I've rewritten this part in the attached patch to
look like this:
% +static int
% +parse_protos(char *protospec)
% +{
% + char *prot;
% + char *tmp = protospec;
% + int proto_type, proto_index;
% +
% + if (protospec == NULL)
% + return -1;
% +
% + init_protos(0);
% + proto_index = 0;
% + while ((prot = strsep(&tmp, ",")) != NULL) {
% + if (strlen(prot) == 0)
% + continue;
% + proto_type = get_proto_type(prot);
% + if (proto_type != -1)
% + protos[proto_index++] = proto_type;
% + }
% + numprotos = proto_index;
% + return proto_index;
% +}
> @@ -209,7 +279,9 @@
> protoname = "div";
> break;
> default:
> - abort();
> + pent = getprotobynumber(proto);
> + printf("protocol `%s' not supported.\n", pent->p_name);
> + exit(EXIT_FAILURE);
I don't like replacing abort() with a custom printf() error. For
one thing, printf() will go to `stdout' and not `stderr'. For
another, we usually prefer err() and warn() instead of custom
printf() calls for errors.
You are also missing a check for the return value of
getprotobynumber() being NULL here.
I've rewritten this part as:
% default:
% - abort();
% + errx(1, "protocol %d not supported", proto);
> default:
> - abort();
> + pent = getprotobynumber(proto);
> + printf("protocol `%s' not supported.\n", pent->p_name);
> + exit(EXIT_FAILURE);
Same as above.
> +static int set_default_protos(void)
> +{
> + struct protoent *prot;
> + const char **curr_proto;
> + int proto_index = 0;
> + int proto_count = 0;
> +
> + /* determine the number of default protocols */
> + for(curr_proto = default_protos; *curr_proto; curr_proto++, proto_count++)
> + ;
> +
> + init_protos(proto_count);
> +
> + for(curr_proto = default_protos; *curr_proto; curr_proto++) {
> + prot = getprotobyname(*curr_proto);
> + if(prot == NULL) {
> + printf("Cannot determine default protocols\n");
> + exit(EXIT_FAILURE);
> + }
> + protos[proto_index++] = prot->p_proto;
> + }
> +
> + return proto_index;
> +}
This function suffers a bit from bloat, because of the choise to
use a NULL entry at the end of default_protos[] as the protocol
list terminator. It also uses printf() for error reporting,
which (as I said previously) is not a good idea.
A much smaller, but equivalent in functionality version, which
depends on `default_numprotos' being already set for us by the
compile-time sizeof() stuff near default_protos[]'s definition,
can be:
% +static int set_default_protos(void)
% +{
% + struct protoent *prot;
% + const char *pname;
% + size_t pindex;
% +
% + init_protos(default_numprotos);
% +
% + for (pindex = 0; pindex < default_numprotos; pindex++) {
% + pname = default_protos[pindex];
% + prot = getprotobyname(pname);
% + if (prot == NULL)
% + err(1, "getprotobyname: %s", pname);
% + protos[pindex] = prot->p_proto;
% + }
% + numprotos = pindex;
% + return pindex;
% +}
> int
> main(int argc, char *argv[])
> {
> - int o;
> + /* if protos_defined remains -1, no -P was provided, sowe avoid
> + attempting to read from that int array later */
> + int protos_defined = -1;
> +
> + int o, i;
Style bugs above. We don't write comments using this sort of
style and we don't use so large comments explaining the internal
meaning of a variable near its definition. Explaining the
semantics of protos_defined near the place where it is being used
will be much more useful IMHO.
> - if (!opt_4 && !opt_6 && !opt_u)
> - opt_4 = opt_6 = opt_u = 1;
> +
> + if (!opt_4 && !opt_6 && !opt_u && protos_defined == -1) {
> + opt_u = 1;
> + /* show tcp, udp and divert by default if no -P was
> specified */
> + /* restore protos_defined to the number of default protocols
> */
> + protos_defined = set_default_protos();
> + }
Style bugs above too. The comments are too long, wrapping in an
80-column terminal and they seem useless near this place. This
has been changed in my attached patch to:
% - if (!opt_4 && !opt_6 && !opt_u)
% - opt_4 = opt_6 = opt_u = 1;
% + /*
% + * If protos_defined remains -1, no -P was provided, so we have to
% + * set up the default protocol list in protos[] first.
% + */
% + if (!opt_4 && !opt_6 && !opt_u && protos_defined == -1) {
% + opt_u = 1;
% + protos_defined = set_default_protos();
% + }
% +
> if (opt_4 || opt_6) {
> - gather_inet(IPPROTO_TCP);
> - gather_inet(IPPROTO_UDP);
> - gather_inet(IPPROTO_DIVERT);
> + for(i=0; i < protos_defined; i++)
> + gather_inet(protos[i]);
> }
> - if (opt_u) {
> +
> + if ( opt_u || (protos_defined == -1 && !opt_4 && !opt_6)) {
> gather_unix(SOCK_STREAM);
> gather_unix(SOCK_DGRAM);
> }
Only whitespace style bugs above. These are fixed in the
attached version of the patch.
On 2006-11-10 11:15, Josh Carroll <josh.carroll at gmail.com> wrote:
>> Below is the patch using strsep instead. I also updated the
>> gather_inet function to print a message that a protocol is not
>> supported if it exists in /etc/protocols, but does not have a case in
>> the gather_inet function.
>
> So, anyone interested in committing this? :) Do people think this
> would be useful going forward? Or does anyone else have suggestions
> for the code?
There are a few nits, like the ones I described above, which I'd
like to see fixed before I suggest that this is committed. All
in all, this is a good feature patch though, and I've just tested
it on CURRENT. It runs quite nicely...
% $ pwd
% /home/keramida/hg/freebsd-7.0-keramida/usr.bin/sockstat
%
% $ ./sockstat -P udp
% USER COMMAND PID FD PROTO LOCAL ADDRESS FOREIGN ADDRESS
% root cupsd 935 5 udp4 *:631 *:*
% root ntpd 877 4 udp4 *:123 *:*
% root ntpd 877 5 udp6 *:123 *:*
% root ntpd 877 6 udp6 ::1:123 *:*
% root ntpd 877 7 udp4 127.0.0.1:123 *:*
% bind named 797 20 udp4 127.0.0.1:53 *:*
% bind named 797 22 udp4 *:61747 *:*
% root syslogd 735 8 udp6 *:514 *:*
% root syslogd 735 9 udp4 *:514 *:*
%
% $ ./sockstat -P udp,,
% USER COMMAND PID FD PROTO LOCAL ADDRESS FOREIGN ADDRESS
% root cupsd 935 5 udp4 *:631 *:*
% root ntpd 877 4 udp4 *:123 *:*
% root ntpd 877 5 udp6 *:123 *:*
% root ntpd 877 6 udp6 ::1:123 *:*
% root ntpd 877 7 udp4 127.0.0.1:123 *:*
% bind named 797 20 udp4 127.0.0.1:53 *:*
% bind named 797 22 udp4 *:61747 *:*
% root syslogd 735 8 udp6 *:514 *:*
% root syslogd 735 9 udp4 *:514 *:*
%
% $ ./sockstat -P ,,tcp
% USER COMMAND PID FD PROTO LOCAL ADDRESS FOREIGN ADDRESS
% keramida firefox-bi 3508 9 tcp4 192.168.99.185:65501 10.6.0.131:80
% keramida irssi 3276 3 tcp4 192.168.99.185:65165 194.159.164.195:6667
% root Xorg 1118 1 tcp6 *:6000 *:*
% root Xorg 1118 3 tcp4 *:6000 *:*
% root sshd 961 3 tcp4 *:22 *:*
% root cupsd 935 2 tcp6 ::1:631 *:*
% root cupsd 935 3 tcp4 127.0.0.1:631 *:*
% root sendmail 903 4 tcp4 127.0.0.1:25 *:*
% bind named 797 21 tcp4 127.0.0.1:53 *:*
% bind named 797 23 tcp4 127.0.0.1:953 *:*
%
% $ ./sockstat -P ,,tcp,udp
% USER COMMAND PID FD PROTO LOCAL ADDRESS FOREIGN ADDRESS
% keramida firefox-bi 3508 9 tcp4 192.168.99.185:65501 10.6.0.131:80
% keramida irssi 3276 3 tcp4 192.168.99.185:65165 194.159.164.195:6667
% root Xorg 1118 1 tcp6 *:6000 *:*
% root Xorg 1118 3 tcp4 *:6000 *:*
% root sshd 961 3 tcp4 *:22 *:*
% root cupsd 935 2 tcp6 ::1:631 *:*
% root cupsd 935 3 tcp4 127.0.0.1:631 *:*
% root cupsd 935 5 udp4 *:631 *:*
% root sendmail 903 4 tcp4 127.0.0.1:25 *:*
% root ntpd 877 4 udp4 *:123 *:*
% root ntpd 877 5 udp6 *:123 *:*
% root ntpd 877 6 udp6 ::1:123 *:*
% root ntpd 877 7 udp4 127.0.0.1:123 *:*
% bind named 797 20 udp4 127.0.0.1:53 *:*
% bind named 797 21 tcp4 127.0.0.1:53 *:*
% bind named 797 22 udp4 *:61747 *:*
% bind named 797 23 tcp4 127.0.0.1:953 *:*
% root syslogd 735 8 udp6 *:514 *:*
% root syslogd 735 9 udp4 *:514 *:*
Can you go through the attached, modified version and see if this
is better in any way than the original patch? If you like the
changes, we can probably ask Brooks for help in getting this
committed. Then we can work a bit on making the protos[] array
dynamically sized, without having to iterate through the entire
list of getprotoent() protocols, and we're done I think :-)
-------------- next part --------------
Add support for filtering sockets by protocol type. The default
behavior of sockstat(1) will still be to show "udp", "tcp" and
"divert" protocols, but we can now provide a (comma-separated)
list of protocols, as in:
% sockstat -P tcp
to list only TCP sockets, or we can filter more than one protocol
by separating the protocol names with a comma:
% sockstat -P tcp,udp
Protocol names are parsed with getprotobyname(3), so any protocol
whose name is listed in `/etc/protocols' should work fine.
Submitted by: Josh Carroll <josh.carroll at psualum.com>
diff --git a/usr.bin/sockstat/sockstat.c b/usr.bin/sockstat/sockstat.c
--- a/usr.bin/sockstat/sockstat.c
+++ b/usr.bin/sockstat/sockstat.c
@@ -66,6 +66,16 @@ static int opt_u; /* Show Unix domain
static int opt_u; /* Show Unix domain sockets */
static int opt_v; /* Verbose mode */
+/*
+ * Default protocols to use if no -P was defined.
+ */
+static const char *default_protos[] = {"tcp", "udp", "divert" };
+static size_t default_numprotos =
+ sizeof(default_protos) / sizeof(default_protos[0]);
+
+static int *protos; /* protocols to use */
+static size_t numprotos; /* allocated size of protos[] */
+
static int *ports;
#define INT_BIT (sizeof(int)*CHAR_BIT)
@@ -103,6 +113,66 @@ xprintf(const char *fmt, ...)
err(1, "printf()");
return (len);
}
+
+
+static int
+get_proto_type(const char *proto)
+{
+ struct protoent *pent;
+
+ if (strlen(proto) == 0)
+ return 0;
+ pent = getprotobyname(proto);
+ if (pent == NULL) {
+ warn("getprotobyname");
+ return -1;
+ }
+ return pent->p_proto;
+}
+
+
+static void init_protos(int num)
+{
+ int proto_count = 0;
+
+ if (num > 0) {
+ proto_count = num;
+ } else {
+ /* Find the maximum number of possible protocols. */
+ while (getprotoent() != NULL)
+ proto_count++;
+ endprotoent();
+ }
+
+ if ((protos = malloc(sizeof(int) * proto_count)) == NULL)
+ err(1, "malloc");
+ numprotos = proto_count;
+}
+
+
+static int
+parse_protos(char *protospec)
+{
+ char *prot;
+ char *tmp = protospec;
+ int proto_type, proto_index;
+
+ if (protospec == NULL)
+ return -1;
+
+ init_protos(0);
+ proto_index = 0;
+ while ((prot = strsep(&tmp, ",")) != NULL) {
+ if (strlen(prot) == 0)
+ continue;
+ proto_type = get_proto_type(prot);
+ if (proto_type != -1)
+ protos[proto_index++] = proto_type;
+ }
+ numprotos = proto_index;
+ return proto_index;
+}
+
static void
parse_ports(const char *portspec)
@@ -209,7 +279,7 @@ gather_inet(int proto)
protoname = "div";
break;
default:
- abort();
+ errx(1, "protocol %d not supported", proto);
}
buf = NULL;
@@ -264,7 +334,7 @@ gather_inet(int proto)
so = &xip->xi_socket;
break;
default:
- abort();
+ errx(1, "protocol %d not supported", proto);
}
if ((inp->inp_vflag & vflag) == 0)
continue;
@@ -573,19 +643,40 @@ display(void)
}
}
+static int set_default_protos(void)
+{
+ struct protoent *prot;
+ const char *pname;
+ size_t pindex;
+
+ init_protos(default_numprotos);
+
+ for (pindex = 0; pindex < default_numprotos; pindex++) {
+ pname = default_protos[pindex];
+ prot = getprotobyname(pname);
+ if (prot == NULL)
+ err(1, "getprotobyname: %s", pname);
+ protos[pindex] = prot->p_proto;
+ }
+ numprotos = pindex;
+ return pindex;
+}
+
+
static void
usage(void)
{
- fprintf(stderr, "Usage: sockstat [-46clu] [-p ports]\n");
+ fprintf(stderr, "Usage: sockstat [-46clu] [-p ports] [-P protos]\n");
exit(1);
}
int
main(int argc, char *argv[])
{
- int o;
-
- while ((o = getopt(argc, argv, "46clp:uv")) != -1)
+ int protos_defined = -1;
+ int o, i;
+
+ while ((o = getopt(argc, argv, "46clp:P:uv")) != -1)
switch (o) {
case '4':
opt_4 = 1;
@@ -602,6 +693,9 @@ main(int argc, char *argv[])
case 'p':
parse_ports(optarg);
break;
+ case 'P':
+ protos_defined = parse_protos(optarg);
+ break;
case 'u':
opt_u = 1;
break;
@@ -618,22 +712,30 @@ main(int argc, char *argv[])
if (argc > 0)
usage();
- if (!opt_4 && !opt_6 && !opt_u)
- opt_4 = opt_6 = opt_u = 1;
+ /*
+ * If protos_defined remains -1, no -P was provided, so we have to
+ * set up the default protocol list in protos[] first.
+ */
+ if (!opt_4 && !opt_6 && !opt_u && protos_defined == -1) {
+ opt_u = 1;
+ protos_defined = set_default_protos();
+ }
+
+ if (!opt_4 && !opt_6)
+ opt_4 = opt_6 = 1;
if (!opt_c && !opt_l)
opt_c = opt_l = 1;
if (opt_4 || opt_6) {
- gather_inet(IPPROTO_TCP);
- gather_inet(IPPROTO_UDP);
- gather_inet(IPPROTO_DIVERT);
- }
- if (opt_u) {
+ for (i = 0; i < protos_defined; i++)
+ gather_inet(protos[i]);
+ }
+
+ if (opt_u || (protos_defined == -1 && !opt_4 && !opt_6)) {
gather_unix(SOCK_STREAM);
gather_unix(SOCK_DGRAM);
}
getfiles();
display();
-
exit(0);
}
More information about the freebsd-hackers
mailing list