getnameinfo: Do not restore errno on error
Commit Message
POSIX does not require it, and this behavior is not documented
in the manual page, either.
Florian
Comments
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
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
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.
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
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
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.
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
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(-)
@@ -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.
@@ -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