[Toybox] [CLEANUP] ifconfig.c commit 921

[Rob Landley]

I couldn't find a writeup of 921 and I'm trying to fill out the cleanup.html
page, so:

Commit 921:

http://landley.net/hg/toybox/rev/921

This pass is does a couple things:

 1) make sockfd a global so we don't keep reopening the socket
 2) inline stuff for future cleanups.

The inlines are intentionally left alone this time,
because the move makes it hard to see changes in the diff so combining
cleanups and inlining is hard to follow.  But some of the obvious cleanups
suggested by the sockfd stuff were in this pass.

Looking at the diff: Switching on "default y" was an accident, just ignore
it. (I hadn't implemented scripts/single.sh yet so I was building defconfig
to test.)

Inlining constants: the a block of #defines for SIOCSKEEPALIVE and
SIOCSOUTFILL copied from the headers were only used in one place (down in
ifconfig_main). If it's really a problem that headers have trouble
#defining something reliably, and we only use it once, just inline what
the defines are doing with a comment. Having the #define in one place
isn't a win over having the use in one place.

Inline get_socket_stream() into get_sockaddr(). In general when there's only
one caller of a function, having it _be_ a function is silly unless you need
a function pointer (ala loopfiles). It's easier to spot cleanups when the
code is together than when it's fragmented. Inlining also allows us to
use *swl instead of **swl, so we avoid a layer of dereferencing, and
turn the return into an error_exit().

While we're in get_sockaddr(), trivial little cleanup of the "barely worth
doing" kind, which are always the ones that require the most agonizing.
(Obvious wins are obvious, lesser of two evils can be close and dubious.
That's where polishing gets hard, the places nobody else will even notice.)

The cleanup::

     s = strrchr(host, ':');
-    if (s && strchr(host, ':') != s) s = 0;
+    if (strchr(host, ':') != s) s = 0;

If s was already 0, we may assign 0 to s. (Although we won't because if
strrchr didn't find a colon, strchr won't either, so it's a guaranteed
NOP either way.) The test avoids a strchr on a string we just did a strrchr
on (thus cache hot), and if they cared about speed they'd be doing
strchr(s+1) for the second check without a strrchr at all (which has to
traverse the entire string to find the null terminator anyway).

Given that toybox is going for simple and the test isn't necessary, I yanked
it.

(The whole stanza is going "is there exactly one colon in this string",
and it took me long enough to work out that's what they wanted that I
probably should have added a comment. But it seemed silly to comment two
fairly simple lines...)

Next up it looks like we collapsed together set_address() and set_ipv6_addr(),
but what actualy happened is we inlined set_ipv6_addr() down in ifconfig_main
(at its only call site), and diff is getting confused. Here's a diff of just
the two versions of set_address():

-static void set_address(int sockfd, char *host_name, struct ifreq *ifre, int request)
+static void set_address(char *host_name, struct ifreq *ifre, int request)
 {
-  struct sockaddr_in sock_in;
+  struct sockaddr_in *sock_in = (struct sockaddr_in *)&ifre->ifr_addr;
   sockaddr_with_len *swl = NULL;
-  sock_in.sin_family = AF_INET;
-  sock_in.sin_port = 0;
+
+  memset(sock_in, 0, sizeof(struct sockaddr_in));
+  sock_in->sin_family = AF_INET;
 
   //Default 0.0.0.0
-  if(strcmp(host_name, "default") == 0) sock_in.sin_addr.s_addr = INADDR_ANY;
+  if(strcmp(host_name, "default") == 0) sock_in->sin_addr.s_addr = INADDR_ANY;
   else {
     swl = get_sockaddr(host_name, 0, AF_INET);
-    if(!swl) error_exit("error in resolving host name");
-
-    sock_in.sin_addr = swl->sock_u.sock_in.sin_addr;
-  }
-  memcpy((char *)&ifre->ifr_addr, (char *) &sock_in, sizeof(struct sockaddr));
-  xioctl(sockfd, request, ifre);
-
-  if(swl != NULL) {
+    sock_in->sin_addr = swl->sock_u.sock_in.sin_addr;
     free(swl);
-    swl = NULL;
   }
+  xioctl(TT.sockfd, request, ifre);
 }

set_address(): move sockfd to GLOBALS() so we don't have to pass it in as
an argument. Blank line between declarations and other code. Instead of
declaring a local "struct sockaddr_in sock_in;", make that a pointer to the
instance out of ifre->ifr_addr to avoid an unnecessary memcpy later.
Do a memset instead of assigning 0 to a single field (dunno what else is
in there that might be relevant, so be clean).

Move the free(swl) up into the else { } case that allocated it, so we
don't need to test. (We don't need to anyway, free(0) is a guaranteed NOP.
Assigning null to a local variable right before we exit was a bit strange,
too.) I could have moved the declaration of swl into the same else case,
and avoided the now-unnecessary init to NULL that's overwritten as the
first use, but sockaddr_with_len is going away in a future cleanup anyway.

