[Toybox] [CLEANUP] ifconfig (commits 878 and 879)
Rob Landley
rob at landley.net
Wed Apr 24 23:00:14 PDT 2013
http://landley.net/hg/toybox/rev/878
The diff adds xsocket(), cleanups up some free() logic (free is defined
to accept NULL and treat it as a NOP, and we don't need to set a local
variable on the way out of a function), get_device_info() can return
void and handle its own error_exit(), remove a half-dozen redundant
strncpy() of the same value into the same field that's still there, and
move the llist_traverse() call into show_ifconfig().
http://landley.net/hg/toybox/rev/879
This is basically inlining three functions in display_ifconfig(), and
take advantage of the opportunities for further cleanup revealed by
this.
1) Inline print_hw_addr() at the one callsite (in display_ifconfig()),
and perform obvious simplifications around it: replace the function's
variable names to the arguments fed to the function. Move printing the
"HWaddr" label inside the ARPHRD_ETHER test (since that's the only type
we support the alternative is potentially printing a label with nothing
after it), and consolidate the two if statements together. Move the
declaration of "i" to the start of display_ifconfig() and eliminate the
other declaration of it later in the function (since it can re-use the
one variable).
2) get_ip_addr() was only called from print_ip_addr(), and that was
only called from an if (il->hasaddr) test in display_ifconfig(). So I
inlined print_ip_addr() into display_ifconfig(), and refactored it so
the xprintf(" label:value ", one, two) was done within get_ip_addr()
(which I renamed show_ip_addr). I also defaulted the name field to
"unspec" and had the tests set it to other values, which made the
"could be used unsigned" error go away. (That xprintf() is still local
printing out the varying part, but it's followed by show_ip_addr using
a hardcoded label "addr", with a space before it, so the result works
out the same and we avoid repeating the constant string.)
Slight behavior change here: show_ip_addr() always prints a trailing
space including at the very end. The previous one did a newline without
a trailing space. I'm ok with it.
I'm still changing xprintf("%10s", " "); to xprintf("%10c", ' '); for
the trivially smaller argument. This indent is repeated enough it might
be worth finding a way to factor it out, but making it into its own
function is kind of silly.
3) get_hw_info() is also only called from display_ifconfig(). First,
replace it with table-driven logic: the first pass at the table had
four fields (int type, addrlen; char *name, title;), and it filled out
hw_info. Then inline it, and notice that only one field from hw_info
(title) is ever actually consumed, so remove the other two fields from
the table.
Note that this simplification only showed up when we inlined _both_
get_hw_info() and print_hw_addr(). In the original print_hw_addr() it
had if (!hw_info.hw_addrlen) return; but then only had code to print
out an address for hw_type == ARPHRD_ETHER (which has a nonzero
addrlen). That test was also the only consumer of addrlen, and only
cared that it's nonzero, so the constants to set it to various other
values weren't necessary. Possibly this saves us a header #include or
#define, I haven't looked yet. In any case, addrlen can be completely
removed now, we just need type->title associations in the table.
Since that's all struct HW_INFO did, we can eliminate that structure.
(I forgot to remove the constants for the field lengths, future todo.)
Instead we find an index into the local table, intentionally stopping
one before the end so anything unrecognized becomes "UNSPEC". Then just
print the string straight out of the table rather than making a copy of
it.
With this checkin, ifconfig is now under 900 lines.
Rob
More information about the Toybox
mailing list