This is the mail archive of the cygwin-patches mailing list for the Cygwin project.
Index Nav: | [Date Index] [Subject Index] [Author Index] [Thread Index] | |
---|---|---|
Message Nav: | [Date Prev] [Date Next] | [Thread Prev] [Thread Next] |
Other format: | [Raw text] |
According to Corinna Vinschen on 1/13/2010 2:25 PM: > Hi, > > the below patch implements the Linux dup3/O_CLOEXEC/F_DUPFD_CLOEXEC > extension. I hope I didn't miss anything important since it affects > quite a few fhandlers. > Eric, you asked for it in the first place, do you have a fine testcase > for this functionality? For dup3, fcntl(F_DUPFD_CLOEXEC), and pipe2, yes. For open, accept4, and others, you'll have to use your imagination, but it is is similar. http://git.sv.gnu.org/cgit/gnulib.git/tree/tests/test-dup3.c?id=84ab6921 then modified slightly: /* Test duplicating file descriptors. Copyright (C) 2009 Free Software Foundation, Inc. This program is free software: you can redistribute it and/or modify it under the terms of the GNU General Public License as published by the Free Software Foundation; either version 3 of the License, or (at your option) any later version. This program is distributed in the hope that it will be useful, but WITHOUT ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for more details. You should have received a copy of the GNU General Public License along with this program. If not, see <http://www.gnu.org/licenses/>. */ /* Written by Eric Blake <ebb9@byu.net>, 2009, and Bruno Haible <bruno@clisp.org>, 2009. */ #include <unistd.h> #include <errno.h> #include <fcntl.h> #include <stdbool.h> #include <stdio.h> #include <stdlib.h> #include <io.h> #define ASSERT(expr) \ do \ { \ if (!(expr)) \ { \ fprintf (stderr, "%s:%d: assertion failed\n", __FILE__, \ __LINE__); \ fflush (stderr); \ abort (); \ } \ } \ while (0) /* Return true if FD is open. */ static bool is_open (int fd) { return 0 <= fcntl (fd, F_GETFL); } /* Return true if FD is not inherited to child processes. */ static bool is_cloexec (int fd) { int flags; ASSERT ((flags = fcntl (fd, F_GETFD)) >= 0); return (flags & FD_CLOEXEC) != 0; } int main (void) { int use_cloexec; for (use_cloexec = 0; use_cloexec <= 1; use_cloexec++) { const char *file = "test-dup3.tmp"; int fd = open (file, O_CREAT | O_TRUNC | O_RDWR, 0600); int o_flags; char buffer[1]; o_flags = 0; if (use_cloexec) o_flags |= O_CLOEXEC; /* Assume std descriptors were provided by invoker. */ ASSERT (STDERR_FILENO < fd); ASSERT (is_open (fd)); /* Ignore any other fd's leaked into this process. */ close (fd + 1); close (fd + 2); ASSERT (!is_open (fd + 1)); ASSERT (!is_open (fd + 2)); /* Assigning to self is invalid. */ errno = 0; ASSERT (dup3 (fd, fd, o_flags) == -1); ASSERT (errno == EINVAL); ASSERT (is_open (fd)); /* If the source is not open, then the destination is unaffected. */ errno = 0; ASSERT (dup3 (fd + 1, fd + 2, o_flags) == -1); ASSERT (errno == EBADF); ASSERT (!is_open (fd + 2)); errno = 0; ASSERT (dup3 (fd + 1, fd, o_flags) == -1); ASSERT (errno == EBADF); ASSERT (is_open (fd)); /* The destination must be valid. */ errno = 0; ASSERT (dup3 (fd, -2, o_flags) == -1); ASSERT (errno == EBADF); errno = 0; ASSERT (dup3 (fd, 10000000, o_flags) == -1); ASSERT (errno == EBADF); /* Using dup3 can skip fds. */ ASSERT (dup3 (fd, fd + 2, o_flags) == fd + 2); ASSERT (is_open (fd)); ASSERT (!is_open (fd + 1)); ASSERT (is_open (fd + 2)); if (use_cloexec) ASSERT (is_cloexec (fd + 2)); else ASSERT (!is_cloexec (fd + 2)); /* Verify that dup3 closes the previous occupant of a fd. */ ASSERT (open ("/dev/null", O_WRONLY, 0600) == fd + 1); ASSERT (dup3 (fd + 1, fd, o_flags) == fd); ASSERT (close (fd + 1) == 0); ASSERT (write (fd, "1", 1) == 1); ASSERT (dup3 (fd + 2, fd, o_flags) == fd); ASSERT (lseek (fd, 0, SEEK_END) == 0); ASSERT (write (fd + 2, "2", 1) == 1); ASSERT (lseek (fd, 0, SEEK_SET) == 0); ASSERT (read (fd, buffer, 1) == 1); ASSERT (*buffer == '2'); /* Clean up. */ ASSERT (close (fd + 2) == 0); ASSERT (close (fd) == 0); ASSERT (unlink (file) == 0); } return 0; } Likewise for fcntl and pipe2 (although I won't paste them here): http://git.sv.gnu.org/cgit/gnulib.git/tree/tests/test-fcntl.c?id=b72fe29b http://git.sv.gnu.org/cgit/gnulib.git/tree/tests/test-pipe2.c?id=d72a5819 But there's a few things we need to fix first before it is even worth trying to run those tests. > +++ winsup/cygwin/dtable.cc 13 Jan 2010 21:22:13 -0000 > @@ -579,7 +579,11 @@ dtable::dup_worker (fhandler_base *oldfh > } > else > { > - newfh->close_on_exec (false); > + /* The O_CLOEXEC flag enforces close-on-exec behaviour. */ > + if (flags & O_CLOEXEC) > + newfh->set_close_on_exec (true); Is that a typo? > + else > + newfh->close_on_exec (false); If not, why not just newfh->close_on_exec (!!(flags & O_CLOEXEC)), instead of the if-else? > { > int res = -1; > fhandler_base *newfh = NULL; // = NULL to avoid an incorrect warning > > MALLOC_CHECK; > - debug_printf ("dup2 (%d, %d)", oldfd, newfd); > + debug_printf ("dup3 (%d, %d, %d)", oldfd, newfd, flags); I'd prefer %#x for flags, rather than %d (two instances in this function). > lock (); > > if (not_open (oldfd)) > @@ -602,21 +606,25 @@ dtable::dup2 (int oldfd, int newfd) > set_errno (EBADF); > goto done; > } > - > if (newfd < 0) > { > syscall_printf ("new fd out of bounds: %d", newfd); > set_errno (EBADF); > goto done; > } > - > + if ((flags & ~O_CLOEXEC) != 0) > + { > + syscall_printf ("invalid flags value %x", flags); > + set_errno (EINVAL); > + return -1; > + } > if (newfd == oldfd) > { > res = newfd; > goto done; > } Per Linux, newfd==oldfd must fail with EINVAL: http://www.kernel.org/doc/man-pages/online/pages/man2/dup3.2.html I see how you are filtering this case out in dup3(), but I wonder if it would be easier to make this function assume that newfd!=oldfd (in other words, make both dup2 and dup3 do the appropriate special casing, so this function doesn't have to do any). More on that below. > +++ winsup/cygwin/fcntl.cc 13 Jan 2010 21:22:13 -0000 > @@ -49,6 +49,15 @@ fcntl64 (int fd, int cmd, ...) > res = -1; > } > break; > + case F_DUPFD_CLOEXEC: > + if ((int) arg >= 0 && (int) arg < OPEN_MAX_MAX) > + res = dup3 (fd, cygheap_fdnew (((int) arg) - 1), O_CLOEXEC); > + else > + { > + set_errno (EINVAL); > + res = -1; > + } > + break; Why not consolidate the two branches, and write this as: case F_DUPFD: case F_DUPFD_CLOEXEC: if ((int) arg >= 0 && (int) arg < OPEN_MAX_MAX) res = dup3 (fd, cygheap_fdnew (((int) arg) - 1), (cmd == F_DUPFD_CLOEXEC) * O_CLOEXEC); else ... > +++ winsup/cygwin/fhandler.cc 13 Jan 2010 21:22:13 -0000 > +++ winsup/cygwin/fhandler_fifo.cc 13 Jan 2010 21:22:14 -0000 > @@ -94,7 +94,9 @@ fhandler_fifo::open (int flags, mode_t) > { > char char_sa_buf[1024]; > LPSECURITY_ATTRIBUTES sa_buf = > - sec_user ((PSECURITY_ATTRIBUTES) char_sa_buf, cygheap->user.sid()); > + flags & O_CLOEXEC > + ? sec_user_nih ((PSECURITY_ATTRIBUTES) char_sa_buf, cygheap->user.sid()) > + : sec_user ((PSECURITY_ATTRIBUTES) char_sa_buf, cygheap->user.sid()); In addition to cgf's comment about this repetition, is it any more compact to write: ((flags & O_CLOEXEC) ? sec_user_nih : sec_user) ((PSECURITY_ATTRIBUTES) char_sa_buf, cygheap->user.sid()); > +++ winsup/cygwin/syscalls.cc 13 Jan 2010 21:22:16 -0000 > @@ -119,7 +119,7 @@ close_all_files (bool norelease) > int > dup (int fd) > { > - return cygheap->fdtab.dup2 (fd, cygheap_fdnew ()); > + return cygheap->fdtab.dup3 (fd, cygheap_fdnew (), 0); > } > > int > @@ -131,7 +131,25 @@ dup2 (int oldfd, int newfd) > set_errno (EBADF); > return -1; > } > - return cygheap->fdtab.dup2 (oldfd, newfd); > + return cygheap->fdtab.dup3 (oldfd, newfd, 0); If you fix fdtab.dup3 above to omit the oldfd==newfd, then you need to add something like this before trying dup3: if (oldfd == newfd) { cygheap_fdget cfd (oldfd); if (cfd < 0) { set_errno (EBADF); return -1; } return oldfd; } > +} > + > +int > +dup3 (int oldfd, int newfd, int flags) > +{ > + if (newfd >= OPEN_MAX_MAX) > + { > + syscall_printf ("-1 = dup3 (%d, %d, %d) (%d too large)", oldfd, newfd, flags, newfd); Again with the %#x instead of %d for flags. > + set_errno (EBADF); > + return -1; > + } > + if (!cygheap->fdtab.not_open (oldfd) && oldfd == newfd) Is not_open() expensive? If so, reverse the order of the conditionals for speed. > + { > + syscall_printf ("-1 = dup3 (%d, %d, %d) (newfd==oldfd)", oldfd, newfd, flags); > + set_errno (EINVAL); > + return -1; > + } > + return cygheap->fdtab.dup3 (oldfd, newfd, flags); According to spec, Linux dup3(-1,-1,0) can fail with either EBADF or EINVAL; I haven't tested that on Linux yet, but am assuming that your choice of EBADF for this condition was intentional? As long as we are exporting dup3, why not also pipe2, accept4, mkostemp, and mkostemps? Plus there are tweaks to existing interfaces (the 'e' flag to fopen, fdopen, freopen, popen; the SOCK_CLOEXEC flag to socket and socketpair), some of which need more newlib patches. And while it looks like mq_open should not care about O_CLOEXEC, there may be some cleanup needed there. Linux also has other functions that now take a flag for O_CLOEXEC purposes, such as epoll_create1 (compared to the older epoll_create); but none of the other functions appeared to be based on standardized interfaces, nor to have a counterpart in cygwin yet. -- Don't work too hard, make some time for fun as well! Eric Blake ebb9@byu.net
Attachment:
signature.asc
Description: OpenPGP digital signature
Index Nav: | [Date Index] [Subject Index] [Author Index] [Thread Index] | |
---|---|---|
Message Nav: | [Date Prev] [Date Next] | [Thread Prev] [Thread Next] |