[Toybox] [CLEANUP] eject commit 947

[Rob Landley]

Eject cleanups in commit 947

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

To start with, this isn't just "not in susv4", it isn't in LSB 4.1  
either. So say "no standard".

The usage message: collate the option flags into one [-block]. The  
square brackets mean something is optional, an eplipsis (...) means  
something can be repeated an arbitrary number of times (this is more or  
less copied from man pages), but the device argument can occur zero or  
one times, so [DEVICE] is the way to say that. Use one tab instead of  
spaces between the option and its description. (This is how the others  
do it, saves a couple bytes.)

The #defines are essentially comments: the magic constant is here in  
the C file anyway, it might as well be at the call site with a comment  
about the macro name the constant stands for. Note that DRIVE_NOT_READY  
was never used.


--- remove_scsi():

for one line comments I tend to use // instead of /* */ because c99  
allows it. The int argument is a file descriptor so it's standard to  
say "fd" for that. (If you see "fd" you generally know what it is, just  
like i is probably a loop counter.)

I generally have variable declarations as a single block at the start  
of a function, separated from non-declaration statements by a space.  
This had two blocks with a space between them, so collate. While I'm at  
it: we have "toybuf" so declaring structs and arrays on the stack isn't  
really necessary, use chunks of toybuf instead. (The reason I did  
toybuf+64 instead of toybuf+sizeof(blah) is I dunno what the alignment  
of the struct is, or what alignment considerations the buffers have.  
This way everything's 32-byte aligned.

Using toybuf means the memset() can go away: is starts zeroed. I  
renamed in_out_header to just "header" because it's a local variable  
and the smaller the scope the simpler the name you can get away with.

Ripped out the ioctl() querying the version, on the theory A) we don't  
really support old pre-2.0 kernels anyway, B) the failure is to exit  
without ejecting, which is pretty much the failure if we did run on  
such an old kernel somehow. (We wouldn't compile against its libc, but  
oh well.)

--- eject_main():

Move rc declaration into the else {} block that actually uses it, and  
don't bother initializing it to 0 since the only use starts with an  
unconditional assignment.

Move "/dev/cdrom" assignment into variable declaration, and then  
conditionally assign to it if we have an argument. (Not only is this  
one fewer line of code, but the compiler can turn the if () into a  
conditional assignment avoiding a jump and corresponding branch  
prediction, and it's the same amount of assignment code either way so  
the binary isn't any larger.) Put a space between variable declarations  
and non-declaration code. And have the test the same thing we're  
assigning instead of a different variable so we load into a register  
and then re-use that register. (The command line argument array is null  
terminated so toys.optargs[toys.optc] is guaranteed to be NULL the same  
way argv[argc] is guaranteed to be NULL. I don't remember if that's the  
C standard, posix, or the ELF spec, but I tracked down where it was  
standardized once and it is, not just "happens to be true" but  
required.)

The above #defines substitued in as constants start kicking in here,  
not the comments at end of line. The if (TRAY_OPEN) ioctl(EJECT) stuff  
could be on one line, but they're on two lines so each line can have a  
trailing comment with the symbol name that should theoretically be in  
some header somewhere, but isn't.

Inserted extra parentheses:

-    if (toys.optflags & FLAG_T || toys.optflags & FLAG_t) {
+    if ((toys.optflags & FLAG_T) || (toys.optflags & FLAG_t)) {

-      if (toys.optflags & FLAG_t || rc == 2) //CDS_TRAY_OPEN = 2
+      if ((toys.optflags & FLAG_t) || rc == 2)         // CDS_TRAY_OPEN

Mixing logical and boolean operators, the precedence is nonobvious.  
Dennis Ritchie wrote about the historical reasons in his History of  
Programming Languages conference talk:

   http://cm.bell-labs.com/cm/cs/who/dmr/chist.html

Relevant chunk:

> At the suggestion of Alan Snyder, I introduced the && and || operators
> to make the mechanism more explicit. Their tardy introduction explains
> an infelicity of C's precedence rules. In B one writes
> 
>   if (a==b & c) ...
> 
> to check whether a equals b and c is non-zero; in such a conditional
> expression it is better that & have lower precedence than ==. In
> converting from B to C, one wants to replace & by && in such a  
> statement;
> to make the conversion less painful, we decided to keep the precedence
> of the & operator the same relative to ==, and merely split the
> precedence of && slightly from &. Today, it seems that it would have
> been preferable to move the relative precedences of & and ==, and  
> thereby
> simplify a common C idiom: to test a masked value against another  
> value,
> one must write
> 
>   if ((a&mask) == b) ...
> 
> where the inner parentheses are required but easily forgotten.

And this means whenever I have a boolean operation mixed with logical  
operators, I parenthesize even when it's not strictly required.

The trailing comments are off to the right a bit because there's  
several of them so lining them up helps readability.

The close isn't strictly required, so I put it in CFG_TOYBOX_FREE which  
is basically the "I'm going to run this under valgrind and want to  
reduce the noise" config symbol.

(And xclose only actually matters on NFS, no other filesystem can  
return an error from close() that I know of, but as long as it's  
there...)

Rob
 1373605879.0