[Toybox] [CLEANUP] ifconfig commit 957

[Rob Landley]

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

It's been a while. I got a bit blocked on get_sockaddr() cleanup,  
because although it's only ever called twice (once for ipv6 and once  
for ipv4), I don't understand what the addresses it's parsing should  
look like. For example, does "local:" apply to just ipv6 of both ipv6  
and ipv4? The "[ipv6]:port" thing... can I put an ipv4 address in that  
first bit? Would anyone ever do that? Does just :port by itself apply  
to ipv4?

I dunno what addresses people feed to this, or why "ifconfig eth0 add  
addr" is ipv6 but just "ifconfig eth0 addr" couldnt _also_ be ipv6?  
(Why is ipv6 special requiring the "add" keyword?) The man page for  
ifconfig isn't hugely revealing, so I don't know how to untangle this  
function yet.

But I need to finish this cleanup at some point, so here's another stab  
at it:

The "socklen" field of sockaddr_with_len is only ever assigned to,  
never read. So it can go. This pretty STRONGLY implies that the rest of  
sockaddr_with_len can just be "feed the right sockaddr_in or  
sockaddr_in6 to the function" and we don't need the wrapper type, but  
see "don't understand the addresses being parsed" here. Need to find or  
create some test code with use cases...

I spent a while cleaning up address_to_name before realizing... it's  
never used. The whole function can just be deleted.

add_iface_to_list():

There's no real reason for a linked list of interfaces here except to  
eliminate duplicates between the two interface scanning methods. We  
don't need to defer interface processing, we just need a simple string  
list of interface names to skip ones we've already seen.

The only two callers to add_iface_to_list are show_iface() and  
readconf(), and the only caller of readconf is the else case of  
show_iface(). So start by inlining readconf() in show_iface().

A common source of code bloat is bureaucracy, I.E. filling out forms  
and then reading the data back out of the forms. It's simpler to just  
use the data at the point it's generated rather than creating  
unnecessary structures to store and transport it from producer to  
consumer.

In this case it applies to both creating an unnecessary list, _and_ to  
get_device_info being separate from display_ifconfig(). The only  
consumer of get_device_info() output is display_ifconfig(), and it's a  
1:1 ratio, so they might as well be the same function without an  
unnecessary structure passing data between them. (This may involve  
moving some of the "show this or not" decisions into the combined  
function.)

A symptom of this unnecessary layering is show_iface() and  
display_ifconfig(). Going by the names, could you guess what the  
difference between those functions is? I couldn't. Show and display  
mean approximately the same thing.

Execution starts in show_iface(), which receives an interface name as  
its argument. Convert "struct if_list il" to just a local structure on  
the stack, where we fill out one instance and immediately display or  
discard it. Add a "struct string_list *ifaces" to hold duplicate  
interface names. So the list is nothing _but_ names, and the data  
structure only has one instance.

Make the first loop traversing /proc/net/dev actually display the  
interface in the loop. When we're looking for a specific interface  
displaying it means we return then, otherwise add its name to the  
duplicate checking list, display it if it's up or we have -a, and  
continue on.

That means the if (iface_name) test after the loop means we didn't find  
it in /proc/net/dev, so just have a get/display pair on the supplied  
name there. The else case used to be readconf, but we're swapping in  
the string list instead of the old one for the duplciate skipping, and  
then an adjacent get_device_info()/display_ifconfig() operating on the  
one local copy of interface_list.

So we don't need a "display everything" loop at the end: by the time we  
get there we've displayed everything we're going to. We also don't need  
to free iface_list, instead we free the new string_list.

At this point we've paired up all the  
get_device_info()/display_ifconfig() calls and have them each using a  
structure with a lifetime of just those two lines. This means on the  
next pass we can combine those functions and disintegrate the structure.

I changed the calling convention of get_device_info and  
display_ifconfig to take the name of the interface as a separate  
argument, rather than part of the structure, because the name is the  
main thing it should need when the structure goes away. (It'll also be  
able to take the val[] array, but that can be NULL when it's not  
available.) I had to fix a conflict in display_ifconfig that was  
already using char *name for ipv6 scope display: replaced it with  
"scope", and then there was already a scope int value so I replaced  
_that_ with iscope.

Ifconfig is now down under 600 lines. (Closing in on my original  
estimate of this much functionality being around 500 lines cleaned up,  
which is a lucky guess more than anything else.)

Rob
 1374230293.0