This is the mail archive of the libc-alpha@sourceware.org mailing list for the glibc project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

[PATCH 1.2] Clean up netgroupcache


On Fri, Mar 28, 2014 at 08:17:37AM +0530, Siddhesh Poyarekar wrote:
> On Thu, Mar 27, 2014 at 10:15:16PM +0100, OndÅej BÃlka wrote:
> > Had typo there, here is fixed patch.
> 
> Please add a note on how you tested the fix.
> 
> > 	* nscd/netgroupcache.c: Clean up implementation.
> 
> Incorrect ChangeLog entry.
> 
New one below.

> > -				|| nhost > nuser || nuser > ndomain)
> > +			    size_t newlen = buffilled + hostlen + userlen + domainlen;
> > +			    if (newlen + req->key_len > buflen)
> 
> Might as well put req->key_len in newlen and simplify this further.
>
ok 

> > @@ -328,8 +289,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);
> 
> This ought to be unlikely now, since the consolidated lengths of user,
> host and domain should fit in the initially allocated 1024 bytes.
> 
I made change that previous branch is likely. I am not sure about this
one because its conditional probability. This condition is checked only
on failure. So likeliness is determined by probability that failure was
caused by insufficient size.


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))
 		      {
 			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;


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]