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

[Richard Palethorpe <rpalethorpe@suse.de>]

Hello,

Petr Vorel <pvorel@suse.cz> writes:

> 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")

I suppose, thanks. I'm not sure how much fixes tags help in LTP? In
kernel they are used in automatic backporting and such.

>
> (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?

I find it difficult to tell honestly. Pre 2.28 perhaps which is just
what "git describe --contains" suggests.

Muslc appears to always use 64-bit fcntl.

>
> Anyway, thanks!
>
> Reviewed-by: Petr Vorel <pvorel@suse.cz>
> Tested-by: Petr Vorel <pvorel@suse.cz>

Will merge, thanks!

>
> 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.

Yes, my main thinking was that it is easy to include and I'm not sure if
__sparc__ also gets set on sparc64 or whatever is actually in use.

>
>> +	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?

The function already does it.

>
> 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__ */


-- 
Thank you,
Richard.

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