NSS test harness upgrade

  Update to https://www.sourceware.org/ml/libc-alpha/2017-05/msg00074.html
and https://www.sourceware.org/ml/libc-alpha/2017-05/msg00075.html

I replaced the core copy_*() functions with the new alloc_buffer.h code.

This also has a test for BZ#21654
  

On 07/13/2017 08:39 PM, DJ Delorie wrote:
> 
> Update to https://www.sourceware.org/ml/libc-alpha/2017-05/msg00074.html
> and https://www.sourceware.org/ml/libc-alpha/2017-05/msg00075.html
> 
> I replaced the core copy_*() functions with the new alloc_buffer.h code.
> 
> This also has a test for BZ#21654

Looking forward to v2.

(1) Architecture.

You have exactly the right design down. You have abstracted the common
code, and provided a way to write tests easily for NSS which compare
results to expected tables with N number of modules.

(2) Design.

Design looks clean, easy to understand compare functions with excellent
comments.

(3) Implementation.

The only nits I have are that #if 0 / #endif should not be left in tests
and we should have if (test_verbose) and thus always compile the verbose
code to avoid it bit rotting. Similarly there are just some other nits.
See below.

Thank you very much for adding tests. You set a great example to follow!

Comments inline.
 
> diff --git a/nss/Makefile b/nss/Makefile
> index 430be87..d9f6d41 100644
> --- a/nss/Makefile
> +++ b/nss/Makefile
> @@ -50,8 +50,12 @@ extra-objs		+= $(makedb-modules:=.o)
>  
>  tests-static            = tst-field
>  tests-internal		= tst-field
> -tests			= test-netdb tst-nss-test1 test-digits-dots \
> -			  tst-nss-getpwent bug17079
> +tests			= test-netdb test-digits-dots tst-nss-getpwent bug17079 \
> +			  tst-nss-test1 \
> +			  tst-nss-test2 \
> +			  tst-nss-test3 \
> +			  tst-nss-test4 \
> +			  tst-nss-test5

OK.

>  xtests			= bug-erange
>  
>  # If we have a thread library then we can test cancellation against
> @@ -94,7 +98,7 @@ routines                += $(libnss_files-routines)
>  static-only-routines    += $(libnss_files-routines)
>  tests-static		+= tst-nss-static
>  endif
> -extra-test-objs		+= nss_test1.os
> +extra-test-objs		+= nss_test1.os nss_test2.os

OK.

>  
>  include ../Rules
>  
> @@ -123,14 +127,29 @@ $(objpfx)makedb: $(makedb-modules:%=$(objpfx)%.o)
>  $(inst_vardbdir)/Makefile: db-Makefile $(+force)
>  	$(do-install)
>  
> +libnss_test1.so-no-z-defs = 1
> +libnss_test2.so-no-z-defs = 1
> +
> +rtld-tests-LDFLAGS += -Wl,--dynamic-list=nss_test.ver
> +
>  libof-nss_test1 = extramodules
> +libof-nss_test2 = extramodules
>  $(objpfx)/libnss_test1.so: $(objpfx)nss_test1.os $(link-libc-deps)
>  	$(build-module)
> +$(objpfx)/libnss_test2.so: $(objpfx)nss_test2.os $(link-libc-deps)
> +	$(build-module)
> +$(objpfx)nss_test2.os : nss_test1.c
>  ifdef libnss_test1.so-version
>  $(objpfx)/libnss_test1.so$(libnss_test1.so-version): $(objpfx)/libnss_test1.so
>  	$(make-link)
>  endif
> -$(objpfx)tst-nss-test1.out: $(objpfx)/libnss_test1.so$(libnss_test1.so-version)
> +ifdef libnss_test2.so-version
> +$(objpfx)/libnss_test2.so$(libnss_test2.so-version): $(objpfx)/libnss_test2.so
> +	$(make-link)
> +endif
> +$(patsubst %,$(objpfx)%.out,$(tests)) : \
> +	$(objpfx)/libnss_test1.so$(libnss_test1.so-version) \
> +	$(objpfx)/libnss_test2.so$(libnss_test2.so-version)

OK.

>  
>  ifeq (yes,$(have-thread-library))
>  $(objpfx)tst-cancel-getpwuid_r: $(shared-thread-library)
> diff --git a/nss/nss_test.h b/nss/nss_test.h
> new file mode 100644
> index 0000000..6c2e3d4
> --- /dev/null
> +++ b/nss/nss_test.h
> @@ -0,0 +1,302 @@
> +/* Common stuff for NSS test cases.

s/stuff/code/g

> +   Copyright (C) 2017 Free Software Foundation, Inc.
> +   This file is part of the GNU C Library.
> +
> +   The GNU C Library is free software; you can redistribute it and/or
> +   modify it under the terms of the GNU Lesser General Public
> +   License as published by the Free Software Foundation; either
> +   version 2.1 of the License, or (at your option) any later version.
> +
> +   The GNU C Library is distributed in the hope that it will be useful,
> +   but WITHOUT ANY WARRANTY; without even the implied warranty of
> +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> +   Lesser General Public License for more details.
> +
> +   You should have received a copy of the GNU Lesser General Public
> +   License along with the GNU C Library; if not, see
> +   <http://www.gnu.org/licenses/>.  */
> +
> +> +/* There are two (or more) NSS test modules named nss_test1,
> +   nss_test2, etc.  Each one will call a function IN THE TEST CASE
> +   called _nss_test1_init_hook(test_tables *) (or _nss_test2_*, etc).
> +
> +   In your copy of the hook function, you may change the *_table
> +   pointers in the passed struct to point to static tables in your
> +   test case, and the test modules will use that table instead.
> +
> +   Your tables MUST end with an entry that has a *_LAST() macro.
> +   Use the *_ISLAST() macro to test for end of list.
> +
> +   Use __nss_configure_lookup("passwd", "test1 test2") (for example) to
> +   configure NSS to use the test modules.

OK. Nice leading comment.

> +*/

GNU-style required.

Please move '*/' up a line and place two spaces after last period.

