[Toybox] [CLEANUP] Next ifconfig pass (commit 905)

[Rob Landley]

Cleaning up ifconfig, commit 905:

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

get_strtou() isn't actually needed: it's a wrapper for strtoul() that's  
only ever called once. It sets most of its effort setting errno, but  
all you need to know is that the endptr from strtou isn't the end of  
the string. So the one and only caller can just become:

+    unsigned long p = strtoul(s, &ss, 0);
+    if (*ss || p > 65535) error_exit("bad port '%s'", s);

Using the libc function, and then get_strtou() can go away.

Two more functions, is_host_unix() and get_host_and_port(), are only  
ever called from one place, both in get_sockaddr(). so inline both of  
them.

The is_host_unix() logic only triggers if host starts with "local:", so  
wrap the function's guts in a big if() for that. The rest of the  
function is mostly the same except that swl is just a pointer instead  
of a pointer to a pointer, so dereference one less level.

Next we have the get_host_and_port() plumbing, which is a bit tricky  
for me because I don't have a strong understand of ipv6, haven't used  
it much. I'd really like some test cases, a near-term todo item is  
probably to do a
scripts/test/ifconfig.test and work out an aboriginal linux test  
environment to run it in.

So the function was:

static void get_host_and_port(char **host, int *port)
{
   char *ch_ptr;
   char *org_host = *host;
   if (*host[0] == '[') {
     (*host)++;
     ch_ptr = strchr(*host, ']');
     if (!ch_ptr || (ch_ptr[1] != ':' && ch_ptr[1] != '\0'))
       error_exit("bad address '%s'", org_host);
   } else {
     ch_ptr = strrchr(*host, ':');
     //There is more than one ':' like "::1"
     if(ch_ptr && strchr(*host, ':') != ch_ptr) ch_ptr = NULL;
   }
   if (ch_ptr) { //pointer to ":" or "]:"
     int size = ch_ptr - (*host) + 1;
     xstrncpy(*host, *host, size);
     if (*ch_ptr != ':') {
       ch_ptr++; //skip ']'
       //[nn] without port
       if (!*ch_ptr) return;
     }
     ch_ptr++; //skip ':' to get the port number.
     *port = get_strtou(ch_ptr, NULL, 10);
     if (errno || (unsigned)*port > 65535) error_exit("bad port '%s'",  
org_host);
   }
}

The logic boils down to: if the first character of host is '[' then we  
have a "[blah]" address format that may be "[blah]:port". The strange  
part is that although we adjust host to point to the first character  
within the brackets, we don't turn the ] into a null terminator? So I  
dunno who's going to be using this later? (Something to watch out for.)

If host didn't start with a [ then look for one colon. If we have more  
than one colon then it's ipv6 again, but if we have exactly one it's  
host:port.

Then at the end we parse the port number and barf if there was trailing  
garbage of if it's over 65k. (Port number of 0 is apparently fine  
though.)

So inlined with a "char *s;" up at the top of the function, that's:

+  if (*host == '[') {
+    host++;
+    s = strchr(host, ']');
+    if (s && !s[1]) s = 0;
+    else {
+      if (!s || s[1] != ':') error_exit("bad address '%s'", host-1);
+      s++;
+    }
+  } else {
+    s = strrchr(host, ':');
+    if (s && strchr(host, ':') != s) s = 0;
+  }
+
+  if (s++) {
+    char *ss;
+    unsigned long p = strtoul(s, &ss, 0);
+    if (*ss || p > 65535) error_exit("bad port '%s'", s);
+    port = p;
+  }

Minor tweaks to reset_flags() along the way (it's going away once all  
the instances are inlined), but wordwrap the arguments and make the &=  
and |= logic look like the inlined version.

Inline set_mtu(), which has exactly one caller and is just two lines  
anyway.

set_address() doesn't use its req_name argument: xioctl() took care of  
that; the error message isn't quite as good but it's simpler code and  
end-users won't know what SIOWTFOMGBBQ means without looking it up  
anyway. The important error message is "it didn't work".

While I was there, replace the IPV6_ADDR_* checks with a loop over an  
array of possible error message names (using an unfortunately clever  
(!!j)<<(j+3) to produce all the macro values in order), and then  
collate it all into a single xprintf(). This is taking advantage of  
Linux's binary backwards compatability: if you know what the macros  
resolve to, and it doesn't vary by architecture, you can rely on that  
because the kernel changing it would break existing binaries. Since  
that was the only user of the IPV6_ADDR_* macros, remove the #defines  
from the top of the file.

Down in ifconfig_main(), suck some more of the if/else staircase into  
the big table and loop. Add another field to the table indicating the  
set_address() call to make (only in the positive case, not in the  
starts-with-minus case), and use that to incorporate pointtopoint,  
broadcast, netmask, and dstaddr.

Add a guard so the SIOCGIFFLAGS stuff only gets called if there's a  
change to make, since netmask and dstaddr won't.

A further modification I'd _like_ to do is collapsing that new field  
into the second field of flags[], since it's always 0 when that's set.  
I note that all the macros are 0x89XX and "man ioctl_list" actually  
gives a reason for this: 0x89 is the "type" for socket calls, using the  
old _IO(type,nr) macros. Given that in terms of IFF_FLAGS 0x8900 would  
be IFF_DYNAMIC|IFF_SLAVE|IFF_PROMISC (I.E. an impossible flags  
combination), I could just test that flags[1]|0xFF=0x89FF and not have  
it be a separate field.

I haven't done this yet because it's one of those "being clever" vs  
"bigger code" tradeoffs that I'm not quite comfortable with, but I  
might do it in a future pass.

Back down in the if/else staircase, I also reordered "metric" and  
"txqueuelen" so all the commands that boil down to "set an ifre field,  
call xioctl()" are grouped together, and all the ones that call some  
function are together. I put some whitespace gaps around 'em to make  
the groups easier to spot.

Down at the end, the interface name parsing: this is_colon stuff is  
basically a manual inlined strchr(). I replaced it with a strchr().

This commit drops us down to 795 lines, and there's still more to do.

Rob
 1368981880.0