Re: getting EBADF on MSVC

[Bruno Haible]

Hi Paul,

>   This patch protects the dup2() calls in gnulib.
> 
> ... I suspect there will be similar problems with printf, close, etc.

Yes, I suspect the idiom used in dup2.c will be used ca. 20 times in gnulib.

> I can't achieve this omission by invoking
> gnulib-tool --avoid=msvc-inval, because dup2.c now includes
> msvc-inval.h unilaterally.
> 
> I'd like a solution where --avoid=msvc-inval works.

I'm repeating myself when I explain that the usual ways to customize the
contents of gnulib for a package are the following two approaches:

  a) Use the --local-dir option to point to a directory where you have
     files that override the files from gnulib. You can override a module
     description, or the source code, or both. You can also store .diff
     files that only apply small changes.

  b) Use --avoid options _and_ replacement code for the modules that you
     want to avoid.

In this particular case, for Emacs, this means the 3 easiest choices for
you are:

  a) Have a file gnulib-local/modules/msvc-inval that is a reduced variant
     of gnulib's module/msvc-inval. Likewise for gnulib-local/lib/msvc-inval.h.
     No gnulib-local/lib/msvc-inval.c is needed.

  b.1) You store this file in Emacs' VCS

         ============= msvc-inval.h ==========
         #define TRY_MSVC_INVAL if (1)
         #define CATCH_MSVC_INVAL else
         #define DONE_MSVC_INVAL
         =====================================

       and use --avoid=msvc-inval.

  b.2) You store no additional file in Emacs' VCS. Just one addition to
       configure.ac or aclocal.m4:

         AH_VERBATIM([
         #define TRY_MSVC_INVAL if (1)
         #define CATCH_MSVC_INVAL else
         #define DONE_MSVC_INVAL
         ])

       And you have a Makefile rule that does

         echo '/* This file is a dummy. See config.h. */' > msvc-inval.h

       And use --avoid=msvc-inval.

I hope you can choose among these three solutions.

Now back to gnulib.

Most modules offer a certain API; you can not just --avoid them without
providing a replacement for the header file and/or macros the module
defines.

When you are asking for --avoid=some-module to work without any replacement
code, you are asking for gnulib to change the way modules are structured
or what the modules provide.

  * If you ask for "--avoid=msvc-inval" to work without any replacement code,
    you ask to change the contract of the module, so that its include file
    may only be conditionally included. This will not happen. One feature that
    makes gnulib attractive is that you can include its header files
    unconditionally. Before gnulib, we were writing

      #if HAVE_UNISTD_H
      # include <unistd.h>
      #endif

    Now we are writing

      #include <unistd.h>

    This simple style is one of the major features of gnulib.

  * If you ask for a "--avoid=dup2 dup2-no-msvc", you ask to introduce
    additional modules for the sake of easier removal of MSVC specific
    code. But this means all users of gnulib will then get 2 files
    (dup2.c and dup2-no-msvc.c) instead of a single file. That means,
    make gnulib look more bloated to most packages, for the sake of
    saving 2 files in one package, Emacs. This is not a good trade-off.

    Additionally, people will then ask for modules like 'dup2-no-aix',
    'dup2-no-obsolete-platforms' etc. etc. This path would lead to a
    maintenance nightmare, for little benefit.

> Here's one way to do it.  In dup2.c:
> 
> #if MSVC_HACK_NEEDED
> # include "msvc-inval.h"
> #else
> # define TRY_MSVC_INVAL if (1)
> # define CATCH_MSVC_INVAL else
> # define DONE_MSVC_INVAL
> #endif

You wouldn't want to have 20 copies of this block in code that you maintain.
I don't want it either.

>   > @@ -64,7 +78,18 @@
>   >    if (fd == desired_fd)
>   >      return fcntl (fd, F_GETFL) == -1 ? -1 : fd;
>   >  # endif
>   > -  result = dup2 (fd, desired_fd);
>   > +
>   > +  TRY_MSVC_INVAL
>   > +    {
>   > +      result = dup2 (fd, desired_fd);
>   > +    }
>   > +  CATCH_MSVC_INVAL
>   > +    {
>   > +      result = -1;
>   > +      errno = EBADF;
>   > +    }
>   > +  DONE_MSVC_INVAL
> 
> This style is unsatisfying, because it propagates MSVC workarounds
> into the rest of gnulib.

Gnulib uses the "all workarounds for function foo() in file foo.c" approach
since the beginning, and has been working very well with that. If you suggest
to treat platform specific workarounds in platform specific files, then

  - Workarounds for one platform are not automatically applied to all other
    platforms that have the same bug. But that is the very basis of autoconf.

  - The total number of files increases a lot.

  - There is a lot of code duplication between the different platform-dependent
    files. (Just yesterday I discovered a select() bug that exists on Linux,
    BSD systems, and OSF/1. Would it make sense to add the same workaround to
    select-linux.c, select-bsd.c, select-osf1.c?)

>      # if MSVC_HACK_NEEDED            /* NEW */
>      #  include "msvc-hack.h"         /* NEW */
>      # endif                          /* NEW */

No.

> I suspect it won't scale well when used
> throughout gnulib for other functions.

I don't see what you mean. Sure, 9 lines of code instead of 1 line of code
is not pretty. But the idiom can be applied 10 times or 100 times without
problems.

> I'd rather have a solution
> that puts the MSVC workarounds in a separate place, where MSVC-aware
> programmers can work on them, a place which people who don't know or
> care about MSVC ports can omit and ignore.

I don't see why a lack of MSVC knowledge would prevent anyone from modifying
these files. The code structure and intent are clear enough. I mean, Jim and
you and I have been modifying lib/fsusge.c without having working knowledge
about Ultrix, Dynix, or Dolphin. lib/dup2.c is not different.

Bruno
-- 
In memoriam James A. Garfield <http://en.wikipedia.org/wiki/James_A._Garfield>