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 > > Is this implementation correct? If so any objection to adding this as > a supported syscall? > > > Index: sys/kern/kern_descrip.c > =================================================================== > --- 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 */ > > static int do_dup(struct thread *td, int flags, int old, int new, > register_t *retval); > @@ -307,7 +308,39 @@ > return (0); > } > > +#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 != NULL, ("%s: td == NULL", __func__));
> + KASSERT(uap != NULL, ("%s: uap == NULL", __func__));
The asserts are useless and must be removed.
> +
> + if (uap->from == uap->to)
> + return EINVAL;
Style(9) recommends to brace the values of return operator.
> +
> + if (uap->flags & ~O_CLOEXEC)
> + return EINVAL;
> +
> + const int dupflags = (uap->flags == 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 ==, 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 = new;
> *retval = new;
>
> + if (flags & DUP_CLOEXEC)
> + fdp->fd_ofileflags[new] |= 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
> ===================================================================
> --- 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
> ===================================================================
> --- 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.
pgpnIpOkAjXZm.pgp
Description: PGP signature
