diff mbox

getnameinfo: Do not restore errno on error

Message ID 56DEE16A.5010305@redhat.com
State New
Headers show

Commit Message

Florian Weimer March 8, 2016, 2:27 p.m. UTC
POSIX does not require it, and this behavior is not documented
in the manual page, either.

Florian

Comments

Zack Weinberg March 8, 2016, 3:14 p.m. UTC | #1
On Tue, Mar 8, 2016 at 9:27 AM, Florian Weimer <fweimer@redhat.com> wrote:
> POSIX does not require it, and this behavior is not documented
> in the manual page, either.

This might be OK in the actual error case, but you're stomping on
errno in the *non*-error case too, which, even if allowed, should be
avoided as a matter of QoI.

zw
Florian Weimer March 8, 2016, 3:20 p.m. UTC | #2
On 03/08/2016 04:14 PM, Zack Weinberg wrote:
> On Tue, Mar 8, 2016 at 9:27 AM, Florian Weimer <fweimer@redhat.com> wrote:
>> POSIX does not require it, and this behavior is not documented
>> in the manual page, either.
> 
> This might be OK in the actual error case, but you're stomping on
> errno in the *non*-error case too, which, even if allowed, should be
> avoided as a matter of QoI.

We currently do not have this as a general goal for glibc functions.  I
don't think getnameinfois special so that an exception is warranted (
(unlike, say, free).

Florian
Andreas Schwab March 8, 2016, 3:27 p.m. UTC | #3
Zack Weinberg <zackw@panix.com> writes:

> This might be OK in the actual error case, but you're stomping on
> errno in the *non*-error case too, which, even if allowed, should be
> avoided as a matter of QoI.

There are only very few functions (as required by POSIX) that preserve
errno on success.  Why does getnameinfo need to be in this list?

Andreas.
Zack Weinberg March 8, 2016, 3:33 p.m. UTC | #4
unintentional off-list reply


---------- Forwarded message ----------
From: Zack Weinberg <zackw@panix.com>
Date: Tue, Mar 8, 2016 at 10:33 AM
Subject: Re: [PATCH] getnameinfo: Do not restore errno on error
To: Florian Weimer <fweimer@redhat.com>


On Tue, Mar 8, 2016 at 10:20 AM, Florian Weimer <fweimer@redhat.com> wrote:
> On 03/08/2016 04:14 PM, Zack Weinberg wrote:
>> On Tue, Mar 8, 2016 at 9:27 AM, Florian Weimer <fweimer@redhat.com> wrote:
>>> POSIX does not require it, and this behavior is not documented
>>> in the manual page, either.
>>
>> This might be OK in the actual error case, but you're stomping on
>> errno in the *non*-error case too, which, even if allowed, should be
>> avoided as a matter of QoI.
>
> We currently do not have this as a general goal for glibc functions.

... well, maybe we *should*.

> I don't think getnameinfois special so that an exception is warranted (
> (unlike, say, free).

It's special in that it currently *does* preserve errno on success, so
taking that out is a step in the wrong direction.

zw
Florian Weimer March 8, 2016, 4:46 p.m. UTC | #5
On 03/08/2016 04:33 PM, Zack Weinberg wrote:
> On Tue, Mar 8, 2016 at 10:20 AM, Florian Weimer <fweimer@redhat.com> wrote:
>> On 03/08/2016 04:14 PM, Zack Weinberg wrote:
>>> On Tue, Mar 8, 2016 at 9:27 AM, Florian Weimer <fweimer@redhat.com> wrote:
>>>> POSIX does not require it, and this behavior is not documented
>>>> in the manual page, either.
>>>
>>> This might be OK in the actual error case, but you're stomping on
>>> errno in the *non*-error case too, which, even if allowed, should be
>>> avoided as a matter of QoI.
>>
>> We currently do not have this as a general goal for glibc functions.
> 
> ... well, maybe we *should*.

I doubt it.  We support interposition, so this will never be very
portable even within GNU.

>> I don't think getnameinfois special so that an exception is warranted (
>> (unlike, say, free).
> 
> It's special in that it currently *does* preserve errno on success, so
> taking that out is a step in the wrong direction.

Why?

I doubt applications which rely on a preserved errno value exist, and if
they do so, they are not portable, and can even break when tested with
cwrap/nss_wrapper.

