[Toybox] [CLEANUP] ifconfig commit 958

[Rob Landley]

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

This commit removes struct if_list by unifying get_device_info() and  
display_ifconfig().

Basically we had a giant pile of ioctl() calls fetching data in the  
first function, each of which had its results copied into a temporary  
structure, and then the second function used the data out of the  
temporary structure. But pasing data to _ourselves_ like that is kind a  
pointless, so this commit moved each ioctl() to right before the data's  
user, and just uses the data directly out of the ioctl structure  
without unnecessary copies to temp storage.

All the get_device_info()/display_ifconfig() calls were paired up last  
commit, but
There are four calls:

1) explicit name, found in /proc/net/dev.
2) display all, found in /proc/net/dev.
3) explit name, _not_ found in /proc/net/dev.
4) display all, virtual interfaces queried via ioctl.

The behavior of these calls varies:

1) for "non-virtual" interfaces, it fetches a list of 16 values out of  
/proc/net/dev and stuffs them into an array of uint64_t. I just made  
that into an explicit argument, and made it NULL if we didn't have it.  
This let me drop the "non_virtual_interface" member of the temp struct,  
since null and non-null signaled the same thing.

2) Some interfaces we find should _not_ be displayed, namely when we  
display all interfaces but aren't using the -a flag, only display the  
interfaces that are up. This was previously filtered out at the call  
sites, but now that the display function is querying the flags the  
caller doesn't know whether or not the interface is up, so this has to  
move into the display function.

So I added an "always" argument to display_ifconfig() that says whether  
or not to display this interface even if it's not up. The logic change  
at the call sites looks like:

-      if(!sl) {
-        get_device_info(ifre->ifr_name, &il);
-        if ((il.flags & IFF_UP) || (toys.optflags & FLAG_a))
-          display_ifconfig(ifre->ifr_name, &il);
-      }
+      if(!sl) display_ifconfig(ifre->ifr_name, toys.optflags & FLAG_a,  
0);

And then at the start of display_ifconfig, the first thing it queries  
is flags (saving it ito a local variable), and right after that it goes:

+  if (!always && !(flags & IFF_UP)) return;

That gives us the current set of arguments to display_ifconfig:

static void display_ifconfig(char *name, int always, unsigned long long  
val[])

Another slightly tricky bit: displaying the address types previously  
had a table:

-      {"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)}

The offsetof() was an offset into the temporary structure that went  
away. This is no longer needed, because the ioctl() uses struct ifreq,  
which is discribed in "man 7 netdevice" as:

            struct ifreq {
                char ifr_name[IFNAMSIZ]; /* Interface name */
                union {
                    struct sockaddr ifr_addr;
                    struct sockaddr ifr_dstaddr;
                    struct sockaddr ifr_broadaddr;
                    struct sockaddr ifr_netmask;
                    struct sockaddr ifr_hwaddr;
                    short           ifr_flags;
                    int             ifr_ifindex;
                    int             ifr_metric;
                    int             ifr_mtu;
                    struct ifmap    ifr_map;
                    char            ifr_slave[IFNAMSIZ];
                    char            ifr_newname[IFNAMSIZ];
                    char           *ifr_data;
                };
            };

And since it's a giant union, all of the ifr_* fields are at the same  
offset. (Certainly the ones of the same type, since we don't have to  
worry about padding differing for that.) And that means we can just use  
ifr_addr for all of them: broadcast, netmask, etc. (Yay common  
codepath.)

But what we _do_ need differing this time is which ioctl to call, so we  
change the table to look like:

+      {"addr", 0, 0},
+      {"P-t-P", IFF_POINTOPOINT, SIOCGIFDSTADDR},
+      {"Bcast", IFF_BROADCAST, SIOCGIFBRDADDR},
+      {"Mask", 0, SIOCGIFNETMASK}

And then if it's 0 we don't call an ioctl, because _that_ ioctl has to  
be before the loop so we can test whether or not to display the line at  
all (including the whitespace padding at the beginning). If the address  
is all zero bits, don't display it.

We actually zero out the addr with memset before the ioctl, because in  
theory the ioctl could return success but still return all zeroes in  
the data, so testing ioctl success/failure doesn't tell us whether or  
not to display.

There's a semiliar problem with the "metric" field: it succeeds and  
returns 0 on the interfaces my netbook has, and 0 is supposed to  
display as 1. So it needs two tests to display right.

The code displaying that array of values for non-virtual interfaces  
used to special case one value during the display, because txqueuelen  
was ioctl-fetched data stored in the temp structure, and everything  
else was read out of proc. To simplify things I just extended the array  
to 17 places and had the ioctl read the data into the last array  
element, and then the table-based display code can refer to element 17  
without otherwise special casing it.

Down to 558 lines.

There are still two major areas of cleanup: lines 482-550 are argument  
parsing code that's currently mis-indented to remind me to deal with  
it. There might be a way to shoehorn that into the table-driven logic  
of lines 411-480. (The numeric address and "default" handling is fairly  
similar to the "up" handling, for example.)

And the other is the get_sockaddr() and set_address() stuff at the top  
of the file, which still means I need to understand more about ipv6  
address formats. (And why "add" and "del" seem to be the only path to  
setting them, instead of just supplying an ipv6 address argument like  
you would ipv4...)

Rob
 1374528898.0