> +
> +#include <pwd.h>
> +#include <grp.h>
> +
> +typedef struct test_tables {
> +  struct passwd *pwd_table;
> +  struct group *grp_table;
> +} test_tables;
> +
> +extern void _nss_test1_init_hook (test_tables *) __attribute__((weak));
> +extern void _nss_test2_init_hook (test_tables *) __attribute__((weak));
> +
> +#define PWD_LAST()    { .pw_name = NULL, .pw_uid = 0 }
> +#define GRP_LAST()    { .gr_name = NULL, .gr_gid = 0 }
> +
> +#define PWD_ISLAST(p)    ((p)->pw_name == NULL && (p)->pw_uid == 0)
> +#define GRP_ISLAST(g)    ((g)->gr_name == NULL && (g)->gr_gid == 0)
> +
> +/* Macros to fill in the tables easily.  */
> +
> +#define PWD(u) \
> +    { .pw_name = (char *) "name" #u, .pw_passwd = (char *) "*", .pw_uid = u,  \
> +      .pw_gid = 100, .pw_gecos = (char *) "*", .pw_dir = (char *) "*",	      \

Magic constant. Why use pwd_gid 100? Please add a comment saying that it is an
arbitrary choice or that it has a specific purpose.

> +      .pw_shell = (char *) "*" }
> +
> +#define PWD_N(u,n)								\
> +    { .pw_name = (char *) n, .pw_passwd = (char *) "*", .pw_uid = u,  \
> +      .pw_gid = 100, .pw_gecos = (char *) "*", .pw_dir = (char *) "*",	      \

Likewise.

> +      .pw_shell = (char *) "*" }
> +
> +#define GRP(u) \
> +    { .gr_name = (char *) "name" #u, .gr_passwd = (char *) "*", .gr_gid = u, \
> +      .gr_mem = (char **) group_##u }
> +
> +#define GRP_N(u,n,m)						     \
> +    { .gr_name = (char *) n, .gr_passwd = (char *) "*", .gr_gid = u, \
> +      .gr_mem = (char **) m }
> +
> +/*------------------------------------------------------------*/
> +
> +/* Helper functions for testing passwd entries.  Call
> +   compare_passwds() passing a test index, the passwd entry you got,
> +   and the expected passwd entry.  The function will return the number
> +   of mismatches, or zero of the two records are the same.  */
> +
> +static void __attribute__((used))
> +print_passwd (struct passwd *p)
> +{
> +  printf ("    passwd %u.%s (%s) :", p->pw_uid, p->pw_name, p->pw_passwd);
> +  printf (" %u, %s, %s, %s\n", p->pw_gid, p->pw_gecos, p->pw_dir, p->pw_shell);
> +  printf ("\n");
> +}

OK.

> +
> +static int  __attribute__((used))
> +compare_passwd_field (int i, struct passwd *p, const char *got, const char *exp, const char *name)

Line too long. Reformat please.

e.g.
compare_passwd_field (int i, struct passwd *p, 
		      const char *got, const char *exp,
		      const char *name)

> +{
> +  /* Does the entry have a value?  */
> +  if (got == NULL)
> +    {
> +      printf ("[%d] passwd %s for %u.%s was (null)\n",
> +	      i, name,
> +	      p->pw_uid, p->pw_name);
> +      return 1;
> +    }
> +  /* Does the entry have an unexpected name?  */
> +  else if (exp == NULL)
> +    {
> +      printf ("[%d] passwd %s for %u.(null) was %s\n",
> +	      i, name,
> +	      p->pw_uid, got);
> +      return 1;
> +    }
> +  /* And is it correct?  */
> +  else if (got && strcmp (got, exp) != 0)
> +    {
> +      printf("[%d] passwd entry %u.%s had %s \"%s\" (expected \"%s\") \n",
> +	     i,
> +	     p->pw_uid, p->pw_name, name,
> +	     got, exp);
> +      return 1;
> +    }
> +  return 0;

OK. Gold star for the function comments.

> +}
> +
> +#define COMPARE_PWD_FIELD(f) retval += compare_passwd_field (i, e, p->f, e->f, #f)
> +
> +/* Compare passwd to expected passwd, return number of "problems".
> +   "I" is the index into the testcase data.  */
> +static int  __attribute__((used))
> +compare_passwds (int i, struct passwd *p, struct passwd *e)
> +{
> +  int retval = 0;
> +
> +  /* Did we get the expected uid?  */
> +  if (p->pw_uid != e->pw_uid)
> +    {
> +      printf("[%d] passwd entry %u.%s had uid %u\n", i,
> +	     e->pw_uid, e->pw_name,
> +	     p->pw_uid);
> +      ++ retval;

Not normal coding style.
Use ++retval; please.

> +    }
> +
> +  /* Did we get the expected gid?  */
> +  if (p->pw_gid != e->pw_gid)
> +    {
> +      printf("[%d] passwd entry %u.%s had gid %u (expected %u)\n", i,
> +	     e->pw_uid, e->pw_name,
> +	     p->pw_gid, e->pw_gid);
> +      ++ retval;

Likewise.

> +    }
> +
> +  COMPARE_PWD_FIELD (pw_name);
> +  COMPARE_PWD_FIELD (pw_passwd);
> +  COMPARE_PWD_FIELD (pw_gecos);
> +  COMPARE_PWD_FIELD (pw_dir);
> +  COMPARE_PWD_FIELD (pw_shell);
> +
> +  if (retval)

Avoid boolean coercion.

Please use 'if (retval > 0)'

> +    {
> +      /* Left in for debugging later, if needed.  */
> +      print_passwd (p);
> +      print_passwd (e);
> +    }
> +
> +  return retval;
> +}

OK.

> +
> +/*------------------------------------------------------------*/
> +
> +/* Likewise, helpers for checking group entries.  */

Suggest just 'Helpers for checking group entries.'

The word 'Likewise' is only helpful if you are reading the entire set
of disjoint comments as textual whole, and most people don't, so it's
cleaner to just call this section what it is, or make a back reference
e.g. "Helpers for checking group entries (similar to password checking
      functions like compare_passwds)."

