Re: [LTP] [PATCH 2/2] fcntl{34, 36}: Always use 64-bit flock struct to avoid EINVAL

[Petr Vorel <pvorel@suse.cz>]

Hi Richie,

[ Cc Khem Raj ]

> Recently we switched to using struct fcntl with _FILE_OFFSET_BITS
> instead of the transitional type fcntl64 which has been removed from
> some libcs.

Do you mean b0320456cd ("testcases: Fix largefile support") ?
Because this commit really broke both 32 bit emulation
+ (obviously) LTP on native 32bit.

Please add before merge:

Fixes: b0320456c ("testcases: Fix largefile support")

(although this needs also previous fix)

> This broke testing with 32-bit executables on a 64bit kernel when
> FILE_OFFSET_BITS was not set to 64. Because the fcntl64 syscall does
> not exist on 64 bit kernels.

> The reason we are making the syscall directly is because of old
> glibc's which don't do any conversion.

> So we now do a conversion unconditionally and call fcntl64 if the
> kernel is 32-bit.
+1.

> When we no longer support old glibcs we can drop this compat function
> altogether.
I wonder which glibc these are. And how about musl?

Anyway, thanks!

Reviewed-by: Petr Vorel <pvorel@suse.cz>
Tested-by: Petr Vorel <pvorel@suse.cz>

Few unimportant notes below.
...

> +++ b/testcases/kernel/syscalls/fcntl/fcntl_common.h
> @@ -1,38 +1,71 @@
>  #ifndef FCNTL_COMMON_H__
>  #define FCNTL_COMMON_H__

> +#include <inttypes.h>
> +
> +#include "tst_test.h"
> +#include "tst_kernel.h"
> +
>  #include "lapi/syscalls.h"
>  #include "lapi/abisize.h"
> +#include "lapi/fcntl.h"
> +
> +struct my_flock64 {
> +	int16_t l_type;
> +	int16_t l_whence;
> +	int64_t l_start;
> +	int64_t l_len;
> +	int32_t l_pid;
> +#if defined(__sparc__)
nit: #ifdef __sparc__

Well, we still support 32bit sparc (you did support for atomic in
include/tst_atomic.h), but IMHO it's dead (I might ask if John Paul Adrian
Glaubitz knows about anybody using LTP for testing on sparc).  But as this is in
kernel struct, there is no harm to keep it for LTP as well.

> +	int16_t padding;
> +#endif
> +};

...

> +static inline int fcntl_compat(const char *file, const int line, const char *cmd_name,
> +			       int fd, int cmd, struct flock *lck)
>  {
> -	int ret = tst_syscall(__NR_fcntl64, fd, cmd, lck);
> -	if (ret == -1)
> -		tst_brk(TBROK|TERRNO, "fcntl64");
> +	struct my_flock64 l64 = {
> +		.l_type = lck->l_type,
> +		.l_whence = lck->l_whence,
> +		.l_start = lck->l_start,
> +		.l_len = lck->l_len,
> +		.l_pid = lck->l_pid,
> +	};
> +	const long sysno = tst_kernel_bits() > 32 ? __NR_fcntl : __NR_fcntl64;
> +	const int ret = tst_syscall(sysno, fd, cmd, &l64);
> +
> +	lck->l_type = l64.l_type;
> +	lck->l_whence = l64.l_whence;
> +	lck->l_start = l64.l_start;
> +	lck->l_len = l64.l_len;
> +	lck->l_pid = l64.l_pid;
> +
> +	if (ret != -1)
> +		return ret;
> +
> +	tst_brk_(file, line, TBROK | TERRNO,
> +		 "%s(%d, %s, { %d, %d, %"PRId64", %"PRId64", %d })",
> +		 tst_kernel_bits() > 32 ? "fcntl" : "fcntl64",
nit: maybe cache tst_kernel_bits() to variable?

Kind regards,
Petr

> +		 fd,
> +		 cmd_name,
> +		 l64.l_type, l64.l_whence, l64.l_start, l64.l_len, l64.l_pid);
> +
>  	return ret;
>  }
> -#endif
> +
> +#define FCNTL_COMPAT(fd, cmd, flock) \
> +	fcntl_compat(__FILE__, __LINE__, #cmd, fd, cmd, flock)

>  #endif /* FCNTL_COMMON_H__ */

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp