From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [216.205.24.124]) by sourceware.org (Postfix) with ESMTP id 9B8E439A002C for ; Fri, 4 Jun 2021 19:38:10 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 9B8E439A002C Received: from mail-qt1-f200.google.com (mail-qt1-f200.google.com [209.85.160.200]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-315-WxvKKIJcM2KSHRDWc1y19Q-1; Fri, 04 Jun 2021 15:38:08 -0400 X-MC-Unique: WxvKKIJcM2KSHRDWc1y19Q-1 Received: by mail-qt1-f200.google.com with SMTP id b20-20020ac87fd40000b02901e1370c5e12so5796136qtk.17 for ; Fri, 04 Jun 2021 12:38:08 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:from:subject:to:cc:references:organization :message-id:date:user-agent:mime-version:in-reply-to :content-language:content-transfer-encoding; bh=83aYCNo2CXMWALiJ9wvCJTkDxxLbaEy6qx6vvjIUhH8=; b=dtX+YWp10DHPRDxuWAHWm8jZLFKSjbmWIa+XU3kOns638b3pyuhRz+9JK3nGZmYZ3e MKYDZGurJG3pNPWyp5SJ+l4Xm4kmCFqANTkFZ5mKrgSZcClga5k5W7K7czqJ7+KMh4UM rruv81c9cOnr8guhgiMhga/ZsRP5UANq0nB/zbosWyLiWMZDEX+VEGjntR2qGpBF676P /DE0VP7MBwkQHWgI70E7R/nXsu0/UUyB+Gr2cMj2qKaUNSjZdZHfta02qFD6py9o0tTO zTPPYV/2eukAkcdx7AsB/iK0j7ezEGFRoljpMgx9H2Les0QeL49K/PlSPPMvRdtbZW3Y YAYQ== X-Gm-Message-State: AOAM530xprVUzSI9aH7Dcu3XRJc1y5E+6+Tnae36ipEQe127W3Zj6wUy aYg3BLF4nx1IPTV5EHo6I9iKGTgWVYB7vbyVV2lgOlxjfpxd/KJu02yhDJwBe+9bOf5vICJ4RWC npao3MtvOdUqB37WDbgNa X-Received: by 2002:a05:620a:14b5:: with SMTP id x21mr5801426qkj.336.1622835488155; Fri, 04 Jun 2021 12:38:08 -0700 (PDT) X-Google-Smtp-Source: ABdhPJyPauQnztn29gA6hFFN+AxHZLaQA0aut/+F3JN3Qp0wMi3MnrzkMPAWVGuZ0M6szu0/bjdMUw== X-Received: by 2002:a05:620a:14b5:: with SMTP id x21mr5801395qkj.336.1622835487717; Fri, 04 Jun 2021 12:38:07 -0700 (PDT) Received: from [192.168.1.16] (198-84-214-74.cpe.teksavvy.com. [198.84.214.74]) by smtp.gmail.com with ESMTPSA id d6sm4656385qkf.109.2021.06.04.12.38.06 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Fri, 04 Jun 2021 12:38:07 -0700 (PDT) From: Carlos O'Donell Subject: Re: [PATCH v2 14/25] y2038: Use a common definition for msqid_ds To: Adhemerval Zanella , libc-alpha@sourceware.org References: <20210518205613.1487824-1-adhemerval.zanella@linaro.org> <20210518205613.1487824-15-adhemerval.zanella@linaro.org> Organization: Red Hat Message-ID: Date: Fri, 4 Jun 2021 15:38:06 -0400 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.10.1 MIME-Version: 1.0 In-Reply-To: <20210518205613.1487824-15-adhemerval.zanella@linaro.org> X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit X-Spam-Status: No, score=-12.2 required=5.0 tests=BAYES_00, DKIMWL_WL_HIGH, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, GIT_PATCH_0, KAM_SHORT, NICE_REPLY_A, RCVD_IN_DNSWL_LOW, RCVD_IN_MSPIKE_H4, RCVD_IN_MSPIKE_WL, 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: Fri, 04 Jun 2021 19:38:13 -0000 On 5/18/21 4:56 PM, Adhemerval Zanella wrote: > From: Lukasz Majewski > > Instead of replicate the same definitions from struct_msqid64_ds.h > on the multiple struct_msqid_ds.h, use a common header which is included > when required (struct_msqid64_ds_helper.h). > > 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. What I would like to see is that __glibc_reserved4 and __glibc_reserved5 are *preserved* for both ABIs in the dual-time case. The kernel for msqid still has the following: 26 struct msqid64_ds { 27 struct ipc64_perm msg_perm; 28 #if __BITS_PER_LONG == 64 29 long msg_stime; /* last msgsnd time */ 30 long msg_rtime; /* last msgrcv time */ 31 long msg_ctime; /* last change time */ 32 #else 33 unsigned long msg_stime; /* last msgsnd time */ 34 unsigned long msg_stime_high; 35 unsigned long msg_rtime; /* last msgrcv time */ 36 unsigned long msg_rtime_high; 37 unsigned long msg_ctime; /* last change time */ 38 unsigned long msg_ctime_high; 39 #endif 40 unsigned long msg_cbytes; /* current number of bytes on queue */ 41 unsigned long msg_qnum; /* number of messages in queue */ 42 unsigned long msg_qbytes; /* max number of bytes on queue */ 43 __kernel_pid_t msg_lspid; /* pid of last msgsnd */ 44 __kernel_pid_t msg_lrpid; /* last receive pid */ 45 unsigned long __unused4; 46 unsigned long __unused5; 47 }; For maximum compatibility with future kernel changes we should keep the unused fields in the glibc equivalent structure. If the values are ever used then they would already be allocated as part of the size of the structure without the need for an ABI change. Given that these are somewhat legacy APIs we should be aiming for preserving existing behaviour rather than aiming for the smallest possible size structure. On i686 the new 64-bit time_t struct is only 80 bytes, while the original struct size is 88 bytes. Adding the reserved entries brings this back to 88 bytes in the new ABI. We can add static asserts if we think we need to check exactly for alignment of 64-bit time_t, but since we copy from a kernel structure I'm largely worried about the ABI being too small to cary the information that we might need to copy from the matching kernel struct. See my suggestions below for changes. > --- > sysdeps/unix/sysv/linux/Makefile | 3 +- > .../linux/bits/struct_stat_time64_helper.h | 2 +- > .../sysv/linux/bits/types/struct_msqid64_ds.h | 10 +------ > .../bits/types/struct_msqid64_ds_helper.h | 28 +++++++++++++++++++ > .../sysv/linux/bits/types/struct_msqid_ds.h | 12 ++++++-- > .../linux/hppa/bits/types/struct_msqid_ds.h | 12 ++++++-- > .../linux/mips/bits/types/struct_msqid_ds.h | 18 ++++++++---- > .../powerpc/bits/types/struct_msqid_ds.h | 12 ++++++-- > .../linux/sparc/bits/types/struct_msqid_ds.h | 12 ++++++-- > 9 files changed, 80 insertions(+), 29 deletions(-) > create mode 100644 sysdeps/unix/sysv/linux/bits/types/struct_msqid64_ds_helper.h > > diff --git a/sysdeps/unix/sysv/linux/Makefile b/sysdeps/unix/sysv/linux/Makefile > index f83f147ed1..193b7c46b9 100644 > --- a/sysdeps/unix/sysv/linux/Makefile > +++ b/sysdeps/unix/sysv/linux/Makefile > @@ -101,7 +101,8 @@ sysdep_headers += sys/mount.h sys/acct.h \ > bits/types/struct_shmid_ds.h \ > bits/ipc-perm.h \ > bits/struct_stat.h \ > - bits/struct_stat_time64_helper.h > + bits/struct_stat_time64_helper.h \ > + bits/types/struct_msqid64_ds_helper.h OK. Create new helper. > > tests += tst-clone tst-clone2 tst-clone3 tst-fanotify tst-personality \ > tst-quota tst-sync_file_range tst-sysconf-iov_max tst-ttyname \ > diff --git a/sysdeps/unix/sysv/linux/bits/struct_stat_time64_helper.h b/sysdeps/unix/sysv/linux/bits/struct_stat_time64_helper.h > index 77385e9073..b37056f5c4 100644 > --- a/sysdeps/unix/sysv/linux/bits/struct_stat_time64_helper.h > +++ b/sysdeps/unix/sysv/linux/bits/struct_stat_time64_helper.h > @@ -1,4 +1,4 @@ > -/* Definition for helper to define struct stat with 64 bit time. > +/* Common defitions for struct stat with 64 bit time. This is a spurious cleanup from the last patch IMO. If possible please move it into the previous commit. s/defitions/definitions/g > Copyright (C) 2021 Free Software Foundation, Inc. > This file is part of the GNU C Library. > > diff --git a/sysdeps/unix/sysv/linux/bits/types/struct_msqid64_ds.h b/sysdeps/unix/sysv/linux/bits/types/struct_msqid64_ds.h > index 43e8cd7cfc..992734914a 100644 > --- a/sysdeps/unix/sysv/linux/bits/types/struct_msqid64_ds.h > +++ b/sysdeps/unix/sysv/linux/bits/types/struct_msqid64_ds.h > @@ -25,14 +25,6 @@ > #else > struct __msqid64_ds > { > - struct ipc_perm msg_perm; /* structure describing operation permission */ > - __time64_t msg_stime; /* time of last msgsnd command */ > - __time64_t msg_rtime; /* time of last msgsnd command */ > - __time64_t msg_ctime; /* time of last change */ > - __syscall_ulong_t __msg_cbytes; /* current number of bytes on queue */ > - msgqnum_t msg_qnum; /* number of messages currently on queue */ > - msglen_t msg_qbytes; /* max number of bytes allowed on queue */ > - __pid_t msg_lspid; /* pid of last msgsnd() */ > - __pid_t msg_lrpid; /* pid of last msgrcv() */ > +# include OK. Used unconditionally since this is 64-bit time_t. > }; > #endif > diff --git a/sysdeps/unix/sysv/linux/bits/types/struct_msqid64_ds_helper.h b/sysdeps/unix/sysv/linux/bits/types/struct_msqid64_ds_helper.h > new file mode 100644 > index 0000000000..02dfddaa2b > --- /dev/null > +++ b/sysdeps/unix/sysv/linux/bits/types/struct_msqid64_ds_helper.h > @@ -0,0 +1,28 @@ > +/* Common defintions for struct msqid_ds with 64 bit time. s/defintions/definitions/g > + Copyright (C) 2021 Free Software Foundation, Inc. > + This file is part of the GNU C Library. > + > + The GNU C Library is free software; you can redistribute it and/or > + modify it under the terms of the GNU Lesser General Public > + License as published by the Free Software Foundation; either > + version 2.1 of the License, or (at your option) any later version. > + > + The GNU C Library is distributed in the hope that it will be useful, > + but WITHOUT ANY WARRANTY; without even the implied warranty of > + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU > + Lesser General Public License for more details. > + > + You should have received a copy of the GNU Lesser General Public > + License along with the GNU C Library. If not, see > + . */ > + Reviewing... > + /* Content of internal __msqid64_ds. */ > + struct ipc_perm msg_perm; /* structure describing operation permission */ OK. > + __time64_t msg_stime; /* time of last msgsnd command */ > + __time64_t msg_rtime; /* time of last msgsnd command */ > + __time64_t msg_ctime; /* time of last change */ OK. Usually a tuple of * and *_high (endian swapping for order applied). > + __syscall_ulong_t __msg_cbytes; /* current number of bytes on queue */ OK. > + msgqnum_t msg_qnum; /* number of messages currently on queue */ OK. > + msglen_t msg_qbytes; /* max number of bytes allowed on queue */ OK. > + __pid_t msg_lspid; /* pid of last msgsnd() */ > + __pid_t msg_lrpid; /* pid of last msgrcv() */ OK. Add __glibc_reserved4/5 entries here. > diff --git a/sysdeps/unix/sysv/linux/bits/types/struct_msqid_ds.h b/sysdeps/unix/sysv/linux/bits/types/struct_msqid_ds.h > index 1ed041ae30..ae10a48452 100644 > --- a/sysdeps/unix/sysv/linux/bits/types/struct_msqid_ds.h > +++ b/sysdeps/unix/sysv/linux/bits/types/struct_msqid_ds.h > @@ -20,23 +20,28 @@ > # error "Never use directly; include instead." > #endif > > +#include > + > /* Structure of record for one message inside the kernel. > The type `struct msg' is opaque. */ > struct msqid_ds > { > +#ifdef __USE_TIME_BITS64 > +# include OK. Use helper. > +#else > struct ipc_perm msg_perm; /* structure describing operation permission */ > -#if __TIMESIZE == 32 > +# if __TIMESIZE == 32 > __time_t msg_stime; /* time of last msgsnd command */ > unsigned long int __msg_stime_high; > __time_t msg_rtime; /* time of last msgsnd command */ > unsigned long int __msg_rtime_high; > __time_t msg_ctime; /* time of last change */ > unsigned long int __msg_ctime_high; > -#else > +# else > __time_t msg_stime; /* time of last msgsnd command */ > __time_t msg_rtime; /* time of last msgsnd command */ > __time_t msg_ctime; /* time of last change */ > -#endif > +# endif > __syscall_ulong_t __msg_cbytes; /* current number of bytes on queue */ > msgqnum_t msg_qnum; /* number of messages currently on queue */ > msglen_t msg_qbytes; /* max number of bytes allowed on queue */ > @@ -44,4 +49,5 @@ struct msqid_ds > __pid_t msg_lrpid; /* pid of last msgrcv() */ > __syscall_ulong_t __glibc_reserved4; > __syscall_ulong_t __glibc_reserved5; > +#endif > }; > diff --git a/sysdeps/unix/sysv/linux/hppa/bits/types/struct_msqid_ds.h b/sysdeps/unix/sysv/linux/hppa/bits/types/struct_msqid_ds.h > index d943edeb78..5b82dd7f5e 100644 > --- a/sysdeps/unix/sysv/linux/hppa/bits/types/struct_msqid_ds.h > +++ b/sysdeps/unix/sysv/linux/hppa/bits/types/struct_msqid_ds.h > @@ -20,23 +20,28 @@ > # error "Never use directly; include instead." > #endif > > +#include > + > /* Structure of record for one message inside the kernel. > The type `struct msg' is opaque. */ > struct msqid_ds > { > +#ifdef __USE_TIME_BITS64 > +# include OK. Use helper. > +#else > struct ipc_perm msg_perm; /* structure describing operation permission */ > -#if __TIMESIZE == 32 > +# if __TIMESIZE == 32 > unsigned long int __msg_stime_high; > __time_t msg_stime; /* time of last msgsnd command */ > unsigned long int __msg_rtime_high; > __time_t msg_rtime; /* time of last msgsnd command */ > unsigned long int __msg_ctime_high; > __time_t msg_ctime; /* time of last change */ > -#else > +# else > __time_t msg_stime; /* time of last msgsnd command */ > __time_t msg_rtime; /* time of last msgsnd command */ > __time_t msg_ctime; /* time of last change */ > -#endif > +# endif > __syscall_ulong_t __msg_cbytes; /* current number of bytes on queue */ > msgqnum_t msg_qnum; /* number of messages currently on queue */ > msglen_t msg_qbytes; /* max number of bytes allowed on queue */ > @@ -44,4 +49,5 @@ struct msqid_ds > __pid_t msg_lrpid; /* pid of last msgrcv() */ > __syscall_ulong_t __glibc_reserved4; > __syscall_ulong_t __glibc_reserved5; > +#endif > }; > diff --git a/sysdeps/unix/sysv/linux/mips/bits/types/struct_msqid_ds.h b/sysdeps/unix/sysv/linux/mips/bits/types/struct_msqid_ds.h > index bdca5e5fe2..00c1804245 100644 > --- a/sysdeps/unix/sysv/linux/mips/bits/types/struct_msqid_ds.h > +++ b/sysdeps/unix/sysv/linux/mips/bits/types/struct_msqid_ds.h > @@ -20,32 +20,37 @@ > # error "Never use directly; include instead." > #endif > > +#include > + > /* Structure of record for one message inside the kernel. > The type `struct msg' is opaque. */ > struct msqid_ds > { > +#ifdef __USE_TIME_BITS64 > +# include OK. Use helper. > +#else > struct ipc_perm msg_perm; /* structure describing operation permission */ > -#if __TIMESIZE == 32 > -# ifdef __MIPSEL__ > +# if __TIMESIZE == 32 > +# ifdef __MIPSEL__ > __time_t msg_stime; /* time of last msgsnd command */ > unsigned long int __msg_stime_high; > __time_t msg_rtime; /* time of last msgsnd command */ > unsigned long int __msg_rtime_high; > __time_t msg_ctime; /* time of last change */ > unsigned long int __msg_ctime_high; > -# else > +# else > unsigned long int __msg_stime_high; > __time_t msg_stime; /* time of last msgsnd command */ > unsigned long int __msg_rtime_high; > __time_t msg_rtime; /* time of last msgsnd command */ > unsigned long int __msg_ctime_high; > __time_t msg_ctime; /* time of last change */ > -# endif > -#else > +# endif > +# else > __time_t msg_stime; /* time of last msgsnd command */ > __time_t msg_rtime; /* time of last msgsnd command */ > __time_t msg_ctime; /* time of last change */ > -#endif > +# endif > __syscall_ulong_t __msg_cbytes; /* current number of bytes on queue */ > msgqnum_t msg_qnum; /* number of messages currently on queue */ > msglen_t msg_qbytes; /* max number of bytes allowed on queue */ > @@ -53,4 +58,5 @@ struct msqid_ds > __pid_t msg_lrpid; /* pid of last msgrcv() */ > __syscall_ulong_t __glibc_reserved4; > __syscall_ulong_t __glibc_reserved5; > +#endif > }; > diff --git a/sysdeps/unix/sysv/linux/powerpc/bits/types/struct_msqid_ds.h b/sysdeps/unix/sysv/linux/powerpc/bits/types/struct_msqid_ds.h > index 72842ed747..8c296d2342 100644 > --- a/sysdeps/unix/sysv/linux/powerpc/bits/types/struct_msqid_ds.h > +++ b/sysdeps/unix/sysv/linux/powerpc/bits/types/struct_msqid_ds.h > @@ -20,23 +20,28 @@ > # error "Never use directly; include instead." > #endif > > +#include > + > /* Structure of record for one message inside the kernel. > The type `struct msg' is opaque. */ > struct msqid_ds > { > +#ifdef __USE_TIME_BITS64 > +# include OK. > +#else > struct ipc_perm msg_perm; /* structure describing operation permission */ > -#if __TIMESIZE == 32 > +# if __TIMESIZE == 32 > unsigned long int __msg_stime_high; > __time_t msg_stime; /* time of last msgsnd command */ > unsigned long int __msg_rtime_high; > __time_t msg_rtime; /* time of last msgsnd command */ > unsigned long int __msg_ctime_high; > __time_t msg_ctime; /* time of last change */ > -#else > +# else > __time_t msg_stime; /* time of last msgsnd command */ > __time_t msg_rtime; /* time of last msgsnd command */ > __time_t msg_ctime; /* time of last change */ > -#endif > +# endif > __syscall_ulong_t __msg_cbytes; /* current number of bytes on queue */ > msgqnum_t msg_qnum; /* number of messages currently on queue */ > msglen_t msg_qbytes; /* max number of bytes allowed on queue */ > @@ -44,4 +49,5 @@ struct msqid_ds > __pid_t msg_lrpid; /* pid of last msgrcv() */ > __syscall_ulong_t __glibc_reserved4; > __syscall_ulong_t __glibc_reserved5; > +#endif > }; > diff --git a/sysdeps/unix/sysv/linux/sparc/bits/types/struct_msqid_ds.h b/sysdeps/unix/sysv/linux/sparc/bits/types/struct_msqid_ds.h > index 22e1839de1..3c1b68ccc0 100644 > --- a/sysdeps/unix/sysv/linux/sparc/bits/types/struct_msqid_ds.h > +++ b/sysdeps/unix/sysv/linux/sparc/bits/types/struct_msqid_ds.h > @@ -20,23 +20,28 @@ > # error "Never use directly; include instead." > #endif > > +#include > + > /* Structure of record for one message inside the kernel. > The type `struct msg' is opaque. */ > struct msqid_ds > { > +#ifdef __USE_TIME_BITS64 > +# include OK. Use helper. > +#else > struct ipc_perm msg_perm; /* structure describing operation permission */ > -#if __TIMESIZE == 32 > +# if __TIMESIZE == 32 > unsigned long int __msg_stime_high; > __time_t msg_stime; /* time of last msgsnd command */ > unsigned long int __msg_rtime_high; > __time_t msg_rtime; /* time of last msgsnd command */ > unsigned long int __msg_ctime_high; > __time_t msg_ctime; /* time of last change */ > -#else > +# else > __time_t msg_stime; /* time of last msgsnd command */ > __time_t msg_rtime; /* time of last msgsnd command */ > __time_t msg_ctime; /* time of last change */ > -#endif > +# endif > __syscall_ulong_t __msg_cbytes; /* current number of bytes on queue */ > msgqnum_t msg_qnum; /* number of messages currently on queue */ > msglen_t msg_qbytes; /* max number of bytes allowed on queue */ > @@ -44,4 +49,5 @@ struct msqid_ds > __pid_t msg_lrpid; /* pid of last msgrcv() */ > __syscall_ulong_t __glibc_reserved4; > __syscall_ulong_t __glibc_reserved5; > +#endif > }; > -- Cheers, Carlos.