bug-gnulib
[Top][All Lists]
Advanced

[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);




reply via email to

[Prev in Thread] Current Thread [Next in Thread]