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.
