diff mbox series

[2/3] inet: Remove strdupa from nrl_domainname()

Message ID 20211110185832.1931688-3-adhemerval.zanella@linaro.org
State Superseded
Headers
Series Fixes for getnameinfo() with NI_NOFQDN |

Checks

Context Check Description
dj/TryBot-apply_patch success Patch applied to master at the time it was sent

Commit Message

Adhemerval Zanella Nov. 10, 2021, 6:58 p.m. UTC 
  We can use the already in place scratch_buffer.

Checked on x86_64-linux-gnu.
---
 inet/getnameinfo.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

Comments

Florian Weimer Nov. 11, 2021, 8:18 a.m. UTC | #1 
* Adhemerval Zanella via Libc-alpha:

> We can use the already in place scratch_buffer.
>
> Checked on x86_64-linux-gnu.
> ---
>  inet/getnameinfo.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/inet/getnameinfo.c b/inet/getnameinfo.c
> index 58ebbb1154..69a94604bd 100644
> --- a/inet/getnameinfo.c
> +++ b/inet/getnameinfo.c
> @@ -127,10 +127,10 @@ nrl_domainname (void)
>  	  else
>  	    {
>  	      /* We need to preserve the hostname.  */
> -	      const char *hstname = strdupa (tmpbuf.data);
> -	      while (__gethostbyname_r (hstname, &th,
> -					tmpbuf.data,
> -					tmpbuf.length,
> +	      size_t hstnamelen = strlen (tmpbuf.data) + 1;
> +	      while (__gethostbyname_r (tmpbuf.data, &th,
> +					tmpbuf.data + hstnamelen,
> +					tmpbuf.length - hstnamelen,
>  					&h, &herror))
>  		{
>  		  if (herror == NETDB_INTERNAL && errno == ERANGE)

Can you use malloc instead?  scratch_buffer_grow_preserve is a bit of an
outlier in the interface.

Thanks,
Florian
Adhemerval Zanella Nov. 11, 2021, 1:36 p.m. UTC | #2 
On 11/11/2021 05:18, Florian Weimer wrote:
> * Adhemerval Zanella via Libc-alpha:
> 
>> We can use the already in place scratch_buffer.
>>
>> Checked on x86_64-linux-gnu.
>> ---
>>  inet/getnameinfo.c | 8 ++++----
>>  1 file changed, 4 insertions(+), 4 deletions(-)
>>
>> diff --git a/inet/getnameinfo.c b/inet/getnameinfo.c
>> index 58ebbb1154..69a94604bd 100644
>> --- a/inet/getnameinfo.c
>> +++ b/inet/getnameinfo.c
>> @@ -127,10 +127,10 @@ nrl_domainname (void)
>>  	  else
>>  	    {
>>  	      /* We need to preserve the hostname.  */
>> -	      const char *hstname = strdupa (tmpbuf.data);
>> -	      while (__gethostbyname_r (hstname, &th,
>> -					tmpbuf.data,
>> -					tmpbuf.length,
>> +	      size_t hstnamelen = strlen (tmpbuf.data) + 1;
>> +	      while (__gethostbyname_r (tmpbuf.data, &th,
>> +					tmpbuf.data + hstnamelen,
>> +					tmpbuf.length - hstnamelen,
>>  					&h, &herror))
>>  		{
>>  		  if (herror == NETDB_INTERNAL && errno == ERANGE)
> 
> Can you use malloc instead?  scratch_buffer_grow_preserve is a bit of an
> outlier in the interface.

It would require to add more complexity, since now we have two allocations
to handle (the hstname and the scratch_buffer) and on fast patch it would
actually require a malloc call, where mostly likely it would be done in
the stack fror scratch_buffer.  I am not sure it is really an improvement
here.
Florian Weimer Nov. 11, 2021, 2 p.m. UTC | #3 
* Adhemerval Zanella:

> On 11/11/2021 05:18, Florian Weimer wrote:
>> * Adhemerval Zanella via Libc-alpha:
>> 
>>> We can use the already in place scratch_buffer.
>>>
>>> Checked on x86_64-linux-gnu.
>>> ---
>>>  inet/getnameinfo.c | 8 ++++----
>>>  1 file changed, 4 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/inet/getnameinfo.c b/inet/getnameinfo.c
>>> index 58ebbb1154..69a94604bd 100644
>>> --- a/inet/getnameinfo.c
>>> +++ b/inet/getnameinfo.c
>>> @@ -127,10 +127,10 @@ nrl_domainname (void)
>>>  	  else
>>>  	    {
>>>  	      /* We need to preserve the hostname.  */
>>> -	      const char *hstname = strdupa (tmpbuf.data);
>>> -	      while (__gethostbyname_r (hstname, &th,
>>> -					tmpbuf.data,
>>> -					tmpbuf.length,
>>> +	      size_t hstnamelen = strlen (tmpbuf.data) + 1;
>>> +	      while (__gethostbyname_r (tmpbuf.data, &th,
>>> +					tmpbuf.data + hstnamelen,
>>> +					tmpbuf.length - hstnamelen,
>>>  					&h, &herror))
>>>  		{
>>>  		  if (herror == NETDB_INTERNAL && errno == ERANGE)
>> 
>> Can you use malloc instead?  scratch_buffer_grow_preserve is a bit of an
>> outlier in the interface.
>
> It would require to add more complexity, since now we have two allocations
> to handle (the hstname and the scratch_buffer) and on fast patch it would
> actually require a malloc call, where mostly likely it would be done in
> the stack fror scratch_buffer.  I am not sure it is really an improvement
> here.

Fair enough, let's keep the code simple.

Thanks,
Florian
diff mbox series

Patch

diff --git a/inet/getnameinfo.c b/inet/getnameinfo.c
index 58ebbb1154..69a94604bd 100644
--- a/inet/getnameinfo.c
+++ b/inet/getnameinfo.c
@@ -127,10 +127,10 @@  nrl_domainname (void)
 	  else
 	    {
 	      /* We need to preserve the hostname.  */
-	      const char *hstname = strdupa (tmpbuf.data);
-	      while (__gethostbyname_r (hstname, &th,
-					tmpbuf.data,
-					tmpbuf.length,
+	      size_t hstnamelen = strlen (tmpbuf.data) + 1;
+	      while (__gethostbyname_r (tmpbuf.data, &th,
+					tmpbuf.data + hstnamelen,
+					tmpbuf.length - hstnamelen,
 					&h, &herror))
 		{
 		  if (herror == NETDB_INTERNAL && errno == ERANGE)