Cleaning up ifconfig, commit 905:
http://landley.net/hg/toybox/rev/905
get_strtou() isn't actually needed: it's a wrapper for strtoul() that's
only ever called once. It sets most of its effort setting errno, but
all you need to know is that the endptr from strtou isn't the end of
the string. So the one and only caller can just become:
+ unsigned long p = strtoul(s, &ss, 0);
+ if (*ss || p > 65535) error_exit("bad port '%s'", s);
Using the libc function, and then get_strtou() can go away.
Two more functions, is_host_unix() and get_host_and_port(), are only
ever called from one place, both in get_sockaddr(). so inline both of
them.
The is_host_unix() logic only triggers if host starts with "local:", so
wrap the function's guts in a big if() for that. The rest of the
function is mostly the same except that swl is just a pointer instead
of a pointer to a pointer, so dereference one less level.
Next we have the get_host_and_port() plumbing, which is a bit tricky
for me because I don't have a strong understand of ipv6, haven't used
it much. I'd really like some test cases, a near-term todo item is
probably to do a
scripts/test/ifconfig.test and work out an aboriginal linux test
environment to run it in.
So the function was:
static void get_host_and_port(char **host, int *port)
{
char *ch_ptr;
char *org_host = *host;
if (*host[0] == '[') {
(*host)++;
ch_ptr = strchr(*host, ']');
if (!ch_ptr || (ch_ptr[1] != ':' && ch_ptr[1] != '\0'))
error_exit("bad address '%s'", org_host);
} else {
ch_ptr = strrchr(*host, ':');
//There is more than one ':' like "::1"
if(ch_ptr && strchr(*host, ':') != ch_ptr) ch_ptr = NULL;
}
if (ch_ptr) { //pointer to ":" or "]:"
int size = ch_ptr - (*host) + 1;
xstrncpy(*host, *host, size);
if (*ch_ptr != ':') {
ch_ptr++; //skip ']'
//[nn] without port
if (!*ch_ptr) return;
}
ch_ptr++; //skip ':' to get the port number.
*port = get_strtou(ch_ptr, NULL, 10);
if (errno || (unsigned)*port > 65535) error_exit("bad port '%s'",
org_host);
}
}
The logic boils down to: if the first character of host is '[' then we
have a "[blah]" address format that may be "[blah]:port". The strange
part is that although we adjust host to point to the first character
within the brackets, we don't turn the ] into a null terminator? So I
dunno who's going to be using this later? (Something to watch out for.)
If host didn't start with a [ then look for one colon. If we have more
than one colon then it's ipv6 again, but if we have exactly one it's
host:port.
Then at the end we parse the port number and barf if there was trailing
garbage of if it's over 65k. (Port number of 0 is apparently fine
though.)
So inlined with a "char *s;" up at the top of the function, that's:
+ if (*host == '[') {
+ host++;
+ s = strchr(host, ']');
+ if (s && !s[1]) s = 0;
+ else {
+ if (!s || s[1] != ':') error_exit("bad address '%s'", host-1);
+ s++;
+ }
+ } else {
+ s = strrchr(host, ':');
+ if (s && strchr(host, ':') != s) s = 0;
+ }
+
+ if (s++) {
+ char *ss;
+ unsigned long p = strtoul(s, &ss, 0);
+ if (*ss || p > 65535) error_exit("bad port '%s'", s);
+ port = p;
+ }
Minor tweaks to reset_flags() along the way (it's going away once all
the instances are inlined), but wordwrap the arguments and make the &=
and |= logic look like the inlined version.
Inline set_mtu(), which has exactly one caller and is just two lines
anyway.
set_address() doesn't use its req_name argument: xioctl() took care of
that; the error message isn't quite as good but it's simpler code and
end-users won't know what SIOWTFOMGBBQ means without looking it up
anyway. The important error message is "it didn't work".
While I was there, replace the IPV6_ADDR_* checks with a loop over an
array of possible error message names (using an unfortunately clever
(!!j)<<(j+3) to produce all the macro values in order), and then
collate it all into a single xprintf(). This is taking advantage of
Linux's binary backwards compatability: if you know what the macros
resolve to, and it doesn't vary by architecture, you can rely on that
because the kernel changing it would break existing binaries. Since
that was the only user of the IPV6_ADDR_* macros, remove the #defines
from the top of the file.
Down in ifconfig_main(), suck some more of the if/else staircase into
the big table and loop. Add another field to the table indicating the
set_address() call to make (only in the positive case, not in the
starts-with-minus case), and use that to incorporate pointtopoint,
broadcast, netmask, and dstaddr.
Add a guard so the SIOCGIFFLAGS stuff only gets called if there's a
change to make, since netmask and dstaddr won't.
A further modification I'd _like_ to do is collapsing that new field
into the second field of flags[], since it's always 0 when that's set.
I note that all the macros are 0x89XX and "man ioctl_list" actually
gives a reason for this: 0x89 is the "type" for socket calls, using the
old _IO(type,nr) macros. Given that in terms of IFF_FLAGS 0x8900 would
be IFF_DYNAMIC|IFF_SLAVE|IFF_PROMISC (I.E. an impossible flags
combination), I could just test that flags[1]|0xFF=0x89FF and not have
it be a separate field.
I haven't done this yet because it's one of those "being clever" vs
"bigger code" tradeoffs that I'm not quite comfortable with, but I
might do it in a future pass.
Back down in the if/else staircase, I also reordered "metric" and
"txqueuelen" so all the commands that boil down to "set an ifre field,
call xioctl()" are grouped together, and all the ones that call some
function are together. I put some whitespace gaps around 'em to make
the groups easier to spot.
Down at the end, the interface name parsing: this is_colon stuff is
basically a manual inlined strchr(). I replaced it with a strchr().
This commit drops us down to 795 lines, and there's still more to do.
Rob
_______________________________________________
Toybox mailing list
Toybox@lists.landley.net
http://lists.landley.net/listinfo.cgi/toybox-landley.net
