[Toybox] [CLEANUP] ifconfig commit 866

[Rob Landley]

Bigger one than the last two:

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

In this round, we delete the standalone global iface_list_head and add a 
GLOBALS() block with void *iface_list. (We have a pointer to a list, 
saying we have a pointer to a list_head is silly. The list head is just 
the first node in the list...)

Combine "typedef struct _proc_net_dev_info {...} 
WHY_IS_THIS_CAPITALIZED" and "typedef struct _iface_list {...} 
CAPITAL_LETTERS_MEAN_MACRO" into a single "struct iface_list" which is 
much shorter and not a typedef, plus avoids an extra layer of 
indirection with nested structures so you can say "il->transmit_fifo" 
instead of "l_ptr->dev_info.transmit_fifo". (Do that a dozen times in 
the same function's arguments, it adds up.) The name of the new struct 
comes from the old iface_list_head, that went away but we're keeping the 
name for the struct it was a list of.

Retain the comment that the netdevice man page has lots of relevant info 
on this stuff.

We're inlining or replacing a bunch of stuff this time around. The old 
symbols that went away are field_format, iface_flags_str, 
omit_whitespace(), print_iface_flags(), print_media(), and clear_list(). 
I'll mention each again at the point it's was called.

Replace clear_list() with llist_traverse(free). Collate two similar 
hex_to_binary() calls (with identical error messages) into a single call 
using ? : operator for the single differing argument; no point 
duplicating the function call and marshaling those other arguments twice.

(An aside on rough cost estimates: a function call takes roughly 
sizeof(long), and marshalling an argument ballparks to sizeof(the 
argument). The ? : isn't without cost but we've already _got_ the test 
so moving the test into the parameter instead of around the function 
call is a wash. If I had to guess I'd say a ? : is maybe as expensive as 
a function call with no arguments or return value, but all this stuff 
varies per target and compiler version and so on. This is just so I have 
some vague idea what's more or less expensive. Mostly, it's not having 
the code say the same thing twice. Maybe some optimizers are smart 
enough to collate it themselves these days, but I still don't like 
redundant code.)

The get_proc_info() version field: the version field is for dealing with 
data from different /proc/net/dev versions. How old are these versions, 
has any of them been exported since 3.x came out? Let's see: 
/proc/net/dev is exported from linux/net/core/net-procfs.c where the 
relevant functions are dev_seq_show() (printing the header) and 
dev_seq_printf_stats() (printing the numbers). According to git 
annotate, that recently (feb 2013) moved from net/core/dev.c (commit 
900ff8c63214) so we use the old "git annotate ${VERSION}^1 -- filename" 
trick, and _that_ says the part of dev_seq_show() that's printing out 
the header hasn't changed since 2003 (so an old version would be 2.4 
kernel). So we may be calculating the contents differently, but we 
haven't relabeled the fields so we shouldn't have to parse them 
differently. Let's confirm though, dev_seq_printf_stats() has several 
commits since then. Let's see, 28172739f0a2 was "fix 64 bit counters on 
32 bit arches", not a format change. Annotate before _that_, and commit 
be1f3c2c027c was "64 bit statistics on 32 bit architectures", 
2d13bafeba24 is potentially relevant "ensure there is always a space 
between : and the first number" (but we do a strsep on the : so aren't 
confused if there isn't), eeda3fd64f75 is "replace dev->stats() with 
get_dev_stats()" (different function, same data), 5a1b5898ee9e is just 
reindenting this block...

And at that point, the actual printf block dates back to 2002, so again 
before 2.6 shipped. So we only care about the most recent version.

Back to get_proc_info() in ifconfig.c: we zap the version argument, and 
it's using the new struct iface_list. Zap the memset because the only 
caller did an xzalloc() so we know it's zeroed. Inline 
omit_whitespace(). Tidy up a block using iface_list and combine _4_ 
lines of curly brackets into a single "} else thingy;"

Now that version's gone, field_format[] doesn't need to be an array. It 
never needed to be a global. It can just be a normal sscanf() inline 
format string. And we can collapse the 17 line sscanf() into a 7 line 
sscanf(), and eliminate the if(version) after it because the above 
exercise proved we should never have to care on a kernel shipped in the 
last 10 years.

Convert add_iface_to_list() to struct iface_list, using the TT instance 
instead of the standalone global. Same for get_device_info(), no other 
changes there.

In get_device_info we have #ifdef SIOCGIFMAP and we go back to the 
kernel to see how long that's been there: it's 
include/uapi/linux/sockios.h which I'm not even going to annotate but 
just log to see when that was moved into uapi... commit 607ca46e97a1, an 
using the _big_ linux git tree that goes all the way back to 0.0.1, the 
line #definining SIOCGIFMAP is from 1994. So we can safely yank the #ifdef.

