PATCH: netstat '-p' optional feature
Denys Vlasenko
vda.linux at googlemail.com
Sun Jul 27 12:07:29 UTC 2008
On Saturday 26 July 2008 00:20, L. Gabriel Somlo wrote:
> On Wed, Jul 23, 2008 at 09:38:41AM +0200, Bernhard Fischer wrote:
>
> > bloat-o-meter output? size(1) before and after your patch, with and
> > without -p support?
>
> The following numbers are for the newly reworked patch
> (please see attached).
>
> Before:
> $ size networking/netstat.o
> text data bss dec hex filename
> 3426 5 0 3431 d67 networking/netstat.o
>
> After, without -p support compiled in (but with the nice, fixed,
> standard options and globals code):
> $ size networking/netstat.o
> text data bss dec hex filename
> 3487 0 0 3487 d9f networking/netstat.o
>
>
> After, with -p support built in:
> $ size networking/netstat.o
> text data bss dec hex filename
> 4642 0 0 4642 1222 networking/netstat.o
>
>
> > >+#define PROGNAME_WIDTH 20
> > >+#define PROGNAME_WIDTHs "%-20s"
> >
> > Just curious why you don't use PROGNAME_WIDTH here?
>
> The original, which Denys didn't like on account of being too
> complicated, looked like this:
>
> #define PROGNAME_WIDTH 20
> #define PROGNAME_WIDTHs PROGNAME_WIDTH1(PROGNAME_WIDTH)
> #define PROGNAME_WIDTH1(s) PROGNAME_WIDTH2(s)
> #define PROGNAME_WIDTH2(s) #s
>
> This allowed PROGNAME_WIDTHs to be used inside a printf format string:
>
> printf("%-" PROGNAME_WIDTHs "s", some_string);
>
> You can't use PROGNAME_WIDTH directly, since that would substitute to:
>
> printf("%-" 20 "s", some_string);
>
> which is a syntax error. Also,
>
> printf("%-PROGNAME_WIDTHs", some_string);
>
> is obviously broken, too.
>
> You can't
>
> #define PROGNAME_WIDTHs "PROGNAME_WIDTH"
>
> either, because you'd end up with the literal string instead of the
> desired \"20\"
>
> I'm not an expert on C preprocessor voodoo by any means, but the
> original form which Denys didn't like did accomplish creating a preproc.
> variable PROGNAME_WIDTHs which automatically takes the string form of
> whatever PROGNAME_WIDTH is set to ("20" for 20, "30" for 30, etc).
>
> The (more readable) alternative is to have to modify both by hand. If
> you know how to automatically get PROGNAME_WIDTHs to adjust when
> modifying PROGNAME_WIDTH, any simpler than the WIDTH1() WIDTH2() trick
> above, let me know... Or, how to jam PROGNAME_WIDTH into a printf
> format string directly -- I tried and it didn't get substituted :(
>
> >
> > >+static void prg_cache_add(int inode, char *name)
> ...
> > >+ }
> > >+ *pnp = xmalloc(sizeof(struct prg_node));
> > >+ pn = *pnp;
> > >+ pn->next = NULL;
> >
> > an xzalloc would have rendered nullifying next moot.
>
> Done.
>
> > >+static const char *prg_cache_get(int inode)
> > >+{
> > >+ unsigned hi = PRG_HASHIT(inode);
> > >+ struct prg_node *pn;
> > >+
> > >+ for (pn = prg_hash[hi]; pn; pn = pn->next)
> > >+ if (pn->inode == inode)
> > >+ return pn->name;
> >
> > argh, that's exactly what index_in_strings should have been used for.
>
> Not sure I get that -- index_in_strings searches a contiguous
> concatenation of null-terminated strings; netstat has a hash array
> with linked lists hanging off each entry (and the list elements have a
> string *and* an int member)...
>
> > nah. Use recursive_action() instead of growing your own (huge!) impl
> > for it.
>
> Done. However, this requires the addition of an ACTION_QUIET flag to
> recursive_action() (also included in the attached patch). We do NOT
> want recursive_action() to print to stderr each time it gets a
> permission error when trying to open /proc entries, should the user
> run netstat as non-root !!!
>
> > The preferred way look like:
> >
> > ->+#if ENABLE_FEATURE_NETSTAT_PRG
> > ->+ if (prg_flag)
> > +>+ if (ENABLE_FEATURE_NETSTAT_PRG && prg_flag)
>
> Respectfully disagree. If ENABLE_FEATURE_NETSTAT_PRG is not enabled,
> prg_flag is undeclared, and we get a compile-time error.
> Am I missing anything ?
>
> > >+ printf(PROGNAME_WIDTHs, prg_cache_get(inode));
> > >+#endif
> > >+ printf("\n");
> >
> > Is that \n here on purpose?
>
> Yes. I want to print the string "foo bar\n" with the following
> restrictions:
>
> if ENABLE_FEATURE_NETSTAT_PRG is undefined, I only want "foo\n",
> always.
>
> if ENABLE_FEATURE_NETSTAT_PRG is true, I want "foo bar\n" if the -p flag
> was supplied on the command line, otherwise I still only want "foo\n".
>
> My solution to this was to first print "foo" without \n
> then, #if ENABLE_FEATURE_NETSTAT_PRG to check for prg_flag, and if
> true to print "bar"
> then, last thing, I always print "\n".
>
> You don't *always* want to print the program name and pid if
> ENABLE_FEATURE_NETSTAT_PRG is true, so just ifdef-ing the format
> string from the get-go isn't a solution.
>
> >
> > Sounds a bit like you are too newline friendly ;)
>
> Maybe, and maybe I've been staring at this thing for too long and the
> obvious simple and clean solution escapes me... How would you do it ? :)
>
> > This sounds way too big, please provide the bloat-o-meter and size(1)
> > output i mentioned above, and please take my comments into account.
>
> Done -- please let me know your thoughts !
#define PROGNAME_BANNER "PID/Program name"
#define print_progname_banner() do { if (prg_flag) printf(PROGNAME_WIDTHs," " PROGNAME_BANNER); } while (0)
Why do you need a define which you use just once,
two lines below its definition?
#define PRG_SOCKET_PFX "socket:["
#define PRG_SOCKET_PFXl (sizeof(PRG_SOCKET_PFX)-1)
#define PRG_SOCKET_PFX2 "[0000]:"
#define PRG_SOCKET_PFX2l (sizeof(PRG_SOCKET_PFX2)-1)
Similar situation - you use these only in one not-so-bug function, yet
definition is moved so far from it that you need to excessively
comment that function in order to explain what you are doing there.
You can just move them closer:
#define PRG_SOCKET_PFX "socket:["
#define PRG_SOCKET_PFXl (sizeof(PRG_SOCKET_PFX)-1)
#define PRG_SOCKET_PFX2 "[0000]:"
#define PRG_SOCKET_PFX2l (sizeof(PRG_SOCKET_PFX2)-1)
static long extract_socket_inode(const char *lname) {
size_t llen = strlen(lname);
long inode = -1;
if (llen >= PRG_SOCKET_PFXl + 3
&& memcmp(lname, PRG_SOCKET_PFX, PRG_SOCKET_PFXl) == 0
&& lname[llen - 1] == ']'
) {
/* If lname is of the form "socket:[12345]",
* extract the "12345" as inode.
*/
char inode_str[llen + 1]; /* e.g. "12345" */
const int inode_str_len = llen - PRG_SOCKET_PFXl - 1;
strncpy(inode_str, lname + PRG_SOCKET_PFXl, inode_str_len);
inode_str[inode_str_len] = '\0';
inode = bb_strtol(inode_str, NULL, 0);
} else if (llen >= PRG_SOCKET_PFX2l + 1
&& memcmp(lname, PRG_SOCKET_PFX2, PRG_SOCKET_PFX2l) == 0
) {
/* If lname is of the form "[0000]:12345",
* extract the "12345" as inode.
*/
inode = bb_strtol(lname + PRG_SOCKET_PFX2l, NULL, 0);
}
if (errno) /* bb_strtol() encountered an error */
inode = -1;
return inode;
}
Even better, rewrite it like this (no #defines at all):
static long extract_socket_inode(const char *lname) {
long inode = -1;
if (strncmp(lname, "socket:[", sizeof("socket:[")-1) == 0) {
/* "socket:[12345]", extract the "12345" as inode */
inode = bb_strtol(lname + sizeof("socket:[")-1, &lname, 0);
if (*lname != ']')
inode = -1;
} else if (strncmp(lname, "[0000]:", sizeof("[0000]:")-1) == 0) {
/* "[0000]:12345", extract the "12345" as inode */
inode = bb_strtol(lname + sizeof("[0000]:")-1, NULL, 0);
if (errno) /* not NUL terminated? */
inode = -1;
}
#if 0 /* bb_strtol returns all-ones bit pattern on ERANGE anyway */
if (errno == ERANGE)
inode = -1;
#endif
return inode;
}
Why you use int for inode here? You were using long...
int i;
link = xmalloc_readlink(fileName);
if (link != NULL) {
i = extract_socket_inode(link);
free(link);
Why do you test "if (prg_cache_loaded)"? You _do_ know it is not loaded,
because you call it just once from main:
static void prg_cache_load(void)
{
int load_ok;
if (prg_cache_loaded) ///////// ??!
return;
...
if (opt & OPT_prg) { // -p
prg_flag = 1;
prg_cache_load();
}
This is way too verbose:
if (prg_cache_loaded == 1)
fprintf(stderr, "Can't(No info could be read for \"-p\": geteuid()=%d "
"but you should be root.)\n", geteuid());
else
fprintf(stderr, "(Not all processes could be identified, "
"non-owned process info will not be shown, "
"you would have to be root to see it all.)\n");
How about this?
if (prg_cache_loaded == 1)
bb_error_msg("can't scan /proc - are you root?");
else
bb_error_msg("showing only processes with your user ID");
Please use CONFIG_WERROR=y, because:
networking/netstat.c:262: error: assignment discards qualifiers from pointer target type:
for (p = bb_basename(fileName); *p; p++)
networking/netstat.c:253: error: unused variable 'progname_buf':
char cmdline_buf[512], progname_buf[PROGNAME_WIDTH];
networking/netstat.c:758: error: assignment discards qualifiers from pointer target type:
net_conn_line = PRINT_NET_CONN_WIDE;
etc...
You need to check only first char of proc name here:
if (depth == 1)
for (p = bb_basename(fileName); *p; p++)
if (!isdigit(*p))
return SKIP; /* skip any /proc subdir that's not a process */
One more time: you can use (option_mask32 & OPT_prg) instead of having yet another
variable (prg_flag) for it:
if (opt & OPT_prg) { // -p
prg_flag = 1;
prg_cache_load();
}
You deleted "#if ENABLE_ROUTE". Now it does not compile if route applet
is not selected:
- if (opt & OPT_showroute) { // -r
-#if ENABLE_ROUTE
+ if (opt & OPT_route) { // -r
bb_displayroutes(flags & NETSTAT_NUMERIC, !(opt & OPT_extended));
return 0;
-#else
- bb_show_usage();
-#endif
CC networking/netstat.o
cc1: warnings being treated as errors
networking/netstat.c: In function 'netstat_main':
networking/netstat.c:748: error: implicit declaration of function 'bb_displayroutes'
make[1]: *** [networking/netstat.o] Error 1
make: *** [networking] Error 2
See attached patch where these (and some more) are fixed.
Without FEATURE_NETSTAT_PRG:
function old new delta
recursive_action 416 425 +9
tcp_do_one 420 428 +8
udp_do_one 492 499 +7
raw_do_one 472 479 +7
expand 1697 1701 +4
netstat_main 489 492 +3
unix_do_one 486 488 +2
flags 1 - -1
qgravechar 109 106 -3
net_conn_line 4 - -4
bbunpack 391 383 -8
------------------------------------------------------------------------------
(add/remove: 0/2 grow/shrink: 7/2 up/down: 40/-16) Total: 24 bytes
With FEATURE_NETSTAT_PRG:
file_act - 213 +213
dir_act - 192 +192
netstat_main 489 601 +112
prg_cache_get - 50 +50
tcp_do_one 420 462 +42
udp_do_one 492 533 +41
raw_do_one 472 513 +41
unix_do_one 486 519 +33
recursive_action 416 425 +9
expand 1697 1701 +4
flags 1 - -1
qgravechar 109 106 -3
net_conn_line 4 - -4
bbunpack 391 383 -8
packed_usage 24586 24572 -14
------------------------------------------------------------------------------
(add/remove: 3/2 grow/shrink: 7/3 up/down: 737/-30) Total: 707 bytes
--
vda
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 1.patch
Type: text/x-diff
Size: 14360 bytes
Desc: not available
Url : http://lists.busybox.net/pipermail/busybox/attachments/20080727/2493c73f/attachment.bin
More information about the busybox
mailing list