Re: [PATCH v5 00/14] port C-SKY to glibc

[Joseph Myers <joseph at codesourcery dot com>]

On Wed, 21 Nov 2018, Mao Han wrote:

> I'v modified patch 14/14 to a generic version with __NR3264_fstatat
> conditionals (in the attachment). It is tested in the same environment
> as patch V5 and got same result, but have't test fstat64 path yet.

Thanks, this is the sort of thing I'm looking for, but I'm concerned about 
the use of __NR3264_fstatat.  As I understand, that's an internal 
implementation detail of the asm-generic unistd.h; it shouldn't be 
considered something stable that will reliably be there in future kernel 
versions.  Rather, the stable interface from asm/unistd.h is the 
__NR_<syscall> macros for each <syscall> provided by the kernel.  So I 
think the #ifdef conditionals need to be on the actual syscalls used in 
each case (e.g. you'd condition on __NR_fstat64 when using fstat64 in the 
existing INLINE_SYSCALL use).  In statx_cp.c you'll need to find some stat 
syscall to condition on that's present on all existing architectures, as 
the aim is that no existing glibc architecture should get any code 
compiled from that file (which you could check using 
build-many-glibcs.py).

Also, it's a bad idea to duplicate the contents of 
sysdeps/unix/sysv/linux/fxstat64.c in 
sysdeps/unix/sysv/linux/generic/wordsize-32/fxstat64.c - rather, keep the 
existing #include, and modify sysdeps/unix/sysv/linux/fxstat64.c 
appropriately to handle the statx case (which definitely requires using 
__NR_fstat64 in the #ifdef, since this file is used in cases not using the 
asm-generic syscall list at all).

You're using AT_NO_AUTOMOUNT in these statx calls - does that match what 
the old stat syscalls do on other architectures?  (I think the aim there 
should be to match the old stat syscalls, so if those don't automount for 
stat of an automount point, then using AT_NO_AUTOMOUNT is correct.)

The indentation in the changes to sysdeps/unix/sysv/linux/Makefile seems 
to be off.  In statx_cp.c you're breaking lines after operators, but they 
should be broken before operators (and have parentheses used in such cases 
to ensure the continuation line is properly indented automatically, see 
the GNU Coding Standards).

-- 
Joseph S. Myers
joseph@codesourcery.com