Strangeness in hdparm.c?

[Rob Landley]

On Monday 29 May 2006 10:01 am, Tito wrote:
> On Monday 29 May 2006 07:48, Rob Landley wrote:
> > The function identify_from_stdin() reads 1280 bytes, and then processes
> > 1600 bytes.  Huh?
> >
> > Rob
>
> Latest hdparm 6.6 does this:
>
> static int identify_from_stdin (void)
> {
> 	unsigned short sbuf[800];
> 	unsigned char  buf[1600], *b = (unsigned char *)buf;

Still declaring 1600 bytes...

> 	int i, count;
>
> 	// skip leading cruft
> 	while (1 == read(0, buf, 1) && (*buf < '0' || *buf > '9') && (*buf < 'a'
> || *buf > 'f')) while (1 == read(0, buf, 1) && *buf != '\n');

(side note: That's an ugly pair of nested loops.  A funky way to skip all 
lines that don't start with a hex digit.  Do we actually encounter "leading 
cruft" in real life, and do we care?  Since the old code seemed to get along 
fine without it, do we really care about whatever this broken device is?)

> 	count = read(0, buf+1, 1279);
> 	if (count != 1279) {
> 		fprintf(stderr, "read(1279 bytes) failed (rc=%d)", count);
> 		perror("");
> 		exit(errno);
> 	}

And dying if it can't read exactly 1280 bytes.  (It's just reading the first 
byte in the previous cruft-skipping bit, although where the cruft comes from 
I dunno.)  So it's still declaring 320 bytes more than it needs, and 
presumably a corresponding about of sbuf although who knows what identify() 
is doing with sbuf...

> 	for (i = 0; count >= 4; ++i) {
> 		sbuf[i] = (fromhex(b[0]) << 12) | (fromhex(b[1]) << 8) | (fromhex(b[2])
> << 4) | fromhex(b[3]); __le16_to_cpus((__u16 *)(&sbuf[i]));
> 		b += 5;
> 		count -= 5;
> 	}

Its' reading 4 byte chunks and then skipping a byte, which I can only assume 
is intentional.  (4 hex digits and a newline?)  But why on earth does it need 
_three_ counters for this loop?  There's got to be a less ugly way, even 
something like:

for (i = 0; i<256; i++) {
  sbuf[i] = SWAP_LE16((fromhex(b[0]) << 12) | (fromhex(b[1]) << 8) |
		(fromhex(b[2]) << 4) | (fromhex(b[3]));
  b +=5;
}

would be an improvement.  I mean we _know_ what count is here, it doesn't need 
to be tracked separately.  It's 256 * 5, which is how many sbuf entries are 
being filled out.  (So why did they allocate 300?)

> Should we busyboxify and sync?

Not if it's that ugly, the allocations still don't match up with what's being 
used, and we can't figure out the reason for the change.

Rob
-- 
Never bet against the cheap plastic solution.