> +
> +static void __attribute__((used))
> +print_group (struct group *g)
> +{
> +  int j;
> +
> +  printf ("    group %u.%s (%s) :", g->gr_gid, g->gr_name, g->gr_passwd);
> +  if (g->gr_mem)
> +    for (j=0; g->gr_mem[j]; j++)
> +      printf ("%s%s", j==0 ? " " : ", ", g->gr_mem[j]);
> +  printf ("\n");

OK.

> +}
> +
> +/* Compare group to expected group, return number of "problems".  "I"
> +   is the index into the testcase data.  */
> +static int  __attribute__((used))
> +compare_groups (int i, struct group *g, struct group *e)
> +{
> +  int j;
> +  int retval = 0;
> +
> +  /* Did we get the expected gid?  */
> +  if (g->gr_gid != e->gr_gid)
> +    {
> +      printf("[%d] group entry %u.%s had gid %u\n", i,
> +	     e->gr_gid, e->gr_name,
> +	     g->gr_gid);
> +      ++ retval;

Not normal coding style.
Use ++retval; please.

> +    }
> +
> +  /* Does the entry have a name?  */
> +  if (g->gr_name == NULL)
> +    {
> +      printf ("[%d] group name for %u.%s was (null)\n", i,
> +	      e->gr_gid, e->gr_name);
> +      ++ retval;

Likewise.

> +    }
> +  /* Does the entry have an unexpected name?  */
> +  else if (e->gr_name == NULL)
> +    {
> +      printf ("[%d] group name for %u.(null) was %s\n", i,
> +	      e->gr_gid, g->gr_name);
> +      ++ retval;

Likewise.

> +    }
> +  /* And is it correct?  */
> +  else if (strcmp (g->gr_name, e->gr_name) != 0)
> +    {
> +      printf("[%d] group entry %u.%s had name \"%s\"\n", i,
> +	     e->gr_gid, e->gr_name,
> +	     g->gr_name);
> +      ++ retval;

Likewise.

> +    }> +
> +  /* Does the entry have a password?  */
> +  if (g->gr_passwd == NULL && e->gr_passwd != NULL)
> +    {
> +      printf ("[%d] group password for %u.%s was NULL\n", i,
> +	      e->gr_gid, e->gr_name);
> +      ++ retval;

Likewise.

> +    }
> +  else if (g->gr_passwd != NULL && e->gr_passwd == NULL)
> +    {
> +      printf ("[%d] group password for %u.%s was not NULL\n", i,
> +	      e->gr_gid, e->gr_name);
> +      ++ retval;

Likewise.

> +    }
> +  /* And is it correct?  */
> +  else if (g->gr_passwd && strcmp (g->gr_passwd, e->gr_passwd) != 0)
> +    {
> +      printf("[%d] group entry %u.%s had password \"%s\" (not \"%s\")\n", i,
> +	     e->gr_gid, e->gr_name,
> +	     g->gr_passwd, e->gr_passwd);
> +      ++ retval;

Likewise.

> +    }
> +
> +  /* Now compare group members... */
> +
> +  if (e->gr_mem != NULL && g->gr_mem == NULL)
> +    {
> +      printf("[%d] group entry %u.%s missing member list\n", i,
> +	     e->gr_gid, e->gr_name);
> +      ++ retval;

Likewise.

> +    }
> +  else if (e->gr_mem == NULL && g->gr_mem != NULL)
> +    {
> +      printf("[%d] group entry %u.%s has unexpected member list\n", i,
> +	     e->gr_gid, e->gr_name);
> +      ++ retval;

Likewise.

> +    }
> +  else if (e->gr_mem == NULL && g->gr_mem == NULL)
> +    {
> +      /* This case is OK.  */

OK.

> +    }
> +  else
> +    {
> +      /* Compare two existing lists.  */
> +      j = 0;
> +      for (;;)
> +	{
> +	  if (g->gr_mem[j] == NULL && e->gr_mem[j] == NULL)
> +	    {
> +	      /* Matching end-of-lists.  */
> +	      break;

OK.

> +	    }
> +	  if (g->gr_mem[j] == NULL)
> +	    {
> +	      printf ("[%d] group member list for %u.%s is too short.\n", i,
> +		      e->gr_gid, e->gr_name);
> +	      ++ retval;

++retval;

> +	      break;
> +	    }
> +	  if (e->gr_mem[j] == NULL)
> +	    {
> +	      printf ("[%d] group member list for %u.%s is too long.\n", i,
> +		      e->gr_gid, e->gr_name);
> +	      ++ retval;

++retval;

> +	      break;
> +	    }
> +	  if (strcmp (g->gr_mem[j], e->gr_mem[j]) != 0)

OK.

> +	    {
> +	      printf ("[%d] group member list for %u.%s differs: %s vs %s.\n", i,
> +		      e->gr_gid, e->gr_name,
> +		      e->gr_mem[j], g->gr_mem[j]);
> +	      ++ retval;

++retval;

> +	    }
> +
> +	  j ++;

j++;

> +	}
> +    }
> +
> +  if (retval)

if (retval > 0)

> +    {
> +      /* Left in for debugging later, if needed.  */
> +      print_group (g);
> +      print_group (e);
> +    }
> +
> +  return retval;
> +}
> diff --git a/nss/nss_test.ver b/nss/nss_test.ver
> new file mode 100644
> index 0000000..2e21176
> --- /dev/null
> +++ b/nss/nss_test.ver
> @@ -0,0 +1,4 @@
> +{
> +  _nss_test1_init_hook;
> +  _nss_test2_init_hook;
> +};

OK.

> diff --git a/nss/nss_test1.c b/nss/nss_test1.c
> index 3beb488..6215400 100644
> --- a/nss/nss_test1.c
> +++ b/nss/nss_test1.c
> @@ -1,84 +1,180 @@
> +/* Template generic NSS service provider.  See nss_test.h for usage.
> +   Copyright (C) 2017 Free Software Foundation, Inc.
> +   This file is part of the GNU C Library.
> +
> +   The GNU C Library is free software; you can redistribute it and/or
> +   modify it under the terms of the GNU Lesser General Public
> +   License as published by the Free Software Foundation; either
> +   version 2.1 of the License, or (at your option) any later version.
> +
> +   The GNU C Library is distributed in the hope that it will be useful,
> +   but WITHOUT ANY WARRANTY; without even the implied warranty of
> +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> +   Lesser General Public License for more details.
> +
> +   You should have received a copy of the GNU Lesser General Public
> +   License along with the GNU C Library; if not, see
> +   <http://www.gnu.org/licenses/>.  */
> +
>  #include <errno.h>
>  #include <nss.h>
>  #include <pthread.h>
>  #include <string.h>
> +#include <stdio.h>
> +#include <alloc_buffer.h>
>  
>  
> -#define COPY_IF_ROOM(s) \
> -  ({ size_t len_ = strlen (s) + 1;		\
> -     char *start_ = cp;				\
> -     buflen - (cp - buffer) < len_		\
> -     ? NULL					\
> -     : (cp = mempcpy (cp, s, len_), start_); })
> +/* We need to be able to handle NULLs "properly" (for a testsuite).  */

Suggest:
We need to be able to handle NULLs properly for the testsuite.

> +#define alloc_buffer_maybe_copy_string(b,s) s ? alloc_buffer_copy_string (b, s) : NULL;
>  
> +/* This file is the master template.  Other instances of this test
> +   module should define NAME(x) to have their name instead of "test1",
> +   then include this file.
> +*/
> +#define NAME_(x,n) _nss_##n##_##x
> +#ifndef NAME
> +#define NAME(x) NAME_(x,test1)
> +#endif
> +#define NAMESTR__(x) #x
> +#define NAMESTR_(x) NAMESTR__(x)
> +#define NAMESTR(x) NAMESTR_(NAME(x))
>  
> -/* Password handling.  */
> -#include <pwd.h>
> +#include "nss_test.h"
> +
> +/* -------------------------------------------------- */
> +/* Default Data.  */
>  
> -static struct passwd pwd_data[] =
> +static struct passwd default_pwd_data[] =

OK.

