Re: [PATCH] scsi core: fix uninitialized variable error

[Matthew Wilcox <matthew@wil.cx>]

On Sun, Feb 05, 2006 at 11:11:24AM -0500, Alan Stern wrote:
> > -	if (scsi_host_scan_allowed(shost)) {
> > -		res = scsi_probe_and_add_lun(starget, lun, NULL, &sdev, 1,
> > -					     hostdata);
> > -		if (res != SCSI_SCAN_LUN_PRESENT)
> > -			sdev = ERR_PTR(-ENODEV);
> > -	}
> > +	if (scsi_host_scan_allowed(shost))
> > +		scsi_probe_and_add_lun(starget, lun, NULL, &sdev, 1, hostdata);
> >  	mutex_unlock(&shost->scan_mutex);
> 
> This assumes that scsi_probe_and_add_lun doesn't assign anything to the 
> &sdev pointer if it returns anything other than SCSI_SCAN_LUN_PRESENT.  
> Since that assumption is true for the current code, this version of the 
> patch will work as well as mine.

Perhaps the better way to think about this usage of
scsi_probe_and_add_lun() is "If it finds an sdev, then we should return
it".  Right now, we're assuming that returning SCSI_SCAN_LUN_PRESENT is
equivalent to having found an sdev.

The real problem is that scsi_probe_and_add_lun() has an enormously
complicated interface.  The good news is that it's static, so we can see
all its callers.  The bad news is that the kernel-doc comment is out of
date and not terribly helpful.

Its callers are:

scsi_sequential_lun_scan()
	scsi_probe_and_add_lun(starget, lun, NULL, NULL, rescan, NULL)
		(!= SCSI_SCAN_LUN_PRESENT)
scsi_report_lun_scan()
	scsi_probe_and_add_lun(starget, lun, NULL, NULL, rescan, NULL)
		(== SCSI_SCAN_NO_RESPONSE)
__scsi_add_device()
	scsi_probe_and_add_lun(starget, lun, NULL, &sdev, 1, hostdata)
		(== SCSI_SCAN_LUN_PRESENT in current, unused in my patch)
__scsi_scan_target()
	scsi_probe_and_add_lun(starget, lun, NULL, NULL, rescan, NULL)
		(unused)
	scsi_probe_and_add_lun(starget, 0, &bflags, NULL, rescan, NULL)
		(== SCSI_SCAN_LUN_PRESENT or SCSI_SCAN_TARGET_PRESENT)

I can see some ways to simplify this interface.  As noted in a comment:

                 * XXX add a bflags to scsi_device, and replace the
                 * corresponding bit fields in scsi_device, so bflags
                 * need not be passed as an argument.

I *think* we can get rid of the hostdata parameter.  The only non-NULL
caller is __scsi_add_device().  The only caller of __scsi_add_device()
which specifies hostdata is i2o_scsi.  I don't see why it can't use a
->slave_alloc in the host template to set hostdata rather than passing
it in.

That'd get us down from a 6-argument function to a 4-argument one.  We
still have the problem of wanting to return multiple things (a SCSI_SCAN
constant in most cases and an sdev in another).  Maybe we could do something
like ERR_PTR/IS_ERR ...