[Toybox] Ifconfig cleanup: bonus round!
Rob Landley
rob at landley.net
Sat Nov 23 19:41:29 PST 2013
http://landley.net/hg/toybox/rev/1127
My ifconfig cleanups stalled a while back when I got derailed doing ipv6
research, an area I'm not that familiar with because I don't personally
use it. But the musl-libc has some ipv6 address tests that got me
looking
at this area again, so:
display_ifconfig():
The fopen("/proc/net/if_net6") has wrong whitespace (while() with
function
spacing instead of flow control statement spacing, and no blank line
between
the variable declarations and non-declaration code), which I leave as a
hint to myself that I need to look at this area more deeply.
No need to zero iface_name[] during its declaration if the sscanf() is
going to initialize it. Similarly, nitems is overwritten a couple lines
after
initialization.
The fgets() is reading a structure parsed by sscanf() which has to have
32+8+2+2+2+15 plus 6 whitespace characters for a grand total well under
128.
So using the whole of toybuf is probably overkill...
Don't need "sizeof(ipv6_addr) / (sizeof ipv6_addr[0])" because
ipv6_addr is
a char and we know how big a char is.
Unwind the if (nitems = 4) block because it's 4 lines and two separate
if
statements can be two lines. (Shorter code that isn't significantly more
complex is generally a win.) Similarly, collate the two int variable
declarations onto the same line, which is still nowhere near 80
characters...
except we've already got an "int i" in the enclosing scope, so that
declaration is redundant.
char *p; has too large a scope to grep for where it's used, so rename
to pp.
Then reuse it in the fopen() to copy the path and use the pathname in
the
perror_exit().
Redo the ipv6_addr read to be the start of the string and count down
instead of up inserting the colons. That way we can do &3 instead of %5,
and as a bonus it copies the existing string's null terminator.
(The if () if () is because even though I _think_ && is a sequence point
so (i++ && i) is deterministic, I'm not _sure_. I know if (i++) if (i)
is.)
I'm still a little iffy about the inet_pton()/inet_ntop() pair there.
We're
parsing the a set of 32 hex digits itno a colon separated string,
converting it to a binary address, and then converting that _back_ into
ipv6.
I'm aware that this squishes runs of zeroes and does the :: thing, but
as long as we're parsing anyway it might be better for us to do that
ourselves.
Now the potential reason to use inet_ntop() is that squishing the first
run
of zeroes into :: is trivial but squishing the LONGEST run of zeroes
isn't.
(Or at least requires two passes.)
Let's see, how do zero runs squish in practice...
$ sudo ifconfig eth0 add 1:2:3:4:5:6:7:8:9:ff
1:2:3:4:5:6:7:8:9:ff: Resolver Error 0 (no error)
Oh hey, that's nice. (Ubuntu 12.04 does that. People haven't really
debugged ipv6 much, have they?)
$ ifconfig eth0 add 0:2:3:4:5:6:7:ff
$ ifconfig eth0 add 0:0:3:4:5:6:7:ff
$ ifconfig eth0 add 1:0:3:4:5:6:7:ff
$ ifconfig eth0 add 1111:2222:3333:4444:555:66:7:ff
$ ifconfig eth0
eth0 Link encap:Ethernet HWaddr dc:0e:a1:52:92:77
inet6 addr: 1111:2222:3333:4444:555:66:7:ff/128
Scope:Global
inet6 addr: 1:0:3:4:5:6:7:ff/128 Scope:Global
inet6 addr: ::3:4:5:6:7:ff/128 Scope:Global
inet6 addr: 0:2:3:4:5:6:7:ff/128 Scope:Global
Um, the toybox one isn't printing any inet6 addresses? Ah, because it's
looking for /proc/net/if_net6 and the file is if_inet6 with an i. (My
bad, sorry. I broke it in an earlier cleanup round and had no ipv6
tests ready. Trying to rectify that now...)
Anyway, the zero eliding behavior is complicated enough that relying on
libc to do it for us is probably a good idea, so let's make it work:
The next reason we get no ipv6 output is display_ifconfig() is called
from show_iface() with an argument that lives in toybuf, and then we
overwrite toybuf so the strcmp(name, iface_name) never triggers. Easy
fix: change the display_ifconfig() call from show_iface() to use the
variable that lives in argv[] instead of the one that lies in toybuf.
if (inet_pton(AF_INET6, ipv6_addr, (void *)&sock_in6.sin6_addr) > 0)
{
sock_in6.sin6_family = AF_INET6;
if (inet_ntop(AF_INET6, &sock_in6.sin6_addr, toybuf,
sizeof(toybuf))) {
Riiiight. so we parse it as AF_INET6, we set AF_INET6 in the struct,
and then we
feed AF_INET6 to the expansion function. Do we really need all three?
(Tries commenting out the middle one... still seems to work.)
Don't need an int j; if int i isn't used in scope here...
Ok, that's probably enough for one pass.
Rob
More information about the Toybox
mailing list