>    {
>  #define PWD(u) \
>      { .pw_name = (char *) "name" #u, .pw_passwd = (char *) "*", .pw_uid = u,  \
>        .pw_gid = 100, .pw_gecos = (char *) "*", .pw_dir = (char *) "*",	      \
>        .pw_shell = (char *) "*" }
> -    PWD (100),
>      PWD (30),
> +    PWD (100),

OK.

>      PWD (200),
>      PWD (60),
>      PWD (20000)
>    };
> -#define npwd_data (sizeof (pwd_data) / sizeof (pwd_data[0]))
> +#define default_npwd_data (sizeof (pwd_data) / sizeof (pwd_data[0]))
> +
> +static struct passwd *pwd_data = default_pwd_data;
> +static int npwd_data = default_npwd_data;
> +
> +static struct group *grp_data = NULL;
> +static int ngrp_data = 0;
> +
> +/* This function will get called, and once per session, look back into
> +   the test case's executable for an init hook function, and call
> +   it.  */
> +
> +static int initted = 0;
> +static void
> +init(void)
> +{
> +  test_tables t;
> +  int i;
> +
> +  if (initted)
> +    return;
> +  if (NAME(init_hook))
> +    {
> +      memset (&t, 0, sizeof(t));
> +      NAME(init_hook)(&t);
> +
> +      if (t.pwd_table)
> +	{
> +	  pwd_data = t.pwd_table;
> +	  for (i=0; ! PWD_ISLAST(& pwd_data[i]); i++)
> +	    ;
> +	  npwd_data = i;
> +	}
> +
> +      if (t.grp_table)
> +	{
> +	  grp_data = t.grp_table;
> +	  for (i=0; ! GRP_ISLAST(& grp_data[i]); i++)
> +	    {
> +#if 0
> +	      /* Left in for debugging.  */
> +	      int j;
> +	      fprintf(stderr, "\033[34m%s %s %d : ", NAMESTR(init),
> +		      grp_data[i].gr_name, grp_data[i].gr_gid);
> +	      if (grp_data[i].gr_mem)
> +		for (j=0; grp_data[i].gr_mem[j]; j++)
> +		  fprintf (stderr, "%s%s", j ? ", " : "", grp_data[i].gr_mem[j]);
> +	      fprintf(stderr, "\033[0m\n");
> +#endif

This will lead to bit-rot of the debugging code, the code should be
enabled, but jumped over when not debugging.

I don't know if you can access it, but the main executable test should
have a 'test_verbose' global which is part of the support test
infrastructure and set to > 0 when the test is run with `-v` (for each
-v you get +1 more value).

So IMO all code like this should always be compiled, but conditional on:

if (test_verbose)
  {
    <Do the verbose thing>
  }

> +	    }
> +	  ngrp_data = i;
> +	}
> +    }
> +  initted = 1;
> +}
> +
> +/* -------------------------------------------------- */
> +/* Password handling.  */
>  
>  static size_t pwd_iter;
>  #define CURPWD pwd_data[pwd_iter]
>  
>  static pthread_mutex_t pwd_lock = PTHREAD_MUTEX_INITIALIZER;
>  
> -
>  enum nss_status
> -_nss_test1_setpwent (int stayopen)
> +NAME(setpwent) (int stayopen)
>  {
> +  init();
>    pwd_iter = 0;
>    return NSS_STATUS_SUCCESS;
>  }
>  
>  
>  enum nss_status
> -_nss_test1_endpwent (void)
> +NAME(endpwent) (void)
>  {
> +  init();
>    return NSS_STATUS_SUCCESS;
>  }
>  
> +#define MISALLOC(x) (result->x == NULL && local->x != NULL)

Needs a comment.

> +
> +static enum nss_status
> +copy_passwd (struct passwd *result, struct passwd *local,
> +	    char *buffer, size_t buflen, int *errnop)
> +{
> +  struct alloc_buffer buf = alloc_buffer_create (buffer, buflen);
> +
> +  result->pw_name = alloc_buffer_maybe_copy_string (&buf, local->pw_name);
> +  result->pw_passwd = alloc_buffer_maybe_copy_string (&buf, local->pw_passwd);
> +  result->pw_uid = local->pw_uid;
> +  result->pw_gid = local->pw_gid;
> +  result->pw_gecos = alloc_buffer_maybe_copy_string (&buf, local->pw_gecos);
> +  result->pw_dir = alloc_buffer_maybe_copy_string (&buf, local->pw_dir);
> +  result->pw_shell = alloc_buffer_maybe_copy_string (&buf, local->pw_shell);
> +  
> +  if (alloc_buffer_has_failed (&buf))
> +    {
> +      *errnop = ERANGE;
> +      return NSS_STATUS_TRYAGAIN;
> +    }
> +
> +  return NSS_STATUS_SUCCESS;
> +}

OK.