get_device_info(): Again the diff gets a bit confused, all we did was
replace a lot of local sokfd with TT.sockfd, and a little re-wordwrapping.

print_ip6_addr(): inlined into display_ifconfig(). No other changes
except the fopen()==-1 test becoming a return instead of a big if () { }
block (and the resulting reindentation of several dozen lines).

readconf(): switch to TT.sockfd instead of opening another local copy.

show_iface(): don't zero errno before calling get_device_info(), it's
not actually needed and if it was get_device_info() should do it.

ifconfig_main(): open the global sockfd up front. We mentioned the constant
inlining at the start of this round. More TT.sockfd uses.

Earlier we mentioned inlining set_ipv6_addr(), and here it is in the "add"
and "del" arguments. Even when you chop the relevant lines out and diff
them directly, diff can't make much sense of it. This is because
code at the callsite and code at the function got mixed together and
cleaned up. The closest I can get is:

diff -ub \
  <(hg cat -r 920 toys/pending/ifconfig.c | tail -n +200 | head -n 24) \
  <(hg cat -r 921 toys/pending/ifconfig.c | tail -n +613 | head -n 20)

Let's go through that:

-  char *prefix;
-  int plen = 0;
   sockaddr_with_len *swl = NULL;
+        struct ifreq_inet6 ifre6;
+        char *prefix;
+        int plen = 0, sockfd6 = xsocket(AF_INET6, SOCK_DGRAM, 0);

This hunk does two things:

1) Collate variable declarations (prefix, plen, and swl are
identical, ifre6 was moved up from below)

2) move the socket open to the top. (Note that IPV6 requires a different
kind of socket than IPV4, because the IPV6 designers were COMPLETELY INSANE,
which is why we can't just use the new TT.socketfd. Yes, different ioctl
constant numers AND a different socket type, because just one of those two
might have left open the possibility of common code and they couldn't have
THAT...)

+        if (!argv[1]) show_help();

Code from the original callsite in main(), unchanged.

-  prefix = strchr(ipv6_addr, '/');
-  if(prefix) {
+        prefix = strchr(argv[1], '/');
+        if (prefix) {
     plen = get_int_value(prefix + 1, 0, 128);
-    *prefix = '\0';
+          *prefix = 0;
   }

The two strchr() calls are the same, it's just main's argv[1] isn't being
fed into an argument variable anymore. The rest of that noise is diff
getting confused.

-  swl = get_sockaddr(ipv6_addr, 0, AF_INET6);
+        swl = get_sockaddr(argv[1], 0, AF_INET6);

Same variable pair that diff can't match up.

-  if(!swl) error_exit("error in resolving host name");

Not needed in the new code because get_sockaddr() does its own error_exit()

-    int sockfd6;
-    struct ifreq_inet6 ifre6;

Those moved up to the top.

-    memcpy((char *) &ifre6.ifrinte6_addr,
-        (char *) &(swl->sock_u.sock_in6.sin6_addr),
-        sizeof(struct in6_addr));

We don't need to memcpy this, we can just assign it (and do a few lines down).

-    //Create a channel to the NET kernel.
-    sockfd6 = xsocket(AF_INET6, SOCK_DGRAM, 0);

Moved to a variable initialization in the declaration up top.

-    xioctl(sockfd6, SIOGIFINDEX, ifre);
-    ifre6.ifrinet6_ifindex = ifre->ifr_ifindex;
+        ifre6.ifrinte6_addr = swl->sock_u.sock_in6.sin6_addr;
+        xioctl(sockfd6, SIOCGIFINDEX, &ifre);
+        ifre6.ifrinet6_ifindex = ifre.ifr_ifindex;

The middle line is the assignment that replaced the memcpy, the other pairs
of lines match up except for the layer of indirection removed because we
don't have to pass pointers to a functino.

     ifre6.ifrinet6_prefixlen = plen;
+        xioctl(sockfd6, **argv=='a' ? SIOCSIFADDR : SIOCDIFADDR, &ifre6);
 
-    xioctl(sockfd6, request, &ifre6);

These two xioctl lines are the same, minus the function call putting the
relevant constant into an argument variable.

     free(swl);

And done with the hunk!

The rest of the changes to main() are all sockfd shifting around.

 1403925677.0