Re: [PATCH] scsi: Add VPD helper

[Matthew Wilcox <matthew@wil.cx>]

On Thu, Jan 01, 2009 at 11:19:33AM +0200, Boaz Harrosh wrote:
> +	buffer[1] = -1;
> see below
> > +	/*
> > +	 * I'm not convinced we need to try quite this hard to get VPD, but
> > +	 * all the existing users tried this hard.
> > +	 */
> > +	result = scsi_execute_req(sdev, cmd, DMA_FROM_DEVICE, buffer,
> > +				  len + 4, NULL, 30 * HZ, 3, NULL);
> > +	if (result)
> > +		return result;
> > +
> > +	/* Sanity check that we got the page back that we asked for */
> > +	if (buffer[1] != page)
> > +		return -EIO;
> 
> Maybe it's just me but, if you are going to if() on a buffer byte, then you need
> to make sure its initial value. Or at least document the need for a -1 at
> buffer[1]

buffer[] is an array of unsigned char, so -1 is 255.  We could be asking
for page 0xff, so your initialisation doesn't help reject bogus data.

Your concern here, presumably, is that the device might transfer less than
two bytes, and not return an error.  I think this is quite unlikely --
and in any case, such a misbehaving device is really no different from a
device which transfers complete garbage.

I suppose we could pass a 'resid' to scsi_execute_req and check that at
least four bytes was transferred.

-- 
Matthew Wilcox				Intel Open Source Technology Centre
"Bill, look, we understand that you're interested in selling us this
operating system, but compare it to ours.  We can't possibly take such
a retrograde step."