Skip site navigation (1)Skip section navigation (2)
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>