Re: [PATCH 4/4] Add a test suite for the sethostname module

[Bruno Haible]

Ben Walton wrote:
> Provide a module that tests the functionality of sethostname().

Thanks, I'm applying this as well.

> Signed-off-by: Ben Walton <address@hidden>
> ---
>  ChangeLog                 |    6 ++
>  modules/sethostname-tests |   13 +++++
>  tests/test-sethostname.c  |  117 
> +++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 136 insertions(+), 0 deletions(-)
>  create mode 100644 modules/sethostname-tests
>  create mode 100644 tests/test-sethostname.c
> 
> diff --git a/ChangeLog b/ChangeLog
> index 67f7f42..2e85f16 100644
> --- a/ChangeLog
> +++ b/ChangeLog
> @@ -1,5 +1,11 @@
>  2011-12-01  Ben Walton  <address@hidden>
>  
> +     * modules/sethostname-tests: New file.  A test program
> +     for the sethostname module.
> +     * tests/test-sethostname.c: Likewise.
> +     

A trailing blank here (in place of a blank line) disturbs the 
git-merge-changelog
program.

> +#define TESTHOSTNAME "gnulib-hostname"
> +
> +/* mingw and MSVC 9 lack geteuid, so setup a value that will indicate
> +   we don't have root privilege since we wouldn't know whether to
> +   expect success or failure when setting a name anyway*/
> +#if !HAVE_GETEUID
> +# define geteuid() ((uid_t) -1)
> +#endif

Wow! You have thought at many things. You did it better than if I had
written this test.

Just two things:
  - If a test fails, it's most conventional (in Gnulib) to print the cause
    to stderr, not to stdout. 
  - When we have to skip the major part of the test, by convention it should
    return 77 (so that the Automake generated Makefile rule prints "SKIP:"
    instead of "PASS:") and - by Gnulib convention - also print an explanation
    why the test was skipped. This helps detecting bugs.

> +  if (rcs != 0)
> +    {
> +      if (rcs == -1 && errno == ENOSYS)
> +     return 0;

Oops, that looks like a tab again.

> +  rcs = sethostname ((const char *) longname, (HOST_NAME_MAX + 1));
> +
> +  if (rcs != -1)
> +    {
> +      /* attempt to restore the original name. */
> +      sethostname (origname, strlen (origname));
> +      printf ("expected failure when setting very long hostname.\n");

Error messages should not state in the first place what the program
expected, but what events occurred or what data actually appeared,
optionally with a small hint to what the program expected.


I'm applying these tweaks:


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

        Tweak last commit.
        * modules/sethostname-tests (Files): Sort by decreasing importance.
        (configure.ac): Check for geteuid.
        * tests/test-sethostname.c (main): Emit error messages to stderr. Skip
        the test when there's nothing to test. Drop an unnecessary cast.
        Improve an error message. Verify that the final sethostname() call
        succeeds.

--- modules/sethostname-tests.orig      Sat Dec  3 14:38:36 2011
+++ modules/sethostname-tests   Sat Dec  3 14:32:34 2011
@@ -1,12 +1,13 @@
 Files:
-tests/signature.h
 tests/test-sethostname.c
+tests/signature.h
 tests/macros.h
 
 Depends-on:
 sys_types
 
 configure.ac:
+AC_CHECK_FUNCS_ONCE([geteuid])
 
 Makefile.am:
 TESTS += test-sethostname
--- tests/test-sethostname.c.orig       Sat Dec  3 14:38:36 2011
+++ tests/test-sethostname.c    Sat Dec  3 14:38:16 2011
@@ -55,13 +55,16 @@
      consider things like CAP_SYS_ADMIN (linux) or PRIV_SYS_ADMIN
      (solaris), etc.  systems without a working geteuid (mingw, MSVC
      9) will always skip this test. */
-  if (geteuid() != 0)
-    return 0;
+  if (geteuid () != 0)
+    {
+      fprintf (stderr, "Skipping test: insufficient permissions.\n");
+      return 77;
+    }
 
   /* we want to ensure we can do a get/set/get check to ensure the
      change is accepted. record the current name so it can be restored
      later */
-  ASSERT(gethostname (origname, sizeof (origname)) == 0);
+  ASSERT (gethostname (origname, sizeof (origname)) == 0);
 
   /* try setting a valid hostname.  if it fails -1/ENOSYS, we will
      skip the test for long names as this is an indication we're using
@@ -71,10 +74,14 @@
   if (rcs != 0)
     {
       if (rcs == -1 && errno == ENOSYS)
-       return 0;
+        {
+          fprintf (stderr,
+                   "Skipping test: sethostname is not really implemented.\n");
+          return 77;
+        }
       else
        {
-         printf ("error setting valid hostname.\n");
+         fprintf (stderr, "error setting valid hostname.\n");
          return 1;
        }
     }
@@ -87,7 +94,7 @@
         properly changed. */
       if (strcmp (newname, TESTHOSTNAME) != 0)
        {
-         printf ("set/get comparison failed.\n");
+         fprintf (stderr, "set/get comparison failed.\n");
          return 1;
        }
     }
@@ -100,18 +107,18 @@
 
   longname[i] = '\0';
 
-  rcs = sethostname ((const char *) longname, (HOST_NAME_MAX + 1));
+  rcs = sethostname (longname, (HOST_NAME_MAX + 1));
 
   if (rcs != -1)
     {
       /* attempt to restore the original name. */
-      sethostname (origname, strlen (origname));
-      printf ("expected failure when setting very long hostname.\n");
+      ASSERT (sethostname (origname, strlen (origname)) == 0);
+      fprintf (stderr, "setting a too long hostname succeeded.\n");
       return 1;
     }
 
   /* restore the original name. */
-  sethostname (origname, strlen (origname));
+  ASSERT (sethostname (origname, strlen (origname)) == 0);
 
   return 0;
 }
-- 
In memoriam Rudolf Slánský <http://en.wikipedia.org/wiki/Rudolf_Slánský>