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

Bernhard Fischer rep.nop at aon.at
Sat May 13 16:43:59 UTC 2006


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?

Also, we should use "strng" more often. I renamed 's' to strng for better
grep'ability

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?

>
>Thanks and Ciao,
>Tito
>
>PS.: Rob, I know the patch is rather big, but it does mostly trivial stuff, the only real changes
>       are the bugfix in main and the timing algorithm update in do_time(). Sorry!
>       If you really, really, really want this patch splitted in pieces....... i will do it.
>


>_______________________________________________
>busybox mailing list
>busybox at busybox.net
>http://busybox.net/cgi-bin/mailman/listinfo/busybox
-------------- next part --------------
--- miscutils/hdparm.c.10	2006-05-13 17:04:56.000000000 +0200
+++ miscutils/hdparm.c.11a	2006-05-13 18:25:24.000000000 +0200
@@ -560,7 +560,7 @@ static void print_ascii(uint16_t *p, uin
 			break;
 		if ((cl = (char) 0x00ff&(*p)) != ' ')
 		{
-			if (cl != '\0') printf("%c",cl);
+			if (cl != '\0') putchar(cl);
 			p++;
 			ii++;
 			break;
@@ -575,7 +575,7 @@ static void print_ascii(uint16_t *p, uin
 		printf("%c%c",(char)0x00ff&((*p)>>8),(char)(*p)&0x00ff);
 		p++;
 	}
-	printf("\n");
+	putchar('\n');
 }
 
 /* identify() is the only extern function used across two source files.  The
@@ -592,7 +592,7 @@ static void identify(uint16_t *id_suppli
 	uint8_t  chksum = 0;
 	uint32_t ll, mm, nn, oo;
 	uint64_t bbbig; /* (:) */
-	const char *s;
+	const char *strng;
 
 	if (BB_BIG_ENDIAN) {
 		swab(id_supplied, buf, sizeof(buf));
@@ -602,7 +602,7 @@ static void identify(uint16_t *id_suppli
 	chksum &= 0xff;
 
 	/* check if we recognise the device type */
-	printf("\n");
+	putchar('\n');
 	if(!(val[GEN_CONFIG] & NOT_ATA))
 	{
 		dev = ATA_DEV;
@@ -638,9 +638,9 @@ static void identify(uint16_t *id_suppli
 	{
 		like_std = 5;
 		if((val[CONFIG]==STBY_NID_VAL) || (val[CONFIG]==STBY_ID_VAL))
-			printf("powers-up in standby; SET FEATURES subcmd spins-up.\n");
+			puts("powers-up in standby; SET FEATURES subcmd spins-up.");
 		if(((val[CONFIG]==STBY_NID_VAL) || (val[CONFIG]==PWRD_NID_VAL)) && (val[GEN_CONFIG] & INCOMPLETE))
-			printf("\n\tWARNING: ID response incomplete.\n\tFollowing data may be incorrect.\n\n");
+			puts("\n\tWARNING: ID response incomplete.\n\tFollowing data may be incorrect.\n");
 	}
 
 	/* output the model and serial numbers and the fw revision */
@@ -734,7 +734,7 @@ static void identify(uint16_t *id_suppli
 		else if (like_std > std)
 			printf("& some of %u\n",like_std);
 		else
-			printf("\n");
+			putchar('\n');
 	}
 	else
 	{
@@ -764,7 +764,7 @@ static void identify(uint16_t *id_suppli
 	if (min_std == 0xffff)
 		min_std = like_std > 4 ? like_std - 3 : 1;
 
-	printf("Configuration:\n");
+	puts("Configuration:");
 	/* more info from the general configuration word */
 	if ((eqpt != CDROM) && (like_std == 1))
 	{
@@ -777,23 +777,24 @@ static void identify(uint16_t *id_suppli
 	}
 	if (dev == ATAPI_DEV)
 	{
+		printf("\tDRQ response: "); /* Data Request (DRQ) */
 		if ((val[GEN_CONFIG] & DRQ_RESPONSE_TIME) ==  DRQ_3MS_VAL)
-			s = "3ms";
+			strng = "3ms";
 		else if ((val[GEN_CONFIG] & DRQ_RESPONSE_TIME) ==  DRQ_INTR_VAL)
-			s = "<=10ms with INTRQ";
+			strng = "<=10ms with INTRQ";
 		else if ((val[GEN_CONFIG] & DRQ_RESPONSE_TIME) ==  DRQ_50US_VAL)
-			s ="50us";
-		else 
-			s = "Unknown";
-		printf("\tDRQ response: %s\n", s); /* Data Request (DRQ) */
+			strng = "50us";
+		else
+			strng = "Unknown";
 
+		printf("%s\tPacket size: ", strng);
 		if ((val[GEN_CONFIG] & PKT_SIZE_SUPPORTED) == PKT_SIZE_12_VAL)
-			s = "12 bytes";
+			strng = "12 bytes";
 		else if ((val[GEN_CONFIG] & PKT_SIZE_SUPPORTED) == PKT_SIZE_16_VAL)
-			s = "16 bytes";
+			strng = "16 bytes";
 		else
-			s = "Unknown";
-		printf("\tPacket size: %s\n", s);
+			strng = "Unknown";
+		puts(strng);
 	}
 	else
 	{
@@ -801,7 +802,7 @@ static void identify(uint16_t *id_suppli
 		ll = (uint32_t)val[LBA_SECTS_MSB] << 16 | val[LBA_SECTS_LSB];
 		mm = 0; bbbig = 0;
 		if ( (ll > 0x00FBFC10) && (!val[LCYLS]))
-			printf("\tCHS addressing not supported\n");
+			puts("\tCHS addressing not supported");
 		else
 		{
 			jj = val[WHATS_VALID] & OK_W54_58;
@@ -845,8 +846,8 @@ static void identify(uint16_t *id_suppli
 
 		if (bbbig > 1000)
 			printf("(%llu GB)\n", bbbig/1000);
-		else 
-			printf("\n");
+		else
+			putchar('\n');
 	}
 
 	/* hw support of commands (capabilities) */
@@ -867,10 +868,10 @@ static void identify(uint16_t *id_suppli
 			printf("(can");
 		else
 			printf("(cannot");
-		printf(" be disabled)\n");
+		puts(" be disabled)");
 	}
 	else
-		printf("no IORDY\n");
+		puts("no IORDY");
 
 	if ((like_std == 1) && val[BUF_TYPE])
 	{
@@ -882,25 +883,15 @@ static void identify(uint16_t *id_suppli
 			printf("dual port, multi-sector");
 
 		if (kk > 2) printf(" with read caching ability");
-		printf("\n");
+		putchar('\n');
 	}
-	jj = 0;
+
 	if ((min_std == 1) && (val[BUFFER__SIZE] && (val[BUFFER__SIZE] != NOVAL_1)))
-	{
-		printf("\tBuffer size: %.1fkB",(float)val[BUFFER__SIZE]/2);
-		jj = 1;
-	}
+		printf("\tBuffer size: %.1fkB\n",(float)val[BUFFER__SIZE]/2);
 	if ((min_std < 4) && (val[RW_LONG]))
-	{
-		printf("\tbytes avail on r/w long: %u",val[RW_LONG]);
-		jj = 1;
-	}
+		printf("\tbytes avail on r/w long: %u\n",val[RW_LONG]);
 	if ((eqpt != CDROM) && (like_std > 3))
-	{
-		printf("\tQueue depth: %u",(val[QUEUE_DEPTH] & DEPTH_BITS)+1);
-		jj = 1;
-	}
-	if (jj) printf("\n");
+		printf("\tQueue depth: %u\n",(val[QUEUE_DEPTH] & DEPTH_BITS)+1);
 
 	if (dev == ATA_DEV)
 	{
@@ -923,7 +914,7 @@ static void identify(uint16_t *id_suppli
 			if (val[SECTOR_XFER_CUR] & MULTIPLE_SETTING_VALID)
 				printf("%u\n", val[SECTOR_XFER_CUR] & SECTOR_XFER);
 			else
-				printf("?\n");
+				puts("?");
 		}
 		if ((like_std > 3) && (val[CMDS_SUPP_1] & 0x0008))
 		{
@@ -948,21 +939,21 @@ static void identify(uint16_t *id_suppli
 	{
 		 /* ATAPI */
 		if (eqpt != CDROM && (val[CAPAB_0] & SWRST_REQ))
-			printf("\tATA sw reset required\n");
+			puts("\tATA sw reset required");
 
 		if (val[PKT_REL] || val[SVC_NBSY])
 		{
 			printf("\tOverlap support:");
 			if (val[PKT_REL]) printf(" %uus to release bus.",val[PKT_REL]);
 			if (val[SVC_NBSY]) printf(" %uus to clear BSY after SERVICE cmd.",val[SVC_NBSY]);
-			printf("\n");
+			putchar('\n');
 		}
 	}
 
 	/* DMA stuff. Check that only one DMA mode is selected. */
 	printf("\tDMA: ");
 	if (!(val[CAPAB_0] & DMA_SUP))
-		printf("not supported\n");
+		puts("not supported");
 	else
 	{
 		if (val[DMA_MODE] && !val[SINGLE_DMA] && !val[MULTI_DMA])
@@ -1013,16 +1004,15 @@ static void identify(uint16_t *id_suppli
 			if (jj & 0x0001) printf("pio%d ",ii);
 			jj >>=1;
 		}
-		printf("\n");
 	}
 	else if (((min_std < 5) || (eqpt == CDROM)) && (val[PIO_MODE] & MODE) )
 	{
 		for (ii = 0; ii <= val[PIO_MODE]>>8; ii++)
 			printf("pio%d ",ii);
-		printf("\n");
 	}
 	else
-		printf("unknown\n");
+		printf("unknown");
+	putchar('\n');
 
 	if (val[WHATS_VALID] & OK_W64_70)
 	{
@@ -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)
 	{
-		printf("HW reset results:\n\tCBLID- %s Vih\n", (val[HWRST_RSLT] & CBLID) ? "above" : "below");
+		oo = jj & RST0;
+		printf("HW reset results:\n\tCBLID- %s Vih\n\tDevice num = %i",
+				(jj & CBLID) ? "above" : "below",
+				!oo);
 
-		if (val[HWRST_RSLT] & RST0)
-		{
-			printf("\tDevice num = 0");
-			jj = val[HWRST_RSLT];
-		}
-		else
-		{
-			printf("\tDevice num = 1");
-			jj = val[HWRST_RSLT] >> 8;
-		}
+		if (!oo)
+			jj >>= 8;
 
 		if ((jj & DEV_DET) == JUMPER_VAL)
 			printf(" determined by the jumper");
@@ -1121,12 +1107,13 @@ static void identify(uint16_t *id_suppli
 	/* more stuff from std 5 */
 	if ((like_std > 4) && (eqpt != CDROM))
 	{
-		if (val[CFA_PWR_MODE] & VALID_W160)
+		jj = val[CFA_PWR_MODE];
+		if (jj & VALID_W160)
 		{
-			printf("CFA power mode 1:\n\t%s%s\n", (val[CFA_PWR_MODE] & PWR_MODE_OFF) ? "disabled" : "enabled",
-										(val[CFA_PWR_MODE] & PWR_MODE_REQ) ? " and required by some commands" : "");
+			printf("CFA power mode 1:\n\t%s%s\n", (jj & PWR_MODE_OFF) ? "disabled" : "enabled",
+										(jj & PWR_MODE_REQ) ? " and required by some commands" : "");
 
-			if (val[CFA_PWR_MODE] & MAX_AMPS) printf("\tMaximum current = %uma\n",val[CFA_PWR_MODE] & MAX_AMPS);
+			if (jj & MAX_AMPS) printf("\tMaximum current = %uma\n", jj & MAX_AMPS);
 		}
 		if ((val[INTEGRITY] & SIG) == SIG_VAL)
 		{


More information about the busybox mailing list