From owner-freebsd-hackers@FreeBSD.ORG Thu Jan 12 10:08:54 2012 Return-Path: Delivered-To: freebsd-hackers@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:4f8:fff6::34]) by hub.freebsd.org (Postfix) with ESMTP id 6786B1065673; Thu, 12 Jan 2012 10:08:54 +0000 (UTC) (envelope-from kostikbel@gmail.com) Received: from mail.zoral.com.ua (mx0.zoral.com.ua [91.193.166.200]) by mx1.freebsd.org (Postfix) with ESMTP id 764A88FC17; Thu, 12 Jan 2012 10:08:53 +0000 (UTC) Received: from skuns.kiev.zoral.com.ua (localhost [127.0.0.1]) by mail.zoral.com.ua (8.14.2/8.14.2) with ESMTP id q0CA8ewi015401 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=NO); Thu, 12 Jan 2012 12:08:41 +0200 (EET) (envelope-from kostikbel@gmail.com) Received: from deviant.kiev.zoral.com.ua (kostik@localhost [127.0.0.1]) by deviant.kiev.zoral.com.ua (8.14.5/8.14.5) with ESMTP id q0CA8ebn073427; Thu, 12 Jan 2012 12:08:40 +0200 (EET) (envelope-from kostikbel@gmail.com) Received: (from kostik@localhost) by deviant.kiev.zoral.com.ua (8.14.5/8.14.5/Submit) id q0CA8eVO073426; Thu, 12 Jan 2012 12:08:40 +0200 (EET) (envelope-from kostikbel@gmail.com) X-Authentication-Warning: deviant.kiev.zoral.com.ua: kostik set sender to kostikbel@gmail.com using -f Date: Thu, 12 Jan 2012 12:08:40 +0200 From: Kostik Belousov To: Eitan Adler Message-ID: <20120112100840.GV31224@deviant.kiev.zoral.com.ua> References: Mime-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="+hILXQfXzEY/zLuK" Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.4.2.3i X-Virus-Scanned: clamav-milter 0.95.2 at skuns.kiev.zoral.com.ua X-Virus-Status: Clean X-Spam-Status: No, score=-3.9 required=5.0 tests=ALL_TRUSTED,AWL,BAYES_00 autolearn=ham version=3.2.5 X-Spam-Checker-Version: SpamAssassin 3.2.5 (2008-06-10) on skuns.kiev.zoral.com.ua Cc: jilles@freebsd.org, FreeBSD Hackers , Colin Percival Subject: Re: dup3 syscall - atomic set O_CLOEXEC with dup2 X-BeenThere: freebsd-hackers@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: Technical Discussions relating to FreeBSD List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 12 Jan 2012 10:08:54 -0000 --+hILXQfXzEY/zLuK Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Thu, Jan 12, 2012 at 12:01:29AM -0500, Eitan Adler wrote: > This is an implementation of dup3 for FreeBSD: > man page here (with a FreeBSD patch coming soon): > https://www.kernel.org/doc/man-pages/online/pages/man2/dup.2.html >=20 > Is this implementation correct? If so any objection to adding this as > a supported syscall? >=20 >=20 > Index: sys/kern/kern_descrip.c > =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D > --- sys/kern/kern_descrip.c (revision 229830) > +++ sys/kern/kern_descrip.c (working copy) > @@ -110,6 +110,7 @@ > /* Flags for do_dup() */ > #define DUP_FIXED 0x1 /* Force fixed allocation */ > #define DUP_FCNTL 0x2 /* fcntl()-style errors */ > +#define DUP_CLOEXEC 0x4 /* Enable O_CLOEXEC on the new fs */ >=20 > static int do_dup(struct thread *td, int flags, int old, int new, > register_t *retval); > @@ -307,7 +308,39 @@ > return (0); > } >=20 > +#ifndef _SYS_SYSPROTO_H_ > +struct dup3_args { > + u_int from; > + u_int to; > + int flags; > +}; > +#endif > + The _SYS_SYSPROTO_H_ should have been obsoleted and removed long time ago. I see no reasons to continue its agony for new syscalls. > /* > + * Duplicate a file descriptor and allow for O_CLOEXEC > + */ > + > +/* ARGSUSED */ ARGUSED is from the same category as SYS_SYSPROTO_H. > +int > +sys_dup3(struct thread * const td, struct dup3_args * const uap) { The td and uap are not constant in the prototypes generated by makesyscalls.sh. This makes me seriosly wonder was the patch compiled at all. > + > + KASSERT(td !=3D NULL, ("%s: td =3D=3D NULL", __func__)); > + KASSERT(uap !=3D NULL, ("%s: uap =3D=3D NULL", __func__)); The asserts are useless and must be removed. > + > + if (uap->from =3D=3D uap->to) > + return EINVAL; Style(9) recommends to brace the values of return operator. > + > + if (uap->flags & ~O_CLOEXEC) > + return EINVAL; > + > + const int dupflags =3D (uap->flags =3D=3D O_CLOEXEC) ? > DUP_FIXED|DUP_CLOEXEC : DUP_FIXED; There are too many style violations to enumerate them all, please do not consider the list below exhaustive: - the variable is declared in the executive part of the function body; - line too long; - no spaces around binary '|'. Not stule comments: there is no use in declaring dupflags const; it is better to test the presence of O_CLOEXEC with & instead of comparing with =3D=3D, to allow for ease introduction of future dup3 flags, if any. > + > + return (do_dup(td, dupflags, (int)uap->from, (int)uap->to, > + td->td_retval)); Why casting arguments to int ? > + return (0); Isn't this line never executed ? > +} > + > +/* > * Duplicate a file descriptor to a particular value. > * > * Note: keep in mind that a potential race condition exists when closing > @@ -912,6 +945,9 @@ > fdp->fd_lastfile =3D new; > *retval =3D new; >=20 > + if (flags & DUP_CLOEXEC) > + fdp->fd_ofileflags[new] |=3D UF_EXCLOSE; > + It is better to handle UF_EXCLOSE at the time of assignment to fdp->fd_ofileflags[new] just above, instead of clearing UF_EXCLOSE and then setting it. > /* > * If we dup'd over a valid file, we now own the reference to it > * and must dispose of it using closef() semantics (as if a > Index: sys/kern/syscalls.master > =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D > --- sys/kern/syscalls.master (revision 229830) > +++ sys/kern/syscalls.master (working copy) > @@ -951,5 +951,6 @@ > off_t offset, off_t len); } > 531 AUE_NULL STD { int posix_fadvise(int fd, off_t offset, \ > off_t len, int advice); } > +532 AUE_NULL STD { int dup3(u_int from, u_int to, int flags); } > ; Please copy any additions and changes to the following compatability t= ables: > ; sys/compat/freebsd32/syscalls.master > Index: sys/compat/freebsd32/syscalls.master > =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D > --- sys/compat/freebsd32/syscalls.master (revision 229830) > +++ sys/compat/freebsd32/syscalls.master (working copy) > @@ -997,3 +997,4 @@ > uint32_t offset1, uint32_t offset2,\ > uint32_t len1, uint32_t len2, \ > int advice); } > +532 AUE_NULL STD { int dup3(u_int from, u_int to, int flags); } And again, this part of patch makes me think that you never compiled it. That said, I am not sure that we shall copy the O_CLOEXEC crusade from Linux until it is standartized. And, if copying, I think we shall copy all bits of the new API, like SOCK_CLOEXEC etc, and not just dup3. --+hILXQfXzEY/zLuK Content-Type: application/pgp-signature Content-Disposition: inline -----BEGIN PGP SIGNATURE----- Version: GnuPG v1.4.11 (FreeBSD) iEYEARECAAYFAk8OsSgACgkQC3+MBN1Mb4g06ACfd6w3dAda842SWf73Bqnq3Ajj zDkAn1Wh2RDtBjBjB4flOzlGNw5sZYDQ =VUKQ -----END PGP SIGNATURE----- --+hILXQfXzEY/zLuK--