Re: sys/dev/scsipi/scsipi_base.c

[Manuel Bouyer <bouyer%antioche.eu.org@localhost>]

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

A "gratuitous padding" here would break anyway, as this structure is
filled in by hardware (it's what's returned by a device to an INQUIRY
command).

-- 
Manuel Bouyer <bouyer%antioche.eu.org@localhost>
     NetBSD: 26 ans d'experience feront toujours la difference
--