Re: [PATCH] _nss_nis_initgroups_dyn: Use struct scratch_buffer [BZ #18023]

[DJ Delorie <dj at redhat dot com>]

fweimer@redhat.com (Florian Weimer) writes:
> -      buf = extend_alloca (buf, buflen, 2 * buflen);
> +      if (!scratch_buffer_grow (&tmpbuf))
> +	return 1;

The docs say if _grow() fails, the buffer is still free'able.  Could we
not, or should we, use "break" here so that scratch_buffer_free() is
called when grow() fails?

The docs (scratch_buffer.h) mentions that the old buffer is freed, but
that the scratch_buffer is still "in a freeable state" so it's not clear
if the intention is that you *should* call scratch_buffer_free(), or
*must*, or *must not*, etc.

>      }
>  
> +  scratch_buffer_free (&tmpbuf);
>    return 1;
>  }


> +	if (!scratch_buffer_grow (&tmpbuf))
> +	  {
> +	    status = NSS_STATUS_TRYAGAIN;
> +	    goto done;
> +	  }

In this case, going to "done" *does* call scratch_buffer_free().  At
least we should be consistent within a given file ;-)

>        if (status != NSS_STATUS_SUCCESS)
>  	{
> @@ -331,6 +337,7 @@ done:
>        intern.start = intern.start->next;
>        free (intern.next);
>      }
> +  scratch_buffer_free (&tmpbuf);
>  
>    return status;
>  }

The code itself looks OK, the above are just consistency and
clarification things.