Re: fcntl module

[Eric Blake]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

According to Bruno Haible on 8/22/2009 5:59 AM:
> Hi Eric,
> 
>> First order of business - we need two modules based on the fcntl name, one
>> for the header ... and one for the function.
> 
> Yes, absolutely.
> 
> Your 3 patches look all fine, except for the mingw replacement of fcntl().

I've pushed the first one (after rebasing on top of today's changes),
along with adding trivial O_TTY_INIT support (below).  The second one in
my original series (fcntl-tests) doesn't make sense until we figure out
how to fix the third one for mingw (as otherwise, I will be checking in a
test that is broken under mingw).

So maybe at this point I will regroup a bit and figure out how to replace
broken fcntl on Unix-y systems, and focus on F_DUPFD_CLOEXEC first.  I'm
hoping that it is generally safe to do the following with varargs, even on
64-bit platforms where int and void* are different sizes (since for
unknown platform-specific F_* flags, I can't possibly know what type the
third argument to fcntl is)?  Here, f1 would be the system fcntl(), f2 our
wrapper, and f3 the caller.  My hope is that on a 64-bit platform, even
though f2 reads more bits into its intermediate p than what the caller
passes, the relevant 32-bits are placed back in vararg form in the correct
pattern so that f1 will be reading them with the correct int type.

#include <stdarg.h>
int
f1 (int a, ...)
{
  va_list arg;
  int i;
  va_start (arg, a);
  i = va_arg (arg, int);
  va_end (arg);
  return i;
}
int
f2 (int a, ...)
{
  va_list arg;
  void *p;
  va_start (arg, a);
  p = va_arg (arg, void *);
  va_end (arg);
  return f1 (a, p);
}
int f3 (void)
{
  return f2 (1, 1);
}

> 
> I would not risk a recursion depth of 2047. It's time to make this function
> non-recursive. How about this?
> 
> static int
> dupfd (int fd, int desired_fd)

I agree that a non-recursive function is better.  Actually, I'd like to
see F_DUPFD and F_DUPFD_CLOEXEC be the preferred interfaces (those are
both POSIX) rather than dup_noinherit (or dup_ex); dup(n) is trivially
fcntl(n,F_DUPFD,0), so all we are missing is mapping dup_noinherit to
fcntl(n,F_DUPFD_CLOEXEC,0).  At any rate, that means the helper function
on mingw should probably take a bool argument, to be shared by both
F_DUPFD variants, and call DuplicateHandle (rather than dup) with the
correct InheritHandle argument all the way through the chain, then use a
single _open_osfhandle at the end (and not my approach of attempting
F_SETFD at the end).

I'll see what I can do, once I get time (it won't be until the middle of
next week).

