Re: [PATCH] nsswitch: Add group merging support

[Mike Frysinger <vapier at gentoo dot org>]

On 04 Jan 2016 09:12, Stephen Gallagher wrote:

overall sounds fine.  i have no strong opinions as i don't use nss myself :).
style nits follow.

note: you should also update the NEWS file with a short blurb.
the commit message is great, albeit too verbose for NEWS.

> --- /dev/null
> +++ b/grp/grp-merge.c
> @@ -0,0 +1,177 @@
> +/* Group merging implementation.
> +   Copyright (C) 2015 Free Software Foundation, Inc.

date needs updating

> +#define BUFCHECK(size)		\
> +  do {				\
> +    if (c + size > buflen) {	\
> +        free (members);		\
> +        return ERANGE;		\
> +    }				\
> +  } while(0)

style is incorrect for GNU code.  perhaps (note use of tabs too):

#define BUFCHECK(size)		\
  do {				\
    if (c + (size) > buflen)	\
      {				\
	free (members);		\
	return ERANGE;		\
      }				\
  } while(0)

> +  /* Allocate a temporary holding area for the pointers to the member
> +     contents, including space for a NULL-terminator.  */
> +  members = malloc (sizeof (char *) * (memcount + 1));

do we have to worry about overflow ?  if there's some external logic that
keeps these from overflowing, might be nice to note somewhere (comes up a
few times in this patch).

> +  if (members == NULL)
> +      return ENOMEM;

indented by two too many spaces

> +  /* Copy the pointers from the members array into the buffer and assign them
> +     to the gr_mem member of destgrp.  */
> +  destgrp->gr_mem = (char **) &destbuf[c];
> +  len = sizeof (char *) * (memcount + 1);
> +  BUFCHECK (len);
> +  memcpy (&destbuf[c], members, len);
> +  c += len;
> +  free (members);
> +  members = NULL;

force of habit clearing members ?  seems pointless otherwise.

> +  if (endptr)
> +      *endptr = destbuf + c;

too much indentation

> +int
> +__merge_grp (struct group *savedgrp, char *savedbuf, char *savedend,
> +	     size_t buflen, struct group *mergegrp, char *mergebuf)

same comments for this func as above since it seems to be largely the
same logic wise

> --- /dev/null
> +++ b/grp/grp-merge.h
> @@ -0,0 +1,35 @@
> +/* Group merging implementation.
> +   Copyright (C) 2015 Free Software Foundation, Inc.

update the date

> +/* Duplicate a grp struct (and its members). When no longer needed, the
> +   calling function must free(newbuf). */

two spaces after the .

> +/* Merge the member lists of two grp structs together. */

same here

> +int
> +__merge_grp (struct group *savedgrp, char *savedbuf, char *savedend,
> +	     size_t buflen, struct group *mergegrp, char *mergebuf);

should these two funcs have internal_function & attribute_hidden applied
to them ?

> --- a/manual/nss.texi
> +++ b/manual/nss.texi
>
> +When processing @samp{merge} for @samp{group} membership, the group GID
> +and name must be identical for both entries. If only one or the other is

two spaces after the .

> +a match, the behavior is undefined.

could you clarify "undefined" ?  people could interpret this as memory
corruption / crashes, while others are are inconsistent results.  i think
we just want the latter.

> --- a/nss/getXXbyYY_r.c
> +++ b/nss/getXXbyYY_r.c
>
> +/* Set defaults for merge functions that haven't been defined.  */
> +#ifndef DEEPCOPY_FN
> +static inline int
> +__copy_einval (LOOKUP_TYPE a,
> +	      const size_t b,
> +	      LOOKUP_TYPE *c,
> +	      char *d,
> +	      char **e)

indentation of args looks slightly off

> +#ifndef MERGE_FN
> +static inline int
> +__merge_einval (LOOKUP_TYPE *a,
> +	       char *b,
> +	       char *c,
> +	       size_t d,
> +	       LOOKUP_TYPE *e,
> +	       char *f)

here too

> +#define CHECK_MERGE(err, status)	\
> +do {					\
> +  if (err)				\
> +    {					\
> +      __set_errno (err);		\
> +      if (err == ERANGE)		\
> +          status = NSS_STATUS_TRYAGAIN;	\
> +      else				\
> +          status = NSS_STATUS_UNAVAIL;	\
> +      break;				\
> +    }					\
> +} while(0)

almost there ;).  8 leading spaces -> 1 tab, too many indents for status
assignment, and a space before the ( in the "while (0)".

> +	  if (status == NSS_STATUS_SUCCESS)
> +	    {
> +		/* The previous loop saved a buffer for merging.
> +		   Perform the merge now.  */
> +		err = MERGE_FN (&mergegrp, mergebuf, endptr, buflen, resbuf,
> +				buffer);
> +		CHECK_MERGE (err,status);
> +		do_merge = 0;
> +	    }

indentation here is broken too -- 6 spaces, not 1 tab

> +	  else
> +	    {
> +	      /* If the result wasn't SUCCESS, copy the saved buffer back
> +	         into the result buffer and set the status back to
> +	         NSS_STATUS_SUCCESS to match the previous pass through the loop.
> +	          * If the next action is CONTINUE, it will overwrite the value
> +	            currently in the buffer and return the new value.
> +	          * If the next action is RETURN, we'll return the previously-
> +	            acquired values.
> +	          * If the next action is MERGE, then it will be added to the buffer
> +	            saved from the previous source.  */

some of these lines are too long

> +	  if (!mergebuf)

GNU style says to test values rather than rely on implicit bool.  so this
would be:
	if (mergebuf == NULL)

this comes up quite a bit in this patch

> +  free(mergebuf);

space before the (

> --- a/nss/getnssent_r.c
> +++ b/nss/getnssent_r.c
>
> +	{
> +	  /* This is a special-case. When [SUCCESS=merge] is in play,

two spaces after the .

> +	     _nss_next2() will skip to the next database.  Due to the
> +	     implementation of that function, we can't know whether we're
> +	     in an enumeration or an individual lookup, which behaves
> +	     differently with regards to merging.  We'll treat SUCCESS as
> +	     an indication to start the enumeration at this database.
> +	   */

the trailing */ should be on the previous line.

> +      else
> +	{
> +	  no_more = __nss_next2 (nip, func_name, NULL, &fct.ptr, status, 0);
> +	}

could elide the braces

> +	  if (status == NSS_STATUS_SUCCESS
> +	      && nss_next_action (*nip, status) == NSS_ACTION_MERGE)
> +	    {
> +	      /* This is a special-case. When [SUCCESS=merge] is in play,

two spaces after the .

> +	         _nss_next2() will skip to the next database.  Due to the
> +	         implementation of that function, we can't know whether we're
> +	         in an enumeration or an individual lookup, which behaves
> +	         differently with regards to merging.  We'll treat SUCCESS as
> +	         an indication to return the results here.
> +	       */

cuddle up the */

> +	  else
> +	    {
> +	      no_more = __nss_next2 (nip, getent_func_name, NULL, &fct.ptr,
> +				     status, 0);
> +	    }

could elide the braces
-mike

Attachment: signature.asc
Description: Digital signature