Date: Fri, 10 Jul 2015 13:54:03 +0000 (UTC) From: Mateusz Guzik <mjg@FreeBSD.org> To: src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-head@freebsd.org Subject: svn commit: r285357 - in head/sys: kern sys Message-ID: <201507101354.t6ADs3hV086216@repo.freebsd.org>
next in thread | raw e-mail | index | archive | help
Author: mjg Date: Fri Jul 10 13:54:03 2015 New Revision: 285357 URL: https://svnweb.freebsd.org/changeset/base/285357 Log: fd: further cleanup of kern_dup - make mode enum start from 0 so that the assertion covers all cases [1] - rename prefix _CLOEXEC flag with _FLAG - postpone fhold on the old file descriptor, which eliminates the need to fdrop in error cases. - fixup FDDUP_FCNTL check missed in the previous commit This removes 'fp == oldfde->fde_file' assertion which had little value. kern_dup only calls fd-related functions which cannot drop the lock or a whole lot of races would be introduced. Noted by: kib [1] Modified: head/sys/kern/kern_descrip.c head/sys/sys/filedesc.h Modified: head/sys/kern/kern_descrip.c ============================================================================== --- head/sys/kern/kern_descrip.c Fri Jul 10 11:01:30 2015 (r285356) +++ head/sys/kern/kern_descrip.c Fri Jul 10 13:54:03 2015 (r285357) @@ -224,7 +224,6 @@ fd_last_used(struct filedesc *fdp, int s return (-1); } -#ifdef INVARIANTS static int fdisused(struct filedesc *fdp, int fd) { @@ -234,7 +233,6 @@ fdisused(struct filedesc *fdp, int fd) return ((fdp->fd_map[NDSLOT(fd)] & NDBIT(fd)) != 0); } -#endif /* * Mark a file descriptor as used. @@ -486,7 +484,7 @@ kern_fcntl(struct thread *td, int fd, in case F_DUPFD_CLOEXEC: tmp = arg; - error = kern_dup(td, FDDUP_FCNTL, FDDUP_CLOEXEC, fd, tmp); + error = kern_dup(td, FDDUP_FCNTL, FDDUP_FLAG_CLOEXEC, fd, tmp); break; case F_DUP2FD: @@ -496,7 +494,7 @@ kern_fcntl(struct thread *td, int fd, in case F_DUP2FD_CLOEXEC: tmp = arg; - error = kern_dup(td, FDDUP_FIXED, FDDUP_CLOEXEC, fd, tmp); + error = kern_dup(td, FDDUP_FIXED, FDDUP_FLAG_CLOEXEC, fd, tmp); break; case F_GETFD: @@ -794,14 +792,13 @@ kern_dup(struct thread *td, u_int mode, struct filedesc *fdp; struct filedescent *oldfde, *newfde; struct proc *p; - struct file *fp; struct file *delfp; int error, maxfd; p = td->td_proc; fdp = p->p_fd; - MPASS((flags & ~(FDDUP_CLOEXEC)) == 0); + MPASS((flags & ~(FDDUP_FLAG_CLOEXEC)) == 0); MPASS(mode < FDDUP_LASTMODE); /* @@ -812,26 +809,23 @@ kern_dup(struct thread *td, u_int mode, if (old < 0) return (EBADF); if (new < 0) - return (flags & FDDUP_FCNTL ? EINVAL : EBADF); + return (mode == FDDUP_FCNTL ? EINVAL : EBADF); maxfd = getmaxfd(td); if (new >= maxfd) - return (flags & FDDUP_FCNTL ? EINVAL : EBADF); + return (mode == FDDUP_FCNTL ? EINVAL : EBADF); FILEDESC_XLOCK(fdp); if (fget_locked(fdp, old) == NULL) { FILEDESC_XUNLOCK(fdp); return (EBADF); } - oldfde = &fdp->fd_ofiles[old]; if ((mode == FDDUP_FIXED || mode == FDDUP_MUSTREPLACE) && old == new) { td->td_retval[0] = new; - if (flags & FDDUP_CLOEXEC) + if (flags & FDDUP_FLAG_CLOEXEC) fdp->fd_ofiles[new].fde_flags |= UF_EXCLOSE; FILEDESC_XUNLOCK(fdp); return (0); } - fp = oldfde->fde_file; - fhold(fp); /* * If the caller specified a file descriptor, make sure the file @@ -843,20 +837,15 @@ kern_dup(struct thread *td, u_int mode, case FDDUP_FCNTL: if ((error = fdalloc(td, new, &new)) != 0) { FILEDESC_XUNLOCK(fdp); - fdrop(fp, td); return (error); } - newfde = &fdp->fd_ofiles[new]; break; case FDDUP_MUSTREPLACE: /* Target file descriptor must exist. */ - if (new >= fdp->fd_nfiles || - fdp->fd_ofiles[new].fde_file == NULL) { + if (fget_locked(fdp, new) == NULL) { FILEDESC_XUNLOCK(fdp); - fdrop(fp, td); return (EBADF); } - newfde = &fdp->fd_ofiles[new]; break; case FDDUP_FIXED: if (new >= fdp->fd_nfiles) { @@ -875,28 +864,24 @@ kern_dup(struct thread *td, u_int mode, PROC_UNLOCK(p); if (error != 0) { FILEDESC_XUNLOCK(fdp); - fdrop(fp, td); return (EMFILE); } } #endif fdgrowtable_exp(fdp, new + 1); - oldfde = &fdp->fd_ofiles[old]; } - newfde = &fdp->fd_ofiles[new]; - if (newfde->fde_file == NULL) + if (!fdisused(fdp, new)) fdused(fdp, new); break; -#ifdef INVARIANTS default: - newfde = NULL; /* silence the compiler */ KASSERT(0, ("%s unsupported mode %d", __func__, mode)); -#endif } - KASSERT(fp == oldfde->fde_file, ("old fd has been modified")); KASSERT(old != new, ("new fd is same as old")); + oldfde = &fdp->fd_ofiles[old]; + fhold(oldfde->fde_file); + newfde = &fdp->fd_ofiles[new]; delfp = newfde->fde_file; /* @@ -908,7 +893,7 @@ kern_dup(struct thread *td, u_int mode, filecaps_free(&newfde->fde_caps); memcpy(newfde, oldfde, fde_change_size); filecaps_copy(&oldfde->fde_caps, &newfde->fde_caps); - if ((flags & FDDUP_CLOEXEC) != 0) + if ((flags & FDDUP_FLAG_CLOEXEC) != 0) newfde->fde_flags = oldfde->fde_flags | UF_EXCLOSE; else newfde->fde_flags = oldfde->fde_flags & ~UF_EXCLOSE; Modified: head/sys/sys/filedesc.h ============================================================================== --- head/sys/sys/filedesc.h Fri Jul 10 11:01:30 2015 (r285356) +++ head/sys/sys/filedesc.h Fri Jul 10 13:54:03 2015 (r285357) @@ -136,7 +136,7 @@ struct filedesc_to_leader { /* Operation types for kern_dup(). */ enum { - FDDUP_NORMAL = 0x01, /* dup() behavior. */ + FDDUP_NORMAL, /* dup() behavior. */ FDDUP_FCNTL, /* fcntl()-style errors. */ FDDUP_FIXED, /* Force fixed allocation. */ FDDUP_MUSTREPLACE, /* Target must exist. */ @@ -144,7 +144,7 @@ enum { }; /* Flags for kern_dup(). */ -#define FDDUP_CLOEXEC 0x1 /* Atomically set FD_CLOEXEC. */ +#define FDDUP_FLAG_CLOEXEC 0x1 /* Atomically set UF_EXCLOSE. */ struct thread;
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?201507101354.t6ADs3hV086216>