Re: sys/dev/scsipi/scsipi_base.c

[jnemeth%victoria.tc.ca@localhost (John Nemeth)]

On May 29,  9:54am, Manuel Bouyer wrote:
} 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).

     The hardware returns a sequence of bytes not a struct.

}-- End of excerpt from Manuel Bouyer