GNU bug report logs - #9141
Coreutils Compiler Warnings on OSX 10.7 (Lion)

Previous Next

Full log

Message #32 received at 9141 <at> debbugs.gnu.org (full text, mbox):

From: Bruno Haible <bruno <at> clisp.org>
To: bug-gnulib <at> gnu.org
Cc: 9141 <at> debbugs.gnu.org, Paul Eggert <eggert <at> cs.ucla.edu>
Subject: Re: fdatasync module proposal
Date: Sat, 23 Jul 2011 04:42:23 +0200

Hi Paul,

> Surely coreutils is not the only program that will have problems
> with fdatasync on Mac OS.  How about the following gnulib patches?
> One is for fdatasync, the other for its tests.

Looks mostly good. Just small comments:

> --- a/lib/unistd.in.h
> +++ b/lib/unistd.in.h
> @@ -483,6 +483,34 @@ _GL_WARN_ON_USE (fchownat, "fchownat is not portable - "
>  #endif
>  
>  
> +#if @GNULIB_FDATASYNC@
> +/* Synchronize data changes to a file.
> +   Return 0 if successful, otherwise -1 and errno set.
> +   See POSIX:2001 specification
> +   <http://www.opengroup.org/susv3xsh/fdatasync.html>.  */

POSIX:2001 is superseded by POSIX:2008. Even if the wording in both standards
is the same for this function, it is good to encourage people to look at and
to refer to the newest standard. So, here I would write

    See POSIX:2008 specification
    <http://www.opengroup.org/onlinepubs/9699919799/functions/fdatasync.html>.  */

> diff --git a/m4/fdatasync.m4 b/m4/fdatasync.m4
> new file mode 100644
> index 0000000..af17970
> --- /dev/null
> +++ b/m4/fdatasync.m4
> @@ -0,0 +1,34 @@
> ...
> +  else
> +    gl_saved_libs=$LIBS
> +      AC_SEARCH_LIBS([fdatasync], [rt posix4],
> +                     [test "$ac_cv_search_fdatasync" = "none required" ||
> +                      LIB_FDATASYNC=$ac_cv_search_fdatasync])
> +      AC_CHECK_FUNCS([fdatasync])
> +    LIBS=$gl_saved_libs

Here one could add a comment like:

       dnl Solaris <= 2.6 has fdatasync() in libposix4.
       dnl Solaris 7..10 has it in librt.

Just for reference, because in 5 years nobody would remember.

> diff --git a/modules/fdatasync b/modules/fdatasync
> new file mode 100644
> index 0000000..94980ec
> --- /dev/null
> +++ b/modules/fdatasync
> ...
> +fsync           [test $HAVE_FDATASYNC = 0]
> +unistd
> +
> +configure.ac:
> +gl_FUNC_FDATASYNC
> +if test $HAVE_FDATASYNC = 0; then

It is safer (w.r.t. future modifications) and more consistent with the
hundreds of other modules to also test $REPLACE_FDATASYNC here:

                   [test $HAVE_FDATASYNC = 0 || test $REPLACE_FDATASYNC = 1]

>    const char *file = "test-fsync.txt";

With this definition, the test-fsync and test-fdatasync programs will write
to the same file. That is, when run with "make -j2", they may collide.
You need to take a different file name for test-fdatasync.

> --- /dev/null
> +++ b/tests/test-fdatasync.c
> @@ -0,0 +1,2 @@
> +#define FSYNC fdatasync
> +#include "test-fsync.c"

> diff --git a/tests/test-fsync.c b/tests/test-fsync.c
> index 2627d0c..6bac01c 100644
> --- a/tests/test-fsync.c
> +++ b/tests/test-fsync.c
> @@ -18,8 +18,12 @@
>  
>  #include <unistd.h>
>  
> +#ifndef FSYNC
> +# define FSYNC fsync
> +#endif
> +
>  #include "signature.h"
> -SIGNATURE_CHECK (fsync, int, (int));
> +SIGNATURE_CHECK (FSYNC, int, (int));
>  
>  #include <errno.h>
>  #include <fcntl.h>
> @@ -32,7 +36,7 @@ main (void)
>    int fd;
>    const char *file = "test-fsync.txt";
>  
> -  if (fsync (0) != 0)
> +  if (FSYNC (0) != 0)
>      {
>        ASSERT (errno == EINVAL /* POSIX */
>                || errno == ENOTSUP /* seen on MacOS X 10.5 */
> @@ -42,7 +46,7 @@ main (void)
>    fd = open (file, O_WRONLY|O_CREAT|O_TRUNC, 0644);
>    ASSERT (0 <= fd);
>    ASSERT (write (fd, "hello", 5) == 5);
> -  ASSERT (fsync (fd) == 0);
> +  ASSERT (FSYNC (fd) == 0);
>    ASSERT (close (fd) == 0);
>    ASSERT (unlink (file) == 0);
>  

Here, like with test-pselect, I would move everything after the signature test
to a separate file, test-fsync.h, that is included by both test-fsync.c and
test-fdatasync.c. This avoids #ifdefs (following the general rule to prefer
C functions over C macros), and gives freedom to add tests that apply to one
of the functions but not both.

Bruno
-- 
In memoriam Dmitry Pavlov <http://en.wikipedia.org/wiki/Dmitry_Pavlov_(general)>

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.