Re: [PATCH 4/5] SCSI: fix bad method call in scsi_target_destroy

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

On Fri, 2010-02-19 at 14:53 -0500, Alan Stern wrote:
> On Fri, 19 Feb 2010, James Bottomley wrote:
> 
> > On Fri, 2010-02-12 at 12:14 -0500, Alan Stern wrote:
> > > This patch (as1336) fixes a bug in scsi_target_destroy().  It calls
> > > the host template's target_destroy method even if the target_alloc
> > > method failed.  (Also, the target_destroy method is called inside the
> > > scope of the host lock, which is unnecessary and probably a mistake.)
> > 
> > OK, so this isn't a mistake.  The act of create/destroy on the target
> > and removing it from the lists has to be atomic for devices which use
> > fixed slots (most SPI cards).  If the target is still in the list after
> > destroy, it could end up getting spuriously reused.  Conversely, if you
> > remove it from the list and then destroy, we could end up getting target
> > alloc for a new target called *before* the target_destroy of the old
> > one.
> 
> If the rest of the system is designed properly, targets won't get 
> spuriously reused.  Of course, that's a big "if" -- but it's more 
> relevant to the patch 5/5 discussion than to this one.
> 
> If you would prefer, I can revise this patch leaving the target_destroy
> method call within the spinlocked region.
> 
> > > A new flag is added to struct scsi_target to remember whether or not
> > > the target_alloc has succeeded.  There also are a couple of minor
> > > whitespace formatting improvements.
> > 
> > I don't really see the need for this.  None of the users assumes the
> > target was created ... if they do allocations, they all check before
> > unwinding.  If there is a case for needing this, it should be part of
> > the target state flags.
> 
> Maybe none of the current users make this assumption, but future users
> might.  It's only natural to assume that the core won't call
> your target_destroy function if target_alloc returned an error.  And 
> neither of these methods is mentioned in 
> Documentation/scsi/scsi_mid_low_api.txt, so the unnatural calling 
> sequence isn't documented.

So documenting it would be fine ...

> Some time ago I did post a patch adding an new state (STARGET_NEW) in
> order to record whether or not target_alloc had failed.  I can't find
> your response in the email archives, but I vaguely recall you saying
> that adding a new state to the state machine wasn't an appropriate
> approach.
> 
> What do you think is the best way to handle this?

Just add documentation ... there's no real need for a new state or a
flag.

James