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=0.3 required=5.0 tests=DKIM_ADSP_ALL,DKIM_INVALID, DKIM_SIGNED,MAILING_LIST_MULTI,RCVD_IN_MSPIKE_H2,T_SCC_BODY_TEXT_LINE autolearn=no autolearn_force=no version=3.4.4 Received: (qmail 19805 invoked from network); 27 Aug 2022 23:11:48 -0000 Received: from second.openwall.net (193.110.157.125) by inbox.vuxu.org with ESMTPUTF8; 27 Aug 2022 23:11:48 -0000 Received: (qmail 7417 invoked by uid 550); 27 Aug 2022 23:11:45 -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 7385 invoked from network); 27 Aug 2022 23:11:44 -0000 Content-Type: text/plain; charset=utf-8 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=denkimushi.com; s=dkim; t=1661641892; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=LbxjjLWcVUaHrUlXX3x10+ogupdqC2SoaEPKpIHybcE=; b=vo+ToCm+wV/AUAcXt1UBBdKm3PWTzujsHkNZ88VUSN59VaT50LZ9hoWdBWtOLxyGzMylFX 0/qmFXa6Cy1o/o7pbWJEPUzVz+Aw87movIQfQ71dZ0gRCGqbYWguah1cq8Ll1/Km5ql+JP Y9nvo5pL84w1DgJ4xh/TauZXp1Yo2zw= Content-Transfer-Encoding: quoted-printable From: Dunk MIME-Version: 1.0 (1.0) Date: Sun, 28 Aug 2022 00:11:30 +0100 Message-Id: <96BC8712-13B0-41C6-852B-ABF791B9A20A@denkimushi.com> References: <20220827181018.GA11483@brightrain.aerifal.cx> Cc: info@bnoordhuis.nl In-Reply-To: <20220827181018.GA11483@brightrain.aerifal.cx> To: musl@lists.openwall.com ARC-Seal: i=1; s=dkim; d=denkimushi.com; t=1661641892; a=rsa-sha256; cv=none; b=j/GypxEmP/swzIZYYOU251g26t3cz9nDafINDmy0PNMW8dMBUDWkCoKO3cbVYWWFiePL0A XA9r4+OzZ7y69avxAcBbLXFjD0oW/PexpYePcexkI6V0VJ51l7IE0uIAwfdl5q/aVCKUvO 8X/7gUaBzR4U0XiUbgvLVmKeGb9pIs4= ARC-Authentication-Results: i=1; ORIGINATING; auth=pass smtp.auth=dunk@denkimushi.com smtp.mailfrom=dunk@denkimushi.com ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=denkimushi.com; s=dkim; t=1661641892; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=LbxjjLWcVUaHrUlXX3x10+ogupdqC2SoaEPKpIHybcE=; b=WxSKWoqWIYa31fnWA1mYP348t+TQmSc5RIU3GJ50Ul5cfdFGORmxsjYwTIz7/CDXkDB1rR kcVCQ63mCl9+xmCsGTaKoABvdrF2EiFVmuVX/WpOsLbd7IoYJ+GXvv6krzDyBU/QZXLfAP 2hwvKdPfJk+aQ4r+xCRcMCDiD2EQXmU= Subject: Re: [musl] [PATCH 1/1] resubmitting old statx patch with changes > On 27 Aug 2022, at 19:10, Rich Felker wrote: >=20 > =EF=BB=BFOn Sat, Aug 27, 2022 at 03:57:52PM +0100, Duncan Bellamy wrote: >> --- >> include/sys/stat.h | 49 ++++++++++++++++++++++++++++++++++++++++++++++ >> src/stat/fstatat.c | 27 +------------------------ >> src/stat/statx.c | 35 +++++++++++++++++++++++++++++++++ >> 3 files changed, 85 insertions(+), 26 deletions(-) >> create mode 100644 src/stat/statx.c >>=20 >> diff --git a/include/sys/stat.h b/include/sys/stat.h >> index 10d446c4..81424462 100644 >> --- a/include/sys/stat.h >> +++ b/include/sys/stat.h >> @@ -5,6 +5,7 @@ extern "C" { >> #endif >>=20 >> #include >> +#include >=20 > This can't be done unconditionally. Either the #include directive > needs to be within the preprocessor conditional where statx is, or the > individual __NEED_uint32_t etc need to be defined conditional on the > same condition before bits/alltypes.h is included. The latter is > probably better here. I didn=E2=80=99t understand the later so did the former. >> #define __NEED_dev_t >> #define __NEED_ino_t >> @@ -70,6 +71,54 @@ extern "C" { >> #define UTIME_NOW 0x3fffffff >> #define UTIME_OMIT 0x3ffffffe >>=20 >> +#if defined(_GNU_SOURCE) >> +#define STATX_TYPE 1U >> +#define STATX_MODE 2U >> +#define STATX_NLINK 4U >> +#define STATX_UID 8U >> +#define STATX_GID 0x10U >> +#define STATX_ATIME 0x20U >> +#define STATX_MTIME 0x40U >> +#define STATX_CTIME 0x80U >> +#define STATX_INO 0x100U >> +#define STATX_SIZE 0x200U >> +#define STATX_BLOCKS 0x400U >> +#define STATX_BASIC_STATS 0x7ffU >> +#define STATX_BTIME 0x800U >> +#define STATX_ALL 0xfffU >> + >> +struct statx_timestamp { >> + int64_t tv_sec; >> + uint32_t tv_nsec; >> + int32_t __pad; >> +}; >=20 > Minor nit but this could probably just be tv_nsec, __pad (both same > type). This also eliminates the gratuitous need to expose the signed > 32-bit type which is not used elsewhere. changed > I was looking at whether *all* of the types here could be replaced > with equivalent ones that don't require exposing extra types, but the > ones documented as uint64_t probably can't. All the uint32_t in > principle could just be unsigned, and tv_sec time_t, but... >=20 >> + >> +struct statx { >> + uint32_t stx_mask; >> + uint32_t stx_blksize; >> + uint64_t stx_attributes; >> + uint32_t stx_nlink; >> + uint32_t stx_uid; >> + uint32_t stx_gid; >> + uint16_t stx_mode; >> + uint16_t __pad0[1]; >> + uint64_t stx_ino; >> + uint64_t stx_size; >> + uint64_t stx_blocks; >> + uint64_t stx_attributes_mask; >> + struct statx_timestamp stx_atime; >> + struct statx_timestamp stx_btime; >> + struct statx_timestamp stx_ctime; >> + struct statx_timestamp stx_mtime; >> + uint32_t stx_rdev_major; >> + uint32_t stx_rdev_minor; >> + uint32_t stx_dev_major; >> + uint32_t stx_dev_minor; >> + uint64_t __pad1[14]; >> +}; >=20 > stx_ino etc. should not be assuming ino_t etc. are defined the same as > uint64_t rather than something like unsigned long long. So we probably > just go with writing the types as documented... >=20 >> + >> +int statx(int, const char *__restrict, int, unsigned, struct statx *__re= strict); >> +#endif >> int stat(const char *__restrict, struct stat *__restrict); >> int fstat(int, struct stat *); >> int lstat(const char *__restrict, struct stat *__restrict); >> diff --git a/src/stat/fstatat.c b/src/stat/fstatat.c >> index 74c51cf5..5b2248a9 100644 >> --- a/src/stat/fstatat.c >> +++ b/src/stat/fstatat.c >> @@ -7,36 +7,11 @@ >> #include >> #include "syscall.h" >>=20 >> -struct statx { >> - uint32_t stx_mask; >> - uint32_t stx_blksize; >> - uint64_t stx_attributes; >> - uint32_t stx_nlink; >> - uint32_t stx_uid; >> - uint32_t stx_gid; >> - uint16_t stx_mode; >> - uint16_t pad1; >> - uint64_t stx_ino; >> - uint64_t stx_size; >> - uint64_t stx_blocks; >> - uint64_t stx_attributes_mask; >> - struct { >> - int64_t tv_sec; >> - uint32_t tv_nsec; >> - int32_t pad; >> - } stx_atime, stx_btime, stx_ctime, stx_mtime; >> - uint32_t stx_rdev_major; >> - uint32_t stx_rdev_minor; >> - uint32_t stx_dev_major; >> - uint32_t stx_dev_minor; >> - uint64_t spare[14]; >> -}; >> - >> static int fstatat_statx(int fd, const char *restrict path, struct stat *= restrict st, int flag) >> { >> struct statx stx; >>=20 >> - int ret =3D __syscall(SYS_statx, fd, path, flag, 0x7ff, &stx); >> + int ret =3D __syscall(SYS_statx, fd, path, flag, STATX_BASIC_STATS, &= stx); >> if (ret) return ret; >>=20 >> *st =3D (struct stat){ >=20 > This can be a separate change from adding statx, but it's easy to > separate when merging. moved to separate commit >> diff --git a/src/stat/statx.c b/src/stat/statx.c >> new file mode 100644 >> index 00000000..ff49841b >> --- /dev/null >> +++ b/src/stat/statx.c >> @@ -0,0 +1,35 @@ >> +#define _GNU_SOURCE >> +#include >> +#include >> +#include >> +#include >> + >> +int statx(int dirfd, const char *restrict path, int flags, unsigned mask= , struct statx *restrict stx) >> +{ >> + int ret =3D syscall(SYS_statx, dirfd, path, flags, mask, stx); >> + if (ret =3D=3D ENOSYS) { >> + struct stat st; >> + fstatat(dir_fd, path, &st, flags); >> + stx.stx_dev_major =3D major(st.st_dev); >> + stx.stx_dev_minor =3D minor(st.st_dev); >> + stx.stx_ino =3D st.st_ino; >> + stx.stx_mode =3D st.st_mode; >> + stx.stx_nlink =3D st.st_nlink; >> + stx.stx_uid =3D st.st_uid; >> + stx.stx_gid =3D st.st_gid; >> + stx.stx_size =3D st.st_size; >> + stx.stx_blksize =3D st.st_blksize; >> + stx.stx_blocks =3D st.st_blocks; >> + stx.stx_atime.tv_sec =3D st.st_atim.tv_sec; >> + stx.stx_atime.tv_nsec =3D st.st_atim.tv_nsec; >> + stx.stx_mtime.tv_sec =3D st.st_mtim.tv_sec; >> + stx.stx_mtime.tv_nsec =3D st.st_mtim.tv_nsec; >> + stx.stx_ctime.tv_sec =3D st.st_ctim.tv_sec; >> + stx.stx_ctime.tv_nsec =3D st.st_ctim.tv_nsec; >> + stx.stx_btime =3D 0; >> + stx.stx_mask =3D STATX_BASIC_STATS; >> + ret =3D EINVAL; >> + } >> + >> + return ret; >> +} >> --=20 >> 2.34.1 >=20 > I think this wasn't tested because it won't even compile (stx. instead > of stx->). It's also wrongly assuming syscall() returned a positive > error code rather than -1 on error and setting errno, but you should > be using the __syscall form that returns a negated error code, then > __syscall_ret at the end. I would write it as: >=20 > int ret =3D __syscall(SYS_statx, dirfd, path, flags, mask, stx); > if (ret !=3D -ENOSYS) return __syscall_ret(ret); >=20 > then the fallback case outside a conditional. The fallback can't > assume fstatat succeeded like you're doing either. It needs to return > -1 immediately if fstatat fails. Yes it wasn=E2=80=99t compiled, I copied the old patch and code from `fstata= t_statx` for copying the data. Compiled it locally now. >=20 > All of this code needs to be conditional on SYS_fstatat existing, as > new archs don't have it and only have SYS_statx. Changed to this > One annoying thing about this but I don't know a good fix; maybe you > have an idea: if SYS_statx fails with ENOSYS, the call to fstatat will > immediately perform SYS_statx again, only to have it fail, then > finally fall back to SYS_fstatat or other syscalls after two failures. > I'm not sure if it makes sense to expose __fstatat_kstat > libc-internally (hidden) to use here or do something else; that's > kinda getting into more complexity around this than I'd like for the > sake of optimizing old systems. I don=E2=80=99t really understand the fallback mechanism, or why statx would= fail and fstatat work on some systems. Maybe use defines at compile time s= o either one is used or the other?=20 > Rich Duncan=