bug-gnulib
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: getting EBADF on MSVC


From: Paul Eggert
Subject: Re: getting EBADF on MSVC
Date: Fri, 23 Sep 2011 22:19:13 -0700
User-agent: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.2.21) Gecko/20110831 Thunderbird/3.1.13

On 09/19/11 14:15, Bruno Haible wrote:
> Gnulib uses the "all workarounds for function foo() in file foo.c" approach
> since the beginning, and has been working very well with that.

I think this can work, but still, we should make it easy for
non-Windows programmers to see which part of foo.c is for Windows,
so that they can easily and safely ignore the Windows-related stuff.

On 09/20/11 02:51, Bruno Haible wrote:

> Paul Eggert wrote:
>> Emacs doesn't need to have a dummy msvc-inval.h file. ...
> you can create a replacement for this file in a single-line Makefile 
> statement.

One line would not be enough; the dummy file would need to be cleaned,
and there may well have to be some other stuff due to the way Emacs is
packaged.  I'd rather not have to think about that, though, and
luckily it's not needed; please see below.

> I want the module description to contain this:
> 
>   Include:
>   #include "msvc-inval.h"

That's fine.

> Because people tend to use the .h file without the #if protection,
> leading to compilation errors.

That was an issue for other include files, but for msvc-inval.h it's
preferable to use the .h file inside #if protection, and it's OK to do
that even if the "Include:" line is as quoted above.  Putting the
include inside the #if helps readers to easily see that this stuff is
needed only on Windows.

There is plenty of precedent for putting Windows-only #includes inside
appropriate #if, and there's plenty of precedent for putting
gnulib-private #includes inside appropriate #ifs.  Here, there's a
known and immediate technical win to putting the #include inside an
#if (as this simplifies Emacs), whereas any technical loss is only
hypothetical.  This is an easy call.


>> we won't need an msvc-inval.h file in Gnulib.
> 
> That cannot work. The three macros need to reference variables and functions
> defined in msvc-inval.c.

Good point.  OK, gnulib needs an msvc-inval.h file.

I briefly looked at the more recent changes you installed, and have
some further comments and suggestions and a couple of patches.

  * Why two files, msvc-inval.h and msvc-nothrow.h?
    Would it be simpler to have just one file?
    Is it likely that a package would want one but not the other?
    The "AC_DEFUN([gl_MSVC_NOTHROW], [AC_REQUIRE([gl_MSVC_INVAL])])"
    suggests that the two msvc-* modules should be merged, or at
    least that gl_MSVC_NOTHROW and m4/msvc-nothrow.m4 should be removed.

  * Why is the body of msvc-inval.c protected inside
    HAVE_MSVC_INVALID_PARAMETER_HANDLER?  Surely that file is compiled
    only when the macro is true.  There's a similar issue for
    msvc-nothrow.c.

  * Why does dup2.c need to use 'inline'?  When a static function is
    used in only one place, 'inline' is typically verbosity that's not
    needed: a compiler that's optimizing will inline anyway, and if
    we're not optimizing, then typically we won't want the inlining
    (for debugging purposes).  A similar comment applies to other
    *_nothrow functions.

  * Many uses of '#include "msvc-*' are protected inside
    Windows-specific #if directives, so they look safe and are easy
    for non-Windows programmers to ignore.  But some of them aren't.
    I suggest these be protected more consistently.  One file
    (sigprocmask.c) I did myself (see below), partly as an example,
    and partly because Emacs needs that particular change.  (However,
    this change shouldn't be done to files that are compiled only
    under Windows, where it's already obvious from the context that
    the #includes are Windows-only.)

  * dup2.c can benefit from more reorganization to make it shorter and
    easier to follow (especially for us non-Windows guys).  The basic
    idea is to put the Windows-only stuff into a separate section,
    easily delimited via an #if.  I took a stab at doing that (see
    below), and the result is shorter (and to my eyes) clearer and
    works for Emacs.  I haven't tested the new dup2.c under Windows
    but I hope that any Windows-related errors are trivial.

>From 7f621ee5a80e584d0aad33ede73f2317becd4dfd Mon Sep 17 00:00:00 2001
From: Paul Eggert <address@hidden>
Date: Fri, 23 Sep 2011 21:54:43 -0700
Subject: [PATCH 1/2] sigprocmask: move #include directive

