[Toybox] Explaining the ifconfig cleanup: part I.

[Rob Landley]

I blogged a bit (http://landley.net/notes-2013.html#31-03-2013) about  
how I'd like more cleanup submissions. Possibly if I explain the kind  
of cleanup I'm going for, it might help with that.

When ifconfig was submitted, it touched a half-dozen files. I glued it  
together into a single self-contained file, which needs a lot of  
cleanup.

The reason I collected the half-dozen files is that nothing else is  
using any of that code yet. I try to avoid what I call "infrastructure  
in search of a user". Putting code into common directories should wait  
until it has more than one command actually calling it.

Another thing I'm trying to do is collect code together so functions  
and their callers are in physical proximity, because that lets you  
inline, untangle, and simplify. For example, code that does:

   #define CONSTANT 42
   printf("%d", CONSTANT);

There's no reason for the #define, you can just substitute it straight  
in the only place it's ever used. But if define is far away, you can't  
always see that it's not needed.

Defines are actually a specific category of bloat, because people like  
to pull values out and collect them together in what they think of as  
configuration sections. But this isn't always an improvement. Asking  
whether this config knob will ever be changed, and even if it is would  
it be any harder to find it inline in one or two places that actually  
use it than figuring out you need to change the config block? (Which  
involves reading the code and understanding what it's doing anyway to  
know that changing the #define is enough to change the behavior, and  
there's not some other hardwired assumption that the change breaks?)

Let's look at an example:

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

Commit 843 was a recent cleanup to ifconfig. The first hunk I changed  
is:

--- a/toys/pending/ifconfig.c   Thu Apr 04 19:39:44 2013 -0500
+++ b/toys/pending/ifconfig.c   Thu Apr 04 20:27:08 2013 -0500
@@ -37,14 +37,6 @@
  #include <net/ethernet.h>
  #include <alloca.h>

-#define TRUE ((int) 1)
-#define HTTP_DEFAULT_PORT 80
-#define FTP_DEFAULT_PORT 21
-
-#ifndef MAX_PORT_VALUE
-#define MAX_PORT_VALUE 65535
-#endif
-

Let's look at that. TRUE is 1. This is built into C. (c99 actually says  
that any nonzero value is true in a comparison function like if (), but  
that logical operators && and || and == will return 0 or 1. So you can  
pull tricks like rewriting:

   if (y == 4) x = 20;
   else x = 7;
   mangle(thingy, x);

as
   mangle (thingy, 7+(y==4)*13);

And it's actually portable on a c99 compiler. Whether or not it's  
better code is complicated (digression warning: this parenthetical is  
irrelevant. Modern processors rely heavily on speculative execution to  
keep multiple cores busy and work around prefetch stalls and other  
pipeline bubbles, and avoiding branches makes speculative execution  
much more efficient because if it guesses the wrong branch it wastes  
work it has to discard. But in this case a modern processor probably  
has conditional assignment instructions, although more power efficient  
architectures do less speculative execution because wasted work still  
comsumes power so avoiding it slows absolute speed but increases  
battery efficiency. But if you're plugged into wall current you can  
statistically turn some of those wasted cycles into performance by  
being lucky guessing what comes next.)

The reason _I_ like the one line version instead of the three line  
version is it's a lot less code to read. If you fit more on the screen  
it's easier to keep what it's doing in your head, because it's right  
there in front of you instead of something you have to flip back and  
forth to follow.

Going back to the #define TRUE: in theory there's probaby some header  
we could #include to get this, but there turned out to be exactly one  
user, resulting in this hunk to remove it:

@@ -1483,7 +1419,7 @@
                 perror_msg("error: no inet socket available");
                 return -1;
         }
-       while(TRUE) {
+       for (;;) {
                 ifcon.ifc_len = sizeof(struct ifreq) * num_of_req;  
//Size of buffer.
                 ifcon.ifc_buf = xrealloc(ifcon.ifc_buf, ifcon.ifc_len);

Saying while (1) wouldn't be any less clear than while (TRUE) if you're  
at all familiar with C, and saying for (;;) lets you not have a test at  
all. (The compiler edits out constant true/false tests, but still. I  
find it cleaner to not have them if they're not doing anything.)

Going back to the rest of that #define block: HTTP_DEFAULT_PORT and  
FTP_DEFAULT_PORT are 20 year old standardized values. This isn't a  
config knob anybody can ever change. same for MAX_PORT_VALUE, it's  
hardwired for both IPv4 and IPv6, and changing this code to work with  
other packet procols is pretty much a complete rewrite anyway.

The two DEFAULT_PORT macros are each only used once, in a function that  
is never called from anywhere, nor is the one after it. Easy cleanup  
that: remove the lot of it. (It's easy enough to put back if something  
does start using it. And if so, just use the constants. If they really  
need a comment to explain that 80 is HTTP, add a comment, but if you're  
doing network stuff that may be common knowledge by now and what the  
function was doing was strcmp(blah, "http") and strcmp(blah, "ftp") so  
it's there in the code without needing a comment anyway. So if we  
hadn't removed the function, the defines still would have gone away.)

MAX_PORT_VALUE was used twice, both times to sanity check a command  
line input. There's range checking built into the lib/args.c option  
parsing stuff, you can go "#<1>65535". I dunno if it applies here, but  
a todo item would be to check that. Also, why is ifconfig dealing with  
port numbers? For now just swap in the constants and make the #define  
go away.

And that's one cleanup pass.

Rob