[PATCH] make devfsd use the new ENABLE_* stuff

[Rob Landley]

On Saturday 08 October 2005 11:15, Tito wrote:
> Hi,
> this patch ports devfsd to the new ENABLE_* stuff.
> The patch is rather big and only compilation is tested.
> BTW there is also some little size reduction if I have not done
> something too stupid somewhere with the ENABLE_* magic...

If somebody else is going to do the work, I'm not going to object too 
loudly. :)

Query: you're adding several new __attribute__ markers.  Are these gcc 
specific?  (Is there a standard for them?)

A question I've been ambivalent about is "how deeply dependent on gcc are 
we/should we be?"  Avoiding gcc-isms like (thingy ? : otherthing) isn't worth 
it, but doing something the gcc-specific way when there's an equivalent c99 
way (such as the structure initializers switchover) is also bad.

Busybox now needs a c99 compliant compiler, and that's fine.  But using 
gcc-specific features has this nasty habit of being version specific, which 
sucks.  Can we perhaps someday document what we _need_ out of a compiler (c99 
support, thing ? (blank) : otherthing, an optimizer that does -unit-at-a-time 
style dead code elimination, and so on.

__attribute__ strikes me as "#pragma, the next generation", and we're not even 
using them consistently.   (For example, "__attribute__ ((noreturn));" vs 
"__attribute__((__noreturn__));".  We use both.)

I suppose this came to my attention because one of our only remaining warnings 
is an __attribute__...

Right, on other notes:

-const char * const bb_msg_proto_rev   = "protocol revision";
-#ifdef CONFIG_DEVFSD_VERBOSE
-const char * const bb_msg_bad_config     = "bad %s config file: %s\n";
-const char * const bb_msg_small_buffer  = "buffer too small\n";
-const char * const bb_msg_variable_not_found = "variable: %s not found\n";
-#endif
+static const char * const bb_msg_proto_rev   = "protocol revision";
+static const char * const bb_msg_bad_config     = "bad %s config file: %s";
+static const char * const bb_msg_small_buffer  = "buffer too small";
+static const char * const bb_msg_variable_not_found = "variable: %s not 
found";

That does actually increase the size of the program (until we start using 
unit-at-a-time optimization in gcc 3.4 or 4.0).  The static functions even 
more so than the static variables.  See earlier confused mutterings on this 
topic. :)

I don't currently care about devfsd, and a compiler upgrade will fix this, and 
it doesn't affect the two common cases (devfsd with all features enabled and 
devfsd totally disabled), it just makes disabling subfeatures less effective.  
But still.  Something to be careful about in future.

The name "do_ioctl_and_die" should really be "do_ioctl_or_die", but I see 
you're being consistent with libbb.   It's still a stupid name. :)

This bit:
-#ifdef CONFIG_DEVFSD_MODLOAD
    case AC_MODLOAD:
-    action_modload (info, entry);
+    if(ENABLE_DEVFSD_MODLOAD)
+     action_modload (info, entry);
     break;
-#endif

Leaves an extra jump target in the disabled case.  In theory, that could be 
fixed by moving it right above the "default" case and moving the break inside 
the if() so it's a fallthrough that only a really _stupid_ optimizer would 
fail to remove.  Not that I care in the devfsd case, but for future 
reference...

By the way, I've generally found if(); else if(); else if(); chains to be 
_smaller_ than the code produced by switch/case statements.  (YMMV.)

>    text    data     bss     dec     hex filename
>   10176     392     543   11111    2b67 devfsd.o.orig.with_all_options
>   11339     392     543   12274    2ff2   devfsd.o.new.with_all_options

*blink* *blink*.

Why?  (Each individual option got smaller, but all options got larger...?)

>    text    data     bss     dec     hex filename
>   16770     392     543   17705    4529
> devfsd.o.orig.with_all_options_and_debug 16197     392     543   17132   
> 42ec devfsd.o.new.with_all_options_and_debug

And now the new one is smaller again...

> Yes, Rob I know that you don't care about devfsd....
> but please apply the patch the same as maybe there is
> people  out there still using 2.4 kernels (and if
> I've broken something they will complain  ;-P ).

Well it's certainly more _readable_.  If nothing else turning all the #ifdef 
CONFIG_DEBUG stuff into macros was a win...

Applied.

Rob