From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from hall.aurel32.net (hall.aurel32.net [IPv6:2001:bc8:30d7:100::1]) by sourceware.org (Postfix) with ESMTPS id 891D23858CDB for ; Tue, 1 Nov 2022 21:31:52 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 891D23858CDB Authentication-Results: sourceware.org; dmarc=none (p=none dis=none) header.from=aurel32.net Authentication-Results: sourceware.org; spf=none smtp.mailfrom=aurel32.net DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=aurel32.net ; s=202004.hall; h=In-Reply-To:Content-Type:MIME-Version:References: Message-ID:Subject:Cc:To:From:Date:Content-Transfer-Encoding:From:Reply-To: Subject:Content-ID:Content-Description:X-Debbugs-Cc; bh=rx0VeHvf/heKb8TPQ+1QN+YF9StOoWyfxLTB0YdPA6c=; b=Vfeasn5coHNcce5uwc/+Qg/TEU dFaSqvk9Y03I+kbwI3pBr8fyGzkbkGUGPlRTVIEsvpw9FgiRUV5D9oXz1HeEPOcP+qr0jD4HBRLxf COruYuUirtLLSDI/RLgpRI3JNqxGRip9f0hD1BcUctv2tGofaIiTmyW00QoHHz6mT9KDj4PIFlDef X5IUl7Dn74qn7N2PyGOF7j0W2FX0IqjJqOSKw+lkkrDLkxxGILsvKBAFfl820Pw6IOD+WRzv+Ui4k j1VmZCGGKuS+SdJunK6iDRLqCNoCSZLBkbV3wvGZ8cnRV9EGd//0rh0Iz2wM47ezGGMqkjM8LAnTy 5N95GekQ==; Received: from [2a01:e34:ec5d:a741:8a4c:7c4e:dc4c:1787] (helo=ohm.rr44.fr) by hall.aurel32.net with esmtpsa (TLS1.3) tls TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384 (Exim 4.94.2) (envelope-from ) id 1opyrP-002WgE-4G; Tue, 01 Nov 2022 22:31:51 +0100 Received: from aurel32 by ohm.rr44.fr with local (Exim 4.96) (envelope-from ) id 1opyrO-0005o9-2O; Tue, 01 Nov 2022 22:31:50 +0100 Date: Tue, 1 Nov 2022 22:31:50 +0100 From: Aurelien Jarno To: Adhemerval Zanella Netto Cc: YunQiang Su , libc-alpha@sourceware.org Subject: Re: [PATCH] linux: Fix fstatat with !XSTAT_IS_XSTAT64 and __TIMESIZE=64 (BZ #29730) Message-ID: Mail-Followup-To: Adhemerval Zanella Netto , YunQiang Su , libc-alpha@sourceware.org References: <20221028205440.1091298-1-aurelien@aurel32.net> <85bbb5fa-735e-254e-4467-e2726fd0ecc3@linaro.org> <6fe4affc-cce4-ba36-7e7d-2cbb7908a3a6@linaro.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <6fe4affc-cce4-ba36-7e7d-2cbb7908a3a6@linaro.org> User-Agent: Mutt/2.2.7 (2022-08-07) X-Spam-Status: No, score=-7.4 required=5.0 tests=BAYES_00,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,SPF_HELO_PASS,SPF_NONE,TXREP autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org List-Id: On 2022-11-01 09:40, Adhemerval Zanella Netto via Libc-alpha wrote: > > > On 01/11/22 08:57, Aurelien Jarno wrote: > > On 2022-11-01 11:55, Aurelien Jarno wrote: > >> On 2022-10-31 23:20, Aurelien Jarno wrote: > >>> On 2022-10-31 14:25, Adhemerval Zanella Netto via Libc-alpha wrote: > >>>> > >>>> > >>>> On 31/10/22 13:03, Adhemerval Zanella Netto wrote: > >>>>> > >>>>> > >>>>> On 28/10/22 17:54, Aurelien Jarno wrote: > >>>>>> Commit 6e8a0aac2f883 ("time: Fix overflow itimer tests on 32-bit > >>>>>> systems") changed in_time_t_range to assume a 32-bit time_t. Therefore > >>>>>> stop calling it from __fstatat for systems with 64-bit time_t. > >>>>>> > >>>>>> Resolves: BZ #29730 > >>>>> > >>>>> Although this change is not incorrect, I think the fix I just sent [1] is > >>>>> slight better because it just alias non-LFS stat to LFS stat for mips > >>>>> (which is also a small runtime and code optimization), while avoiding > >>>>> touching generic code (so we need to check if trigger another issue on > >>>>> different architecture). > >>>>> > >>>>> [1] https://sourceware.org/pipermail/libc-alpha/2022-October/143089.html > >>>> > >>>> So I was wrong that we can assume stat and stat64 are equal on mips. Maybe > >>>> instead of adding the fix on stat we can make it more generic and move it > >>>> to is_time_t_range (so it can potentially fix usage not only on stat code) > >>>> as my comment on https://sourceware.org/bugzilla/show_bug.cgi?id=29730 ? > >>> > >>> Yes, we can move it to is_time_t_range, but then it gets trickier to get > >>> the condition. Your comment in BZ #29730 uses TIME64_NON_DEFAULT, but it > >>> is not defined for OpenRISC, so it means it will cast to time_t. However > >>> this has been explicitly changed that way in commit 6e8a0aac2f883 to > >>> unbreak OpenRISC. > >>> > >>> We probably want a condition that is "kernel ABI uses 32-bit time_t". Do > >>> we have something like that already defined? > >> > >> I guess this can be checked with: > >> (__WORDSIZE == 32 && (!defined __SYSCALL_WORDSIZE || __SYSCALL_WORDSIZE == 32)) > >> > >> And then we can rename the function to is_ktime_t_range to better > >> reflect its behaviour. > > > > Looking at it more in details, I am not sure it's a good idea, as this > > function is also used for non-syscall cases, like in mktime or timegm. > > While that works because there is a __TIMESIZE != 64 guard, this still > > seems fragile. > > The "(__WORDSIZE == 32 && (!defined __SYSCALL_WORDSIZE || __SYSCALL_WORDSIZE == 32))" > is a Linuxism and I agree it should not be used in generic code. > > > > > In that regard, I think the approach from YunQiang to explicitly call > > either one or the other function might be better. > > > > An hybrid solution would be to define both in_time_t_range and > > in_ktime_t_range, the later being the one called in_int32_t_range in > > YunQiang's patch. > > > > What do you think? > > > > I am more inclined to either go with your initial patch that handles it on > fstatat or go with a mips specific fstatat.c to make clear about MIPS ABI > idiosyncrasies (which is something I tried to avoid, but for this specific > case seems to make sense): > > * sysdeps/unix/sysv/linux/mips/mips64/n64/fstatat.c: > > /* Different than other ABIS, mips64 has different layouts for non-LFS > and LFS struct stat. */ > int > __fstatat (int fd, const char *file, struct stat *buf, int flag) > { > struct __stat64_t64 st64; > int r = __fstatat64_time64 (fd, file, &st64, flag); > if (r == 0) > { > /* Clear internal pad and reserved fields. */ > memset (buf, 0, sizeof (*buf)); > > buf->st_dev = st64.st_dev; > buf->st_ino = st64.st_ino; > buf->st_mode = st64.st_mode; > buf->st_nlink = st64.st_nlink; > buf->st_uid = st64.st_uid; > buf->st_gid = st64.st_gid; > buf->st_rdev = st64.st_rdev; > buf->st_size = st64.st_size; > buf->st_blksize = st64.st_blksize; > buf->st_blocks = st64.st_blocks; > buf->st_atim = st64.st_atim; > buf->st_mtim = st64.st_mtim; > buf->st_ctim = st64.st_ctim; > } > return r; > } > > We can then treat the YunQiang patch as a code refactor instead. Sounds good. I have tested that solution and confirm it works fine. I have just sent a proper patch for that. -- Aurelien Jarno GPG: 4096R/1DDD8C9B aurelien@aurel32.net http://www.aurel32.net