[PATCH] one more hdparm patch with command line parsing bug fix

[Tito]

On Saturday 13 May 2006 22:04, Bernhard Fischer wrote:
> On Sat, May 13, 2006 at 09:24:48PM +0200, Tito wrote:
> >On Saturday 13 May 2006 18:43, Bernhard Fischer wrote:
> >> 
> >> Tito,
> >> 
> >> On Sat, May 13, 2006 at 04:20:46PM +0200, Tito wrote:
> >> >Hi,
> >> >this is a new hdparm patch to fix one bug, reduce size and clean up the code.
> >> >The main changes are:
> >> >
> >> >Fixed bug in command line arg parsing:  flags for options without args are reset on next getopt_long iteration.
> >> >Updated the timing algorithm  in do_time()  to that of hdparm 6.6  (gives better results on my drives and code is cleaner).
> >> >Changed switch() to if() else if() else  to reduce size.
> >> >Renamed if_printf_on_off() to print_flag_on() and optimised for size.
> >> >Removed if_printf() as it increases size and substituted with print_flag() where appropriate.
> >> >Removed check_if_min_and_set_val() as it increases size.
> >> >Removed check_if_maj_and_set_val() as it increases size.
> >> >Removed if_else_printf() as it increases size.
> >> >Removed sync_and_sleep() as now it was used only in one place.
> >> >Moved code used 2 times in do_time() to print_timing().
> >> >Optimised some text strings to reduce size.
> >> >
> >> >Commandline option parsing should now work in all cases.
> >> >Size reduction for hdparm.o (with all options enabled) is:
> >> >   text    data     bss     dec     hex filename
> >> >  23525     176     872   24573    5ffd hdparm.o.orig 
> >> >  22209     176     864   23249    5ad1hdparm.o
> >> >
> >> >Please apply before feature freeze.
> >> 
> >> allnoconfig, enable all hdparm stuff:
> >> $ size miscutils/hdparm.o*
> >>    text	   data	    bss	    dec	    hex	filename
> >>   31717	    148	    872	  32737	   7fe1	miscutils/hdparm.o.oorig
> >>   29870	    148	    864	  30882	   78a2	miscutils/hdparm.o.10
> >>   29743	    148	    864	  30755	   7823	miscutils/hdparm.o.11a
> >
> >
> >
> >> For my compiler it helps if you explicitely cache the array-derefs like
> >> in identify() near /* reset result */, can you reproduce this with your
> >> compiler?
> >
> >Sorry, but due to my limited knowledge,  English and programming,
> >i don't understand exactly  what you're talking about here?
> >
> >How can I "explicitely cache the array-derefs"?  
> 
> Since landley profoundly broke the build again by dropping all and every
> optimisation, I'd have to recheck if caching these are of any benefit.
> 
> What I was referring to was the last hunk in the patch, i.e.:
> 
> @@ -1096,20 +1086,16 @@ static void identify(uint16_t *id_suppli
>         }
> 	 
> 	 /* reset result */
> -       if ((val[HWRST_RSLT] & VALID) == VALID_VAL)
> +       jj = val[HWRST_RSLT];
> +       if ((jj & VALID) == VALID_VAL)
> 	 {
> 
> etc.
> 
> >
> >> Also, we should use "strng" more often. I renamed 's' to strng for better
> >> grep'ability
> >
> >Yes, you are right, didn't think  about this.
> >
> >> The putchar and puts usage makes absolutely no difference for any
> >> version of gcc i have here. Do you see any adverse/positive effect if you
> >> change these?
> >
> >Yes, i can confirm this, for gcc version 4.0.3 (Ubuntu 4.0.3-1ubuntu5),
> >it seems that:
> >
> >printf("\n");
> >putchar('\n');
> >puts("");
> >
> >produce binaries of the same size.
> >Same thing for:
> >
> >printf("Prova\n");
> >puts("Prova");
> >
> >That's why I don't change them anymore.
> 
> nod.
> >
> >Should we merge the 2 patches?
> 
> We should drop the puts and putchar hunks completely and we'll have to
> recheck if the changes make any sense if optimisation is turned on.
> *sigh*

ok i'll do it tommorrow (morning?....maybe).


hdparm.11a__to__11c.diff looks promising,  i haven't thought about a solution like this  :-(

> 
> The set_* get_* should be treated the same way, i.e just mask the bits
> which are set and use one unsigned long flgs or something like this.

If i understand exactly the way it works i will try to implement it......
but maybe it's better to split that patch or it will grow too big  (it already is...)
and Rob will kill me.
I think i will incorporate only the stuff in  hdparm.10__to__11a.diff that shows to be ok
and move opt_* and_set_* and get_* stuff to a new patch.

Ciao and good weekend,
Tito 
 
> Feel free to incooperate any of these changes into your patch.
> 
> Cheers and sorry for being grumpy
>