[Toybox] [CLEANUP] 898: ifconfig argument parsing, part 1.
rob at landley.net
Sun May 12 19:20:44 PDT 2013
Design issue for today: replace if/else staircase with loop over a
The existing option parsing in ifconfig_main() is a giant if/else
staircase with repeated function calls. It makes multiple calls to
strcmp() with mostly the same arguments, the first several of which
call set_flags() (again with mostly the same arguments).
The set_flags() function calls an ioctl to read an interface's flags,
switch off one set of bits and switch on another set of bits, and then
call a second ioctl to write those new flags to the interface. The
existing logic calling set_flags() does a strcmp("string"), and then a
second strcmp("-string") which mostly does the same thing but with the
set/clear arguments reversed.
So, new code:
1) Remove redundant curly bracket scope and move local variable
declarations to the start of function. (Don't reindent everything, just
the bits we're changing now. Leave the rest indented an extra two
spaces so the diff doesn't look bigger than it is.
While we're here, use xstrncpy() and xsocket() to simplify setup code.
2) Redo the argument parsing loop to table-based logic.
Declare a structure with three elements: name string, flags to set,
flags to clear, and fill out an array (named "try") of those structures
for the 8 names that do nothing but set flags. The set/clear entries
are in an array so we can easily reverse the order we refer to them in,
based on a variable that's either 0 or 1. (The first is that variable,
the second is that variable xor 1.)
Check the argument for an initial - and set a "rev" state variable to
indicate we're reversing the operation, and increment the argument to
the next character so we're testing the right string for a match. Next,
loop through try and check each name. If we get a match, call the
pair of ioctl() calls, setting and clearing bits in between.
What I did here was:
A) replace if/else staircase with a loop over an array.
B) Genericize the - prefix logic to swap set/clear instead of having
two copies of each entry. (This means that "up" and "down" are not
perfect synonyms for "-down" and "-up", but they weren't before either.)
C) Inline this use of set_flags(). (We didn't make the function go away
because there are still more uses of it, but we should be able to suck
more stuff into this table next time.)
More information about the Toybox