On Tue, Jul 22, 2008 at 10:39:38PM -0400, L. Gabriel Somlo wrote:
>On Tue, Jul 22, 2008 at 11:00:57PM +0200, Denys Vlasenko wrote:
>> Can you do it?
>
>Allright, how about this one ?
bloat-o-meter output? size(1) before and after your patch, with and
without -p support?
>diff -NarU5 busybox-svn-22735/networking/netstat.c
>busybox-svn-22735-netstatp/networking/netstat.c
>--- busybox-svn-22735/networking/netstat.c 2008-07-09 23:04:03.000000000
>-0400
>+++ busybox-svn-22735-netstatp/networking/netstat.c 2008-07-22
>22:32:52.000000000 -0400
>+#define PROGNAME_WIDTH 20
>+#define PROGNAME_WIDTHs "%-20s"
Just curious why you don't use PROGNAME_WIDTH here?
>+static void prg_cache_add(int inode, char *name)
>+{
>+ unsigned hi = PRG_HASHIT(inode);
>+ struct prg_node **pnp, *pn;
>+
>+ prg_cache_loaded = 2;
>+ for (pnp = prg_hash + hi; (pn = *pnp); pnp = &pn->next) {
>+ if (pn->inode == inode) {
>+ /* Some warning should be appropriate here
>+ as we got multiple processes for one i-node */
>+ return;
>+ }
>+ }
>+ *pnp = xmalloc(sizeof(struct prg_node));
>+ pn = *pnp;
>+ pn->next = NULL;
an xzalloc would have rendered nullifying next moot.
>+ pn->inode = inode;
>+ safe_strncpy(pn->name, name, PROGNAME_WIDTH);
>+}
>+
>+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.
>+static void prg_cache_load(void)
>+{
>+ char line[LINE_MAX], eacces = 0;
>+ int procfdlen, fd, cmdllen, lnamelen;
>+ char lname[30], cmdlbuf[512], finbuf[PROGNAME_WIDTH];
>+ long inode;
>+ const char *cs, *cmdlp;
>+ DIR *dir_proc = NULL, *dir_fd = NULL;
>+ struct dirent *pde, *fde;
>+
>+ if (prg_cache_loaded)
>+ return;
>+ prg_cache_loaded = 1;
>+
>+ cmdlbuf[sizeof(cmdlbuf) - 1] = '\0';
>+
>+ dir_proc = opendir(PATH_PROC);
nah. Use recursive_action() instead of growing your own (huge!) impl
for it.
>+ if (!dir_proc)
>+ goto fail;
>+ while ((pde = readdir(dir_proc))) {
>+#endif //ENABLE_FEATURE_NETSTAT_PRG
>
> #if ENABLE_FEATURE_IPV6
> static void build_ipv6_addr(char* local_addr, struct sockaddr_in6* localaddr)
> {
> char addr6[INET6_ADDRSTRLEN];
>@@ -193,10 +442,15 @@
> char *r = ip_port_str(
> (struct sockaddr *) &remaddr, rem_port,
> "tcp", flags & NETSTAT_NUMERIC);
> printf(net_conn_line,
> "tcp", rxq, txq, l, r, tcp_state[state]);
The preferred way look like:
->+#if ENABLE_FEATURE_NETSTAT_PRG
->+ if (prg_flag)
+>+ if (ENABLE_FEATURE_NETSTAT_PRG && prg_flag)
>+ printf(PROGNAME_WIDTHs, prg_cache_get(inode));
>+#endif
>+ printf("\n");
Is that \n here on purpose?
> free(l);
> free(r);
> }
> return 0;
> }
>@@ -274,10 +528,15 @@
> char *r = ip_port_str(
> (struct sockaddr *) &remaddr, rem_port,
> "udp", flags & NETSTAT_NUMERIC);
> printf(net_conn_line,
> "udp", rxq, txq, l, r, state_str);
>+#if ENABLE_FEATURE_NETSTAT_PRG
>+ if (prg_flag)
>+ printf(PROGNAME_WIDTHs, prg_cache_get(inode));
>+#endif
>+ printf("\n");
ditto, twice.
> free(l);
> free(r);
> }
> }
> return 0;
>@@ -330,10 +589,15 @@
> char *r = ip_port_str(
> (struct sockaddr *) &remaddr, rem_port,
> "raw", flags & NETSTAT_NUMERIC);
> printf(net_conn_line,
> "raw", rxq, txq, l, r, itoa(state));
>+#if ENABLE_FEATURE_NETSTAT_PRG
>+ if (prg_flag)
>+ printf(PROGNAME_WIDTHs, prg_cache_get(inode));
>+#endif
>+ printf("\n");
ditto, twice.
> free(l);
> free(r);
> }
> }
> return 0;
>@@ -441,10 +705,15 @@
>
> printf("%-5s %-6ld %-11s %-10s %-13s %6lu ",
> ss_proto, refcnt, ss_flags, ss_type, ss_state, inode
> );
>
>+#if ENABLE_FEATURE_NETSTAT_PRG
>+ if (prg_flag)
>+ printf(PROGNAME_WIDTHs, prg_cache_get(inode));
>+#endif
ditto.
>+
> /* TODO: currently we stop at first NUL byte. Is it a problem? */
> line += path_ofs;
> *strchrnul(line, '\n') = '\0';
> while (*line)
> fputc_printable(*line++, stdout);
>@@ -495,10 +764,15 @@
> smallint inet6 = 1;
> #else
> enum { inet = 1, inet6 = 0 };
> #endif
>
>+ INIT_G();
>+#if ENABLE_FEATURE_NETSTAT_PRG
>+ INIT_PRG();
>+#endif
>+
> /* Option string must match NETSTAT_xxx constants */
> opt = getopt32(argv, NETSTAT_OPTS);
> if (opt & 0x1) { // -l
> flags &= ~NETSTAT_CONNECTED;
> flags |= NETSTAT_LISTENING;
>@@ -508,24 +782,25 @@
> if (opt & 0x8) flags |= NETSTAT_NUMERIC; // -n
> //if (opt & 0x10) // -t: NETSTAT_TCP
> //if (opt & 0x20) // -u: NETSTAT_UDP
> //if (opt & 0x40) // -w: NETSTAT_RAW
> //if (opt & 0x80) // -x: NETSTAT_UNIX
>- 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
> }
>
>- if (opt & OPT_widedisplay) { // -W
>+ if (opt & OPT_wide) { // -W
> net_conn_line = PRINT_NET_CONN_WIDE;
> net_conn_line_header = PRINT_NET_CONN_HEADER_WIDE;
> }
>
>+ if (opt & OPT_prg) { // -p
>+ prg_flag = 1;
>+ prg_cache_load();
>+ }
>+
> opt &= NETSTAT_ALLPROTO;
> if (opt) {
> flags &= ~NETSTAT_ALLPROTO;
> flags |= opt;
> }
>@@ -537,10 +812,14 @@
> else if (flags & NETSTAT_LISTENING)
> printf("(only servers)");
> else
> printf("(w/o servers)");
> printf(net_conn_line_header, "Local Address", "Foreign
> Address");
>+#if ENABLE_FEATURE_NETSTAT_PRG
>+ print_progname_banner();
>+#endif
>+ printf("\n");
Sounds a bit like you are too newline friendly ;)
> }
> if (inet && flags & NETSTAT_TCP)
> do_info(_PATH_PROCNET_TCP, "AF INET (tcp)", tcp_do_one);
> #if ENABLE_FEATURE_IPV6
> if (inet6 && flags & NETSTAT_TCP)
>@@ -564,10 +843,17 @@
> printf("(servers and established)");
> else if (flags & NETSTAT_LISTENING)
> printf("(only servers)");
> else
> printf("(w/o servers)");
>- printf("\nProto RefCnt Flags Type State
>I-Node Path\n");
>+ printf("\nProto RefCnt Flags Type State
>I-Node");
>+#if ENABLE_FEATURE_NETSTAT_PRG
>+ print_progname_banner();
>+#endif
>+ printf(" Path\n");
> do_info(_PATH_PROCNET_UNIX, "AF UNIX", unix_do_one);
> }
>+#if ENABLE_FEATURE_NETSTAT_PRG
if (ENABLE_FEATURE_CLEAN_UP)
>+ prg_cache_clear();
>+#endif
> return 0;
> }
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.
TIA,
Bernhard
_______________________________________________
busybox mailing list
[email protected]
http://busybox.net/cgi-bin/mailman/listinfo/busybox
