| Message ID | 56DEE16A.5010305@redhat.com |
|---|---|
| State | New |
| Headers | show |
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(-) 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