Florian
Carlos O'Donell April 28, 2016, 2:55 p.m. UTC | #6
On 03/08/2016 11:46 AM, Florian Weimer wrote:
> On 03/08/2016 04:33 PM, Zack Weinberg wrote:
>> On Tue, Mar 8, 2016 at 10:20 AM, Florian Weimer <fweimer@redhat.com> wrote:
>>> On 03/08/2016 04:14 PM, Zack Weinberg wrote:
>>>> On Tue, Mar 8, 2016 at 9:27 AM, Florian Weimer <fweimer@redhat.com> wrote:
>>>>> POSIX does not require it, and this behavior is not documented
>>>>> in the manual page, either.
>>>>
>>>> This might be OK in the actual error case, but you're stomping on
>>>> errno in the *non*-error case too, which, even if allowed, should be
>>>> avoided as a matter of QoI.
>>>
>>> We currently do not have this as a general goal for glibc functions.
>>
>> ... well, maybe we *should*.
> 
> I doubt it.  We support interposition, so this will never be very
> portable even within GNU.
> 
>>> I don't think getnameinfois special so that an exception is warranted (
>>> (unlike, say, free).
>>
>> It's special in that it currently *does* preserve errno on success, so
>> taking that out is a step in the wrong direction.
> 
> Why?
> 
> I doubt applications which rely on a preserved errno value exist, and if
> they do so, they are not portable, and can even break when tested with
> cwrap/nss_wrapper.

I agree with Andreas and Florian.

POSIX (Issue 7) says:
~~~
The value of errno should only be examined when it is indicated to be 
valid by a function's return value.
~~~
(would be better if it said "must only")

Which means that after a failed call to getnameinfo with NI_NAMEREQD
which returns EAI_NONAME, the value of errno cannot be examined.

The user can't do anything useful with the value, so we have no need
to restore it.

If we were to broadly accept that all functions should not modify
errno on success and on failures that don't document it being valid,
then we would be saving and restoring errno in almost all functions.

This is an unacceptable performance burden that isn't portable.
It also isn't clear what use cases this would help developers with.

Florian's patch looks good to me.
Florian Weimer April 28, 2016, 3:50 p.m. UTC | #7
On 04/28/2016 04:55 PM, Carlos O'Donell wrote:

>> I doubt applications which rely on a preserved errno value exist, and if
>> they do so, they are not portable, and can even break when tested with
>> cwrap/nss_wrapper.
>
> I agree with Andreas and Florian.

Thanks, I've pushed the patch.  It simplifies future cleanups—I want to 
split up getnameinfo into separate functions for host and service name 
processing.

Florian
diff mbox

Patch

From 3816c2f4c8fee95fc6a18e432c1a22702fb08e3d Mon Sep 17 00:00:00 2001
Message-Id: <3816c2f4c8fee95fc6a18e432c1a22702fb08e3d.1457447223.git.fweimer@redhat.com>
From: Florian Weimer <fweimer@redhat.com>
Date: Tue, 8 Mar 2016 15:15:10 +0100
Subject: [PATCH] getnameinfo: Do not restore errno on error
To: libc-alpha@sourceware.org

POSIX does not require it, and this behavior is not documented
in the manual page, either.
---
 ChangeLog          |  5 +++++
 inet/getnameinfo.c | 12 ++----------
 2 files changed, 7 insertions(+), 10 deletions(-)

diff --git a/ChangeLog b/ChangeLog
index 2a7abbc..a3b4324 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,5 +1,10 @@ 
 2016-03-08  Florian Weimer  <fweimer@redhat.com>
 
+	* inet/getnameinfo.c (getnameinfo): Do not preserve errno on
+	error.
+
+2016-03-08  Florian Weimer  <fweimer@redhat.com>
+
 	* sunrpc/key_call.c (key_call_keyenvoy): Use int status instead of
 	union wait.  Report any non-zero exit status as error.
 
diff --git a/inet/getnameinfo.c b/inet/getnameinfo.c
index 40f67f0..9b1847b 100644
--- a/inet/getnameinfo.c
+++ b/inet/getnameinfo.c
@@ -175,7 +175,6 @@  getnameinfo (const struct sockaddr *sa, socklen_t addrlen, char *host,
 	     socklen_t hostlen, char *serv, socklen_t servlen,
 	     int flags)
 {
-  int serrno = errno;
   int herrno;
   struct hostent th;
   int ok = 0;
@@ -326,10 +325,7 @@  getnameinfo (const struct sockaddr *sa, socklen_t addrlen, char *host,
 	if (!ok)
 	  {
 	    if (flags & NI_NAMEREQD)
-	      {
-		__set_errno (serrno);
-		return EAI_NONAME;
-	      }
+	      return EAI_NONAME;
 	    else
 	      {
 		const char *c;
@@ -406,10 +402,7 @@  getnameinfo (const struct sockaddr *sa, socklen_t addrlen, char *host,
 	  };
 
 	if (flags & NI_NAMEREQD)
-	   {
-	    __set_errno (serrno);
-	    return EAI_NONAME;
-	  }
+	  return EAI_NONAME;
 
 	strncpy (host, "localhost", hostlen);
 	break;
@@ -463,7 +456,6 @@  getnameinfo (const struct sockaddr *sa, socklen_t addrlen, char *host,
     host[hostlen-1] = 0;
   if (serv && (servlen > 0))
     serv[servlen-1] = 0;
-  errno = serrno;
   return 0;
 }
 libc_hidden_def (getnameinfo)
-- 
2.4.3