Date: Thu, 12 Jan 2012 12:23:06 +0200 From: Kostik Belousov <kostikbel@gmail.com> To: Eitan Adler <lists@eitanadler.com> Cc: jilles@freebsd.org, FreeBSD Hackers <freebsd-hackers@freebsd.org>, Colin Percival <cperciva@freebsd.org> Subject: Re: dup3 syscall - atomic set O_CLOEXEC with dup2 Message-ID: <20120112102306.GW31224@deviant.kiev.zoral.com.ua> In-Reply-To: <20120112100840.GV31224@deviant.kiev.zoral.com.ua> References: <CAF6rxg=EjkwFbXQt3i2Nnz6_dcZtdek-2YdqyZnAdVPxVaWR_Q@mail.gmail.com> <20120112100840.GV31224@deviant.kiev.zoral.com.ua>
next in thread | previous in thread | raw e-mail | index | archive | help
--gaVk6OYrcNQmNLCV Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Thu, Jan 12, 2012 at 12:08:40PM +0200, Kostik Belousov wrote: > 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. >=20 > > /* > > + * Duplicate a file descriptor and allow for O_CLOEXEC > > + */ > > + > > +/* ARGSUSED */ > ARGUSED is from the same category as SYS_SYSPROTO_H. >=20 > > +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. >=20 > > + > > + 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. >=20 > > + > > + if (uap->from =3D=3D uap->to) > > + return EINVAL; > Style(9) recommends to brace the values of return operator. >=20 > > + > > + 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 '|'. >=20 > 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. >=20 > > + > > + 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 clos= ing > > @@ -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. >=20 > > /* > > * 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= tables: > > ; 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. Oh, and it misses the versioning for the new syscall symbols in libc. The symbols not listed in the Symbol.maps are made local during the final linkage. --gaVk6OYrcNQmNLCV Content-Type: application/pgp-signature Content-Disposition: inline -----BEGIN PGP SIGNATURE----- Version: GnuPG v1.4.11 (FreeBSD) iEYEARECAAYFAk8OtIoACgkQC3+MBN1Mb4htvQCglv77VlHDkghe4Lr7HtDdt2IN WiIAoNo77zMGmpzG/ncvfK30kNxXdbPj =MMik -----END PGP SIGNATURE----- --gaVk6OYrcNQmNLCV--
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20120112102306.GW31224>