[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