Subject: Re: sys/dev/scsipi/scsipi_base.c
To: John Nemeth <jnemeth@victoria.tc.ca>
From: Paul Goyette <paul@whooppee.com>
List: current-users
Date: 01/06/2008 06:08:43
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?

#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.

:)


----------------------------------------------------------------------
|   Paul Goyette   | PGP DSS Key fingerprint: |  E-mail addresses:   |
| Customer Service | FA29 0E3B 35AF E8AE 6651 |  paul@whooppee.com   |
| Network Engineer | 0786 F758 55DE 53BA 7731 | pgoyette@juniper.net |
----------------------------------------------------------------------