From patchwork Thu Nov 17 21:30:38 2016 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Adhemerval Zanella Netto X-Patchwork-Id: 17556 Received: (qmail 111005 invoked by alias); 17 Nov 2016 21:30:56 -0000 Mailing-List: contact libc-alpha-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Unsubscribe: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: libc-alpha-owner@sourceware.org Delivered-To: mailing list libc-alpha@sourceware.org Received: (qmail 110992 invoked by uid 89); 17 Nov 2016 21:30:55 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-1.4 required=5.0 tests=BAYES_00, RCVD_IN_DNSWL_NONE, RCVD_IN_SORBS_SPAM, SPF_PASS autolearn=no version=3.3.2 spammy= X-HELO: mail-ua0-f171.google.com X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:subject:to:references:from:message-id:date :user-agent:mime-version:in-reply-to:content-transfer-encoding; bh=84poFe5zRb6yLD599sz7ouEf3o0VhdB64i+Psjgo/Do=; b=XLzleQcBgSV3zj7+h+kaf6OP6Euu3D2W4Axuxi0zUUvpYmzAwmiiRnKqWuDRsGMNUo /es/koQDzuOYSIsh5RIa86DRIvG9x+i4LlhTUkmtgU1YTMSTUeamLHretlfRoveeGsSr hRV3T/oex6Xsd8OFXye2HyZybAimMAp31x+K8SChysPKaYrnkaq7tW0qb2pBGW3R91gt ZS1eYb0t6khhRh2UeO6d/dWVsELSbEB/7/hKoYj3CqQQsSUyQuwvmD+9J81VFtGruK6t lAGQvXjA7a33Kyq2as4AKavjES4rDPZogYe2NsS2XYWifd1JuQYjOcCxPyoOQVbSrQVc dD0A== X-Gm-Message-State: ABUngvc2Age9Ngt3ULnYLSdb1yuyciu3zqwOBcybcJQmaNkNsbFXiyjsx/XQ34YGX3qgZZ5v X-Received: by 10.176.6.74 with SMTP id f68mr3596630uaf.37.1479418243555; Thu, 17 Nov 2016 13:30:43 -0800 (PST) Subject: Re: [PATCH 2/2] New internal function __access_noerrno To: libc-alpha@sourceware.org References: <1478797446-12213-1-git-send-email-adhemerval.zanella@linaro.org> <1478797446-12213-2-git-send-email-adhemerval.zanella@linaro.org> <083c6da8-5c67-acba-f7f0-79fb4253d7d2@gotplt.org> <23ef1354-7ade-70fe-46f0-59392798ef82@linaro.org> <60199742-d6f4-3f2a-ad38-619dcfdc7954@linaro.org> <87r369j093.fsf@Niukka.kon.iki.fi> From: Adhemerval Zanella Message-ID: Date: Thu, 17 Nov 2016 19:30:38 -0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.4.0 MIME-Version: 1.0 In-Reply-To: <87r369j093.fsf@Niukka.kon.iki.fi> Thanks for the review. On 17/11/2016 18:54, Kalle Olavi Niemitalo wrote: > Adhemerval Zanella writes: > >> +/* __access variant that does not set errno. Used in very early initialization >> + code in libc.a and ld.so. */ >> +extern __typeof (__access) __access_noerrno attribute_hidden; > > Please document what __access_noerrno is supposed to return on error. > The NaCl and Linux implementations appear to return the error code, > but the stub and Hurd implementations return -1 like access does, > so I'm not sure what you intended. > In "[PATCH 1/4] Add framework for tunables", the caller > only checks whether the result is zero or nonzero. My understanding of the tunables usage is it should returns what is current described by access minus without setting errno, i.e, on success zero is returned otherwise a value different than 0. > >> +__hurd_fail_noerrno (error_t err) > > Your __hurd_fail_noerrno just returns (err == 0 ? 0 : -1). > All the assignments to err in the switch statement are dead. > > If __access_noerrno is not required to return the error code, > then I don't think __hurd_fail_noerrno is necessary at all... > >> +static int >> +hurd_fail_noerrno (error_t err) >> +{ >> + return __hurd_fail_noerrno (err); >> +} > > ... instead, hurd_fail_noerrno could just return -1 > unconditionally, because it is not even called if err == 0. > (Perhaps then rename it, to avoid suggesting it's similar to __hurd_fail.) Alright, I removed __hurd_fail_noerrno and made hurd_fail_noerrno just return -1. > >> - return __hurd_fail (EACCES); >> + return errfunc (EACESS); > > Typo here. > > The t/faccessat branch at git://git.sv.gnu.org/hurd/glibc.git > changes this code path quite a lot. Until that branch is merged > to upstream glibc, I think your function-pointer scheme is OK, > apart from the comments above. > I didn't yet try building it though. > Below is all the fixes you proposed, if you think it is ok I will prepare a patch and commit. diff --git a/hurd/hurd.h b/hurd/hurd.h index c089d4c..ec07827 100644 --- a/hurd/hurd.h +++ b/hurd/hurd.h @@ -75,35 +75,6 @@ __hurd_fail (error_t err) errno = err; return -1; } - -_HURD_H_EXTERN_INLINE int -__hurd_fail_noerrno (error_t err) -{ - switch (err) - { - case EMACH_SEND_INVALID_DEST: - case EMIG_SERVER_DIED: - /* The server has disappeared! */ - err = EIEIO; - break; - - case KERN_NO_SPACE: - err = ENOMEM; - break; - - case KERN_INVALID_ARGUMENT: - err = EINVAL; - break; - - case 0: - return 0; - - default: - break; - } - - return -1; -} /* Basic ports and info, initialized by startup. */ diff --git a/include/unistd.h b/include/unistd.h index 6144f41..16d68a1 100644 --- a/include/unistd.h +++ b/include/unistd.h @@ -183,7 +183,8 @@ extern int __getlogin_r_loginuid (char *name, size_t namesize) # if IS_IN (rtld) || !defined SHARED /* __access variant that does not set errno. Used in very early initialization - code in libc.a and ld.so. */ + code in libc.a and ld.so. It follows access return semantics (zero for + sucess otherwise a value different than 0). */ extern __typeof (__access) __access_noerrno attribute_hidden; # endif diff --git a/sysdeps/mach/hurd/access.c b/sysdeps/mach/hurd/access.c index 93e2166..66bf7d5 100644 --- a/sysdeps/mach/hurd/access.c +++ b/sysdeps/mach/hurd/access.c @@ -31,7 +31,7 @@ hurd_fail_seterrno (error_t err) static int hurd_fail_noerrno (error_t err) { - return __hurd_fail_noerrno (err); + return -1; } static int @@ -149,7 +149,7 @@ access_common (const char *file, int type, int (*errfunc) (error_t)) if (flags & ~allowed) /* We are not allowed all the requested types of access. */ - return errfunc (EACESS); + return errfunc (EACCES); return 0; }