[illumos-Developer] webrev: 1282 nsmb_open has a race

[Garrett D'Amore]

On Wed, 2011-07-27 at 18:04 -0400, Gordon Ross wrote:
> On Wed, Jul 27, 2011 at 5:02 PM, Garrett D'Amore <garrett at damore.org> wrote:
> > Instead of
> >  279         case DDI_INFO_DEVT2INSTANCE:
> >  280                 *result = NULL;
> >
> > I would prefer *result = nsmb_dip ? ddi_get_instance(nsmb_dip) : NULL;
> 
> Why that preference?  Several other pseudo drivers do this, i.e.
>   io/log.c io/ksyms.c io/sysevent.c
> 
> > Better yet, why not simply decode the minor number, like this:
> >
> >        ddi_get_minor(dev);
> 
> The major can vary too, in theory.  It's tricky code.

The major changing should not matter one way or the other...  Generally
its a mistake to ever consider the *major* part of a dev_t.

> 
> The code to get major+minor was originally copied from...
> well, I forget which place, exactly, but here's an example:
>   http://bugs.ec.illumos.org:8080/source/xref/illumos-gate/usr/src/uts/common/fs/zfs/zfs_vfsops.c#194
> 
> > (On that note, using soft state, like you had before, but dereferencing
> > the soft state by minor number, seems like the more clean solution,
> > rather than relying on the only one instance rule.)
> 
> I'm not sure I understand this suggestion.
> It is getting the soft state by minor, right?
> That's in nsmb_open, not attach.  It's here:
>  http://bugs.ec.illumos.org:8080/source/xref/illumos-gate/usr/src/uts/common/fs/smbclnt/netsmb/smb_dev.c#480
> 
> The code I took out (getting a softc for unit zero)
> has nothing to do with the normal code path in this
> driver where it gets a (per-unit) softc object.

Your new code insists that the unit is always zero.  It will be easier
to explain this in a phone call.

	- Garrett

> 
> Would appreciate clarifications.
> 
> Thanks,
> Gordon