Re: [PATCH 2/4] Add a new sethostname module

[Bruno Haible]

Ben Walton wrote:
> Define sethostname on platforms that do not provide the declaration.
> Provide a function for platforms that lack it.  The general handling
> of the provided function is to simply return -1 and set errno to
> ENOSYS.  A specific handler is provided for Minix.

Thanks, I'm applying this for you. I trust that you will follow through
with the paperwork.

I'm applying a couple of followup tweaks.

> address@hidden
> +On some platforms the Gnulib replacement always fails with ENOSYS.

The ENOSYS failure is not a problem the Gnulib fixes, but it is more a
limitation that Gnulib has, when fixing the "missing function" problem.

> +  /* NAME does not need to be null terminated so leave room to terminate
> +     regardless of input. */
> +  char hostname[HOST_NAME_MAX + 1];
> +  memcpy ((void *) hostname, (const void *) name, len);
> +  hostname[len] = '\0';

Copying the host name argument into a stack allocated array is actually
not needed, if we don't use functions like strlen(). The fprintf()
call below can be changed to not assume a NUL terminated string.

> +#ifdef __minix /* Minix */
> +  {
> +    FILE *hostf;
> +    int r = 0;
> +
> +    /* glibc returns EFAULT, EINVAL, and EPERM on error.  None of
> +       these are appropriate for us to set, even if they may match the
> +       situation, during failed open/write/close operations, so we
> +       leave errno alone and rely on what the system sets up. */
> +    hostf = fopen ("/etc/hostname.file", "w");
> +    if (hostf == NULL)
> +      r = -1;
> +    else
> +      {
> +        fprintf (hostf, "%s\n", hostname);
> +        if (ferror (hostf))
> +          {
> +            clearerr (hostf);
> +            r = -1;

Here the clearerr call is unnecessary, because the next call is fclose (hostf),
which doesn't care about the stream's error flag.

But here you need to save errno. Otherwise, if fprintf failed but the fclose
then succeeds, you end up with whatever error code the fclose call may have
put into errno. If fclose succeeds you have to ignore the value that it
may have put into errno - that is a general rule about POSIX functions and
errno.

> +          }
> +
> +        /* use return value of fclose for function return value as it
> +           matches our needs.  fclose will also set errno on
> +           failure */
> +        r = fclose (hostf);

Using the same return value here is not portable. When fclose fails, it
returns EOF. Whereas when sethostname fails, it should return a negative
value. There is no guarantee that EOF is negative. While EOF happens to be
-1 on most systems, it could also be defined to 0x10000000 on some systems.

> diff --git a/m4/sethostname.m4 b/m4/sethostname.m4
> new file mode 100644
> index 0000000..be5b036
> --- /dev/null
> +++ b/m4/sethostname.m4
> @@ -0,0 +1,25 @@
> +# sethostname.m4 serial 1
> +dnl Copyright (C) 2011 Free Software Foundation, Inc.
> +dnl This file is free software; the Free Software Foundation
> +dnl gives unlimited permission to copy and/or distribute it,
> +dnl with or without modifications, as long as this notice is preserved.
> +
> +# Ensure
> +# - the sethostname() function,

I updated the comment here, to mention HOST_NAME_MAX.

> +AC_DEFUN([gl_FUNC_SETHOSTNAME],
> +[
> +  AC_REQUIRE([gl_UNISTD_H_DEFAULTS])
> +
> +  gl_PREREQ_HOST_NAME_MAX
> +
> +  AC_REPLACE_FUNCS([sethostname])
> +  AC_CHECK_FUNCS([sethostname])

AC_REPLACE_FUNCS([sethostname]) is equivalent to
     AC_CHECK_FUNCS([sethostname])
     if test $ac_cv_func_sethostname = no; then
       AC_LIBOBJ([sethostname])
     fi

But in gnulib we now avoid to put AC_LIBOBJ calls into *.m4 files; we put them
into the module description instead.

> +Include:
> +<unistd.h>
> +
> +Link:
> +

An empty 'Link' section can be removed. Just a matter of style. The
modules/TEMPLATE file contains a skeleton of the fields that should
be present, even when empty.


2011-12-03  Bruno Haible  <address@hidden>

        Tweak last commit.
        * lib/sethostname.c: Don't include <string.h>.
        (sethostname): No need to copy the argument string to the stack. Don't
        call clearerr. Preserve errno when fprintf failed.
        * m4/sethostname.m4 (gl_FUNC_SETHOSTNAME): Comment about HOST_NAME_MAX.
        Don't invoke AC_REPLACE_FUNCS.
        * modules/sethostname (Link): Remove empty section.
        * doc/glibc-functions/sethostname.texi: Gnulib does not fix the ENOSYS
        failure problem.

--- doc/glibc-functions/sethostname.texi.orig   Sat Dec  3 13:54:54 2011
+++ doc/glibc-functions/sethostname.texi        Sat Dec  3 13:54:31 2011
@@ -9,6 +9,7 @@
 @item
 This function is missing on some platforms:
 Minix 3.1.8, Cygwin, mingw, MSVC 9, Interix 3.5, BeOS.
+Note that the Gnulib replacement may fail with ENOSYS on some platforms.
 @item
 This function is not declared on some platforms:
 AIX 7.1, OSF/1 5.1, Solaris 10.
@@ -16,8 +17,6 @@
 On Solaris 10, the first argument is @code{char *} instead of
 @code{const char *} and the second parameter is @code{int} instead of
 @code{size_t}.
address@hidden
-On some platforms the Gnulib replacement always fails with ENOSYS.
 @end itemize
 
 Portability problems not fixed by Gnulib:
--- lib/sethostname.c.orig      Sat Dec  3 13:54:54 2011
+++ lib/sethostname.c   Sat Dec  3 13:44:01 2011
@@ -26,7 +26,6 @@
 
 #include <errno.h>
 #include <stdio.h>
-#include <string.h>
 #include <limits.h>
 
 /* Set up to LEN chars of NAME as system hostname.
@@ -43,12 +42,6 @@
       return -1;
     }
 
-  /* NAME does not need to be null terminated so leave room to terminate
-     regardless of input. */
-  char hostname[HOST_NAME_MAX + 1];
-  memcpy ((void *) hostname, (const void *) name, len);
-  hostname[len] = '\0';
-
 #ifdef __minix /* Minix */
   {
     FILE *hostf;
@@ -63,24 +56,28 @@
       r = -1;
     else
       {
-        fprintf (hostf, "%s\n", hostname);
+        fprintf (hostf, "%.*s\n", (int) len, name);
         if (ferror (hostf))
           {
-            clearerr (hostf);
+            /* Close hostf, preserving the errno from the fprintf call.  */
+            int saved_errno = errno;
+            fclose (hostf);
+            errno = saved_errno;
             r = -1;
           }
-
-        /* use return value of fclose for function return value as it
-           matches our needs.  fclose will also set errno on
-           failure */
-        r = fclose (hostf);
+        else
+          {
+            if (fclose (hostf))
+              /* fclose sets errno on failure.  */
+              r = -1;
+          }
       }
 
     return r;
   }
 #else
   /* For platforms that we don't have a better option for, simply bail
-     out */
+     out.  */
   errno = ENOSYS;
   return -1;
 #endif
--- m4/sethostname.m4.orig      Sat Dec  3 13:54:54 2011
+++ m4/sethostname.m4   Sat Dec  3 13:32:48 2011
@@ -6,13 +6,13 @@
 
 # Ensure
 # - the sethostname() function,
+# - the HOST_NAME_MAX macro in <limits.h>.
 AC_DEFUN([gl_FUNC_SETHOSTNAME],
 [
   AC_REQUIRE([gl_UNISTD_H_DEFAULTS])
 
   gl_PREREQ_HOST_NAME_MAX
 
-  AC_REPLACE_FUNCS([sethostname])
   AC_CHECK_FUNCS([sethostname])
   if test $ac_cv_func_sethostname = no; then
     HAVE_SETHOSTNAME=0
--- modules/sethostname.orig    Sat Dec  3 13:54:54 2011
+++ modules/sethostname Sat Dec  3 13:31:33 2011
@@ -22,8 +22,6 @@
 Include:
 <unistd.h>
 
-Link:
-
 License:
 LGPLv2+
 


-- 
In memoriam Rudolf Slánský <http://en.wikipedia.org/wiki/Rudolf_Slánský>