Re: [PATCH] zfcp: Test kmalloc failure in scsi_get_vpd_page()

[James Bottomley <James.Bottomley@suse.de>]

On Sun, 2009-08-30 at 14:45 +0300, Boaz Harrosh wrote:
> On 08/28/2009 02:45 AM, James Bottomley wrote:
> > On Mon, 2009-08-24 at 18:21 +0200, Roel Kluin wrote:
> >> kmalloc() may fail, so test whether it succeeded.
> >>
> >> Signed-off-by: Roel Kluin <roel.kluin@gmail.com>
> >> ---
> >> diff --git a/drivers/scsi/scsi.c b/drivers/scsi/scsi.c
> >> index 2de5f3a..34fdde0 100644
> >> --- a/drivers/scsi/scsi.c
> >> +++ b/drivers/scsi/scsi.c
> >> @@ -1056,6 +1056,9 @@ unsigned char *scsi_get_vpd_page(struct scsi_device *sdev, u8 page)
> >>  
> >>  	kfree(buf);
> >>  	buf = kmalloc(len + 4, GFP_KERNEL);
> >> +	if (!buf)
> >> +		return NULL;
> >> +
> > 
> > Firstly, this won't actually apply ... you should be developing against
> > either the SCSI trees or linux-next to get the latest versions in git.
> > 
> > Secondly it's not really right for the most common use cases, which
> > don't usually want the whole vpd buffer anyway.  I don't really see a
> > simple way of fixing it without altering the interface, though.
> > 
> 
> "most common use cases" do not have pages bigger then 255. Can you think
> of any? For these few places that anticipate pages bigger then 255 I don't
> see much choice, we just freed 255 bytes and tried a new size, say 317, which
> failed with GFP_KERNEL, In such a busy system, better fail with NULL then BUG,
> No? In any way caller must check for NULL because of the first allocation.

Other way around: common use cases means the callers.  What I'm saying
is that regardless of the size of the VPD page, the caller usually only
wants a few bytes into it.  Once we have all the information the caller
ever needs, there's no point in trying again with a larger buffer just
because the VPD page is larger ... to code this into the interface,
though, the caller would need to specify the max length it is looking
for.

James