[Toybox] [CLEANUP] ifconfig commit 906
Rob Landley
rob at landley.net
Mon May 20 22:49:02 PDT 2013
http://landley.net/hg/toybox/rev/906
I started by doing the table cleanup I mentioned last message, bringing
the table back down to three members. I took out the array and made all
three members separate, called "name", "on", and "off", and just had
the flag setting logic doing two ? : swaps to handle rev. what used to
be the t->addr != 0 case is now ((t->off | 0xff) == 0x89ff), in which
case I zero off before continuing. (I copy on and off to local
variables so I don't muck up the table in case people do more than one
instance of the same command, although why they'd do that... Oh well,
it also makes the ? : stanzas fit in 80 chars more easily.)
Another cleanup: remove every instance of char *req_name in function
arguments. They stopped being used when xioctl() went in, but they're
still vestigially there in a lot of places.
The rest of this one is basically just a frenzy of inlining:
set_metric(), set_qlen(), set_memstart(), and set_ioaddr() all do
basically the same thing: read the interface state with SIOCGIFMAP into
ifre, strtoul() a command line argument and assign it to a field of
ifre, and xioctl() it back to the interface. In fact set_irq() is doing
pretty much the same thing, but it's doing unnecessary casting and
pointer arithmetic to refer to "ifre.ifr_map.irq". I don't know why.
All of it's called exactly once, so having it be a function serves no
obvious purpose.
I also don't know why the ioctl number for the second call to xioctl()
isn't in each function, but is instead passed in as an argument from
the caller. Presumably this is intended as some kind of documentation
of what the function we're calling is actually doing... except over
half of them are repeats of SIOCSIFMAP? (Get interface hardware
mapping, set interface hardware mapping. And the get is hardwared, it's
just the set that isn't...)
Yes, this is fairly regular code that can probably get sucked into the
table driven logic in a future cleanup round. Especially since most of
them (possibly all) are actually union members buried inside the
structure, so we wouldn't even have to store separate offsets in the
table.
I have been wondering if the "get" and "set" should be moved outside
the loop. That way we don't do redundant gets and sets, but just do one
get, adjust everything, do one set. This might help with the "default"
case at the end of the remaining if/else staircase, which is the only
remaining user of set_flags() and is preventing the (already inlined)
function from going away. I'm not quite sure how to put the isdigit()
case into the table yet, corresponding to "ifconfig eth0 192.168.1.1".
Eh, worry about it in a future pass.
At the very end, slightly better error reporting. When error_exit()
reports a bad argument, say what the argument was.
More information about the Toybox
mailing list