[Toybox] [CLEANUP] ifconfig commit 907

[Rob Landley]

http://landley.net/hg/toybox/rev/907

--- show_ip_addr

The function show_ip_addr() is only ever called from display_ifconfig.  
It's called four times in a row, so it's better to have it be a  
function that repeated inline four times. However, it's even better to  
have it be the body of a loop that we iterate over four times, parsing  
a table of data. (Locality of reference makes code easier to read: a  
loop that does its work inline is all in front of you, a loop that  
calls a function has to marshal data into and out of the function and  
you have to go to another page of code to see what it's doing.)

The question is: does the overhead of the table outweigh the advantage  
of having the code moved inline at its only callsite? This is an  
apples/oranges comparison: making the code bigger vs making it easier  
to understand. However, each function call with multiple arguments has  
a certain amount of overhead, as does the extra function body itself,  
so if the table doesn't have more entries than the functino has  
arguments we're probably not actually losing space. (We could check  
with "make baseline" and "make bloatcheck" if we wanted to be sure, but  
with inling making the code easier to read, it seems worth it to me.)

The new table is:

+    struct {
+      char *name;
+      int flag, offset;
+    } addr[] = {
+      {"addr", 0, offsetof(struct if_list, addr)},
+      {"P-t-P", IFF_POINTOPOINT, offsetof(struct if_list, dstaddr)},
+      {"Bcast", IFF_BROADCAST, offsetof(struct if_list, broadaddr)},
+      {"Mask", 0, offsetof(struct if_list, netmask)}
+    };

And code showing the actual IPs becomes:

-    show_ip_addr("addr", &il->addr);
-    if(il->flags & IFF_POINTOPOINT) show_ip_addr("P-t-P",  
&il->dstaddr);
-    if(il->flags & IFF_BROADCAST) show_ip_addr("Bcast",  
&il->broadaddr);
-    show_ip_addr("Mask", &il->netmask);
+    for (i=0; i < sizeof(addr)/sizeof(*addr); i++) {
+      struct sockaddr_in *s = (struct sockaddr_in  
*)(addr[i].offset+(char *)il);
+
+      if (!addr[i].flag || (il->flags & addr[i].flag))
+        xprintf(" %s:%s ", addr[i].name,
+          (s->sin_family == 0xFFFF || !s->sin_family) ? "[NOT SET]" :
+            inet_ntoa(s->sin_addr));
+    }
+

This includes the inlined version of show_ip_addr(), so the function  
can go away.

While I'm there:

-    char *name = "unspec";

-    if (af == AF_INET) name = "inet";
-    else if (af == AF_INET6) name = "inet6";
-    xprintf("%10c%s", ' ', name);
+    xprintf("%10c%s", ' ',
+      (af == AF_INET) ? "inet" : (af == AF_INET6) ? "inet6" :  
"unspec");

It's really another way of saying the same thing, but in half as many  
lines. You can go overboard putting too much on one line (especially in  
languages like perl that pretty much start that way), but there's an  
advantage to being able to see more code on the screen at once. It's  
all tradeoffs...

--- hex_to_binary and set_hw_address

Scrolling over the function looking for the next bite-sized chunk, I  
see hex_to_binary(). Another function called from one place: in  
set_hw_address().

So it can be inlined, and also there are several library functions that  
can convert hex strings to integers, but let's work out what  
hex_to_binary() does:

Take an ascii representation of a big hex number in char *hw_addr, and  
write the data out into struct sockaddr *sock field sa_data. The data  
is written out big-endian, on all targets. (Is that right for  
little-endian x86? Testing... Yes, apparently so.) The length field is  
checked against ETH_ALEN and INFINIBAND_ALEN, but the only caller is  
doing:

   if(hex_to_binary(hw_addr, &sock, hw_class == 1 ? ETH_ALEN :  
INFINIBAND_ALEN))

So we're parsing/validating data we supplied. Sanity-checking yourself  
this way generally isn't useful, if the code's wrong fix it. If there's  
enough code you can't tell it's wrong or not, simplify.

The colons in the string we're parsing are optional, it accepts strings  
with an without them. Bit tricker to use standard functions then.  
Hmmm...

