From owner-cvs-all@FreeBSD.ORG Thu May 29 11:55:09 2008 Return-Path: Delivered-To: cvs-all@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:4f8:fff6::34]) by hub.freebsd.org (Postfix) with ESMTP id 885981065672; Thu, 29 May 2008 11:55:09 +0000 (UTC) (envelope-from brde@optusnet.com.au) Received: from mail03.syd.optusnet.com.au (mail03.syd.optusnet.com.au [211.29.132.184]) by mx1.freebsd.org (Postfix) with ESMTP id 112568FC0C; Thu, 29 May 2008 11:55:08 +0000 (UTC) (envelope-from brde@optusnet.com.au) Received: from c220-239-252-11.carlnfd3.nsw.optusnet.com.au (c220-239-252-11.carlnfd3.nsw.optusnet.com.au [220.239.252.11]) by mail03.syd.optusnet.com.au (8.13.1/8.13.1) with ESMTP id m4TBsiwv017581 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=NO); Thu, 29 May 2008 21:54:47 +1000 Date: Thu, 29 May 2008 21:54:44 +1000 (EST) From: Bruce Evans X-X-Sender: bde@delplex.bde.org To: Ed Schouten In-Reply-To: <20080529082433.GV64397@hoeg.nl> Message-ID: <20080529211748.N89774@delplex.bde.org> References: <200805282025.m4SKPJgY097901@repoman.freebsd.org> <20080529085549.J39873@fledge.watson.org> <20080529082433.GV64397@hoeg.nl> MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII; format=flowed Cc: cvs-src@freebsd.org, src-committers@freebsd.org, Robert Watson , cvs-all@freebsd.org Subject: Re: cvs commit: src/sys/kern kern_descrip.c X-BeenThere: cvs-all@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: CVS commit messages for the entire tree List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 29 May 2008 11:55:09 -0000 On Thu, 29 May 2008, Ed Schouten wrote: > * Robert Watson wrote: >> 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. No, EBADF applies to both the open fd and the new fd for dup2() (since that is what dup2() did initially and has become Standard). F_DUP2FD is apparently supposed to be equivalent to dup2(), so it must be bug-for bug equivalent. dup2() was broken in rev.1.219. It actually returns EMFILE if the new fd is too high. It remains unbroken and returns EBADF if the new fd is below zero, but this gives another inconsistency. I think F_DUP2FD inherits these bugs. F_DUPFD consistently returns EINVAL if the new fd is either too high or below zero. More details in previously-sent private mail. > 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: Style bug (missing spaces around binary operator). > --- 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? I think F_DUP2FD and its man page are as intended and don't need changing. But dup2()'s man page needs changes to spell out the details of the differences between the error handling of dup() and dup2() (it is shared with dup()'s man page but no differences are mentioned). Comparing with POSIX shows that the main differences are: - [EMFILE] doesn't apply to dup2(), since the new fd is available unless it is out of bounds, and the error for out of bounds is EBADF - for dup2(), it is not an error for the new fd to be "inactive", and that the main bugs which aren't differences are: - "active" is a bad way of saying "open" - "valid" is a fuzzy way of saying "not (negative or >= {OPEN_MAX})". fcntl.2 already says this better (see above quote; in the unquoted part it spells {OPEN_MAX} as "see getdtablesize(2)"). I think FreeBSD man pages should explicitly refer to intro(2) for the the details of this, but intro(2) is about 20 years out of date here -- it says that releases have a fixed limit of 64 and refers to getdtablesize(2) and never mentions {OPEN_MAX}. - [EINTR] is not described. I'm not sure if it applies to the dup() family when syscalls are restarted, but it applies when they are not and is mentioned for open(2). Bruce