From owner-svn-src-head@FreeBSD.ORG Fri May 10 03:12:10 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 15504193; Fri, 10 May 2013 03:12:10 +0000 (UTC) (envelope-from brde@optusnet.com.au) Received: from fallbackmx07.syd.optusnet.com.au (fallbackmx07.syd.optusnet.com.au [211.29.132.9]) by mx1.freebsd.org (Postfix) with ESMTP id A94E95EE; Fri, 10 May 2013 03:12:09 +0000 (UTC) Received: from mail10.syd.optusnet.com.au (mail10.syd.optusnet.com.au [211.29.132.191]) by fallbackmx07.syd.optusnet.com.au (8.13.1/8.13.1) with ESMTP id r4A363KB002733; Fri, 10 May 2013 13:06:03 +1000 Received: from etaplex.bde.org ([139.218.225.48]) (authenticated sender brde) by mail10.syd.optusnet.com.au (8.13.1/8.13.1) with ESMTP id r4A35dfF016972 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=NO); Fri, 10 May 2013 13:05:55 +1000 Date: Fri, 10 May 2013 13:05:46 +1000 (EST) From: Bruce Evans X-X-Sender: bde@etaplex.bde.org To: John Baldwin Subject: Re: svn commit: r250220 - head/sys/kern In-Reply-To: <201305061355.20826.jhb@freebsd.org> Message-ID: <20130510123842.B637@etaplex.bde.org> References: <201305031908.r43J8xnI094418@svn.freebsd.org> <20130504184721.E619@etaplex.bde.org> <201305061355.20826.jhb@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=e/de0tV/ c=1 sm=1 a=5CeyE4nVUmB46QZCKVgZLQ==:17 a=8aH1kwsY6JcA:10 a=kj9zAlcOel0A:10 a=PO7r1zJSAAAA:8 a=JzwRw_2MAAAA:8 a=M4roAWbnUW4A:10 a=BqGE20DC9-uUo3ouRuIA:9 a=CjuIK1q_8ugA:10 a=5CeyE4nVUmB46QZCKVgZLQ==:117 Cc: svn-src-head@freebsd.org, svn-src-all@freebsd.org, Bruce Evans 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: Fri, 10 May 2013 03:12:10 -0000 On Mon, 6 May 2013, John Baldwin wrote: > On Saturday, May 04, 2013 4:47:43 am Bruce Evans wrote: >>> 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. > > What about this: My mail is barely working while I'm away. This is the only reply that I received recently. > static int > vn_ioctl(fp, com, data, active_cred, td) > struct file *fp; > u_long com; > void *data; > struct ucred *active_cred; > struct thread *td; > { > struct vnode *vp = fp->f_vnode; > struct vattr vattr; > > switch (vp->v_type) { > case VREG: > case VDIR: > switch (com) { > case FIONREAD: > 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; > return (error); > case FIONBIO: > case FIOASYNC: > return (0); /* XXX */ > default: > return (VOP_IOCTL(vp, com, data, fp->f_flag, > active_cred, td)); > } > default: > return (ENOTTY); > } > } Yes, that is the simplification for the logic that I want. This function has many other style bugs, starting witht the unsorting of vp relative to vattr and the initialization of vp in its declaration. I don't count the old-style function definition as a style bug :). Also, the case statment is unsorted (which used to be needed for the logic of falling through), and !error is used for the non-boolean 'error'. !error is near the overflow bug in *(int *) for large files. INT_MAX should probably be returned when the correct value is unrepresentable. This can't be worse than overflowing to a garbage (possibly small or negative value). Testing 'error' is not even needed, since *(int *)data is indeterminate after an error. > (The 'XXX' comment could perhaps be expanded to something along the lines of > 'Allow fcntl() to toggle FNONBLOCK and FASYNC.') Is that what it is about? IIRC, upper layers do some partial handling and then call lower layers to possibly do some more handling. But here and in some other places there is nothing more to be done. No comment is needed, but maybe the XXX's were reminders to clean up the layering. FreeBSD has cleaned up the layering a bit, by using differnt fops for different file types. Bruce