This is the mail archive of the
libc-alpha@sourceware.org
mailing list for the glibc project.
Re: [PATCH 4/4] Consolidate posix_fadvise implementations
On 24/08/2016 00:52, Yury Norov wrote:
> On Fri, Aug 19, 2016 at 11:41:49AM -0300, Adhemerval Zanella wrote:
> Ah, forgot to notice...
>
> [...]
>
>> diff --git a/sysdeps/unix/sysv/linux/arm/kernel-features.h b/sysdeps/unix/sysv/linux/arm/kernel-features.h
>> index 6ca607e..628d27f 100644
>> --- a/sysdeps/unix/sysv/linux/arm/kernel-features.h
>> +++ b/sysdeps/unix/sysv/linux/arm/kernel-features.h
>> @@ -27,6 +27,13 @@
>> # undef __ASSUME_SET_ROBUST_LIST
>> #endif
>>
>> +/* ARM fadvise64_64 reorganize the syscall arguments. */
>> +#define __ASSUME_FADVISE64_64_6ARG 1
>
> [...]
>
>> diff --git a/sysdeps/unix/sysv/linux/posix_fadvise.c b/sysdeps/unix/sysv/linux/posix_fadvise.c
>> index 093d707..869a642 100644
>> --- a/sysdeps/unix/sysv/linux/posix_fadvise.c
>> +++ b/sysdeps/unix/sysv/linux/posix_fadvise.c
>> @@ -22,27 +22,46 @@
>> /* Advice the system about the expected behaviour of the application with
>> respect to the file associated with FD. */
>>
>> +#ifndef __OFF_T_MATCHES_OFF64_T
>> +
>> +/* Both arm and powerpc implements fadvise64_64 with last 'advise' argument
>> + just after 'fd' to avoid the requirement of implementing 7-arg syscalls.
>> + ARM also defines __NR_fadvise64_64 as __NR_arm_fadvise64_64.
>> +
>> + tile requires __ASSUME_ALIGNED_REGISTER_PAIRS but implements the 32-bit
>> + fadvise64_64 without the padding 0 after fd.
>> +
>> + s390 implements fadvice64_64 using a specific struct with arguments
>> + packed inside. This is the only implementation handled in arch-specific
>> + code. */
>> +
>> int
>> posix_fadvise (int fd, off_t offset, off_t len, int advise)
>> {
>> -#if defined(__NR_fadvise64) || defined(__NR_fadvise64_64)
>> INTERNAL_SYSCALL_DECL (err);
>> # ifdef __NR_fadvise64
>> - int ret = INTERNAL_SYSCALL (fadvise64, err, 5, fd,
>> - __LONG_LONG_PAIR (offset >> 31, offset), len,
>> - advise);
>> + int ret = INTERNAL_SYSCALL_CALL (fadvise64, err, fd,
>> + __ALIGNMENT_ARG SYSCALL_LL (offset),
>> + len, advise);
>> # else
>> - int ret = INTERNAL_SYSCALL (fadvise64_64, err, 6, fd,
>> - __LONG_LONG_PAIR ((long) (offset >> 31),
>> - (long) offset),
>> - __LONG_LONG_PAIR ((long) (len >> 31),
>> - (long) len),
>> - advise);
>> +# ifdef __ASSUME_FADVISE64_64_6ARG
>
> You should define __ASSUME_FADVISE64_64_6ARG for all ports,
> and use #if instead of #ifdef.
>
> For all new options you introduce. This is coding style rule. :(
>
My understanding is this is not the general idea of kernel-features.h
current organization. It is included several times in object build and the
idea is to have linux default behaviour in default Linux file with
ports undefining non-supported features. There were some cleanup recently
in this directions by increasing the minimum kernel version supported.
So I the way to follow kernel-features conventions is to invert current
logic and add:
diff --git a/sysdeps/unix/sysv/linux/kernel-features.h b/sysdeps/unix/sysv/linux/kernel-features.h
index 1d3b554..362c2ef 100644
--- a/sysdeps/unix/sysv/linux/kernel-features.h
+++ b/sysdeps/unix/sysv/linux/kernel-features.h
@@ -147,3 +147,6 @@
separate syscalls were only added later. */
#define __ASSUME_SENDMSG_SYSCALL 1
#define __ASSUME_RECVMSG_SYSCALL 1
+
+#define __ASSUME_FADVISE64_64_DEFAULT_ARG 1
+#define __ASSUME_FADVISE64_64_ALIGN 1
And set fallocate.c to act as:
# ifdef __ASSUME_FADVISE64_64_DEFAULT_ARG
# ifndef __ASSUME_FADVISE64_64_ALIGN
# undef __ALIGNMENT_ARG
# define __ALIGNMENT_ARG
# endif
int ret = INTERNAL_SYSCALL_CALL (fadvise64_64, err, fd,
__ALIGNMENT_ARG SYSCALL_LL (offset),
SYSCALL_LL (len), advise);
# else
int ret = INTERNAL_SYSCALL_CALL (fadvise64_64, err, fd, advise,
__ALIGNMENT_ARG SYSCALL_LL (offset),
SYSCALL_LL (len));
# endif
I would strongly prefer to *not* have to define two new define on every
kernel-features.h and even create extra files for ports that do not
require such header (as for aarch64).