Re: sys/dev/scsipi/scsipi_base.c

["Steven M. Bellovin" <smb%cs.columbia.edu@localhost>]

On Sun, 6 Jan 2008 18:40:30 +0200
Alan Barrett <apb%cequrux.com@localhost> wrote:

> On Sun, 06 Jan 2008, Paul Goyette wrote:
> >> } > memcpy(inqbuf->vendor, "ADAPTEC ACB-4000            ", 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 :-)
> > Shouldn't we at least replace the constant 28 with a macro that
> > gives a hint of what's going on?
> 
> Or encapsulate it in a union with three separate strings on one side
> and a single string on the other side.
> 
> > 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,                                               
> 
> That problem is not worth worrying about in practice, given that we
> have so much code that assumes we know how the compiler will lay out
> structs in memory.
> 
There are two problems.  One, as you say, is the compiler; we *think*
we know what it will do in such cases.  The other problem is confused
programmers who want to know when certain fields are being referenced
or changed.  Let me put it in concrete terms: how often do you use grep
(or a more sophisticated tool) to find out where some variables are
touched?  I did it as recently as an hour ago -- and this sort of code
would hide variables from me.

A union is the best solution, especially if accompanied by an assertion
that the size of the single string equals the total sizes of the other
three strings.  After all, a good optimizer would delete the assertion,
since the condition would be constant if the compiler is doing what we
think...


                --Steve Bellovin, http://www.cs.columbia.edu/~smb