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

[Bruno Haible]

Ben Walton wrote:
> --- a/doc/glibc-functions/sethostname.texi
> +++ b/doc/glibc-functions/sethostname.texi
> @@ -2,10 +2,15 @@
>  @subsection @code{sethostname}
>  @findex sethostname
>  
> -Gnulib module: ---
> +Gnulib module: sethostname

Good.

>  Portability problems fixed by Gnulib:
>  @itemize
> address@hidden
> +On AIX 7.1, OSF/1 5.1 and Solaris 10 the declaration is missing.

Here you can just move the wording from the "problems not fixed by Gnulib"
to the "problems fixed by Gnulib" section.

> On
> +Minix 3.1.8, Cygwin, mingw, MSVC 9, Interix 3.5, BeOS the function is
> +missing.

Likewise.

> Provide a fallback for all platforms that returns -1 and
> +sets ENOSYS.

Can you reword this as a limitation? "But on some platforms
the Gnulib replacement always fails with ENOSYS."

> Provide a specific implementation for Minix 3.1.8

It's not worth mentioning that the function will work on Minix. Here
in the doc we mention only the problems.

> address@hidden
> +On Solaris 10, the first argument is @code{char *} instead of
> address@hidden char *} and the second parameter is @code{int} instead of
> address@hidden

With further adjustments to the patch, this will go to the section
"problems fixed by Gnulib", I guess?

> +/* Set up to LEN chars of NAME as system hostname.
> +   Return 0 if ok, -1 if error.  */

The comments should also say that errno is set upon error.

> +int
> +sethostname (const char *name, size_t len)
> +{
> +  /* glibc seems to allow a zero length hostname: bail on names that
> +     are too long or too short */
> +  if (len < 0 || len > HOST_NAME_MAX)
> +    {
> +      errno = EINVAL;
> +      return -1;
> +    }

Good idea to check the length. But the "len < 0" expression will always
evaluate to false, since size_t is an unsigned type.

> +  /* NAME does not need to be null terminated so leave room to terminate
> +     regardless of input. */
> +  char hostname[len + 1];

An array with dynamically computed size, as a local variable in a function,
is an ISO C 99 feature that many compilers don't yet support. The alternative
is a fixed-size array or a malloc(). Since you already verified the bounds on
len, a fixed-size array seems more appropriate to me.

> +  strncpy(hostname, name, len);

GNU coding style prefers to have a space before the opening parenthesis for
function and macro calls:

     strncpy (hostname, name, len);

Oh, and why not use memcpy? It's a simpler function than strncpy.

> +  hostname[len] = '\0';
> +
> +#ifdef __minix /* Minix */
> +  FILE *hostf;

Declaration after statement is also a C99 feature that we cannot assume 
portably.
Workaround: Insert braces around the code that needs the 'hostf' variable.

> +
> +  /* leave errno alone in this case as it will provide a more useful error
> +     indication than overriding with ENOSYS */
> +  if ((hostf = fopen("/etc/hostname.file", "w")) == NULL)

Stylistically, we find it better to not have assignments inside 'if' conditions.
Yeah, I know the Linux kernel code style is just the opposite...

> +    return -1;
> +  else
> +    {
> +      fprintf(hostf, "%s\n", hostname);
> +      fclose(hostf);

Robust programming: Check the return value of fclose(), and check whether there
was an error on the stream before fclose().
> +      return 0;
> +    }
> +#else
> +  /* For platforms that we don't have a better option for, simply bail
> +     out */
> +  errno = ENOSYS;
> +  return -1;
> +#endif
> +}

OK for the rest.

> diff --git a/m4/sethostname.m4 b/m4/sethostname.m4
> new file mode 100644
> index 0000000..dbdbb39
> --- /dev/null
> +++ b/m4/sethostname.m4
> @@ -0,0 +1,21 @@
> +# 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,
> +AC_DEFUN([gl_FUNC_SETHOSTNAME],
> +[
> +  AC_REQUIRE([gl_UNISTD_H_DEFAULTS])
> +
> +  AC_REPLACE_FUNCS([sethostname])
> +  AC_CHECK_DECLS([sethostname])

Good, but you also need to set HAVE_SETHOSTNAME.

> +  if test $ac_cv_func_sethostname = no; then
> +    gl_PREREQ_HOST_NAME_MAX
> +  fi

The caller of sethostname() may want to use the HOST_NAME_MAX
unconditionally. I would therefore invoke gl_PREREQ_HOST_NAME_MAX
unconditionally. (We have no module for <limits.h>, otherwise we
would do it there.)

> diff --git a/modules/sethostname b/modules/sethostname
> new file mode 100644
> index 0000000..a0f36a2
> --- /dev/null
> +++ b/modules/sethostname
> @@ -0,0 +1,32 @@
> +Description:
> +sethostname() function: Set machine's hostname.
> +
> +Files:
> +lib/sethostname.c
> +m4/sethostname.m4
> +
> +Depends-on:
> +unistd
> +gethostname

The sethostname replacement does not need the code (lib/gethostname.c),
nor does it need to invoke gl_FUNC_GETHOSTNAME. All it needs is the
file m4/gethostname.m4. Therefore I would remove the dependency and
instead add m4/gethostname.m4 to the list of files.

> +configure.ac:
> +gl_FUNC_SETHOSTNAME
> +if test $HAVE_SETHOSTNAME = 0; then
> +  AC_LIBOBJ([sethostname])
> +fi
> +gl_UNISTD_MODULE_INDICATOR([sethostname])
> +
> +Makefile.am:
> +
> +Include:
> +<unistd.h>

Good.

> +Link:
> +$(SETHOSTNAME_LIB)

On all platforms which have sethostname(), the function is contained in
libc. No need to link with anything special.

Bruno
-- 
In memoriam Alfred Herrhausen <http://en.wikipedia.org/wiki/Alfred_Herrhausen>