Date: Sat, 11 Nov 2006 16:48:50 +0100 From: Giorgos Keramidas <keramida@freebsd.org> To: Josh Carroll <josh.carroll@psualum.com> Cc: freebsd-hackers@freebsd.org Subject: Re: sockstat tcp/udp switches Message-ID: <20061111154849.GC1972@kobe.laptop> In-Reply-To: <8cb6106e0611101115k25df8cddu98c980481cd591f2@mail.gmail.com> <8cb6106e0611071051y6cbbca1padc24ba315d4a2b3@mail.gmail.com> References: <8cb6106e0611021834h17737556y4bb2fda39a4bfa0c@mail.gmail.com> <20061103024621.GB16445@kobe.laptop> <20061103024837.GB79357@lor.one-eyed-alien.net> <20061103025442.GB16543@kobe.laptop> <8cb6106e0611031550y1381b67agdc74144b89de763b@mail.gmail.com> <20061104062439.GD854@turion.vk2pj.dyndns.org> <8cb6106e0611061517k62c9193fnbbfc8e36db328282@mail.gmail.com> <20061107174151.GA51473@lor.one-eyed-alien.net> <8cb6106e0611071008y6811a79x9ba056c2be94d773@mail.gmail.com> <8cb6106e0611071051y6cbbca1padc24ba315d4a2b3@mail.gmail.com>
next in thread | previous in thread | raw e-mail | index | archive | help
--SkvwRMAIpAhPCcCJ Content-Type: text/plain; charset=us-ascii Content-Disposition: inline On 2006-11-07 10:51, Josh Carroll <josh.carroll@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@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 :-) --SkvwRMAIpAhPCcCJ Content-Type: text/plain; charset=us-ascii Content-Disposition: attachment; filename="sockstat-protocol.patch" 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@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); } --SkvwRMAIpAhPCcCJ--
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20061111154849.GC1972>