On Sun, Jan 06, 2008 at 06:08:43AM -0800, Paul Goyette wrote: > On Sun, 6 Jan 2008, John Nemeth wrote: > > >On May 29, 5:30am, David Laight wrote: > >} On Sun, Jan 06, 2008 at 03:41:49AM -0700, John R. Shannon wrote: > >} > Line 1124 has: > >} > > >} > memcpy(inqbuf->vendor, "ADAPTEC ACB-4000 ", 28); > >} > > >} > and line 1144 has: > >} > > >} > memcpy(inqbuf->vendor, "EMULEX MT-02 QIC ", 28); > >} > > >} > yet inqbuf->vendor is declared in struct scsipi_inquiry_data as: > >} > > >} > char vendor[8]; > >} > >} and is followed by: > >} char product[16]; > >} char revision[4]; > >} so the memcpy updates all 3 fields :-) > > > > That is extremely grotty code! > > Shouldn't we at least replace the constant 28 with a macro that gives a > hint of what's going on? Yes. This also would help w/ smb's comment about cscope'ing variables. When you see the definitio below, it'll let a reader know s/he needs to add INQBUF_TRIPLET_SIZE to the search list. > #define INQBUF_TRIPLET_SIZE (sizeof(inqbuf->vendor) + \ > sizeof(inqbuf->product) + \ > sizeof(inqbuf->revision)) > > It's still going to be grotty code, and won't address the issue of a > "perverse introduction of gratuitous padding" mentioned in another > message in this thread, but at least a reader would have some idea that > the constant used in the memcpy() has some "interesting" implications. The thing is that if the padding changes, the whole SCSI INQUIRY processing code fails. This structure represents data sent on the wire by SCSI (and ATAPI I gather) commands. Mess the padding up, and the operation explodes. Explodes. :-) So we're more than safe in trusting that it works. Take care, Bill
Attachment:
pgpmddpJMhSU1.pgp
Description: PGP signature