modprobe update [Was: Re: Bug triage.]

[Yann E. MORIN]

Hello all!
Rob,

On Monday 12 December 2005 203, Rob Landley wrote:
>                p = strchr (buffer, ' ');
>                if (p) {
>                        *p = 0;
> +                       for( p = buffer; ENABLE_FEATURE_2_6_MODULES && *p; 
> p++ )
>  {
> +                               *p = ((*p)=='-')?'_':*p;
> +                       }
> I'm not 100% certain that can cleanly be optimized away, since p lives outside 
> your current scope and the for(;;) will assign to p before terminating (and 
> hopefully optimizing out the body of) the loop.
> One reason I've been holding on the CONFIG_->ENABLE_ conversion is to get more 
> familiar with the optimization capabilities of gcc.  (What kind of guards can 
> we get away with doing without accidentally bloating the code?  if(ENABLE) 
> seems pretty safe.  I'd want to check this vs putting an explicit if(ENABLE) 
> around it.  Might be clearer from a code reading point anyway...)

You are right. I'm not familiar to C compiler optimisation either, but your
reasoning sounds perfectly, well, sound, here. I was thinking, the loop
condition is always false, so gcc (and others) will clean that up for us.

On the other hand, we will always stumble on dumb (or 'broken') compilers
that will not optimises those things out. If we want to be 100% sure no
unused code sneaks into the binary, then #ifdef's are the only option, at
the expense of readability.

Do you want a patch with if(ENABLE) ?

> -#ifdef CONFIG_FEATURE_MODPROBE_MULTIPLE_OPTIONS
> -#ifdef CONFIG_FEATURE_CLEAN_UP
> -                               argc_malloc = argc;
> -#endif
> +                               if( ENABLE_FEATURE_CLEAN_UP )
> +                                       argc_malloc = argc;
> You dropped a MODPROBE_MULTIPLE_OPTIONS guard here.  I'm assuming it's 
> intentional, could you walk me through the logic?  (What lets this optimize 
> out, or why shouldn't it?)

It is intentional, yes: in this place, options are already parsed (wether it
be from config file, or from command line. This allows to free up the memory
used by the array to exec(insmod). We want to free up that array, wether we do
options parsing, or not.

argc_malloc is always declared.
If CONFIG_FEATURE_CLEAN_UP is declared, then argc_malloc gets used (written,
then read).
If CONFIG_FEATURE_CLEAN_UP is not declared, then argc_malloc is only declared,
and gcc optimises it out, at the expense of one compiler warning.
Am I wrong?

Note that the array is filled by the parent before the fork, copied to the
child when forking, and freed by the parent. The child automatically frees
the copy array with the exec() call.

> -#if defined(CONFIG_FEATURE_2_6_MODULES)
> -               if ( argc ) {
> +               if( argc ) {
> Why was that guard unnecessary?

Because we want to use the options from the command line, for both 2.6 and 2.4.
That solves bug #272.

> I just checked it in.  Does it fix bugs 276 and 272?

Good. And yes, both bugs are fixed with this patch.

Regards,
Yann E. MORIN.

-- 
.-----------------.--------------------.------------------.--------------------.
|  Yann E. MORIN  | Real-Time Embedded | /"\ ASCII RIBBON | Erics' conspiracy: |
| +0/33 662376056 | Software  Designer | \ / CAMPAIGN     |   ^                |
| --==< °_° >==-- °---.----------------:  X  AGAINST      |  /e\  There is no  |
| web: ymorin.free.fr | SETI at home 3808 | / \ HTML MAIL    |  """  conspiracy.  |
°---------------------°----------------°------------------°--------------------°