Re: [PATCH 1.2] Clean up netgroupcache

[Siddhesh Poyarekar <siddhesh at redhat dot com>]

On Fri, Apr 11, 2014 at 04:30:00PM +0200, OndÅej BÃlka wrote:
> New version is here.
> 
> 
> 	* nscd/netgroupcache.c (addgetnetgrentX): Use separate buffer
> 	for result and temporary data.
> 
> diff --git a/nscd/netgroupcache.c b/nscd/netgroupcache.c
> index 820d823..9582ffb 100644
> --- a/nscd/netgroupcache.c
> +++ b/nscd/netgroupcache.c
> @@ -138,8 +138,10 @@ addgetnetgrentX (struct database_dyn *db, int fd, request_header *req,
>    char *key_copy = NULL;
>    struct __netgrent data;
>    size_t buflen = MAX (1024, sizeof (*dataset) + req->key_len);
> +  size_t tempbuflen = 1024;
>    size_t buffilled = sizeof (*dataset);
>    char *buffer = NULL;
> +  char *tempbuf;
>    size_t nentries = 0;
>    size_t group_len = strlen (key) + 1;
>    union
> @@ -159,6 +161,8 @@ addgetnetgrentX (struct database_dyn *db, int fd, request_header *req,
>  
>    memset (&data, '\0', sizeof (data));
>    buffer = xmalloc (buflen);
> +  tempbuf = xmalloc (tempbuflen);
> +
>    first_needed.elem.next = &first_needed.elem;
>    memcpy (first_needed.elem.name, key, group_len);
>    data.needed_groups = &first_needed.elem;
> @@ -201,14 +205,13 @@ addgetnetgrentX (struct database_dyn *db, int fd, request_header *req,
>  		while (1)
>  		  {
>  		    int e;
> -		    status = getfct.f (&data, buffer + buffilled,
> -				       buflen - buffilled - req->key_len, &e);
> +		    status = getfct.f (&data, tempbuf, tempbuflen, &e);
>  		    if (status == NSS_STATUS_RETURN
>  			|| status == NSS_STATUS_NOTFOUND)
>  		      /* This was either the last one for this group or the
>  			 group was empty.  Look at next group if available.  */
>  		      break;
> -		    if (status == NSS_STATUS_SUCCESS)
> +		    if (__glibc_likely (status == NSS_STATUS_SUCCESS))

This is an unrelated change.  It could be proposed as a separate
change, but I'm not convinced that it is necessary or even useful
unlike the glibc_unlikely below, which is unlikely because of the fact
that host, user and domain sizes should not normally exceed 1024 bytes
given their defined limitations.

>  		      {
>  			if (data.type == triple_val)
>  			  {
> @@ -220,53 +223,13 @@ addgetnetgrentX (struct database_dyn *db, int fd, request_header *req,
>  			    size_t userlen = strlen (nuser ?: "") + 1;
>  			    size_t domainlen = strlen (ndomain ?: "") + 1;
>  
> -			    if (nhost == NULL || nuser == NULL || ndomain == NULL
> -				|| nhost > nuser || nuser > ndomain)
> -			      {
> -				const char *last = nhost;
> -				if (last == NULL
> -				    || (nuser != NULL && nuser > last))
> -				  last = nuser;
> -				if (last == NULL
> -				    || (ndomain != NULL && ndomain > last))
> -				  last = ndomain;
> -
> -				size_t bufused
> -				  = (last == NULL
> -				     ? buffilled
> -				     : last + strlen (last) + 1 - buffer);
> -
> -				/* We have to make temporary copies.  */
> -				size_t needed = hostlen + userlen + domainlen;
> -
> -				if (buflen - req->key_len - bufused < needed)
> -				  {
> -				    buflen += MAX (buflen, 2 * needed);
> -				    /* Save offset in the old buffer.  We don't
> -				       bother with the NULL check here since
> -				       we'll do that later anyway.  */
> -				    size_t nhostdiff = nhost - buffer;
> -				    size_t nuserdiff = nuser - buffer;
> -				    size_t ndomaindiff = ndomain - buffer;
> -
> -				    char *newbuf = xrealloc (buffer, buflen);
> -				    /* Fix up the triplet pointers into the new
> -				       buffer.  */
> -				    nhost = (nhost ? newbuf + nhostdiff
> -					     : NULL);
> -				    nuser = (nuser ? newbuf + nuserdiff
> -					     : NULL);
> -				    ndomain = (ndomain ? newbuf + ndomaindiff
> -					       : NULL);
> -				    buffer = newbuf;
> -				  }
> +			    size_t newlen = buffilled + hostlen + userlen
> +					  + domainlen + req->key_len;
>  
> -				nhost = memcpy (buffer + bufused,
> -						nhost ?: "", hostlen);
> -				nuser = memcpy ((char *) nhost + hostlen,
> -						nuser ?: "", userlen);
> -				ndomain = memcpy ((char *) nuser + userlen,
> -						  ndomain ?: "", domainlen);
> +			    if (__glibc_unlikely (newlen > buflen))
> +			      {
> +				buflen = 2 * newlen;
> +				buffer = xrealloc (buffer, buflen);
>  			      }
>  
>  			    char *wp = buffer + buffilled;
> @@ -328,8 +291,8 @@ addgetnetgrentX (struct database_dyn *db, int fd, request_header *req,
>  		      }
>  		    else if (status == NSS_STATUS_UNAVAIL && e == ERANGE)
>  		      {
> -			buflen *= 2;
> -			buffer = xrealloc (buffer, buflen);
> +			tempbuflen *= 2;
> +			tempbuf = xrealloc (tempbuf, tempbuflen);
>  		      }
>  		  }
>  
> @@ -474,6 +437,7 @@ addgetnetgrentX (struct database_dyn *db, int fd, request_header *req,
>      }
>  
>   out:
> +  free (tempbuf);
>    free (buffer);
>  
>    *resultp = dataset;

The patch looks OK otherwise.  However, I'd like to know your feedback
from actually testing this patch.

Thanks,
Siddhesh

Attachment: pgp5_oI6mQauF.pgp
Description: PGP signature