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. _______________________________________________ Toybox mailing list Toybox@lists.landley.net http://lists.landley.net/listinfo.cgi/toybox-landley.net