[Top][All Lists]
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: fix mkostemp, add pipe2-safer
From: |
Bruno Haible |
Subject: |
Re: fix mkostemp, add pipe2-safer |
Date: |
Mon, 7 Dec 2009 01:18:55 +0100 |
User-agent: |
KMail/1.9.9 |
Hi Eric,
Great! I like the direction of your patches, unifying the support of *_safer and
the native Woe32 requirements in a single API. I also like that the cloexec
module now becomes usable for code that is portable to mingw; set_cloexec_flag
is not in this category.
Detailed review:
- In dup_cloexec:
- After DuplicateHandle failed, 'errno = EMFILE;' is just a guess.
Richard Jones has suggested that he might work on that; so I only add a
TODO.
- The code accesses an undefined variable 'result'.
- For functions that return 0 or -1, I find it more clear to test the result
for < 0 than for != 0 (because so many functions can return a result > 0
when
they succeed).
- In set_cloexec_flag: You added a call to
dup2 (desc, desc)
But on a POSIX compliant system this is a no-op, since
<http://www.opengroup.org/onlinepubs/9699919799/functions/dup.html>
says:
"If fildes is equal to fildes2, the FD_CLOEXEC flag associated with
fildes2 shall not be changed."
If this is specifically targeted at non-POSIX systems, it would be good to
have
a comment about it.
- Regarding dup_safer_flag and fd_safer_flag: I find it important to also handle
also O_BINARY and O_TEXT, like we do in the pipe2 function (see the doc in
lib/unistd.in.h). pipe2 supports O_TEXT, but pipe2_safer does not: It
respects the O_TEXT flag in the call to pipe2 but then loses it in the call to
fd_safer_flag (because dup_cloexec has an O_BINARY flag hardwired).
I'm not sure about the way to get it: Should we add O_CLOEXEC support for all
open-like functions, defining O_CLOEXEC ourselves? Or should we add a
O_GL_CLOEXEC that is supported only by selected gnulib functions? The latter
approach can serve as a step-by-step approach to the complete support of
O_CLOEXEC.
Here's what I applying for the first part:
2009-12-06 Bruno Haible <address@hidden>
* lib/cloexec.c (dup_cloexec): Fix handling of _gl_register_dup return
value.
--- lib/cloexec.c.orig 2009-12-07 01:01:22.000000000 +0100
+++ lib/cloexec.c 2009-12-07 01:01:19.000000000 +0100
@@ -79,7 +79,8 @@
prior to exec or spawn. Returns -1 and sets errno if FD could not
be duplicated. */
-int dup_cloexec (int fd)
+int
+dup_cloexec (int fd)
{
int nfd;
@@ -107,6 +108,7 @@
FALSE, /* InheritHandle */
DUPLICATE_SAME_ACCESS)) /* Options */
{
+ /* TODO: Translate GetLastError () into errno. */
errno = EMFILE;
return -1;
}
@@ -133,12 +135,12 @@
nfd = fcntl (fd, F_DUPFD_CLOEXEC, 0);
# if REPLACE_FCHDIR
if (0 <= nfd)
- result = _gl_register_dup (fd, nfd);
+ nfd = _gl_register_dup (fd, nfd);
# endif
# else /* !F_DUPFD_CLOEXEC */
nfd = dup (fd);
- if (0 <= nfd && set_cloexec_flag (nfd, true))
+ if (0 <= nfd && set_cloexec_flag (nfd, true) < 0)
{
int saved_errno = errno;
close (nfd);