[Toybox] vconfig cleanup pass 2.
Rob Landley
rob at landley.net
Sat Nov 2 13:21:56 PDT 2013
Once I collate and rsync this should become
http://landley.net/hg/toybox/rev/1106
Second pass on vconfig cleanup:
Looking further at strtorange(), thinking of genericizing it and moving
it into lib: Despite the original name starting with strtol, this
returns an int and accepts int limits? Didn't notice that last pass.
Long is better.
The lib/args.c logic for converting a string to an int in a range has
an error message "-%c < %ld", which gives the name of the argument that
violated a constraint and doesn't have any english in it at all (no
need to translate). It's also setting a default value, which this
isn't. This version is not a clean drop-in replacement for the
lib/args.c range logic. Best leave them separate for now. Still might
move strtorange() into lib/lib.c, but not this pass.
Whitespace and brackets: more removal of BSD code style brackets, and
inserting a space in flow control statements that look like functions,
ala "if (blah)". Also switch strcmp() == 0 to !strcmp().
Inline MAX_VLAN_ID: #defining a value and then using it exactly once is
not
a net gain.
Replace socket() with xsocket(). No need to comment that we're using
socket. (Comments should explain something the code itself doesn't.) No
need to set a default value when fd is initialized if we're just going
to unconditionally overwrite it here.
The set_name_type bit is operating on an enum, so it can be turned into
a for loop checking an array of strings and the value is just the
position in the array. And the error message might as well print out
the types we accept because they're not currently documented anywhere
else. (I'm tempted to fluff out the usage message to list these four
types and explain what they _do_... but I dunno what they do. Can't
document what I don't know.)
Why is name_type given a default value (VLAN_NAME_TYPE_PLUS_VID)? We
require at least two arguments, therefore this can't _not_ be set, and
if it's not one of the known values we error_exit. (Actually
perror_exit() which prints "No error" when errno is 0, which it should
always be here: error_exit() without the p doesn't do that.) Anyway,
deleted name_type since it's no longer used...
The "Store interface name" bit can be replaced with xstrncpy(). (Again
perror_exit() appends the errno string, but we haven't called a
function that set it to anything, use error_exit() when you don't need
to report errno.) While I'm here I should make xstrncpy report the
string and the length limitation. No need for an intermediate
interface_name variable, adjust the message at the end to just use
toys.optargs[1];
The device1 buffer is char[24], but we're limiting it here to 15 (or 16
with the null terminator). Why?
The "store interface name" bit doesn't need to be an else: the
set_name_type stuff exits on success or failure so we don't get there
otherwise.
I'm not familiar with the vconfig command, is it normally this chatty?
xprintf("Successful set_name_type for VLAN subsystem\n"); Ordinarily a
unix command is silent on success and only outputs stuff on failure or
if somebody requested information... I'd _like_ to turn that into
xioctl(); return; but I dunno how closely we're mirroring compatability
with some other implementation. (I'll probably do that cleanup
anyway...)
We don't need all these "else request.u.blah = 0" since we memset it
all to 0 at the start of the function.
Move the WARNING for add 1 into the add block so we're not doing the
strcmp("add") twice.
I have a largeish problem at this point: I don't have a vconfig test
suite. I've reached another good stopping point and the command
compiles again, but I made changes that weren't just whitespace and
can't really test it. I dunno what success looks like with this
command, or how to test all the fiddly corner cases. (In theory I can
set something up with a couple qemu instances and tun/tap, but I dunno
how...)
Oh well, check it in anyway...
Rob
More information about the Toybox
mailing list