> {
>   unsigned char fds_to_close[4096 / 8];

4096 seems magic - can we justify it with <limits.h>, with OPEN_MAX or the
like?  8 should be replaced by CHAR_BIT, even though we know all clients
of this code will have 8-bit char, since gnulib is trying to set the
example (when possible) about how to be portable even to non-POSIX systems
with larger char.

> This code does not preserve the invariant that for any open fd, the flags
> stored inside MSVCRT for fd contain the bit FNOINHERIT if and only if
> the handle _get_osfhandle (fd) has the InheritHandle flag set to FALSE.

Ouch.  Good call.  Paolo's comment also holds about setting FD_CLOEXEC
being easier than clearing, if we are okay with child processes starting
with an open fd tied to an invalid handle.

>      O_APPEND unknown!

Is there seriously no way to ascertain whether an fd on Woe32 is in append
mode?  Wow, that will make implementing F_GETFL tough.

> I therefore think that instead of offering the POSIXy fcntl call,
> gnulib should offer functions like dup, dup2, dup_safer, fd_safer,
> that take a flags argument as additional parameter.

fcntl still makes sense for at least F_DUPFD{,_CLOEXEC}, F_GETFD.  It just
means that F_SETFD might be asymmetric (can set but not clear CLOEXEC) or
missing (ie. we conscientiously choose to only support methods that use
the appropriate open/dup mechanisms to create new fds with the correct
attributes up front rather than modifying an existing fd).

We've already overridden open() and friends for mingw, so we could
implement O_APPEND tracking for all fds created within our own process
(but not those inherited from a parent process).  I don't know if that is
helpful, or worse because we would then be inconsistent.

- --
Don't work too hard, make some time for fun as well!

Eric Blake             address@hidden
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.9 (Cygwin)
Comment: Public key at home.comcast.net/~ericblake/eblake.gpg
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/

iEYEARECAAYFAkqQuB4ACgkQ84KuGfSFAYAoRQCfZv3of9Nk0SAuO2H+KYvU+Kss
mPAAniwVnLYURmuh8a8KALsWDJlEDGUp
=Vn9N
-----END PGP SIGNATURE-----

From 7d8f602d6f6c1ac2633c23140c93aaca098c6a2a Mon Sep 17 00:00:00 2001
From: Eric Blake <address@hidden>
Date: Sat, 22 Aug 2009 21:24:37 -0600
Subject: [PATCH] fcntl-h: add O_TTY_INIT support

* lib/fcntl.in.h (O_TTY_INIT): Support another POSIX macro.
* tests/test-fcntl-h.c (o): Test it.
* doc/posix-headers/fcntl.texi (fcntl.h): Update documentation.

Signed-off-by: Eric Blake <address@hidden>
---
 ChangeLog                    |    5 +++++
 doc/posix-headers/fcntl.texi |    6 +++---
 lib/fcntl.in.h               |    6 +++++-
 tests/test-fcntl-h.c         |    2 +-
 4 files changed, 14 insertions(+), 5 deletions(-)

diff --git a/ChangeLog b/ChangeLog
index 4486d1a..bf65748 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,5 +1,10 @@
 2009-08-23  Eric Blake  <address@hidden>

+       fcntl-h: add O_TTY_INIT support
+       * lib/fcntl.in.h (O_TTY_INIT): Support another POSIX macro.
+       * tests/test-fcntl-h.c (o): Test it.
+       * doc/posix-headers/fcntl.texi (fcntl.h): Update documentation.
+
        fcntl-h: rename from fcntl, in preparation for fcntl(2)
        * modules/fcntl: Move <fcntl.h> header replacement...
        * modules/fcntl-h: ...to new name, so as not to collide with
diff --git a/doc/posix-headers/fcntl.texi b/doc/posix-headers/fcntl.texi
index f2d5fb2..6e770a3 100644
--- a/doc/posix-headers/fcntl.texi
+++ b/doc/posix-headers/fcntl.texi
@@ -9,8 +9,8 @@ fcntl.h
 @itemize
 @item
 @samp{O_NOCTTY}, @samp{O_DSYNC}, @samp{O_NONBLOCK}, @samp{O_RSYNC},
address@hidden, @samp{O_DIRECTORY}, and @samp{O_NOFOLLOW} are not
-defined on some platforms.
address@hidden, @samp{O_DIRECTORY}, @samp{O_NOFOLLOW}, and
address@hidden are not defined on some platforms.

 @item
 @samp{O_BINARY}, @samp{O_TEXT} (not specified by POSIX, but essential for
@@ -37,7 +37,7 @@ fcntl.h
 replacement is not atomic on these platforms.

 @item
address@hidden, @samp{O_SEARCH}, and @samp{O_EXEC} are not defined
address@hidden and @samp{O_EXEC} are not defined
 on some platforms.

 @item
diff --git a/lib/fcntl.in.h b/lib/fcntl.in.h
index a688fb4..8b92521 100644
--- a/lib/fcntl.in.h
+++ b/lib/fcntl.in.h
@@ -1,6 +1,6 @@
 /* Like <fcntl.h>, but with non-working flags defined to 0.

-   Copyright (C) 2006-2008 Free Software Foundation, Inc.
+   Copyright (C) 2006-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
@@ -130,6 +130,10 @@ extern void _gl_register_fd (int fd, const char *filename);
 # define O_SYNC 0
 #endif

+#ifndef O_TTY_INIT
+# define O_TTY_INIT 0
+#endif
+
 /* For systems that distinguish between text and binary I/O.
    O_BINARY is usually declared in fcntl.h  */
 #if !defined O_BINARY && defined _O_BINARY
diff --git a/tests/test-fcntl-h.c b/tests/test-fcntl-h.c
index 127f497..649c44a 100644
--- a/tests/test-fcntl-h.c
+++ b/tests/test-fcntl-h.c
@@ -22,7 +22,7 @@

 /* Check that the various O_* macros are defined.  */
 int o = O_DIRECT | O_DIRECTORY | O_DSYNC | O_NDELAY | O_NOATIME | O_NONBLOCK
-       | O_NOCTTY | O_NOFOLLOW | O_NOLINKS | O_RSYNC | O_SYNC
+       | O_NOCTTY | O_NOFOLLOW | O_NOLINKS | O_RSYNC | O_SYNC | O_TTY_INIT
        | O_BINARY | O_TEXT;

 /* Check that the various SEEK_* macros are defined.  */
-- 
1.6.3.3.334.g916e1