svn commit: r250220 - head/sys/kern

[John Baldwin]

On Thursday, May 09, 2013 11:05:46 pm Bruce Evans wrote:
> 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.

I'll do only style fixes first and then the overflow check.

> > (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.

The problem is this code in fcntl() which I ran into when working on a
tutorial for writing character devices:

	case F_SETFL:
		error = fget_unlocked(fdp, fd, CAP_FCNTL, F_SETFL, &fp, NULL);
		if (error != 0)
			break;
		do {
			tmp = flg = fp->f_flag;
			tmp &= ~FCNTLFLAGS;
			tmp |= FFLAGS(arg & ~O_ACCMODE) & FCNTLFLAGS;
		} while(atomic_cmpset_int(&fp->f_flag, flg, tmp) == 0);
		tmp = fp->f_flag & FNONBLOCK;
		error = fo_ioctl(fp, FIONBIO, &tmp, td->td_ucred, td);
		if (error != 0) {
			fdrop(fp, td);
			break;
		}
		tmp = fp->f_flag & FASYNC;
		error = fo_ioctl(fp, FIOASYNC, &tmp, td->td_ucred, td);
		if (error == 0) {
			fdrop(fp, td);
			break;
		}
		atomic_clear_int(&fp->f_flag, FNONBLOCK);
		tmp = 0;
		(void)fo_ioctl(fp, FIONBIO, &tmp, td->td_ucred, td);
		fdrop(fp, td);
		break;

Hmm, this seems to have the bug that if you had FNONBLOCK set and
tried to set FASYNC via fcntl() but FIOASYNC isn't supported,
FNONBLOCK is cleared.  It seems we should only clear FNONBLOCK
if it wasn't set in 'flg'.  I think this would fix that:

Index: kern_descrip.c
===================================================================
--- kern_descrip.c	(revision 250451)
+++ kern_descrip.c	(working copy)
@@ -539,7 +539,7 @@ kern_fcntl(struct thread *td, int fd, int cmd, int
 		}
 		tmp = fp->f_flag & FASYNC;
 		error = fo_ioctl(fp, FIOASYNC, &tmp, td->td_ucred, td);
-		if (error == 0) {
+		if (error == 0 || (flg & FNONBLOCK)) {
 			fdrop(fp, td);
 			break;
 		}

This would mean you would no longer have to support the FIOASYNC
ioctl just to allow FIONBLOCK to be set.

Note, btw, that kern_ioctl() doesn't enforce quite the same
requirement as F_SETFL in that FIONBIO doesn't check for FASYNC and vice
versa.  ioctl() also doesn't revert the change if the backing fo_ioctl
method fails.

I actually think that fcntl should probably ignore ENOTTY errors
if the flag is not set.  Otherwise using fcntl to toggle some other
flag can succeed but still return an error.

-- 
John Baldwin