Re: getting EBADF on MSVC

[Paul Eggert]

On 09/24/11 04:53, Bruno Haible wrote:

> the desire to minimize the number of files did not allow the code to be
> structured in a good way.

Yes, of course.  Keeping the number of files small is just one
constraint among many, and is often overridden by other constraints.

> It is more important to have the room for new code to be added in the proper
> places, than to reduce the number of files.

But there is always room for new code.  There's no general need to
create placeholders now for improvements that may or may not happen later.

Anyway, it's not a big deal either way; I was just trying to help keep
things simple.  I still don't understand why a package would want to
have one set of features but not the other, but I'll take your word
that they should be kept separate.

>>   * Why does dup2.c need to use 'inline'?...
> 
> Do you know the optimizing behaviour of MSVC, or are you making assumptions?

It's the other way around, no?  If we don't know the optimizing
behavior then we should omit "inline".  The keyword is extra baggage
to read and maintain, and should be put in only if we have a pretty
good idea that there's a performance benefit that's worth the
maintenance cost.  (The maintenance cost includes the cost of the poor
reader seeing the "inline" and wondering why it's there. :-)


> * It contains the code
> 
>   if (fd == desired_fd)
>     return fcntl (fd, F_GETFL) == -1 ? -1 : fd;
> 
>   unprotected by #ifs, which will lead to compilation failure on native
>   Windows platforms. They don't have fcntl() nor F_GETFL.

Ah, OK, then we should #if-protect the code.  But surely it's simpler
to use "#ifdef F_GETFL" here, as that captures the point more precisely.

> I have tested this patch on Cygwin and mingw.

Thanks for doing that.  I hope you don't mind if I don't take up your
kind offer to hack within a Windows-based environment.  I view Windows
much the same way as I view QNX, C#, iOS, and Cobol -- all
important practical environments to be sure, but
environments that I'd rather minimize my efforts on.

With the above comments in mind, I pushed this further
attempt at simplifying things:

dup2: minor simplifications
* lib/dup2.c (ms_windows_dup2): Omit 'inline' as it's not clear
that it's a performance win.
(rpl_dup2): Change "if !((defined _WIN32 || defined __WIN32__) &&
! defined __CYGWIN__)" to "ifdef F_GETFL".
diff --git a/lib/dup2.c b/lib/dup2.c
index 278261f..790c98a 100644
--- a/lib/dup2.c
+++ b/lib/dup2.c
@@ -40,7 +40,7 @@
 /* Get _get_osfhandle.  */
 #  include "msvc-nothrow.h"

-static inline int
+static int
 ms_windows_dup2 (int fd, int desired_fd)
 {
   int result;
@@ -92,7 +92,7 @@ rpl_dup2 (int fd, int desired_fd)
 {
   int result;

-# if !((defined _WIN32 || defined __WIN32__) && ! defined __CYGWIN__)
+# ifdef F_GETFL
   /* On Linux kernels 2.6.26-2.6.29, dup2 (fd, fd) returns -EBADF.
      On Cygwin 1.5.x, dup2 (1, 1) returns 0.
      On Haiku, dup2 (fd, fd) mistakenly clears FD_CLOEXEC.  */