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
> +@item
> +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.
> +@item
> +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}.
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>