* lib/sigprocmask.c: Move '#include "msvc-inval.h"' to the
Windows-specific section, so that the Emacs source need not
contain msvc-inval.h.
---
 ChangeLog         |    7 +++++++
 lib/sigprocmask.c |    4 ++--
 2 files changed, 9 insertions(+), 2 deletions(-)

diff --git a/ChangeLog b/ChangeLog
index 3c8a7a0..2c77669 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,10 @@
+2011-09-23  Paul Eggert  <address@hidden>
+
+       sigprocmask: move #include directive
+       * lib/sigprocmask.c: Move '#include "msvc-inval.h"' to the
+       Windows-specific section, so that the Emacs source need not
+       contain msvc-inval.h.
+
 2011-09-23  Bruno Haible  <address@hidden>
 
        read: Support for MSVC 9.
diff --git a/lib/sigprocmask.c b/lib/sigprocmask.c
index f1adc54..a9aff47 100644
--- a/lib/sigprocmask.c
+++ b/lib/sigprocmask.c
@@ -24,8 +24,6 @@
 #include <stdint.h>
 #include <stdlib.h>
 
-#include "msvc-inval.h"
-
 /* We assume that a platform without POSIX signal blocking functions
    also does not have the POSIX sigaction() function, only the
    signal() function.  We also assume signal() has SysV semantics,
@@ -61,6 +59,8 @@
 typedef void (*handler_t) (int);
 
 #if HAVE_MSVC_INVALID_PARAMETER_HANDLER
+# include "msvc-inval.h"
+
 static inline handler_t
 signal_nothrow (int sig, handler_t handler)
 {
-- 
1.7.4.4


>From eedc57eb2051c994e80caa24c2ff2451e400629a Mon Sep 17 00:00:00 2001
From: Paul Eggert <address@hidden>
Date: Fri, 23 Sep 2011 22:04:20 -0700
Subject: [PATCH 2/2] dup2: clarify by coalescing Windows-specific material

* lib/dup2.c: Move '#include "msvc-inval.h"' and '#include
"msvc-nothrow.h"' to the Windows-specific section, so that the
Emacs source need not contain these include files.
(ms_windows_dup2): Rename from dup2_nothrow, and move all the
Windows-specific fixes into this function rather than just the
nothrow fix, as this shortens and clarifies the code.  Always
define as a function, as that's a bit cleaner than having it be
sometimes a function and sometimes a macro.
(rpl_dup2): Move the Windows-specific stuff out of here and into
ms_windows_dup2.  Don't protect the Haiku-related fix with
"#if !defined __linux__", as the same code also works around
a Linux kernel bug, and it doesn't add any system calls on any
platform.  Add comment about FreeBSD 6.1.
---
 ChangeLog  |   15 ++++++++++
 lib/dup2.c |   89 +++++++++++++++++++++++++++---------------------------------
 2 files changed, 55 insertions(+), 49 deletions(-)

diff --git a/ChangeLog b/ChangeLog
index 2c77669..b33fd7b 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,5 +1,20 @@
 2011-09-23  Paul Eggert  <address@hidden>
 
+       dup2: clarify by coalescing Windows-specific material
+       * lib/dup2.c: Move '#include "msvc-inval.h"' and '#include
+       "msvc-nothrow.h"' to the Windows-specific section, so that the
+       Emacs source need not contain these include files.
+       (ms_windows_dup2): Rename from dup2_nothrow, and move all the
+       Windows-specific fixes into this function rather than just the
+       nothrow fix, as this shortens and clarifies the code.  Always
+       define as a function, as that's a bit cleaner than having it be
+       sometimes a function and sometimes a macro.
+       (rpl_dup2): Move the Windows-specific stuff out of here and into
+       ms_windows_dup2.  Don't protect the Haiku-related fix with
+       "#if !defined __linux__", as the same code also works around
+       a Linux kernel bug, and it doesn't add any system calls on any
+       platform.  Add comment about FreeBSD 6.1.
+
        sigprocmask: move #include directive
        * lib/sigprocmask.c: Move '#include "msvc-inval.h"' to the
        Windows-specific section, so that the Emacs source need not
diff --git a/lib/dup2.c b/lib/dup2.c
index e2a4473..04f2a4d 100644
--- a/lib/dup2.c
+++ b/lib/dup2.c
@@ -25,48 +25,24 @@
 #include <errno.h>
 #include <fcntl.h>
 
-#if (defined _WIN32 || defined __WIN32__) && ! defined __CYGWIN__
-/* Get declarations of the Win32 API functions.  */
-# define WIN32_LEAN_AND_MEAN
-# include <windows.h>
-/* Get _get_osfhandle.  */
-# include "msvc-nothrow.h"
-#endif
-
-#include "msvc-inval.h"
-
 #if HAVE_DUP2
 
 # undef dup2
 
