From owner-cvs-all@FreeBSD.ORG Thu May 29 08:31:56 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 59555106568F for ; Thu, 29 May 2008 08:31:56 +0000 (UTC) (envelope-from antoine.brodin.freebsd@gmail.com) Received: from fk-out-0910.google.com (fk-out-0910.google.com [209.85.128.187]) by mx1.freebsd.org (Postfix) with ESMTP id CE9E78FC1F for ; Thu, 29 May 2008 08:31:55 +0000 (UTC) (envelope-from antoine.brodin.freebsd@gmail.com) Received: by fk-out-0910.google.com with SMTP id k31so3158099fkk.11 for ; Thu, 29 May 2008 01:31:54 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=gamma; h=domainkey-signature:received:received:message-id:date:from:sender:to:subject:cc:in-reply-to:mime-version:content-type:content-transfer-encoding:content-disposition:references:x-google-sender-auth; bh=CxqUPJf42oJ8VrmadOH7juB2e+q4hgxeHDqwD9uoMtc=; b=GtH6nuOf6GRLjsmNS2SCi1UiWxqA0VWjhc4Nr+lx0QP6CZU4s//rooilaUaH9KG4Iazp5HC2deyOt+fHtBcwCYlbKStmR2lIiPDVHyeziyRTzzBflpQg6jzH/H5nNGjimChuxw8soFl04TUpou2paqQB6s28ik5Hbg4qU32Is0A= DomainKey-Signature: a=rsa-sha1; c=nofws; d=gmail.com; s=gamma; h=message-id:date:from:sender:to:subject:cc:in-reply-to:mime-version:content-type:content-transfer-encoding:content-disposition:references:x-google-sender-auth; b=coF5LS8PF+EciWc5JYqKzdhlXXFs66qfeDe+gdVNHkhLUl5qDbKPd/kY2p9VmYia33YN+Lcl/ant9SwzzdPeEmhmAB6+tskXjPPaRRtA9bO4kMFFRm161Jw5juSCtX1SAHe8bXogAOAjRaLGTBGAGwdLgFb7MwJM+r+wJ9Ie+XI= Received: by 10.78.189.5 with SMTP id m5mr1415565huf.77.1212049914354; Thu, 29 May 2008 01:31:54 -0700 (PDT) Received: by 10.78.136.16 with HTTP; Thu, 29 May 2008 01:31:54 -0700 (PDT) Message-ID: Date: Thu, 29 May 2008 10:31:54 +0200 From: "Antoine Brodin" Sender: antoine.brodin.freebsd@gmail.com To: "Ed Schouten" In-Reply-To: <20080529082433.GV64397@hoeg.nl> MIME-Version: 1.0 Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit Content-Disposition: inline References: <200805282025.m4SKPJgY097901@repoman.freebsd.org> <20080529085549.J39873@fledge.watson.org> <20080529082433.GV64397@hoeg.nl> X-Google-Sender-Auth: bbb1e0ec228d0787 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 08:31:56 -0000 On Thu, May 29, 2008 at 10:24 AM, Ed Schouten wrote: > * Robert Watson 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