Strangeness in hdparm.c?
Rob Landley
rob at landley.net
Tue May 30 06:07:57 UTC 2006
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.
More information about the busybox
mailing list