[PATCH] one more hdparm patch with command line parsing bug fix
Bernhard Fischer
rep.nop at aon.at
Sat May 13 20:04:33 UTC 2006
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*
I'm seriouly thinking about creating me a branch on my own where i can
be relatively sure that stuff works as intended :-/
I would be glad if you could check if the changes (as said without the
puts and putchar parts) do really produce better code. If they do (or
parts of the patch makes sense) then please feel free to merge these
parts.
I was looking at making the bss smaller when it dawned to me that
something with the optimisations must have gone hayward, since the
changes i made *should* have helped to get the .bss down but didn't.
By now, I'm giving up on doing anything more with bb for today,
there's plenty of other stuff todo.
The attached incremental diff were looking at reducing .bss; results:
text data bss dec hex filename
29743 148 864 30755 7823 miscutils/hdparm.o.11a.landley
22423 4 864 23291 5afb miscutils/hdparm.o.11a
22385 4 836 23225 5ab9 miscutils/hdparm.o.11c
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.
Feel free to incooperate any of these changes into your patch.
Cheers and sorry for being grumpy
-------------- next part --------------
--- miscutils/hdparm.c.11a 2006-05-13 18:25:24.000000000 +0200
+++ miscutils/hdparm.c.11c 2006-05-13 21:27:19.000000000 +0200
@@ -1125,10 +1125,35 @@ static void identify(uint16_t *id_suppli
}
#endif
+#if 1
+#define opt_noisy (1<<0)
+#define opt_verbose (1<<1)
+#define opt_get_identity (1<<2)
+#define opt_get_geom (1<<3)
+#define opt_quiet (1<<4)
+#define opt_do_flush (1<<5)
+#define opt_do_ctimings (1<<6)
+#define opt_do_timings (1<<7)
+
+static unsigned int opts = 1; /* noisy is on */
+static unsigned int flagcount;
+
+/* lazy.. avoid growing the diff too much for now */
+#define FOO(bar) (opts&opt_##bar)
+#define verbose FOO(verbose)
+#define get_identity FOO(get_identity)
+#define get_geom FOO(get_geom)
+#define noisy FOO(noisy)
+#define quiet FOO(quiet)
+#define do_flush FOO(do_flush)
+#define do_ctimings FOO(do_ctimings)
+#define do_timings FOO(do_timings)
+
+#else /* vvv ugh vvv */
static int verbose, get_identity, get_geom, noisy = 1, quiet;
static int flagcount, do_flush;
static int do_ctimings, do_timings;
-
+#endif /* ^^^ugh^^^ */
static unsigned long set_readahead, get_readahead, Xreadahead;
static unsigned long set_readonly, get_readonly, readonly;
static unsigned long set_unmask, get_unmask, unmask;
@@ -1890,7 +1915,7 @@ static void process_dev(char *devname)
#endif /* HDIO_DRIVE_CMD */
if (!flagcount)
- verbose = 1;
+ opts |= opt_verbose;//verbose = 1;
if (verbose || get_mult || get_identity)
{
@@ -2127,7 +2152,7 @@ static void parse_opts(unsigned long *ge
/* noisy is a global var */
if (get) { /* *get is initialized to 0 */
*get = noisy;
- noisy = 1;
+ opts|=opt_noisy;// = 1;
}
if (optarg) {
*set = 1;
@@ -2141,7 +2166,7 @@ static void parse_opts_v2(int flag, unsi
if (flag) {
/* noisy is a global var */
*get = noisy;
- noisy = 1;
+ opts|=opt_noisy;// = 1;
*set = 1;
}
}
@@ -2152,7 +2177,7 @@ static void parse_opts_v3(int flag, unsi
if (flag) {
/* noisy is a global var */
*get = noisy;
- noisy = 1;
+ opts|=opt_noisy;// = 1;
if (optarg) {
*set = ((*value = translate_xfermode(optarg)) > -1);
}
@@ -2208,16 +2233,19 @@ int hdparm_main(int argc, char **argv)
if (c == 'V') {
printf("%s %s\n",bb_applet_name, VERSION);
exit(EXIT_SUCCESS);
- }
+ }
- verbose |= (c == 'v');
- USE_FEATURE_HDPARM_GET_IDENTITY(get_IDentity = (c == 'I'));
- USE_FEATURE_HDPARM_GET_IDENTITY(get_identity = (c == 'i'));
- get_geom |= (c == 'g');
- do_flush |= (c == 'f');
+ if (c == 'v')
+ opts |= opt_verbose;
+ USE_FEATURE_HDPARM_GET_IDENTITY(get_IDentity |= (c == 'I'));
+ USE_FEATURE_HDPARM_GET_IDENTITY(if (c == 'i') opts|=get_identity;)
+ if (c == 'g')
+ opts|=get_geom;
+ if (c == 'f')
+ opts|=do_flush;
if (c == 'q') {
- quiet = 1;
- noisy = 0;
+ opts|=opt_quiet;
+ opts^=opt_noisy;
}
if (c == 'u') parse_opts(&get_unmask, &set_unmask, &unmask, 0, 1);
USE_FEATURE_HDPARM_HDIO_GETSET_DMA(if (c == 'd') parse_opts(&get_dma, &set_dma, &dma, 0, 9));
@@ -2229,8 +2257,15 @@ int hdparm_main(int argc, char **argv)
if (c == 'k') parse_opts(&get_keep, &set_keep, &keep, 0, 1);
if (c == 'a') parse_opts(&get_readahead, &set_readahead, &Xreadahead, 0, INT_MAX);
if (c == 'B') parse_opts(&get_apmmode, &set_apmmode, &apmmode, 1, 255);
- do_flush |= do_timings |= (c == 't');
- do_flush |= do_ctimings |= (c == 'T');
+ if (c == 't') {
+ opts|=opt_do_timings;
+ opts|=opt_do_flush;
+ }
+ if (c == 'T') {
+ opts|=opt_do_ctimings;
+ opts|=opt_do_flush;
+ }
+
#ifdef HDIO_DRIVE_CMD
if (c == 'S') parse_opts(&get_standby, &set_standby, &standby_requested, 0, INT_MAX);
if (c == 'D') parse_opts(&get_defects, &set_defects, &defects, 0, INT_MAX);
More information about the busybox
mailing list