[Toybox] [CLEANUP] More ifconfig cleanup.

[Rob Landley]

Helping my friend Adrienne pack out her apartment today, but it's still  
more free time than a weekend with the nieces and nephews.

Commit 869 (http://landley.net/hg/toybox/rev/869):

Just moving code around, no actual code changes. Reorder functions to  
eliminate the need for function prototypes, and put the command_main()  
function at the end of the file. (That way it's easy to spot.)

In general, C code ordered to avoid function prototypes reads from the  
bottom up. The main function is at the end, and each other function is  
after the code it uses.

Commit 870 (http://landley.net/hg/toybox/rev/870):

Looking at the code for a rough edge, I immediately spot this:

-struct iface_list {
-...
-  unsigned long long   receive_bytes; //total bytes received
-  unsigned long long   receive_packets; //total packets received
-  unsigned long     receive_errors; //bad packets received
-  unsigned long     receive_drop; //no space in linux buffers
-  unsigned long     receive_fifo; //receiver fifo overrun
-  unsigned long     receive_frame; //received frame alignment error
-  unsigned long     receive_compressed;
-  unsigned long     receive_multicast; //multicast packets received
-
-  unsigned long long   transmit_bytes; //total bytes transmitted
-  unsigned long long   transmit_packets; //total packets transmitted
-  unsigned long     transmit_errors; //packet transmit problems
-  unsigned long     transmit_drop; //no space available in linux
-  unsigned long     transmit_fifo;
-  unsigned long     transmit_colls;
-  unsigned long     transmit_carrier;
-  unsigned long     transmit_compressed; //num_tr_compressed;
-...
-}

The first thing to do with that is collate the declarations. We don't  
care about allocation order here because it's not an API, so:

+  unsigned long long r_bytes, r_packets, t_bytes, t_packets;
+  unsigned long r_errors, r_drop, r_fifo, r_frame, r_compressed,  
r_multicast;
+  unsigned long t_errors, t_drop, t_fifo, t_colls, t_carrier,  
t_compressed;

Yes I changed the names, because r_ and t_ provide all the information  
the longer prefixes did. That means we need to adjust the users, so the  
first one we find is in get_proc_info():

-  sscanf(buff, "%llu%llu%lu%lu%lu%lu%lu%lu%llu%llu%lu%lu%lu%lu%lu%lu",
-    &il->receive_bytes, &il->receive_packets, &il->receive_errors,
-    &il->receive_drop, &il->receive_fifo, &il->receive_frame,
-    &il->receive_compressed, &il->receive_multicast,  
&il->transmit_bytes,
-    &il->transmit_packets, &il->transmit_errors, &il->transmit_drop,
-    &il->transmit_fifo, &il->transmit_colls, &il->transmit_carrier,
-    &il->transmit_compressed);

And that... isn't right.

get_proc_info() has a giant sscanf reading in 16 values: can that just  
be an array of unsigned long long values? Yes, promoting 32 bit  
integers to 64 bit integers unnecessarily wastes some space, but  
consistency means the guts of get_proc_info() are something like:

     for (j=0; j<16 && !errno; j++) il->val[j] = strtoll(buf, &buf, 0);

Which can be trivially inlined at the only callsite in  
get_ifconfig_info, eliminating one function already. The potential  
_downside_ of this is we now have 16 magic values that aren't getting  
marshalled into named variables, and making an enum of the indexes to  
the array would be as ugly as what we're removing.

But glancing through the code to find consumers for the data, the only  
user seems to be display_ifconfig(). Can we clean that up in a way that  
avoids an enum?

Ok, first things first: let's go back adjust struct iface_list again to  
have an array of unsigned long long. While we're there, let's do a few  
more cleanups:

   - rename iface_list to if_list (global search and replace the name).

   - take the unnecessary ifr prefixes off of structure members (the  
names are
     always used relative to the structure, no need for prefix _within_  
the struct).

   - this reveals an ifaddr/ifraddr name conflict, they can't both be  
"addr". However,
     looking at users of the first one it should really be called  
hasaddr because it's
     0 or 1 depending on whether the interface has an address. (Can we  
do in-band
     signaling in sockaddr to determine that? No, because 0.0.0.0 is a  
valid address.
     Hmmm, come back to that...)

While we're up there, yank the IFF_PORTSEL stuff because we took that  
out last commit (it was obsolete).

Inline get_proc_info() into get_ifconfig_info(), and smooth that over,  
and let's cut and paste out the old sscanf stuff into a cheatsheat for  
fixing up display_ifconfig():

//      &il->r_bytes, &il->r_packets, &il->r_errors, &il->r_drop,  
&il->r_fifo,
//      &il->r_frame, &il->r_compressed, &il->r_multicast, &il->t_bytes,
//      &il->t_packets, &il->t_errors, &il->t_drop, &il->t_fifo,  
&il->t_colls,
//      &il->t_carrier, &il->t_compressed);

There are our 16 values, in order.

While we're at it, get_ifconfig_info() is only ever called from  
show_iface(), so inline _that_ and make another function go away. And  
we have xfopen() that handles the perror_exit() so never returns  
failure, and the caller never does anything after this returns except  
free memory and set the exit code, so make the function return void...  
the "return 1" can become another perror_exit(), and then the return 0  
goes away. Oh, and one more from readconf() but that can use xioctl()  
and perror_exit() for the socket failure (I should do an xsocket()),  
and then it returns void too and handles its own errors. (A nice thing  
about changing the return type to "void" is the compiler tells you when  
you miss a return of non-void type.)

And safe_strncpy() can be replaced by xstrcpy() out of lib/lib.c. Note  
that this is a behavior change: instead of silently truncating strings  
and operating on the wrong date, we error_exit() when asked to perform  
a strcpy we can't do. (Instead of silently malfunctioning, fail  
immediately and loudly.)

display_ifconfig():

(This function is not the same as show_iface()? Um, gotta read through  
more of that at some point, but I'm stil on the "local transforms that  
should do equivalent thing" stage of simplification, not yet to the  
"holistic redesign" stage...)

Sigh. Next commit is going to have to be an inline-fest. Each of the  
first few functions in this (get_hw_info, print_hw_addr, print_ip_addr,  
print_ip6_addr...) they're all called exactly once, from here. So the  
body of those functions might as well go inline here, and then we can  
see what opportunities for simplification that affords us. But that's  
next time, I'm trying to keep these cleanups reasonably sized...

Alright, it looks like the body of if (non_virtual_interface) contains  
all the transmitted/received print logic. It's a bunch of explicit  
xprintf() statements with lots of arguments, which I'd really like to  
replace with table-driven logic.

It prints out entries 1, 2, 3, 4, and 5 from the array, each with  
appropriate "label:" strings. The next line is entries 9, 10, 11, 12,  
and... 14. Ok, not quite consistent. Then it goes back and prints 13 on  
the next line, Then it _might_ print out entry 6 ("compressed:") except  
I have no idea what that is. The comment says "Dummy types for non ARP  
hardware" which means what, exactly? (The host's ifconfig isn't  
printing this out for any interface type I have...) And then it prints  
txqueuelen, which isn't in this array but elsewhere in the if_list  
struct (and there's no corresponding rxqueuelen anywhere). And then the  
last line is entries 0 and 8.

Type 7, multicast, is never printed out. There's no code in ifconfig.c  
to print it out. I'm fine with this: multicast was a bad idea that went  
away around 2000. If netflix, youtube, itunes, and all the random  
podcasts out there aren't using it, it will never matter. Even  
bittorrent isn't using it. The internet is neither television nor  
radio, catching a broadcast as it goes by is not the model here. People  
thought "push" made sense back in 1997, but if you push to a phone  
that's asleep and you wake it up and make it waste battery, people get  
annoyed. (Instead you send a request, you get a response. That's the  
model, and it has no place for multicast. If you want to distribute and  
fan out, you use gnutella or bittorrent or akamai or...)

Next question: does "compressed" matter more than "multicast"? Dunno.  
It's only printed out for CSLIP types, but I can dig into the kernel  
source to find out what this CSLIP thing is...

   http://tools.ietf.org/html/rfc1144

It's a serial header compression technology from 1990 that's never been  
updated once in the past 23 years. Uh-huh. Well, the kernel hasn't  
removed support for it, but I'm not sure specifically reporting extra  
data for it is all that interesting. In fact, I think I'm going to drop  
that bit because I really don't care.

I think I'll make an array indicating what order to print each entry  
in, because it's too crazy to try to control it with if() statements.  
We can make the order array "signed char" because the 60 or so wasted  
bytes from making it int[] is probably more than the mask/shift code  
arm will add to read it, especially if we assign it into a local int  
variable and only read it once at the start of the loop. Making it  
signed means we can include -1 entries in the order to show where to  
stick a newline+indent, with one special case value to print txqueuelen  
instead of an entry out of the table.

(I tried ending with -1, but the last line after the table isn't always  
printed, and I don't like ending with a lot of trailing whitespace. So  
put an xputc('\n') after the table-driven bits.)

Altogether, we wind up with:

   if(il->non_virtual_iface) {
     char *label[] = {"RX bytes", "RX packets", "errors", "dropped",  
"overruns",
       "frame", 0, 0, "TX bytes", "TX packets", "errors", "dropped",  
"overruns",
       "collisions", "carrier", 0, "txqueuelen"};
     signed char order[] = {-1, 1, 2, 3, 4, 5, -1, 9, 10, 11, 12, 14,  
-1,
       13, 16, -1, 0, 8};
     int i;

     for (i = 0; i < sizeof(order); i++) {
       int j = order[i];

       if (j < 0) xprintf("\n%10c", ' ');
       else xprintf("%s:%llu ", label[j],
         j==16 ? (unsigned long long)il->txqueuelen : il->val[j]);
     }
   }
   xputc('\n');

I can probably simplify that further (bytes, packets, errors, dropped,  
overruns: that's five duplicated strings, and it's the first five  
strings of j&7, then RX/TX is inherent j&8 and is printed out at the  
start of each line. The 4 non-repeated strings are positions 5, 13, 14,  
16, the first three of which are 0-3 with j/7 and that last one doesn't  
_have_ to be 16 in "order", it could be 21, and then I could have two  
string tables with the 5 repeated strings and the 4 non-repeated  
ones...)

But it's a good enough stopping place for right now, and I have to stop  
and think if I want to be _that_ clever. (That's spending complexity to  
reduce size. I think it's ok in this instance, but it's the kind of  
thing I prefer to sleep on first.)

And notice: we're now under 1000 lines! (That's 956 lines total. We  
started somewhere over 1500. And we're nowhere near done cleaning up  
this file yet...)

Rob
 1366515612.0