Date: Fri, 10 May 2013 13:05:46 +1000 (EST) From: Bruce Evans <brde@optusnet.com.au> To: John Baldwin <jhb@freebsd.org> Cc: svn-src-head@freebsd.org, svn-src-all@freebsd.org, Bruce Evans <brde@optusnet.com.au> Subject: Re: svn commit: r250220 - head/sys/kern Message-ID: <20130510123842.B637@etaplex.bde.org> In-Reply-To: <201305061355.20826.jhb@freebsd.org> References: <201305031908.r43J8xnI094418@svn.freebsd.org> <20130504184721.E619@etaplex.bde.org> <201305061355.20826.jhb@freebsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20130510123842.B637>