From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-qk1-x732.google.com (mail-qk1-x732.google.com [IPv6:2607:f8b0:4864:20::732]) by sourceware.org (Postfix) with ESMTPS id 314C4385DC22 for ; Mon, 7 Jun 2021 18:07:17 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 314C4385DC22 Received: by mail-qk1-x732.google.com with SMTP id i67so17571070qkc.4 for ; Mon, 07 Jun 2021 11:07:17 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:subject:to:cc:references:from:message-id:date :user-agent:mime-version:in-reply-to:content-language :content-transfer-encoding; bh=aXi7VCX7yhZi4l70Noy9/+W1Ljxkv7UgtusytLubE1g=; b=XLJGMSGnO2UmHCvh6stoNMtD1h3SBx5wnCSyGcLY4zM5MIU/h0dzZMZ4wwxd2Gfy0X vgt83ohovh0StUL3oqYjo02jbl5NshVusLK2SVL5Sge/5hvFI0TVCIBv2DXF3Rn32Ovd zdxelBUyv+NMi4WRIE7GtsJt9TyYnyloiPxKsS4zj+PzEv1U5moyRidiwUxJgqqt5A68 0F/um1qnxn20A5xOFB+1xg85/ImYFYg8//Xu1iaBrINAZ/xv6pmk+qNY+3PrIrI8YpRh lFvmPVfEs8CN7RwTrz6UN8X7PHD7j+yeUnCqhoMBfZwwyZz+1Cjhuf8KMe9bqTnQ5H2D w4Nw== X-Gm-Message-State: AOAM530X3Qe4VIRBK//TkwHtTQfpnSgk1/PKPPmZUsdR/P9/+U6dFkJl 3ohTU/yk/RXHC5IdZ4xxMum3qg== X-Google-Smtp-Source: ABdhPJx1am9jvZMIqZ1qY+eNEiiWB7wlztm/5fa4GumqWNPJ/fNBVpX6ZiB71f1fRNNTWNsnZPd8GQ== X-Received: by 2002:a37:b8f:: with SMTP id 137mr9995957qkl.488.1623089236672; Mon, 07 Jun 2021 11:07:16 -0700 (PDT) Received: from [192.168.1.4] ([177.194.59.218]) by smtp.gmail.com with ESMTPSA id c9sm10917882qke.8.2021.06.07.11.07.15 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Mon, 07 Jun 2021 11:07:16 -0700 (PDT) Subject: Re: [PATCH v2 13/25] y2038: Use a common definition for stat To: Carlos O'Donell , libc-alpha@sourceware.org References: <20210518205613.1487824-1-adhemerval.zanella@linaro.org> <20210518205613.1487824-14-adhemerval.zanella@linaro.org> <1febc644-6089-46b7-f70d-e039e3361b9b@redhat.com> From: Adhemerval Zanella Message-ID: Date: Mon, 7 Jun 2021 15:07:13 -0300 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.8.1 MIME-Version: 1.0 In-Reply-To: <1febc644-6089-46b7-f70d-e039e3361b9b@redhat.com> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit X-Spam-Status: No, score=-6.3 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, NICE_REPLY_A, RCVD_IN_DNSWL_NONE, SPF_HELO_NONE, SPF_PASS, TXREP autolearn=ham autolearn_force=no version=3.4.2 X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) on server2.sourceware.org X-BeenThere: libc-alpha@sourceware.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Libc-alpha mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 07 Jun 2021 18:07:26 -0000 On 04/06/2021 16:37, Carlos O'Donell wrote: > On 5/18/21 4:56 PM, Adhemerval Zanella wrote: >> From: Lukasz Majewski >> >> Instead of replicate the same definitions from struct_stat_time64.h >> on the multiple struct_stat.h, use a common header which is included >> when required (struct_stat_time64_helper.h). The 64-bit time support >> is added only for LFS support. >> >> The __USE_TIME_BITS64 is not defined internally yet, although the >> internal header is used when building the 64-bit stat implementations. > > Requesting a v3 please. > > Please add the __glibc_reserved* members to the structures to preserve > maximum compatibility with the kernel structure. > > See review of patch 14/25 for more general details. > > I am almost OK with this change as-is because in general everyone > should be using statx at this point with the ability to extend and > provide more information. > > However because the kernel contains: > > 24 struct stat { This struct more specifically is the one used on compat non-LFS syscalls, which glibc only issues for compat symbols. But even the 'struct stat64' does add the two extra unused members. > 25 unsigned long st_dev; /* Device. */ > 26 unsigned long st_ino; /* File serial number. */ > 27 unsigned int st_mode; /* File mode. */ > 28 unsigned int st_nlink; /* Link count. */ > 29 unsigned int st_uid; /* User ID of the file's owner. */ > 30 unsigned int st_gid; /* Group ID of the file's group. */ > 31 unsigned long st_rdev; /* Device number, if device. */ > 32 unsigned long __pad1; > 33 long st_size; /* Size of file, in bytes. */ > 34 int st_blksize; /* Optimal block size for I/O. */ > 35 int __pad2; > 36 long st_blocks; /* Number 512-byte blocks allocated. */ > 37 long st_atime; /* Time of last access. */ > 38 unsigned long st_atime_nsec; > 39 long st_mtime; /* Time of last modification. */ > 40 unsigned long st_mtime_nsec; > 41 long st_ctime; /* Time of last status change. */ > 42 unsigned long st_ctime_nsec; > 43 unsigned int __unused4; > 44 unsigned int __unused5; > 45 }; > > With __unused4 and __unused5. I think we should match this ABI as closely as we can > to avoid future compatibility issues with legacy APIs. > These kernel interfaces are really a mess and not really in sync across architectures. Some ABI adds the reserved fields only for non LFS, which the generic interface adds regardless. And from kernel comment the reversed fields were added to allow glibc compatibility, not really to provide further extension. In any case , I think it should not hurt to add two extra unsigned int reserved fields for 64 bit time support. > OK. Reindent. > >> }; >> #endif >> >> @@ -127,5 +135,4 @@ struct stat64 >> /* Nanosecond resolution time values are supported. */ >> #define _STATBUF_ST_NSEC >> >> - > > Please avoid spurious empty line removal when not related to the patch at hand. Ack. > >> + __blksize_t st_blksize; /* Optimal block size for I/O. */ >> + __blkcnt64_t st_blocks; /* Number 512-byte blocks allocated. */ > > OK. > > Plase add __glibc_reserved4 and __glibc_reserved5 here. Ack.