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
_______________________________________________
Toybox mailing list
Toybox@lists.landley.net
http://lists.landley.net/listinfo.cgi/toybox-landley.net
