From mboxrd@z Thu Jan 1 00:00:00 1970 X-Spam-Checker-Version: SpamAssassin 3.4.4 (2020-01-24) on inbox.vuxu.org X-Spam-Level: X-Spam-Status: No, score=-3.3 required=5.0 tests=MAILING_LIST_MULTI, RCVD_IN_DNSWL_MED,RCVD_IN_MSPIKE_H3,RCVD_IN_MSPIKE_WL autolearn=ham autolearn_force=no version=3.4.4 Received: (qmail 24513 invoked from network); 9 Sep 2020 06:08:04 -0000 Received: from mother.openwall.net (195.42.179.200) by inbox.vuxu.org with ESMTPUTF8; 9 Sep 2020 06:08:04 -0000 Received: (qmail 29821 invoked by uid 550); 9 Sep 2020 06:08:01 -0000 Mailing-List: contact musl-help@lists.openwall.com; run by ezmlm Precedence: bulk List-Post: List-Help: List-Unsubscribe: List-Subscribe: List-ID: Reply-To: musl@lists.openwall.com Received: (qmail 29803 invoked from network); 9 Sep 2020 06:08:00 -0000 Date: Wed, 9 Sep 2020 02:07:47 -0400 From: Rich Felker To: musl@lists.openwall.com Message-ID: <20200909060747.GY3265@brightrain.aerifal.cx> References: <20200908015456.GT3265@brightrain.aerifal.cx> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20200908015456.GT3265@brightrain.aerifal.cx> User-Agent: Mutt/1.5.21 (2010-09-15) Subject: Re: [musl] riscv32 v2 On Mon, Sep 07, 2020 at 09:54:56PM -0400, Rich Felker wrote: > On Fri, Sep 04, 2020 at 01:48:19AM -0400, Stefan O'Rear wrote: > > From cd57a6b47783c5302f931e543b608cb3ba58387d Mon Sep 17 00:00:00 2001 > > From: Stefan O'Rear > > Date: Thu, 3 Sep 2020 03:59:59 -0400 > > Subject: [PATCH 06/14] Only call fstatat if defined > > > > riscv32 and future architectures lack it. > > --- > > src/stat/fchmodat.c | 23 ++++++++++++++++++++--- > > src/stat/fstatat.c | 6 ++++++ > > src/stdio/tempnam.c | 9 +++++++-- > > src/stdio/tmpnam.c | 9 +++++++-- > > src/time/__map_file.c | 19 +++++++++++++++---- > > 5 files changed, 55 insertions(+), 11 deletions(-) > > > > diff --git a/src/stat/fchmodat.c b/src/stat/fchmodat.c > > index 4ee00b0a..857e84e5 100644 > > --- a/src/stat/fchmodat.c > > +++ b/src/stat/fchmodat.c > > @@ -1,8 +1,10 @@ > > #include > > #include > > #include > > +#include > > #include "syscall.h" > > #include "kstat.h" > > +#include "statx.h" > > > > int fchmodat(int fd, const char *path, mode_t mode, int flag) > > { > > @@ -11,13 +13,22 @@ int fchmodat(int fd, const char *path, mode_t mode, int flag) > > if (flag != AT_SYMLINK_NOFOLLOW) > > return __syscall_ret(-EINVAL); > > > > - struct kstat st; > > int ret, fd2; > > char proc[15+3*sizeof(int)]; > > > > +#ifdef SYS_fstatat > > + struct kstat st; > > if ((ret = __syscall(SYS_fstatat, fd, path, &st, flag))) > > return __syscall_ret(ret); > > - if (S_ISLNK(st.st_mode)) > > + mode_t get_mode = st.st_mode; > > +#else > > + struct statx st; > > + if ((ret = __syscall(SYS_statx, fd, path, flag, STATX_TYPE, &st))) > > + return __syscall_ret(ret); > > + mode_t get_mode = st.stx_mode; > > +#endif > > + > > + if (S_ISLNK(get_mode)) > > return __syscall_ret(-EOPNOTSUPP); > > > > if ((fd2 = __syscall(SYS_openat, fd, path, O_RDONLY|O_PATH|O_NOFOLLOW|O_NOCTTY|O_CLOEXEC)) < 0) { > > @@ -27,9 +38,15 @@ int fchmodat(int fd, const char *path, mode_t mode, int flag) > > } > > > > __procfdname(proc, fd2); > > +#ifdef SYS_fstatat > > ret = __syscall(SYS_fstatat, AT_FDCWD, proc, &st, 0); > > + get_mode = st.st_mode; > > +#else > > + ret = __syscall(SYS_statx, AT_FDCWD, proc, 0, STATX_TYPE, &st); > > + get_mode = st.stx_mode; > > +#endif > > if (!ret) { > > - if (S_ISLNK(st.st_mode)) ret = -EOPNOTSUPP; > > + if (S_ISLNK(get_mode)) ret = -EOPNOTSUPP; > > else ret = __syscall(SYS_fchmodat, AT_FDCWD, proc, mode); > > } > > > > I was just looking at this file for another reason, and wondered why > we're not just calling the fstatat function here. There's no namespace > reason not to. According to the description of commit > c9ebff4736128186121424364c1c62224b02aee3, use of struct kstat here was > done "as a low-risk fix" right before release of 1.2.0, and I probably > had in mind changing it to fstatat later and just never got around to > it. > > The same could be done for tempnam (POSIX) but not tmpnam (C) or > __map_file (used in plain C interfaces like setlocale and time > functions) without adding a namespace-safe alias for fstatat. > > Of course this does pull in more code that's not needed, so maybe your > version of the change is best, and maybe this is what I had in mind > when I hesitated to make the bigger change back in February. I don't > like that knowledge of different syscall alternatives leaks all over > the place, but I also don't like increasing code size unnecessarily on > all archs... Not something that requires action, but possible streamlining I just discovered: in places where lstat is being used to probe file existence (tmpnam, etc.), SYS_readlink also works, yields smaller code, and is #ifdef-free. You get the same ENOENT for nonexistence, and success or EINVAL indicating existence as a symlink or non-symlink, respectively. Rich