[PATCH] The low-hanging fruit (multiple patches)

[Rob Landley]

On Thursday 27 October 2005 17:39, Rob Sullivan wrote:
> I've attached a bzip2ed tarball of patches that are essentially
> janitorial in nature. They affect ifupdown, makedevs, mktemp, rdate,
> telnet and watchdog, and make the following improvements:
> - getopt is replaced with bb_getopt_ulflags
> - the GPL licence header is replaced with the shorter one

Cool.

> - instances of strdup are replaced with bb_xstrdup (I left well alone
> weird cases like hush.c, where variables are strdup'd from
> themselves...)

Hmmm...  Should be ok, I think.  The point of xstrdup is to avoid checking the 
NULL return.  (It aborts if malloc fails, which in the days of virtual memory 
and automatic overcommit is the only sane response anyway.  If malloc fails, 
the OOM killer is about to start knocking out random processes anyway.)

> The patches reduce the size of the applets concerned a little, and
> there is a separate patch for each applet.

Sorting...

Twiddled the mktemp one a bit, quite possibly broke it.  (Contemplate "flags & 
1 << 0", both the unparenthesized nature and the totally pointless shift by 
zero.  Also, you confirm that optind+1 == argc here, but leave it using 
argc-1 as the array index instead of optind.  And I have _no_ idea what the 
gratuitous (void) before puts is for.  Query: is optarg set to argv[optind] 
here?)

Twiddled rdate too.  Please don't do 1 << 0.  If you're willing to stick 0 in 
there as a constant, why not just leave 1 as a constant?  It's not clarifying 
anything.  If you're going to make #defines for the numbers, fine, but 
anybody who can't figure out what (flags & 4) means is going to have a really 
hard time following the rest of our code.  If you get to the point where it's 
non-obvious, go with the symbolic #defines.

That's my opinion, anyway.

Wow, the telnet code is messy.  And you don't need ? as an explicit option; 
any unrecognized option should dump a usage message so -? ought to work just 
fine...

watchdog_shutdown needs a noreturn indicator so we don't gratuitously have the 
return EXIT_SUCCESS in there that can never be reached.  And while we're at 
it, what would a timer duration of 0 in here mean?  It's in the range of 
acceptable values, but I think it means we'd flood the watchdog device...

On makedevs: I'm thinking of adding some variant of udev functionality to 
busybox.  I'll post about that separately...

Ok, applied.

Rob