First, we delete the blank line before "char *ptr" and put it  
afterwards, to collate the local variable declarations into a space  
separated block at the top. (That's just basic code hygiene.) And a  
space after the address type checking block.

The address checking block can get moved into the caller, and all that  
setup code at the start of set_hw_address() simplified. The old code:

   char *hw_addr;
   struct sockaddr sock;
   char *ptr;
   char *hw_class_strings[] = {
       "ether",
       "infiniband",
       NULL
   };

   if(strcmp(hw_class_strings[0], **argv) == 0)
     hw_class = 1;
   else if(strcmp(hw_class_strings[1], **argv) == 0)
     hw_class = 2;
   if(!hw_class || !(*argv += 1)) {
     errno = EINVAL;
     toys.exithelp++;
     error_exit("bad hardware class");
   }

   memset(&sock, 0, sizeof(struct sockaddr));
   hw_addr = **argv;
   if(hex_to_binary(hw_addr, &sock, hw_class == 1 ? ETH_ALEN :  
INFINIBAND_ALEN))

becomes:

   char *hw_addr;
   struct sockaddr sock;

   memset(&sock, 0, sizeof(struct sockaddr));
   if (!strcmp("ether", **argv)) sock.sa_family = ARPHRD_ETHER;
   else if (!strcmp("infiniband", **argv)) sock.sa_family =  
ARPHRD_INFINIBAND;
   else {
     toys.exithelp++;
     error_exit("bad hw '%s'", **argv);
   }
   hw_addr = *(++*argv);

   if(hw_addr && hex_to_binary(hw_addr, &sock)

For once the table was overkill: there's just two entries and we didn't  
even loop over them. (By the way, you don't need a NULL terminator for  
an array, you can do sizeof(table)/sizeof(*table). Divide the size of  
the table (in bytes) by the size of a member of the table (in bytes) to  
get the number of entries.)

We also don't need the intermediate variable hw_class, since  
sock->sa_family holds the same info, and the "else" case naturally  
tells us we didn't match either of the first two comparisons. The  
hw_addr null check falls through to error_exit that does a printf("%s",  
hw_addr) but libc handles that case with a [NULL] indicator already...  
ah, but posix doesn't _require_ it to.

Grumble... Ok, add a ? : in the error_exit() for portability. (To what,  
I couldn't tell you...) Make *ptr go away since it's only used once on  
the line immediately following its assignment.

And while we're at it, this is wrong:

   memcpy( ((char *) ifre) + offsetof(struct ifreq, ifr_hwaddr),
     &sock, sizeof(struct sockaddr));

Why have a local struct sockaddr at all when there's already one in the  
ifre that's passed into us and we memcpy() this one over it? And why  
are we doing offsetof and adding when we can just take the address of  
ifre->ifr_hwaddr? Right, simplify _that_, sock is now a pointer to  
&ifre->ifr_hwaddr and no memcpy() necessary.

Anyway, back to hex_to_binary, it now looks like:

static int hex_to_binary(char *hw_addr, struct sockaddr *sock)
{
   int count = (sock->sa_family==ARPHRD_ETHER) ? 6 : 20;
   char *ptr = (char *) sock->sa_data, *p = ptr;

   while (*hw_addr && (p-ptr) < count) {
     int val, len = 0;

     if (*hw_addr == ':') hw_addr++;
     sscanf(hw_addr, "%2x%n", &val, &len);
     if (len != 2) break;
     hw_addr += len;
     *p++ = val;
   }

   return (p-ptr) != count || *hw_addr;
}

Toybox is built with -funsigned-char so we don't need to say "unsigned  
char".

Use sscanf() to parse pairs of hex digits, and consider not having  
parsed exactly two digits a failure. No need for an integer index to  
check loop length again, save the starting pointer and do pointer  
subtraction to get the length. (Note: output pointer, not input  
pointer. The colons don't cout.) And check at the end that we _did_  
grab exactly the right number of digits, and just use the actual  
numerical constants that can't change. (So we don't have to #define one  
of them up top to use it exactly once here... Is an infiniband address  
_really_ 40 hexadecimal digits?) The return value is just zero or  
nonzero, doesn't have to specifically be -1.

Now: inline hex_to_binary() at the one callsite. The variable  
declarations move up to the top but the initializations of them move  
down (default count to 6 and then assign to 20 in the if(infiniband)  
body, which now needs curly brackets; the rest of the initialization  
has to take place after "sock" is assigned).

Sanity check: sa_data is a member of a zeroed structure, which we're  
grabbing and using as a pointer. Confirm that it's an array, not a  
pointer, and thus doesn't need to be initialized... ok.

And of course the return value at the end becomes the if () that  
potentially calls error_exit().

Now, inline set_hw_address() in ifconfig_main(). Re-indent everything  
(this whole section is indented two off, I need to fix that later).  
Adjust ***argv to be just **argv (one less dereference level on each  
use), and ifre is a struct instead of a pointer now, and once again  
swap in the xioctl() value (_WHY_ were those arguments passed down from  
the caller?).

And another stopping point; ifconfig.c is now 724 lines