From owner-svn-src-head@FreeBSD.ORG Sat May 4 12:56:14 2013 Return-Path: Delivered-To: svn-src-head@FreeBSD.org Received: from mx1.freebsd.org (mx1.FreeBSD.org [8.8.178.115]) by hub.freebsd.org (Postfix) with ESMTP id 0A72EFF7; Sat, 4 May 2013 12:56:14 +0000 (UTC) (envelope-from brde@optusnet.com.au) Received: from fallbackmx08.syd.optusnet.com.au (fallbackmx08.syd.optusnet.com.au [211.29.132.10]) by mx1.freebsd.org (Postfix) with ESMTP id 9F7871924; Sat, 4 May 2013 12:56:12 +0000 (UTC) Received: from mail35.syd.optusnet.com.au (mail35.syd.optusnet.com.au [211.29.133.51]) by fallbackmx08.syd.optusnet.com.au (8.13.1/8.13.1) with ESMTP id r448mGXj031476; Sat, 4 May 2013 18:48:17 +1000 Received: from etaplex.bde.org ([139.218.225.48]) (authenticated sender brde) by mail35.syd.optusnet.com.au (8.13.1/8.13.1) with ESMTP id r448lfjp003183 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=NO); Sat, 4 May 2013 18:48:12 +1000 Date: Sat, 4 May 2013 18:47:43 +1000 (EST) From: Bruce Evans X-X-Sender: bde@etaplex.bde.org To: jhb@FreeBSD.org Subject: Re: svn commit: r250220 - head/sys/kern In-Reply-To: <201305031908.r43J8xnI094418@svn.freebsd.org> Message-ID: <20130504184721.E619@etaplex.bde.org> References: <201305031908.r43J8xnI094418@svn.freebsd.org> MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII; format=flowed X-Optus-CM-Score: 0 X-Optus-CM-Analysis: v=2.0 cv=QtMGRiOd c=1 sm=1 a=5CeyE4nVUmB46QZCKVgZLQ==:17 a=8aH1kwsY6JcA:10 a=kj9zAlcOel0A:10 a=PO7r1zJSAAAA:8 a=JzwRw_2MAAAA:8 a=M4roAWbnUW4A:10 a=wdVBXaQ1PWgcX4Dgn0YA:9 a=CjuIK1q_8ugA:10 a=5CeyE4nVUmB46QZCKVgZLQ==:117 Cc: svn-src-head@FreeBSD.org, svn-src-all@FreeBSD.org X-BeenThere: svn-src-head@freebsd.org X-Mailman-Version: 2.1.14 Precedence: list List-Id: SVN commit messages for the src tree for head/-current List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Sat, 04 May 2013 12:56:14 -0000 > Log: > Fix FIONREAD on regular files. The computed result was being ignored and > it was being passed down to VOP_IOCTL() where it promptly resulted in > ENOTTY due to a missing else for the past 8 years. While here, use a > shared vnode lock while fetching the current file's size. In another thread I complained to kib about using the bad style of not returning as soon as possible, but instead sometimes returning and sometimes falling through a complicated if-else ladder to get to the return at the end. > Modified: head/sys/kern/vfs_vnops.c > ============================================================================== > --- head/sys/kern/vfs_vnops.c Fri May 3 18:58:37 2013 (r250219) > +++ head/sys/kern/vfs_vnops.c Fri May 3 19:08:58 2013 (r250220) > @@ -1364,13 +1364,12 @@ vn_ioctl(fp, com, data, active_cred, td) > case VREG: > case VDIR: > if (com == FIONREAD) { > - vn_lock(vp, LK_EXCLUSIVE | LK_RETRY); > + vn_lock(vp, LK_SHARED | LK_RETRY); > error = VOP_GETATTR(vp, &vattr, active_cred); > VOP_UNLOCK(vp, 0); > if (!error) > *(int *)data = vattr.va_size - fp->f_offset; Old versions were missing the bug because they just returned here. > - } > - if (com == FIONBIO || com == FIOASYNC) /* XXX */ > + } else if (com == FIONBIO || com == FIOASYNC) /* XXX */ > error = 0; This depended on the FIONREAD clause always returning. This was simpler since it just returned 0 instead of setting error and falling through a not-so-complicated if-else ladder to the return statement. But old versions used the bad style of falling through the default case for the VREG and VDIR cases. > else > error = VOP_IOCTL(vp, com, data, fp->f_flag, Better yet, in the old versions the default case was intended to return ENOTTY, but this was hacked out, so the VREG and VDIR cases that didn't already return fell through the null default statement into the VFIFO+VCHR+VBLK case that did essentially this ioctl, because a few file systems like cd9660 have some magic ioctls. So the old versions needed the earlier returns even more, to avoid special cases on falling through. The default case is now obfuscated by setting error to ENOTTY at the start of the function. After this fix, that setting applies only to the default case. The setting is missing the style bug if initializing error in its declaration. Only the initialization of vp has that bug. Bruce