* [PATCH] nss_files: Use struct scratch_buffer for gethostbyname [BZ #18023]
@ 2017-09-04 17:31 Florian Weimer
2017-09-05 17:53 ` Adhemerval Zanella
0 siblings, 1 reply; 6+ messages in thread
From: Florian Weimer @ 2017-09-04 17:31 UTC (permalink / raw)
To: libc-alpha
2017-09-04 Florian Weimer <fweimer@redhat.com>
[BZ #18023]
* nss/nss_files/files-hosts.c (gethostbyname3_multi): Use struct
scratch_buffer.
diff --git a/nss/nss_files/files-hosts.c b/nss/nss_files/files-hosts.c
index 867c10c2ef..c2cd07584c 100644
--- a/nss/nss_files/files-hosts.c
+++ b/nss/nss_files/files-hosts.c
@@ -22,6 +22,7 @@
#include <arpa/nameser.h>
#include <netdb.h>
#include <resolv/resolv-internal.h>
+#include <scratch_buffer.h>
/* Get implementation for some internal functions. */
@@ -121,15 +122,12 @@ gethostbyname3_multi (FILE * stream, const char *name, int af,
int *errnop, int *herrnop, int flags)
{
/* We have to get all host entries from the file. */
- size_t tmp_buflen = MIN (buflen, 4096);
- char tmp_buffer_stack[tmp_buflen]
- __attribute__ ((__aligned__ (__alignof__ (struct hostent_data))));
- char *tmp_buffer = tmp_buffer_stack;
+ struct scratch_buffer tmp_buffer;
+ scratch_buffer_init (&tmp_buffer);
struct hostent tmp_result_buf;
int naddrs = 1;
int naliases = 0;
char *bufferend;
- bool tmp_buffer_malloced = false;
enum nss_status status;
while (result->h_aliases[naliases] != NULL)
@@ -138,8 +136,8 @@ gethostbyname3_multi (FILE * stream, const char *name, int af,
bufferend = (char *) &result->h_aliases[naliases + 1];
again:
- while ((status = internal_getent (stream, &tmp_result_buf, tmp_buffer,
- tmp_buflen, errnop, herrnop, af,
+ while ((status = internal_getent (stream, &tmp_result_buf, tmp_buffer.data,
+ tmp_buffer.length, errnop, herrnop, af,
flags))
== NSS_STATUS_SUCCESS)
{
@@ -266,52 +264,18 @@ gethostbyname3_multi (FILE * stream, const char *name, int af,
if (status == NSS_STATUS_TRYAGAIN)
{
- size_t newsize = 2 * tmp_buflen;
- if (tmp_buffer_malloced)
+ if (!scratch_buffer_grow (&tmp_buffer))
{
- char *newp = realloc (tmp_buffer, newsize);
- if (newp != NULL)
- {
- assert ((((uintptr_t) newp)
- & (__alignof__ (struct hostent_data) - 1))
- == 0);
- tmp_buffer = newp;
- tmp_buflen = newsize;
- goto again;
- }
- }
- else if (!__libc_use_alloca (buflen + newsize))
- {
- tmp_buffer = malloc (newsize);
- if (tmp_buffer != NULL)
- {
- assert ((((uintptr_t) tmp_buffer)
- & (__alignof__ (struct hostent_data) - 1))
- == 0);
- tmp_buffer_malloced = true;
- tmp_buflen = newsize;
- goto again;
- }
+ *herrnop = NETDB_INTERNAL;
+ status = NSS_STATUS_TRYAGAIN;
}
else
- {
- tmp_buffer
- = extend_alloca (tmp_buffer, tmp_buflen,
- newsize
- + __alignof__ (struct hostent_data));
- tmp_buffer = (char *) (((uintptr_t) tmp_buffer
- + __alignof__ (struct hostent_data)
- - 1)
- & ~(__alignof__ (struct hostent_data)
- - 1));
- goto again;
- }
+ goto again;
}
else
status = NSS_STATUS_SUCCESS;
out:
- if (tmp_buffer_malloced)
- free (tmp_buffer);
+ scratch_buffer_free (&tmp_buffer);
return status;
}
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] nss_files: Use struct scratch_buffer for gethostbyname [BZ #18023]
2017-09-04 17:31 [PATCH] nss_files: Use struct scratch_buffer for gethostbyname [BZ #18023] Florian Weimer
@ 2017-09-05 17:53 ` Adhemerval Zanella
2017-09-05 18:38 ` Florian Weimer
0 siblings, 1 reply; 6+ messages in thread
From: Adhemerval Zanella @ 2017-09-05 17:53 UTC (permalink / raw)
To: libc-alpha
On 04/09/2017 14:31, Florian Weimer wrote:
> 2017-09-04 Florian Weimer <fweimer@redhat.com>
>
> [BZ #18023]
> * nss/nss_files/files-hosts.c (gethostbyname3_multi): Use struct
> scratch_buffer.
LGTM with some nits below.
>
> diff --git a/nss/nss_files/files-hosts.c b/nss/nss_files/files-hosts.c
> index 867c10c2ef..c2cd07584c 100644
> --- a/nss/nss_files/files-hosts.c
> +++ b/nss/nss_files/files-hosts.c
> @@ -22,6 +22,7 @@
> #include <arpa/nameser.h>
> #include <netdb.h>
> #include <resolv/resolv-internal.h>
> +#include <scratch_buffer.h>
>
>
> /* Get implementation for some internal functions. */
> @@ -121,15 +122,12 @@ gethostbyname3_multi (FILE * stream, const char *name, int af,
> int *errnop, int *herrnop, int flags)
> {
> /* We have to get all host entries from the file. */
> - size_t tmp_buflen = MIN (buflen, 4096);
> - char tmp_buffer_stack[tmp_buflen]
> - __attribute__ ((__aligned__ (__alignof__ (struct hostent_data))));
I can't really tell how important is the alignment of this buffer in particular,
since on subsequent 'internal_getent' it does uses a plain char buffer. Do we
need to keep the alignment of this buffer?
> - char *tmp_buffer = tmp_buffer_stack;
> + struct scratch_buffer tmp_buffer;
> + scratch_buffer_init (&tmp_buffer);
> struct hostent tmp_result_buf;
> int naddrs = 1;
> int naliases = 0;
> char *bufferend;
> - bool tmp_buffer_malloced = false;
> enum nss_status status;
>
> while (result->h_aliases[naliases] != NULL)
> @@ -138,8 +136,8 @@ gethostbyname3_multi (FILE * stream, const char *name, int af,
> bufferend = (char *) &result->h_aliases[naliases + 1];
>
> again:
> - while ((status = internal_getent (stream, &tmp_result_buf, tmp_buffer,
> - tmp_buflen, errnop, herrnop, af,
> + while ((status = internal_getent (stream, &tmp_result_buf, tmp_buffer.data,
> + tmp_buffer.length, errnop, herrnop, af,
> flags))
> == NSS_STATUS_SUCCESS)
> {
> @@ -266,52 +264,18 @@ gethostbyname3_multi (FILE * stream, const char *name, int af,
>
> if (status == NSS_STATUS_TRYAGAIN)
> {
> - size_t newsize = 2 * tmp_buflen;
> - if (tmp_buffer_malloced)
> + if (!scratch_buffer_grow (&tmp_buffer))
> {
> - char *newp = realloc (tmp_buffer, newsize);
> - if (newp != NULL)
> - {
> - assert ((((uintptr_t) newp)
> - & (__alignof__ (struct hostent_data) - 1))
> - == 0);
> - tmp_buffer = newp;
> - tmp_buflen = newsize;
> - goto again;
> - }
> - }
> - else if (!__libc_use_alloca (buflen + newsize))
> - {
> - tmp_buffer = malloc (newsize);
> - if (tmp_buffer != NULL)
> - {
> - assert ((((uintptr_t) tmp_buffer)
> - & (__alignof__ (struct hostent_data) - 1))
> - == 0);
> - tmp_buffer_malloced = true;
> - tmp_buflen = newsize;
> - goto again;
> - }
> + *herrnop = NETDB_INTERNAL;
> + status = NSS_STATUS_TRYAGAIN;
> }
> else
> - {
> - tmp_buffer
> - = extend_alloca (tmp_buffer, tmp_buflen,
> - newsize
> - + __alignof__ (struct hostent_data));
> - tmp_buffer = (char *) (((uintptr_t) tmp_buffer
> - + __alignof__ (struct hostent_data)
> - - 1)
> - & ~(__alignof__ (struct hostent_data)
> - - 1));
> - goto again;
> - }
> + goto again;
> }
> else
> status = NSS_STATUS_SUCCESS;
> out:
> - if (tmp_buffer_malloced)
> - free (tmp_buffer);
> + scratch_buffer_free (&tmp_buffer);
> return status;
> }
I do think this it is easier to read and follow the code *without* the goto,
something like:
scratch_buffer_init (...);
while (1)
{
while ((status = internal_getent (...)) == NSS_STATUS_SUCCESS)
{
...
}
if (status == NSS_STATUS_TRYAGAIN)
if (!scratch_buffer_grow (&tmp_buffer))
{
*herrnop = NETDB_INTERNAL;
status = NSS_STATUS_TRYAGAIN;
break;
}
else
status = NSS_STATUS_SUCCESS;
}
scratch_buffer_free (...);
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] nss_files: Use struct scratch_buffer for gethostbyname [BZ #18023]
2017-09-05 17:53 ` Adhemerval Zanella
@ 2017-09-05 18:38 ` Florian Weimer
2017-10-10 13:02 ` Florian Weimer
0 siblings, 1 reply; 6+ messages in thread
From: Florian Weimer @ 2017-09-05 18:38 UTC (permalink / raw)
To: Adhemerval Zanella, libc-alpha
On 09/05/2017 07:53 PM, Adhemerval Zanella wrote:
>> /* Get implementation for some internal functions. */
>> @@ -121,15 +122,12 @@ gethostbyname3_multi (FILE * stream, const char *name, int af,
>> int *errnop, int *herrnop, int flags)
>> {
>> /* We have to get all host entries from the file. */
>> - size_t tmp_buflen = MIN (buflen, 4096);
>> - char tmp_buffer_stack[tmp_buflen]
>> - __attribute__ ((__aligned__ (__alignof__ (struct hostent_data))));
>
> I can't really tell how important is the alignment of this buffer in particular,
> since on subsequent 'internal_getent' it does uses a plain char buffer. Do we
> need to keep the alignment of this buffer?
internal_getent must align on its own because it is called with user
buffers and POSIX doesn't say anything about the alignment.
But struct scratch_buffer supplies max_align_t alignment anyway, which
is at least the alignment of struct hostent_data (by definition).
> I do think this it is easier to read and follow the code *without* the goto,
> something like:
>
> scratch_buffer_init (...);
> while (1)
> {
> while ((status = internal_getent (...)) == NSS_STATUS_SUCCESS)
> {
> ...
> }
> if (status == NSS_STATUS_TRYAGAIN)
> if (!scratch_buffer_grow (&tmp_buffer))
> {
> *herrnop = NETDB_INTERNAL;
> status = NSS_STATUS_TRYAGAIN;
> break;
> }
> else
> status = NSS_STATUS_SUCCESS;
> }
> scratch_buffer_free (...);
Right, I think I'll make this change in the first (refactoring) patch.
Thanks,
Florian
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] nss_files: Use struct scratch_buffer for gethostbyname [BZ #18023]
2017-09-05 18:38 ` Florian Weimer
@ 2017-10-10 13:02 ` Florian Weimer
2017-10-11 5:04 ` Florian Weimer
0 siblings, 1 reply; 6+ messages in thread
From: Florian Weimer @ 2017-10-10 13:02 UTC (permalink / raw)
To: Adhemerval Zanella, libc-alpha
[-- Attachment #1: Type: text/plain, Size: 777 bytes --]
On 09/05/2017 08:38 PM, Florian Weimer wrote:
>> I do think this it is easier to read and follow the code *without* the goto,
>> something like:
>>
>> scratch_buffer_init (...);
>> while (1)
>> {
>> while ((status = internal_getent (...)) == NSS_STATUS_SUCCESS)
>> {
>> ...
>> }
>> if (status == NSS_STATUS_TRYAGAIN)
>> if (!scratch_buffer_grow (&tmp_buffer))
>> {
>> *herrnop = NETDB_INTERNAL;
>> status = NSS_STATUS_TRYAGAIN;
>> break;
>> }
>> else
>> status = NSS_STATUS_SUCCESS;
>> }
>> scratch_buffer_free (...);
>
> Right, I think I'll make this change in the first (refactoring) patch.
I made this change in this patch instead. Still okay?
Thanks,
Florian
[-- Attachment #2: bug18023.patch --]
[-- Type: text/x-patch, Size: 11053 bytes --]
2017-10-10 Florian Weimer <fweimer@redhat.com>
[BZ #18023]
* nss/nss_files/files-hosts.c (gethostbyname3_multi): Use struct
scratch_buffer. Eliminate gotos.
diff --git a/nss/nss_files/files-hosts.c b/nss/nss_files/files-hosts.c
index 867c10c2ef..763fa39a47 100644
--- a/nss/nss_files/files-hosts.c
+++ b/nss/nss_files/files-hosts.c
@@ -22,6 +22,7 @@
#include <arpa/nameser.h>
#include <netdb.h>
#include <resolv/resolv-internal.h>
+#include <scratch_buffer.h>
/* Get implementation for some internal functions. */
@@ -121,15 +122,12 @@ gethostbyname3_multi (FILE * stream, const char *name, int af,
int *errnop, int *herrnop, int flags)
{
/* We have to get all host entries from the file. */
- size_t tmp_buflen = MIN (buflen, 4096);
- char tmp_buffer_stack[tmp_buflen]
- __attribute__ ((__aligned__ (__alignof__ (struct hostent_data))));
- char *tmp_buffer = tmp_buffer_stack;
+ struct scratch_buffer tmp_buffer;
+ scratch_buffer_init (&tmp_buffer);
struct hostent tmp_result_buf;
int naddrs = 1;
int naliases = 0;
char *bufferend;
- bool tmp_buffer_malloced = false;
enum nss_status status;
while (result->h_aliases[naliases] != NULL)
@@ -137,181 +135,165 @@ gethostbyname3_multi (FILE * stream, const char *name, int af,
bufferend = (char *) &result->h_aliases[naliases + 1];
- again:
- while ((status = internal_getent (stream, &tmp_result_buf, tmp_buffer,
- tmp_buflen, errnop, herrnop, af,
- flags))
- == NSS_STATUS_SUCCESS)
+ while (true)
{
- int matches = 1;
- struct hostent *old_result = result;
- result = &tmp_result_buf;
- /* The following piece is a bit clumsy but we want to use the
- `LOOKUP_NAME_CASE' value. The optimizer should do its
- job. */
- do
+ status = internal_getent (stream, &tmp_result_buf, tmp_buffer.data,
+ tmp_buffer.length, errnop, herrnop, af,
+ flags);
+ /* Enlarge the buffer if necessary. */
+ if (status == NSS_STATUS_TRYAGAIN && *herrnop == NETDB_INTERNAL
+ && *errnop == ERANGE)
{
- LOOKUP_NAME_CASE (h_name, h_aliases)
- result = old_result;
- }
- while ((matches = 0));
-
- if (matches)
- {
- /* We could be very clever and try to recycle a few bytes
- in the buffer instead of generating new arrays. But
- we are not doing this here since it's more work than
- it's worth. Simply let the user provide a bit bigger
- buffer. */
- char **new_h_addr_list;
- char **new_h_aliases;
- int newaliases = 0;
- size_t newstrlen = 0;
- int cnt;
-
- /* Count the new aliases and the length of the strings. */
- while (tmp_result_buf.h_aliases[newaliases] != NULL)
- {
- char *cp = tmp_result_buf.h_aliases[newaliases];
- ++newaliases;
- newstrlen += strlen (cp) + 1;
- }
- /* If the real name is different add it also to the
- aliases. This means that there is a duplication
- in the alias list but this is really the user's
- problem. */
- if (strcmp (old_result->h_name,
- tmp_result_buf.h_name) != 0)
- {
- ++newaliases;
- newstrlen += strlen (tmp_result_buf.h_name) + 1;
- }
-
- /* Make sure bufferend is aligned. */
- assert ((bufferend - (char *) 0) % sizeof (char *) == 0);
-
- /* Now we can check whether the buffer is large enough.
- 16 is the maximal size of the IP address. */
- if (bufferend + 16 + (naddrs + 2) * sizeof (char *)
- + roundup (newstrlen, sizeof (char *))
- + (naliases + newaliases + 1) * sizeof (char *)
- >= buffer + buflen)
- {
- *errnop = ERANGE;
- *herrnop = NETDB_INTERNAL;
- status = NSS_STATUS_TRYAGAIN;
- goto out;
- }
-
- new_h_addr_list =
- (char **) (bufferend
- + roundup (newstrlen, sizeof (char *))
- + 16);
- new_h_aliases =
- (char **) ((char *) new_h_addr_list
- + (naddrs + 2) * sizeof (char *));
-
- /* Copy the old data in the new arrays. */
- for (cnt = 0; cnt < naddrs; ++cnt)
- new_h_addr_list[cnt] = old_result->h_addr_list[cnt];
-
- for (cnt = 0; cnt < naliases; ++cnt)
- new_h_aliases[cnt] = old_result->h_aliases[cnt];
-
- /* Store the new strings. */
- cnt = 0;
- while (tmp_result_buf.h_aliases[cnt] != NULL)
- {
- new_h_aliases[naliases++] = bufferend;
- bufferend = (__stpcpy (bufferend,
- tmp_result_buf.h_aliases[cnt])
- + 1);
- ++cnt;
- }
-
- if (cnt < newaliases)
+ if (!scratch_buffer_grow (&tmp_buffer))
{
- new_h_aliases[naliases++] = bufferend;
- bufferend = __stpcpy (bufferend,
- tmp_result_buf.h_name) + 1;
+ *errnop = ENOMEM;
+ /* *herrnop and status already have the right value. */
+ break;
}
-
- /* Final NULL pointer. */
- new_h_aliases[naliases] = NULL;
-
- /* Round up the buffer end address. */
- bufferend += (sizeof (char *)
- - ((bufferend - (char *) 0)
- % sizeof (char *))) % sizeof (char *);
-
- /* Now the new address. */
- new_h_addr_list[naddrs++] =
- memcpy (bufferend, tmp_result_buf.h_addr,
- tmp_result_buf.h_length);
-
- /* Also here a final NULL pointer. */
- new_h_addr_list[naddrs] = NULL;
-
- /* Store the new array pointers. */
- old_result->h_aliases = new_h_aliases;
- old_result->h_addr_list = new_h_addr_list;
-
- /* Compute the new buffer end. */
- bufferend = (char *) &new_h_aliases[naliases + 1];
- assert (bufferend <= buffer + buflen);
-
- result = old_result;
+ /* Loop around and retry with a larger buffer. */
}
- }
-
- if (status == NSS_STATUS_TRYAGAIN)
- {
- size_t newsize = 2 * tmp_buflen;
- if (tmp_buffer_malloced)
+ else if (status == NSS_STATUS_SUCCESS)
{
- char *newp = realloc (tmp_buffer, newsize);
- if (newp != NULL)
+ /* A line was read. Check that it matches the search
+ criteria. */
+
+ int matches = 1;
+ struct hostent *old_result = result;
+ result = &tmp_result_buf;
+ /* The following piece is a bit clumsy but we want to use
+ the `LOOKUP_NAME_CASE' value. The optimizer should do
+ its job. */
+ do
{
- assert ((((uintptr_t) newp)
- & (__alignof__ (struct hostent_data) - 1))
- == 0);
- tmp_buffer = newp;
- tmp_buflen = newsize;
- goto again;
+ LOOKUP_NAME_CASE (h_name, h_aliases)
+ result = old_result;
}
- }
- else if (!__libc_use_alloca (buflen + newsize))
- {
- tmp_buffer = malloc (newsize);
- if (tmp_buffer != NULL)
+ while ((matches = 0));
+
+ if (matches)
{
- assert ((((uintptr_t) tmp_buffer)
- & (__alignof__ (struct hostent_data) - 1))
- == 0);
- tmp_buffer_malloced = true;
- tmp_buflen = newsize;
- goto again;
- }
- }
+ /* We could be very clever and try to recycle a few bytes
+ in the buffer instead of generating new arrays. But
+ we are not doing this here since it's more work than
+ it's worth. Simply let the user provide a bit bigger
+ buffer. */
+ char **new_h_addr_list;
+ char **new_h_aliases;
+ int newaliases = 0;
+ size_t newstrlen = 0;
+ int cnt;
+
+ /* Count the new aliases and the length of the strings. */
+ while (tmp_result_buf.h_aliases[newaliases] != NULL)
+ {
+ char *cp = tmp_result_buf.h_aliases[newaliases];
+ ++newaliases;
+ newstrlen += strlen (cp) + 1;
+ }
+ /* If the real name is different add it also to the
+ aliases. This means that there is a duplication
+ in the alias list but this is really the user's
+ problem. */
+ if (strcmp (old_result->h_name,
+ tmp_result_buf.h_name) != 0)
+ {
+ ++newaliases;
+ newstrlen += strlen (tmp_result_buf.h_name) + 1;
+ }
+
+ /* Make sure bufferend is aligned. */
+ assert ((bufferend - (char *) 0) % sizeof (char *) == 0);
+
+ /* Now we can check whether the buffer is large enough.
+ 16 is the maximal size of the IP address. */
+ if (bufferend + 16 + (naddrs + 2) * sizeof (char *)
+ + roundup (newstrlen, sizeof (char *))
+ + (naliases + newaliases + 1) * sizeof (char *)
+ >= buffer + buflen)
+ {
+ *errnop = ERANGE;
+ *herrnop = NETDB_INTERNAL;
+ status = NSS_STATUS_TRYAGAIN;
+ break;
+ }
+
+ new_h_addr_list =
+ (char **) (bufferend
+ + roundup (newstrlen, sizeof (char *))
+ + 16);
+ new_h_aliases =
+ (char **) ((char *) new_h_addr_list
+ + (naddrs + 2) * sizeof (char *));
+
+ /* Copy the old data in the new arrays. */
+ for (cnt = 0; cnt < naddrs; ++cnt)
+ new_h_addr_list[cnt] = old_result->h_addr_list[cnt];
+
+ for (cnt = 0; cnt < naliases; ++cnt)
+ new_h_aliases[cnt] = old_result->h_aliases[cnt];
+
+ /* Store the new strings. */
+ cnt = 0;
+ while (tmp_result_buf.h_aliases[cnt] != NULL)
+ {
+ new_h_aliases[naliases++] = bufferend;
+ bufferend = (__stpcpy (bufferend,
+ tmp_result_buf.h_aliases[cnt])
+ + 1);
+ ++cnt;
+ }
+
+ if (cnt < newaliases)
+ {
+ new_h_aliases[naliases++] = bufferend;
+ bufferend = __stpcpy (bufferend,
+ tmp_result_buf.h_name) + 1;
+ }
+
+ /* Final NULL pointer. */
+ new_h_aliases[naliases] = NULL;
+
+ /* Round up the buffer end address. */
+ bufferend += (sizeof (char *)
+ - ((bufferend - (char *) 0)
+ % sizeof (char *))) % sizeof (char *);
+
+ /* Now the new address. */
+ new_h_addr_list[naddrs++] =
+ memcpy (bufferend, tmp_result_buf.h_addr,
+ tmp_result_buf.h_length);
+
+ /* Also here a final NULL pointer. */
+ new_h_addr_list[naddrs] = NULL;
+
+ /* Store the new array pointers. */
+ old_result->h_aliases = new_h_aliases;
+ old_result->h_addr_list = new_h_addr_list;
+
+ /* Compute the new buffer end. */
+ bufferend = (char *) &new_h_aliases[naliases + 1];
+ assert (bufferend <= buffer + buflen);
+
+ result = old_result;
+ } /* If match was found. */
+
+ /* If no match is found, loop around and fetch another
+ line. */
+
+ } /* status == NSS_STATUS_SUCCESS. */
else
- {
- tmp_buffer
- = extend_alloca (tmp_buffer, tmp_buflen,
- newsize
- + __alignof__ (struct hostent_data));
- tmp_buffer = (char *) (((uintptr_t) tmp_buffer
- + __alignof__ (struct hostent_data)
- - 1)
- & ~(__alignof__ (struct hostent_data)
- - 1));
- goto again;
- }
- }
- else
+ /* internal_getent returned an error. */
+ break;
+ } /* while (true) */
+
+ /* Propagate the NSS_STATUS_TRYAGAIN error to the caller. It means
+ that we may not have loaded the complete result.
+ NSS_STATUS_NOTFOUND, however, means that we reached the end of
+ the file successfully. */
+ if (status != NSS_STATUS_TRYAGAIN)
status = NSS_STATUS_SUCCESS;
- out:
- if (tmp_buffer_malloced)
- free (tmp_buffer);
+
+ scratch_buffer_free (&tmp_buffer);
return status;
}
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] nss_files: Use struct scratch_buffer for gethostbyname [BZ #18023]
2017-10-10 13:02 ` Florian Weimer
@ 2017-10-11 5:04 ` Florian Weimer
2017-10-11 12:18 ` Adhemerval Zanella
0 siblings, 1 reply; 6+ messages in thread
From: Florian Weimer @ 2017-10-11 5:04 UTC (permalink / raw)
To: Adhemerval Zanella, libc-alpha
On 10/10/2017 03:01 PM, Florian Weimer wrote:
> On 09/05/2017 08:38 PM, Florian Weimer wrote:
>
>>> I do think this it is easier to read and follow the code *without*
>>> the goto,
>>> something like:
>>>
>>> scratch_buffer_init (...);
>>> while (1)
>>> Â Â {
>>> Â Â Â Â while ((status = internal_getent (...)) == NSS_STATUS_SUCCESS)
>>> Â Â Â Â Â Â {
>>> Â Â Â Â Â Â Â Â ...
>>> Â Â Â Â Â Â }
>>> Â Â Â Â if (status == NSS_STATUS_TRYAGAIN)
>>> Â Â Â Â Â Â if (!scratch_buffer_grow (&tmp_buffer))
>>> Â Â Â Â Â Â Â Â {
>>> Â Â Â Â Â Â Â Â Â Â *herrnop = NETDB_INTERNAL;
>>> Â Â Â Â Â Â Â Â Â Â status = NSS_STATUS_TRYAGAIN;
>>> Â Â Â Â Â Â Â Â Â Â break;
>>> Â Â Â Â Â Â Â Â }
>>> Â Â Â Â else
>>> Â Â Â Â Â Â status = NSS_STATUS_SUCCESS;
>>> Â Â }
>>> scratch_buffer_free (...);
>>
>> Right, I think I'll make this change in the first (refactoring) patch.
>
> I made this change in this patch instead. Still okay?
I'm going to push this because you've already acked the subsequent patch.
Thanks,
Florian
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] nss_files: Use struct scratch_buffer for gethostbyname [BZ #18023]
2017-10-11 5:04 ` Florian Weimer
@ 2017-10-11 12:18 ` Adhemerval Zanella
0 siblings, 0 replies; 6+ messages in thread
From: Adhemerval Zanella @ 2017-10-11 12:18 UTC (permalink / raw)
To: Florian Weimer, libc-alpha
On 11/10/2017 02:04, Florian Weimer wrote:
> On 10/10/2017 03:01 PM, Florian Weimer wrote:
>> On 09/05/2017 08:38 PM, Florian Weimer wrote:
>>
>>>> I do think this it is easier to read and follow the code *without* the goto,
>>>> something like:
>>>>
>>>> scratch_buffer_init (...);
>>>> while (1)
>>>> Â Â {
>>>> Â Â Â Â while ((status = internal_getent (...)) == NSS_STATUS_SUCCESS)
>>>> Â Â Â Â Â Â {
>>>> Â Â Â Â Â Â Â Â ...
>>>> Â Â Â Â Â Â }
>>>> Â Â Â Â if (status == NSS_STATUS_TRYAGAIN)
>>>> Â Â Â Â Â Â if (!scratch_buffer_grow (&tmp_buffer))
>>>> Â Â Â Â Â Â Â Â {
>>>> Â Â Â Â Â Â Â Â Â Â *herrnop = NETDB_INTERNAL;
>>>> Â Â Â Â Â Â Â Â Â Â status = NSS_STATUS_TRYAGAIN;
>>>> Â Â Â Â Â Â Â Â Â Â break;
>>>> Â Â Â Â Â Â Â Â }
>>>> Â Â Â Â else
>>>> Â Â Â Â Â Â status = NSS_STATUS_SUCCESS;
>>>> Â Â }
>>>> scratch_buffer_free (...);
>>>
>>> Right, I think I'll make this change in the first (refactoring) patch.
>>
>> I made this change in this patch instead. Still okay?
>
> I'm going to push this because you've already acked the subsequent patch.
LGTM, I though I had acked this patch as well (I did spend some time yesterday
checking the new version).
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2017-10-11 12:18 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-09-04 17:31 [PATCH] nss_files: Use struct scratch_buffer for gethostbyname [BZ #18023] Florian Weimer
2017-09-05 17:53 ` Adhemerval Zanella
2017-09-05 18:38 ` Florian Weimer
2017-10-10 13:02 ` Florian Weimer
2017-10-11 5:04 ` Florian Weimer
2017-10-11 12:18 ` Adhemerval Zanella
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).