get_ifconfig_info(): pretty much a complete rewrite of this function. We 
don't need to do version detection, so just make it a single loop 
reading the file, skipping the first two lines of header, running each 
of the rest through get_proc_info(), add_iface_to_list(), setting it 
non-virtual, and calling get-device_info().

print_hw_addr() and print_ip_addr() just convert to struct iface_list, 
no other changes.

print_media(): just delete it and remove the call from 
display_ifconfig(). The function exists to identify the type of ethernet 
cable connection plugged into the card, but the table (ahem, if/else 
staircase) only has entries for 10baseT and 100baseT. Does anybody still 
care about distinguishing between 10base2 (coax) and 10baseT (cat5)? Or 
about whether or not your 100baseT connection is T or TX or FX? Can 
anybody tell me what that difference _means_? The man page can't:

   media type
     Set  the  physical port or medium type to be used by the device.
     Not all devices can change this setting, and those that can vary
     in  what  values  they  support.   Typical  values  for type are
     10base2 (thin Ethernet), 10baseT (twisted-pair 10Mbps Ethernet),
     AUI  (external  transceiver) and so on.  The special medium type
     of auto can be used to tell the driver to auto-sense the  media.
     Again, not all drivers can do this.

Note we're not _setting_ it, we're just reading it. I dug around in the 
kernel source a bit (since these values are defined in 
linux/netdevice.h) and found drivers/net/ethernet/sis/sis900.c going:

   /* we switch on the ifmap->port field. I couldn't find anything
    * like a definition or standard for the values of that field.
    * I think the meaning of those values is device specific. But
    * since I would like to change the media type via the ifconfig
    * command I use the definition from linux/netdevice.h
    * (which seems to be different from the ifport(pcmcia) definition) */

Git annotate says that comment's from 2001, so even _then_ this was 
obsolete (overtaken by "auto"). So I'm yanking the function, which never 
applied to wireless or gigabit interfaces anyway. (I could have redone 
it as a small inline string array, but it doesn't seem worth keeping.)

print_iv6_addr(): convert to struct iface_list, replace stack buf[] with 
toybuf, and some curly bracket/whitespace cleanups. Remove a memset 
inet_ntop doesn't care about, combine two xprintf() calls into one, and 
replace an xprintf() with an xputc().

display_ifconfig(): Convert to struct iface_list. We don't need a local 
variable copy of int hw_type now that getting it out of the structure is 
less of a pain. Remove the call to print_media because we deleted that 
function as useless.

Inline print_iface_flags(), which also sucks in the global 
iface_flags_str[]. Instead of calling the function, locally test 
ifrflags, declare a string array inside the if {} block, and loop 
through it here. This means we can delete print_iface_flags() and 
iface_flags_str[] because that was the only user.

The rest of this function looks more changed than it is, it's all just 
these changes repeated a lot:

   1) replace lptr->dev_info. with il->
   2) replace %10s with %10c (and change its argument from " " to ' ')
   3) replace local hw_type with il->hw_type.
   4) combine consecutive xprintf() calls into a single xprintf() call.
   5) whitespace (remove newline after if() when it'll fit on one line)

The %s to %c padding change means we don't need an empty string 
constant, marshalling int instead of pointer is slightly cheaper on 64 
bit platforms, and it saves a dereference. (A rounding error "while I 
was there" cleanup.)

readconf(): The bit with the "Escape duplicate values" is a classic loop 
simplification: we don't need a match_found variable if our iterator 
variable will be null if it hit the end of the list and won't if it 
broke out early. So as long as I had to convert IFACE_LIST to struct 
iface_list anyway, I went ahead and turned:

   IFACE_LIST *list_ptr;
   int match_found = 0;
   for(list_ptr = iface_list_head; list_ptr != NULL; list_ptr = 
list_ptr->next) {
     //if interface already in the list then donot add it in the list.
     if(!strcmp(ifre->ifr_name, list_ptr->dev_info.ifrname)) {
       match_found = 1;
       break;
     }
   }
   if(!match_found) {

into:

   struct iface_list *il;

   for(il = TT.iface_list; il; il = il->next)
     if(!strcmp(ifre->ifr_name, il->ifrname)) break;
   if(!il) {

10 lines into 4 lines, and one of those 4 is blank. Yes I removed a 
comment, but this whole thing is right after the "Escape duplicate 
values" comment.

show_iface(): more IFACE_LIST -> struct iface_list conversion, and 
another loop that doesn't need is_dev_found when if (!il) tells us 
whether or not we hit the end.

The "error getting device info" message just becomes a perror_msg() with 
the interface name and the errno string. "Interface" had "problem". Yes, 
maybe ENODEV won't have quite the error string we want, but it'll be 
slightly wrong in the correctly internationalized language.

Moving struct iface_list up a block level means it can replace two local 
IFACE_LIST variables. Their scopes don't overlap, so they can reuse the 
same variable safely.

And at the end of the patch, clear_list() went away as mentioned earlier.

 1388959554.0