>  
>  enum nss_status
> -_nss_test1_getpwent_r (struct passwd *result, char *buffer, size_t buflen,
> +NAME(getpwent_r) (struct passwd *result, char *buffer, size_t buflen,
>  		       int *errnop)
>  {
> -  char *cp = buffer;
>    int res = NSS_STATUS_SUCCESS;
>  
> +  init();
>    pthread_mutex_lock (&pwd_lock);
>  
>    if (pwd_iter >= npwd_data)
>      res = NSS_STATUS_NOTFOUND;
>    else
>      {
> -      result->pw_name = COPY_IF_ROOM (CURPWD.pw_name);
> -      result->pw_passwd = COPY_IF_ROOM (CURPWD.pw_passwd);
> -      result->pw_uid = CURPWD.pw_uid;
> -      result->pw_gid = CURPWD.pw_gid;
> -      result->pw_gecos = COPY_IF_ROOM (CURPWD.pw_gecos);
> -      result->pw_dir = COPY_IF_ROOM (CURPWD.pw_dir);
> -      result->pw_shell = COPY_IF_ROOM (CURPWD.pw_shell);
> -
> -      if (result->pw_name == NULL || result->pw_passwd == NULL
> -	  || result->pw_gecos == NULL || result->pw_dir == NULL
> -	  || result->pw_shell == NULL)
> -	{
> -	  *errnop = ERANGE;
> -	  res = NSS_STATUS_TRYAGAIN;
> -	}
> -
> +      res = copy_passwd (result, &CURPWD, buffer, buflen, errnop);

OK.

>        ++pwd_iter;
>      }
>  
> @@ -89,65 +185,140 @@ _nss_test1_getpwent_r (struct passwd *result, char *buffer, size_t buflen,
>  
>  
>  enum nss_status
> -_nss_test1_getpwuid_r (uid_t uid, struct passwd *result, char *buffer,
> +NAME(getpwuid_r) (uid_t uid, struct passwd *result, char *buffer,
>  		       size_t buflen, int *errnop)
>  {
> +  init();
>    for (size_t idx = 0; idx < npwd_data; ++idx)
>      if (pwd_data[idx].pw_uid == uid)
> -      {
> -	char *cp = buffer;
> -	int res = NSS_STATUS_SUCCESS;
> -
> -	result->pw_name = COPY_IF_ROOM (pwd_data[idx].pw_name);
> -	result->pw_passwd = COPY_IF_ROOM (pwd_data[idx].pw_passwd);
> -	result->pw_uid = pwd_data[idx].pw_uid;
> -	result->pw_gid = pwd_data[idx].pw_gid;
> -	result->pw_gecos = COPY_IF_ROOM (pwd_data[idx].pw_gecos);
> -	result->pw_dir = COPY_IF_ROOM (pwd_data[idx].pw_dir);
> -	result->pw_shell = COPY_IF_ROOM (pwd_data[idx].pw_shell);
> -
> -	if (result->pw_name == NULL || result->pw_passwd == NULL
> -	    || result->pw_gecos == NULL || result->pw_dir == NULL
> -	    || result->pw_shell == NULL)
> -	  {
> -	    *errnop = ERANGE;
> -	    res = NSS_STATUS_TRYAGAIN;
> -	  }
> -
> -	return res;
> -      }
> +      return copy_passwd (result, &pwd_data[idx], buffer, buflen, errnop);

OK.

>  
>    return NSS_STATUS_NOTFOUND;
>  }
>  
>  
>  enum nss_status
> -_nss_test1_getpwnam_r (const char *name, struct passwd *result, char *buffer,
> +NAME(getpwnam_r) (const char *name, struct passwd *result, char *buffer,
>  		       size_t buflen, int *errnop)
>  {
> +  init();
>    for (size_t idx = 0; idx < npwd_data; ++idx)
>      if (strcmp (pwd_data[idx].pw_name, name) == 0)
> +      return copy_passwd (result, &pwd_data[idx], buffer, buflen, errnop);
> +
> +  return NSS_STATUS_NOTFOUND;

OK.

> +}
> +
> +/* -------------------------------------------------- */
> +/* Group handling.  */
> +
> +static size_t grp_iter;
> +#define CURGRP grp_data[grp_iter]
> +
> +static pthread_mutex_t grp_lock = PTHREAD_MUTEX_INITIALIZER;
> +
> +enum nss_status
> +NAME(setgrent) (int stayopen)
> +{
> +  init();
> +  grp_iter = 0;
> +  return NSS_STATUS_SUCCESS;
> +}
> +
> +
> +enum nss_status
> +NAME(endgrent) (void)
> +{
> +  init();
> +  return NSS_STATUS_SUCCESS;
> +}
> +
> +static enum nss_status
> +copy_group (struct group *result, struct group *local,
> +	    char *buffer, size_t buflen, int *errnop)
> +{
> +  struct alloc_buffer buf = alloc_buffer_create (buffer, buflen);
> +  char **memlist;
> +  int i;
> +
> +  if (local->gr_mem)
> +    {
> +      i = 0;
> +      while (local->gr_mem[i])
> +	++ i;

++i;

> +
> +      memlist = alloc_buffer_alloc_array (&buf, char *, i + 1);
> +
> +      if (memlist) {
> +	for (i = 0; local->gr_mem[i]; ++ i)
> +	  memlist[i] = alloc_buffer_maybe_copy_string (&buf, local->gr_mem[i]);
> +	memlist[i] = NULL;
> +      }
> +
> +      result->gr_mem = memlist;
> +    }
> +  else
> +    result->gr_mem = NULL;
> +
> +  result->gr_name = alloc_buffer_maybe_copy_string (&buf, local->gr_name);
> +  result->gr_passwd = alloc_buffer_maybe_copy_string (&buf, local->gr_passwd);
> +  result->gr_gid = local->gr_gid;
> +
> +  if (alloc_buffer_has_failed (&buf))
> +    {
> +      *errnop = ERANGE;
> +      return NSS_STATUS_TRYAGAIN;

OK.

> +    }
> +
> +  return NSS_STATUS_SUCCESS;
> +}
> +
> +
> +enum nss_status
> +NAME(getgrent_r) (struct group *result, char *buffer, size_t buflen,
> +		       int *errnop)
> +{
> +  int res = NSS_STATUS_SUCCESS;
> +
> +  init();
> +  pthread_mutex_lock (&grp_lock);
> +
> +  if (grp_iter >= ngrp_data)
> +    res = NSS_STATUS_NOTFOUND;
> +  else
> +    {
> +      res = copy_group (result, &CURGRP, buffer, buflen, errnop);
> +      ++grp_iter;
> +    }
> +
> +  pthread_mutex_unlock (&pwd_lock);
> +
> +  return res;
> +}

Ok.

> +
> +
> +enum nss_status
> +NAME(getgrgid_r) (gid_t gid, struct group *result, char *buffer,
> +		  size_t buflen, int *errnop)
> +{
> +  init();
> +  for (size_t idx = 0; idx < ngrp_data; ++idx)
> +    if (grp_data[idx].gr_gid == gid)
> +      return copy_group (result, &grp_data[idx], buffer, buflen, errnop);
> +
> +  return NSS_STATUS_NOTFOUND;
> +}

OK.

> +
> +
> +enum nss_status
> +NAME(getgrnam_r) (const char *name, struct group *result, char *buffer,
> +		       size_t buflen, int *errnop)
> +{
> +  init();
> +  for (size_t idx = 0; idx < ngrp_data; ++idx)
> +    if (strcmp (pwd_data[idx].pw_name, name) == 0)
>        {
> -	char *cp = buffer;
> -	int res = NSS_STATUS_SUCCESS;
> -
> -	result->pw_name = COPY_IF_ROOM (pwd_data[idx].pw_name);
> -	result->pw_passwd = COPY_IF_ROOM (pwd_data[idx].pw_passwd);
> -	result->pw_uid = pwd_data[idx].pw_uid;
> -	result->pw_gid = pwd_data[idx].pw_gid;
> -	result->pw_gecos = COPY_IF_ROOM (pwd_data[idx].pw_gecos);
> -	result->pw_dir = COPY_IF_ROOM (pwd_data[idx].pw_dir);
> -	result->pw_shell = COPY_IF_ROOM (pwd_data[idx].pw_shell);
> -
> -	if (result->pw_name == NULL || result->pw_passwd == NULL
> -	    || result->pw_gecos == NULL || result->pw_dir == NULL
> -	    || result->pw_shell == NULL)
> -	  {
> -	    *errnop = ERANGE;
> -	    res = NSS_STATUS_TRYAGAIN;
> -	  }
> -
> -	return res;
> +	return copy_group (result, &grp_data[idx], buffer, buflen, errnop);

OK.

>        }
>  
>    return NSS_STATUS_NOTFOUND;
> diff --git a/nss/nss_test2.c b/nss/nss_test2.c
> new file mode 100644
> index 0000000..0169344
> --- /dev/null
> +++ b/nss/nss_test2.c
> @@ -0,0 +1,20 @@
> +/* Instance of a generic NSS service provider.  See nss_test.h for usage.
> +   Copyright (C) 2017 Free Software Foundation, Inc.
> +   This file is part of the GNU C Library.
> +
> +   The GNU C Library is free software; you can redistribute it and/or
> +   modify it under the terms of the GNU Lesser General Public
> +   License as published by the Free Software Foundation; either
> +   version 2.1 of the License, or (at your option) any later version.
> +
> +   The GNU C Library is distributed in the hope that it will be useful,
> +   but WITHOUT ANY WARRANTY; without even the implied warranty of
> +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> +   Lesser General Public License for more details.
> +
> +   You should have received a copy of the GNU Lesser General Public
> +   License along with the GNU C Library; if not, see
> +   <http://www.gnu.org/licenses/>.  */
> +
> +#define NAME(x) NAME_(x,test2)
> +#include "nss_test1.c"

OK.

> diff --git a/nss/tst-nss-test1.c b/nss/tst-nss-test1.c
> index c5750e0..f7e2475 100644
> --- a/nss/tst-nss-test1.c
> +++ b/nss/tst-nss-test1.c
> @@ -1,9 +1,46 @@
> +/* Basic test of passwd database handling.
> +   Copyright (C) 2017 Free Software Foundation, Inc.
> +   This file is part of the GNU C Library.
> +
> +   The GNU C Library is free software; you can redistribute it and/or
> +   modify it under the terms of the GNU Lesser General Public
> +   License as published by the Free Software Foundation; either
> +   version 2.1 of the License, or (at your option) any later version.
> +
> +   The GNU C Library is distributed in the hope that it will be useful,
> +   but WITHOUT ANY WARRANTY; without even the implied warranty of
> +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> +   Lesser General Public License for more details.
> +
> +   You should have received a copy of the GNU Lesser General Public
> +   License along with the GNU C Library; if not, see
> +   <http://www.gnu.org/licenses/>.  */
> +
>  #include <nss.h>
>  #include <pwd.h>
>  #include <stdio.h>
>  #include <stdlib.h>
>  #include <string.h>
>  
> +#include "nss_test.h"
> +
> +static int hook_called = 0;
> +
> +static struct passwd pwd_table[] = {
> +    PWD (100),
> +    PWD (30),
> +    PWD (200),
> +    PWD (60),
> +    PWD (20000),
> +    PWD_LAST ()

Are these values chosen randomly? Please comment.

> +  };
> +
> +void
> +_nss_test1_init_hook(test_tables *t)
> +{
> +  hook_called = 1;
> +  t->pwd_table = pwd_table;
> +}
>  
>  static int
>  do_test (void)
> @@ -14,18 +51,23 @@ do_test (void)
>  
>    static const unsigned int pwdids[] = { 100, 30, 200, 60, 20000 };
>  #define npwdids (sizeof (pwdids) / sizeof (pwdids[0]))
> +
>    setpwent ();
>  
>    const unsigned int *np = pwdids;
>    for (struct passwd *p = getpwent (); p != NULL; ++np, p = getpwent ())
> -    if (p->pw_uid != *np || strncmp (p->pw_name, "name", 4) != 0
> -	|| atol (p->pw_name + 4) != *np)
> -      {
> -	printf ("passwd entry %td wrong (%s, %u)\n",
> -		np - pwdids, p->pw_name, p->pw_uid);
> -	retval = 1;
> -	break;
> -      }
> +    {
> +      retval += compare_passwds (np-pwdids, p, & pwd_table[np-pwdids]);
> +
> +      if (p->pw_uid != *np || strncmp (p->pw_name, "name", 4) != 0
> +	  || atol (p->pw_name + 4) != *np)
> +	{
> +	  printf ("passwd entry %td wrong (%s, %u)\n",
> +		  np - pwdids, p->pw_name, p->pw_uid);
> +	  retval = 1;
> +	  break;
> +	}

Ok.

> +    }
>  
>    endpwent ();
>  
> @@ -65,6 +107,12 @@ do_test (void)
>  	}
>      }
>  
> +  if (!hook_called)
> +    {
> +      retval = 1;
> +      printf("init hook never called\n");
> +    }
> +
>    return retval;
>  }
>  
> diff --git a/nss/tst-nss-test2.c b/nss/tst-nss-test2.c
> new file mode 100644
> index 0000000..e63ce48
> --- /dev/null
> +++ b/nss/tst-nss-test2.c
> @@ -0,0 +1,132 @@
> +/* Basic test for two passwd databases.
> +   Copyright (C) 2017 Free Software Foundation, Inc.
> +   This file is part of the GNU C Library.
> +
> +   The GNU C Library is free software; you can redistribute it and/or
> +   modify it under the terms of the GNU Lesser General Public
> +   License as published by the Free Software Foundation; either
> +   version 2.1 of the License, or (at your option) any later version.
> +
> +   The GNU C Library is distributed in the hope that it will be useful,
> +   but WITHOUT ANY WARRANTY; without even the implied warranty of
> +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> +   Lesser General Public License for more details.
> +
> +   You should have received a copy of the GNU Lesser General Public
> +   License along with the GNU C Library; if not, see
> +   <http://www.gnu.org/licenses/>.  */
> +
> +#include <nss.h>
> +#include <pwd.h>
> +#include <stdio.h>
> +#include <stdlib.h>
> +#include <string.h>
> +
> +#include "nss_test.h"
> +
> +static struct passwd pwd_table_1[] = {
> +    PWD (100),
> +    PWD (30),
> +    PWD (200),
> +    PWD (60),
> +    PWD (20000),
> +    PWD_LAST ()
> +  };
> +
> +static struct passwd pwd_table_2[] = {
> +    PWD (5),
> +    PWD_N(200, "name30"),
> +    PWD (16),
> +    PWD_LAST ()
> +  };

Similar question. Comment on the choice of data.

> +
> +void
> +_nss_test1_init_hook(test_tables *t)
> +{
> +  t->pwd_table = pwd_table_1;
> +}
> +
> +void
> +_nss_test2_init_hook(test_tables *t)
> +{
> +  t->pwd_table = pwd_table_2;
> +}
> +
> +static struct passwd pwd_expected[] = {
> +  PWD(100),
> +  PWD(30),
> +  PWD(200),
> +  PWD(60),
> +  PWD(20000),
> +  PWD(5),
> +  PWD_N(200, "name30"),
> +  PWD(16),
> +  PWD_LAST ()

Likewise.

> +};
> +
> +static struct {
> +  uid_t uid;
> +  const char *name;
> +} tests[] = {
> +  { 100, "name100" }, /* control, first db */
> +  {  16, "name16"  }, /* second db */
> +  {  30, "name30"  }, /* test overlaps in name */
> +  { 200, "name200" }, /* test overlaps uid */

OK. Great comments.

> +  { 0, NULL }
> +};
> +
> +static int
> +do_test (void)
> +{
> +  int retval = 0;
> +  int i;
> +
> +  __nss_configure_lookup ("passwd", "test1 test2");
> +
> +  setpwent ();
> +
> +  i = 0;
> +  for (struct passwd *p = getpwent (); p != NULL; ++i, p = getpwent ())
> +    {
> +      retval += compare_passwds (i, & pwd_expected[i], p);
> +
> +      if (p->pw_uid != pwd_expected[i].pw_uid || strcmp (p->pw_name, pwd_expected[i].pw_name) != 0)
> +      {
> +	printf ("getpwent for %u.%s returned %u.%s\n",
> +		pwd_expected[i].pw_uid, pwd_expected[i].pw_name,
> +		p->pw_uid, p->pw_name);

Each of these little checks is like a subtest.

Prefer to see "FAIL:" prefix for failed tests e.g. FAIL/INFO/WARN for various results.

Make it immediately obiouvs that this was wrong.


> +	retval = 1;
> +	break;
> +      }
> +    }
> +
> +  endpwent ();
> +
> +  for (i=0; tests[i].name; i++)
> +    {
> +      struct passwd *p = getpwnam (tests[i].name);
> +      if (strcmp (p->pw_name, tests[i].name) != 0
> +	  || p->pw_uid != tests[i].uid)
> +	{
> +	  printf("getpwnam for %u.%s returned %u.%s\n",
> +		 tests[i].uid, tests[i].name,
> +		 p->pw_uid, p->pw_name);

Likewise.

> +	  retval = 1;
> +	}
> +
> +      p = getpwuid (tests[i].uid);
> +      if (strcmp (p->pw_name, tests[i].name) != 0
> +	  || p->pw_uid != tests[i].uid)
> +	{
> +	  printf("getpwuid for %u.%s returned %u.%s\n",
> +		 tests[i].uid, tests[i].name,
> +		 p->pw_uid, p->pw_name);

Likewise.

> +	  retval = 1;
> +	}
> +    }
> +
> +  return retval;
> +}

OK.

> +
> +#define TEST_FUNCTION do_test ()
> +#include "../test-skeleton.c"
> diff --git a/nss/tst-nss-test3.c b/nss/tst-nss-test3.c
> new file mode 100644
> index 0000000..062af00
> --- /dev/null
> +++ b/nss/tst-nss-test3.c
> @@ -0,0 +1,146 @@
> +/* Test error checking for group entries.
> +   Copyright (C) 2017 Free Software Foundation, Inc.
> +   This file is part of the GNU C Library.
> +
> +   The GNU C Library is free software; you can redistribute it and/or
> +   modify it under the terms of the GNU Lesser General Public
> +   License as published by the Free Software Foundation; either
> +   version 2.1 of the License, or (at your option) any later version.
> +
> +   The GNU C Library is distributed in the hope that it will be useful,
> +   but WITHOUT ANY WARRANTY; without even the implied warranty of
> +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> +   Lesser General Public License for more details.
> +
> +   You should have received a copy of the GNU Lesser General Public
> +   License along with the GNU C Library; if not, see
> +   <http://www.gnu.org/licenses/>.  */
> +
> +#include <nss.h>
> +#include <stdio.h>
> +#include <stdlib.h>
> +#include <string.h>
> +#include <sys/signal.h>
> +
> +#include "nss_test.h"
> +

These look random :-) Please comment.

> +static const char *group_2[] = {
> +  "foo", "bar", NULL
> +};
> +
> +static const char *group_3[] = {
> +  "tom", "dick", "harry", NULL
> +};
> +
> +static const char *group_4[] = {
> +  "alpha", "beta", "gamma", "fred", NULL
> +};
> +
> +static const char *group_6[] = {
> +  "larry", "curly", "moe", NULL
> +};
> +
> +static const char *group_7[] = {
> +  "larry", "curly", "darryl", NULL
> +};
> +
> +static const char *group_14[] = {
> +  "huey", "dewey", "louis", NULL
> +};
> +
> +/* Note that we're intentionally causing mis-matches here; the purpose
> +   of this test case is to test each error check and make sure they
> +   detect the errors they check for, and to ensure that the harness
> +   can process all the error cases properly (i.e. a NULL gr_name
> +   field).  We check for the correct number of mismatches at the
> +   end.  */

OK. Great comment.

> +
> +/* This is the data we're giving the service.  */
> +static struct group group_table_data[] = {
> +  GRP(4), /* match */
> +  GRP_N(8, "name6", group_6), /* wrong gid */
> +  GRP_N(14, NULL, group_14), /* missing name */
> +  GRP(14), /* unexpected name */
> +  GRP_N(7, "name7_wrong", group_7), /* wrong name */
> +  { .gr_name =  (char *)"name5", .gr_passwd =  (char *)"wilma", .gr_gid = 5, .gr_mem = NULL }, /* unexpected passwd */
> +  { .gr_name =  (char *)"name5", .gr_passwd = NULL, .gr_gid = 5, .gr_mem = NULL }, /* missing passwd */
> +  { .gr_name =  (char *)"name5", .gr_passwd = (char *)"wilma", .gr_gid = 5, .gr_mem = NULL }, /* wrong passwd */
> +  GRP_N(3, "name3a", NULL),   /* missing member list */
> +  GRP_N(3, "name3b", group_3), /* unexpected member list */
> +  GRP_N(3, "name3c", group_3), /* wrong/short member list */
> +  GRP_N(3, "name3d", group_4), /* wrong/long member list */
> +  GRP_LAST ()

OK. Awesome inline comments.

> +};
> +
> +/* This is the data we compare against.  */
> +static struct group group_table[] = {
> +  GRP(4),
> +  GRP(6),
> +  GRP(14),
> +  GRP_N(14, NULL, group_14),
> +  GRP(7),
> +  { .gr_name =  (char *)"name5", .gr_passwd = NULL, .gr_gid = 5, .gr_mem = NULL },
> +  { .gr_name =  (char *)"name5", .gr_passwd =  (char *)"fred", .gr_gid = 5, .gr_mem = NULL },
> +  { .gr_name =  (char *)"name5", .gr_passwd =  (char *)"fred", .gr_gid = 5, .gr_mem = NULL },
> +  GRP_N(3, "name3a", group_3),
> +  GRP_N(3, "name3b", NULL),
> +  GRP_N(3, "name3c", group_4),
> +  GRP_N(3, "name3d", group_3),
> +  GRP(2),
> +  GRP_LAST ()
> +};

OK.

> +
> +void
> +_nss_test1_init_hook(test_tables *t)
> +{
> +  t->grp_table = group_table_data;
> +}

OK. Adjust the test data.

> +
> +static int
> +do_test (void)
> +{
> +  int retval = 0;
> +  int i;
> +  struct group *g = NULL;
> +
> +  __nss_configure_lookup ("group", "test1");
> +
> +  setgrent ();
> +
> +  i = 0;
> +  for (g = getgrent () ;
> +       g != NULL && ! GRP_ISLAST(&group_table[i]);
> +       ++i, g = getgrent ())
> +    {
> +      retval += compare_groups (i, g, & group_table[i]);
> +    }
> +
> +  endgrent ();
> +
> +  if (g)
> +    {
> +      printf ("[?] group entry %u.%s unexpected\n", g->gr_gid, g->gr_name);
> +      ++ retval;

++retval;

> +    }
> +  if (group_table[i].gr_name || group_table[i].gr_gid)
> +    {
> +      printf ("[%d] group entry %u.%s missing\n", i,
> +	      group_table[i].gr_gid, group_table[i].gr_name);
> +      ++ retval;

++reval;

> +    }
> +
> +#define EXPECTED 18
> +  if (retval == EXPECTED)
> +    {
> +      printf ("Found %d expected errors\n", retval);

Add PASS: prefix.

> +      return 0;
> +    }
> +  else
> +    {
> +      printf ("Found %d errors, expected %d\n", retval, EXPECTED);

Add FAIL: prefix.

> +      return 1;
> +    }

OK.

> +}
> +
> +#define TEST_FUNCTION do_test ()
> +#include "../test-skeleton.c"
> diff --git a/nss/tst-nss-test4.c b/nss/tst-nss-test4.c
> new file mode 100644
> index 0000000..b63c722
> --- /dev/null
> +++ b/nss/tst-nss-test4.c
> @@ -0,0 +1,136 @@
> +/* Test group merging.

Yay! Testing group merge! :-)

> +   Copyright (C) 2017 Free Software Foundation, Inc.
> +   This file is part of the GNU C Library.
> +
> +   The GNU C Library is free software; you can redistribute it and/or
> +   modify it under the terms of the GNU Lesser General Public
> +   License as published by the Free Software Foundation; either
> +   version 2.1 of the License, or (at your option) any later version.
> +
> +   The GNU C Library is distributed in the hope that it will be useful,
> +   but WITHOUT ANY WARRANTY; without even the implied warranty of
> +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> +   Lesser General Public License for more details.
> +
> +   You should have received a copy of the GNU Lesser General Public
> +   License along with the GNU C Library; if not, see
> +   <http://www.gnu.org/licenses/>.  */
> +
> +#include <nss.h>
> +#include <stdio.h>
> +#include <stdlib.h>
> +#include <string.h>
> +#include <sys/signal.h>
> +
> +#include "nss_test.h"
> +

Again, this data seems random or arbitrarily chosen, if it is
then please add a comment.

> +static const char *group_1[] = {
> +  "foo", "bar", NULL
> +};
> +
> +static const char *group_2[] = {
> +  "foo", "dick", "harry", NULL
> +};
> +
> +/* Note that deduplication is NOT supposed to happen.  */
> +static const char *merge_1[] = {
> +  "foo", "bar", "foo", "dick", "harry", NULL
> +};
> +
> +static const char *group_4[] = {
> +  "fred", "wilma", NULL
> +};
> +
> +/* This is the data we're giving the service.  */
> +static struct group group_table_data1[] = {
> +  GRP_N(1, "name1", group_1),
> +  GRP(2),
> +  GRP_LAST ()
> +};
> +
> +/* This is the data we're giving the service.  */
> +static struct group group_table_data2[] = {
> +  GRP_N(1, "name1", group_2),
> +  GRP(4),
> +  GRP_LAST ()
> +};
> +
> +/* This is the data we compare against.  */
> +static struct group group_table[] = {
> +  GRP_N(1, "name1", merge_1),
> +  GRP(2),
> +  GRP(4),
> +  GRP_LAST ()
> +};
> +
> +void
> +_nss_test1_init_hook(test_tables *t)
> +{
> +  t->grp_table = group_table_data1;
> +}
> +
> +void
> +_nss_test2_init_hook(test_tables *t)
> +{
> +  t->grp_table = group_table_data2;
> +}
> +

OK. Setup both tables for both service providers.

> +static int
> +do_test (void)
> +{
> +  int retval = 0;
> +  int i;
> +  struct group *g = NULL;
> +  uintptr_t align_mask;
> +
> +  __nss_configure_lookup ("group", "test1 [SUCCESS=merge] test2");

OK, setup the merge.

> +
> +  align_mask = __alignof__ (struct group *) - 1;
> +
> +  setgrent ();
> +
> +  for (i = 0;
> +       group_table[i].gr_gid ;
> +       ++i)

Not GNU-style and doesn't exceed line width.

> +    {
> +      g = getgrgid (group_table[i].gr_gid);
> +      if (g)
> +	{
> +	  retval += compare_groups (i, g, & group_table[i]);
> +	  if ((uintptr_t)g & align_mask)
> +	    {
> +	      printf("[%d] unaligned group %p\n", i, g);
> +	      retval ++;

retval++;

> +	    }
> +	  if ((uintptr_t)(g->gr_mem) & align_mask)
> +	    {
> +	      printf("[%d] unaligned member list %p\n", i, g->gr_mem);
> +	      retval ++;

retval+++;

> +	    }
> +	}
> +      else
> +	{
> +	  printf ("[%d] group %u.%s not found\n", i,
> +	      group_table[i].gr_gid, group_table[i].gr_name);
> +	  retval += 1;
> +	}
> +    }
> +
> +  endgrent ();
> +
> +#define EXPECTED 0
> +  if (retval == EXPECTED)
> +    {
> +      if (retval)
> +	printf ("Found %d expected errors\n", retval);

Add PASS: prefix.

> +      return 0;
> +    }
> +  else
> +    {
> +      printf ("Found %d errors, expected %d\n", retval, EXPECTED);

Add FAIL: prefix.

> +      return 1;
> +    }

OK.

> +}
> +
> +#define TEST_FUNCTION do_test ()
> +#include "../test-skeleton.c"
> diff --git a/nss/tst-nss-test5.c b/nss/tst-nss-test5.c
> new file mode 100644
> index 0000000..3af5708
> --- /dev/null
> +++ b/nss/tst-nss-test5.c
> @@ -0,0 +1,103 @@
> +/* Test error checking for passwd entries.
> +   Copyright (C) 2017 Free Software Foundation, Inc.
> +   This file is part of the GNU C Library.
> +
> +   The GNU C Library is free software; you can redistribute it and/or
> +   modify it under the terms of the GNU Lesser General Public
> +   License as published by the Free Software Foundation; either
> +   version 2.1 of the License, or (at your option) any later version.
> +
> +   The GNU C Library is distributed in the hope that it will be useful,
> +   but WITHOUT ANY WARRANTY; without even the implied warranty of
> +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> +   Lesser General Public License for more details.
> +
> +   You should have received a copy of the GNU Lesser General Public
> +   License along with the GNU C Library; if not, see
> +   <http://www.gnu.org/licenses/>.  */
> +
> +#include <nss.h>
> +#include <pwd.h>
> +#include <stdio.h>
> +#include <stdlib.h>
> +#include <string.h>
> +
> +#include "nss_test.h"
> +
> +static struct passwd pwd_table[] = {
> +  PWD (100),  /* baseline, matches */
> +  PWD (300),  /* wrong name and uid */
> +  PWD_N (200, NULL), /* missing name */
> +  PWD (60), /* unexpected name */
> +  { .pw_name = (char *)"name20000",  .pw_passwd = (char *) "*", .pw_uid = 20000,  \
> +    .pw_gid = 200, .pw_gecos = (char *) "*", .pw_dir = (char *) "*",	\
> +    .pw_shell = (char *) "*" }, /* wrong gid */
> +  { .pw_name = (char *)"name2",  .pw_passwd = (char *) "x", .pw_uid = 2,  \
> +    .pw_gid = 2, .pw_gecos = (char *) "y", .pw_dir = (char *) "z",	\
> +    .pw_shell = (char *) "*" }, /* spot check other text fields */
> +  PWD_LAST ()

OK.

> +};
> +
> +static struct passwd exp_table[] = {
> +  PWD (100),
> +  PWD (30),
> +  PWD (200),
> +  PWD_N (60, NULL),
> +  PWD (20000),
> +  PWD (2),
> +  PWD_LAST ()

OK.

> +};
> +
> +void
> +_nss_test1_init_hook(test_tables *t)
> +{
> +  t->pwd_table = pwd_table;
> +}
> +
> +static int
> +do_test (void)
> +{
> +  int retval = 0;
> +  int i;
> +  struct passwd *p;
> +
> +  __nss_configure_lookup ("passwd", "test1 test2");

OK.

> +
> +  setpwent ();
> +
> +  i = 0;
> +  for (p = getpwent ();
> +       p != NULL && ! PWD_ISLAST (& exp_table[i]);
> +       ++i, p = getpwent ())
> +    retval += compare_passwds (i, p, & exp_table[i]);
> +
> +  endpwent ();
> +
> +
> +  if (p)
> +    {
> +      printf ("[?] passwd entry %u.%s unexpected\n", p->pw_uid, p->pw_name);
> +      ++ retval;

++retval;

> +    }
> +  if (! PWD_ISLAST (& exp_table[i]))
> +    {
> +      printf ("[%d] passwd entry %u.%s missing\n", i,
> +	      exp_table[i].pw_uid, exp_table[i].pw_name);
> +      ++ retval;

++retval;

> +    }
> +
> +#define EXPECTED 9
> +  if (retval == EXPECTED)
> +    {
> +      printf ("Found %d expected errors\n", retval);

Add PASS: prefix.

> +      return 0;
> +    }
> +  else
> +    {
> +      printf ("Found %d errors, expected %d\n", retval, EXPECTED);

Add FAIL: prefix.

> +      return 1;
> +    }

OK.

> +}
> +
> +#define TEST_FUNCTION do_test ()
> +#include "../test-skeleton.c"
> diff --git a/shlib-versions b/shlib-versions
> index fa3cf1d..b9cb99d 100644
> --- a/shlib-versions
> +++ b/shlib-versions
> @@ -52,6 +52,7 @@ libnss_db=2
>  # Tests for NSS.  They must have the same NSS_SHLIB_REVISION number as
>  # the rest.
>  libnss_test1=2
> +libnss_test2=2

OK.

>  
>  # Version for libnsl with YP and NIS+ functions.
>  libnsl=1
>
  

On Thu, 13 Jul 2017, DJ Delorie wrote:

> +#define TEST_FUNCTION do_test ()
> +#include "../test-skeleton.c"

These tests should all be converted to use <support/test-driver.c>.  New 
tests should not be using the old approach of including 
../test-skeleton.c.