-# if HAVE_MSVC_INVALID_PARAMETER_HANDLER
+# if defined _WIN32 || defined __WIN32__
+#  include "msvc-inval.h"
+#  ifndef __CYGWIN__
+#   define WIN32_LEAN_AND_MEAN
+#   include <windows.h>
+#   include "msvc-nothrow.h"
+#  endif
+
 static inline int
-dup2_nothrow (int fd, int desired_fd)
+ms_windows_dup2 (int fd, int desired_fd)
 {
   int result;
 
-  TRY_MSVC_INVAL
-    {
-      result = dup2 (fd, desired_fd);
-    }
-  CATCH_MSVC_INVAL
-    {
-      result = -1;
-      errno = EBADF;
-    }
-  DONE_MSVC_INVAL;
-
-  return result;
-}
-# else
-#  define dup2_nothrow dup2
-# endif
-
-int
-rpl_dup2 (int fd, int desired_fd)
-{
-  int result;
-# if (defined _WIN32 || defined __WIN32__) && ! defined __CYGWIN__
+#  ifndef __CYGWIN__
   /* If fd is closed, mingw hangs on dup2 (fd, fd).  If fd is open,
      dup2 (fd, fd) returns 0, but all further attempts to use fd in
      future dup2 calls will hang.  */
@@ -79,6 +55,8 @@ rpl_dup2 (int fd, int desired_fd)
         }
       return fd;
     }
+#  endif
+
   /* Wine 1.0.1 return 0 when desired_fd is negative but not -1:
      http://bugs.winehq.org/show_bug.cgi?id=21289 */
   if (desired_fd < 0)
@@ -86,28 +64,41 @@ rpl_dup2 (int fd, int desired_fd)
       errno = EBADF;
       return -1;
     }
-# elif !defined __linux__
-  /* On Haiku, dup2 (fd, fd) mistakenly clears FD_CLOEXEC.  */
-  if (fd == desired_fd)
-    return fcntl (fd, F_GETFL) == -1 ? -1 : fd;
-# endif
-
-  result = dup2_nothrow (fd, desired_fd);
 
-# ifdef __linux__
-  /* Correct a Linux return value.
-     
<http://git.kernel.org/?p=linux/kernel/git/stable/linux-2.6.30.y.git;a=commitdiff;h=2b79bc4f7ebbd5af3c8b867968f9f15602d5f802>
-   */
-  if (fd == desired_fd && result == (unsigned int) -EBADF)
+  TRY_MSVC_INVAL
+    {
+      result = dup2 (fd, desired_fd);
+    }
+  CATCH_MSVC_INVAL
     {
       errno = EBADF;
       result = -1;
     }
-# endif
+  DONE_MSVC_INVAL;
+
+  /* Cygwin 1.5.x dup2 (1, 1) returns 0.  */
   if (result == 0)
     result = desired_fd;
-  /* Correct a cygwin 1.5.x errno value.  */
-  else if (result == -1 && errno == EMFILE)
+
+  return result;
+}
+#  define dup2 ms_windows_dup2
+# endif
+
+int
+rpl_dup2 (int fd, int desired_fd)
+{
+  int result;
+
+  /* On Linux kernels 2.6.26-2.6.29, dup2 (fd, fd) returns -EBADF.
+     On Haiku, dup2 (fd, fd) mistakenly clears FD_CLOEXEC.  */
+  if (fd == desired_fd)
+    return fcntl (fd, F_GETFL) == -1 ? -1 : fd;
+
+  result = dup2 (fd, desired_fd);
+
+  /* Correct an errno value on FreeBSD 6.1 and Cygwin 1.5.x.  */
+  if (result == -1 && errno == EMFILE)
     errno = EBADF;
 # if REPLACE_FCHDIR
   if (fd != desired_fd && result != -1)
-- 
1.7.4.4





reply via email to

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