Date: Thu, 29 May 2008 10:31:54 +0200 From: "Antoine Brodin" <antoine@FreeBSD.org> To: "Ed Schouten" <ed@80386.nl> Cc: cvs-src@freebsd.org, src-committers@freebsd.org, Robert Watson <rwatson@freebsd.org>, cvs-all@freebsd.org Subject: Re: cvs commit: src/sys/kern kern_descrip.c Message-ID: <f19c444a0805290131h49e93167t642d6cc3506fb868@mail.gmail.com> In-Reply-To: <20080529082433.GV64397@hoeg.nl> References: <200805282025.m4SKPJgY097901@repoman.freebsd.org> <20080529085549.J39873@fledge.watson.org> <20080529082433.GV64397@hoeg.nl>
next in thread | previous in thread | raw e-mail | index | archive | help
On Thu, May 29, 2008 at 10:24 AM, Ed Schouten <ed@80386.nl> wrote: > * Robert Watson <rwatson@FreeBSD.org> wrote: >> >> On Wed, 28 May 2008, Ed Schouten wrote: >> >>> Remove redundant checks from fcntl()'s F_DUPFD. >>> >>> Right now we perform some of the checks inside the fcntl()'s F_DUPFD >>> operation twice. We first validate the `fd' argument. When finished, >>> we validate the `arg' argument. These checks are also performed inside >>> do_dup(). >>> >>> The reason we need to do this, is because fcntl() should return different >>> errno's when the `arg' argument is out of bounds (EINVAL instead of >>> EBADF). To prevent the redundant locking of the PROC_LOCK and >>> FILEDESC_SLOCK, patch do_dup() to support the error semantics required >>> by fcntl(). >> >> This sounds like a good candidate for a regression test -- do we have a >> dup/dup2/F_DUPFD/F_DUP2FD test? If not, perhaps we should, in light of >> the opportunity for further bugs and regressions. > > It looks like we already have regression tests for dup/dup2/etc -- > kern_descrip.c 1.325 mentions them. > > I saw FreeBSD also implements F_DUP2FD (which is a non-standard > extension). Right now this command returns EBADF when you do the > following: > > fcntl(0, F_DUP2FD, -1); // below zero > fcntl(0, F_DUP2FD, 1000000); // too high > > This is exactly the same as what dup2() does, but is inconsistent with > fcntl() in general. EBADF should be returned if the `fd' argument is > invalid. It should not apply to the argument. > > We could consider applying the following patch: > > --- sys/kern/kern_descrip.c > +++ sys/kern/kern_descrip.c > @@ -423,7 +423,8 @@ > > case F_DUP2FD: > tmp = arg; > - error = do_dup(td, DUP_FIXED, fd, tmp, td->td_retval); > + error = do_dup(td, DUP_FIXED|DUP_FCNTL, fd, tmp, > + td->td_retval); > break; > > case F_GETFD: > --- lib/libc/sys/fcntl.2 > +++ lib/libc/sys/fcntl.2 > @@ -452,14 +452,6 @@ > The argument > .Fa cmd > is > -.Dv F_DUP2FD , > -and > -.Fa arg > -is not a valid file descriptor. > -.Pp > -The argument > -.Fa cmd > -is > .Dv F_SETLK > or > .Dv F_SETLKW , > @@ -502,6 +494,8 @@ > argument > is > .Dv F_DUPFD > +or > +.Dv F_DUP2FD > and > .Fa arg > is negative or greater than the maximum allowable number > > Any comments? Hello Ed, On Solaris, dup2 and fcntl(F_DUP2FD) are totally equivalent (they return the same errno). I have no strong feelings about this. Cheers, Antoine
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?f19c444a0805290131h49e93167t642d6cc3506fb868>