Date: Sun, 8 Dec 2002 18:27:06 +0300
From: Yar Tikhiy <yar@freebsd.org>
To: Nate Lawson <nate@root.org>
Cc: freebsd-scsi@freebsd.org
Subject: Re: {da,sa,...}open bug?
Message-ID: <20021208182706.B75509@comp.chem.msu.su>
In-Reply-To: <Pine.BSF.4.21.0212061121290.15885-100000@root.org>; from nate@root.org on Fri, Dec 06, 2002 at 11:23:56AM -0800
References: <20021206145942.I80257@comp.chem.msu.su> <Pine.BSF.4.21.0212061121290.15885-100000@root.org>
next in thread | previous in thread | raw e-mail | index | archive | help
On Fri, Dec 06, 2002 at 11:23:56AM -0800, Nate Lawson wrote:
> On Fri, 6 Dec 2002, Yar Tikhiy wrote:
> > Yet another daopen() issue came to my attention. The DA_OPEN_FLAG bit
> > won't be reset back to 0 if daopen() has failed, thus leaving the device
> > marked as open. Isn't the below patch necessary?
> >
> > The rest of scsi_* modules seem to not have this bug.
> >
> > --
> > Yar
> >
> > Index: scsi_da.c
> > ===================================================================
> > RCS file: /home/ncvs/src/sys/cam/scsi/scsi_da.c,v
> > retrieving revision 1.116
> > diff -u -r1.116 scsi_da.c
> > --- scsi_da.c 29 Nov 2002 15:40:10 -0000 1.116
> > +++ scsi_da.c 6 Dec 2002 11:49:57 -0000
> > @@ -612,6 +612,7 @@
> > if ((softc->flags & DA_FLAG_PACK_REMOVABLE) != 0)
> > daprevent(periph, PR_PREVENT);
> > } else {
> > + softc->flags &= ~DA_FLAG_OPEN;
> > cam_periph_release(periph);
> > }
> > cam_periph_unlock(periph);
>
> You are correct. Ok to commit this w/ re@ approval The other periphs
> don't use this flag.
While writing a message to re@, I thought if we were propagating
bad style with such patches. Wouldn't it be better to set the open
flag and acquire the periph if and only if returning success? The
open flag seems to be of no use before the return from daopen().
I.e.,
Index: scsi_da.c
===================================================================
RCS file: /home/ncvs/src/sys/cam/scsi/scsi_da.c,v
retrieving revision 1.116
diff -u -r1.116 scsi_da.c
--- scsi_da.c 29 Nov 2002 15:40:10 -0000 1.116
+++ scsi_da.c 8 Dec 2002 15:12:51 -0000
@@ -545,10 +545,6 @@
if ((error = cam_periph_lock(periph, PRIBIO|PCATCH)) != 0)
return (error); /* error code from tsleep */
- if (cam_periph_acquire(periph) != CAM_REQ_CMP)
- return(ENXIO);
- softc->flags |= DA_FLAG_OPEN;
-
if ((softc->flags & DA_FLAG_PACK_INVALID) != 0) {
/* Invalidate our pack information. */
disk_invalidate(&softc->disk);
@@ -607,12 +603,15 @@
softc->device_stats.flags &= ~DEVSTAT_BS_UNAVAILABLE;
}
}
+
+ if (error == 0)
+ if (cam_periph_acquire(periph) != CAM_REQ_CMP)
+ error = ENXIO;
if (error == 0) {
+ softc->flags |= DA_FLAG_OPEN;
if ((softc->flags & DA_FLAG_PACK_REMOVABLE) != 0)
daprevent(periph, PR_PREVENT);
- } else {
- cam_periph_release(periph);
}
cam_periph_unlock(periph);
return (error);
===================================================================
Notice how we kill two birds with one stone: The "no unlock on error"
bug is gone as well.
--
Yar
To Unsubscribe: send mail to majordomo@FreeBSD.org
with "unsubscribe freebsd-scsi" in the body of the message
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20021208182706.B75509>