| Message ID | 1683623648-22778-4-git-send-email-xuyang2018.jy@fujitsu.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [v5,1/7] include/lapi: Move AT_* related macros to fcntl header | expand |
Hi Yang, On Tue, May 09, 2023 at 05:14:05PM +0800, Yang Xu wrote: > Before this patch, we test stx_mnt_id only when glibc's statx struct has > this member. Now, if glibc miss this filed, we will use __spare2[0], see > url[1]. If glibc miss statx struct, we will use ltp owner definition. I don't think this is the right approach either. Actually, this new proposal is arguably worse than leaving the problem unsolved. The problem with this new proposal is that the fields in struct statx whose names are prefixed with "__spare" were never intended to be used directly. Their names can change from one kernel version to the next, and they can change to a different offset and/or size while keeping the same name. That can result in breakages that only reproduce on very specific versions of <sys/stat.h>. As I've said several times, the proper way to ensure that the tests can be built even when the system struct statx doesn't contain all the needed fields is to use the LTP struct statx whenever the system one does not fully suffice. One way to do that would be to make the tests use the struct via a different name, such as struct ltp_statx, and define that to either the system statx or the LTP statx depending on which one has the right definition. It might also be possible to #define statx to achieve the same effect (though maybe that would cause a collision with the function statx()...) Anyway, I don't want to derail the STATX_DIOALIGN test too much, so if you're having a lot of trouble solving this problem properly, I'm okay with just adding the STATX_DIOALIGN test without it solved for now... - Eric
Hi Eric > Hi Yang, > > On Tue, May 09, 2023 at 05:14:05PM +0800, Yang Xu wrote: >> Before this patch, we test stx_mnt_id only when glibc's statx struct has >> this member. Now, if glibc miss this filed, we will use __spare2[0], see >> url[1]. If glibc miss statx struct, we will use ltp owner definition. > > I don't think this is the right approach either. Actually, this new proposal is > arguably worse than leaving the problem unsolved. The problem with this new > proposal is that the fields in struct statx whose names are prefixed with > "__spare" were never intended to be used directly. Their names can change from > one kernel version to the next, and they can change to a different offset and/or > size while keeping the same name. That can result in breakages that only > reproduce on very specific versions of <sys/stat.h>. > Ok, understood. > As I've said several times, the proper way to ensure that the tests can be built > even when the system struct statx doesn't contain all the needed fields is to > use the LTP struct statx whenever the system one does not fully suffice. > > One way to do that would be to make the tests use the struct via a different > name, such as struct ltp_statx, and define that to either the system statx or > the LTP statx depending on which one has the right definition. Yes, use ltp_statx struct is a better sloution. > > It might also be possible to #define statx to achieve the same effect (though > maybe that would cause a collision with the function statx()...) Indeed, it will report warning as below: note: expected ‘struct statx * restrict’ but argument is of type ‘struct ltp_statx *’\ I change code as below: +#if !defined(HAVE_STRUCT_STATX) || \ + !defined(HAVE_STRUCT_STATX_MNT_ID) || \ + !defined(HAVE_STRUCT_STATX_STX_DIO_MEM_ALIGN) +struct ltp_statx { /* 0x00 */ uint32_t stx_mask; uint32_t stx_blksize; @@ -102,6 +104,8 @@ struct statx { uint64_t __spare3[12]; /* 0x100 */ }; +#else +#define ltp_statx statx; > > Anyway, I don't want to derail the STATX_DIOALIGN test too much, so if you're > having a lot of trouble solving this problem properly, I'm okay with just > adding the STATX_DIOALIGN test without it solved for now... In fact, I just want to leave run or not run this case on glibc header ie I do in testing mnt_id[1]. It is simple and clean alough it will miss some overage if using a custom new kernel and old glibc. In other situation(ie various offical linux distributions), it should work well. So I will send a v6 that simply just use #ifndef HAVE_STRUCT_STATX_STX_DIO_MEM_ALIGN. [1]https://github.com/linux-test-project/ltp/commit/0578cfb3dc175426aff516025b3ca76103e5f551 Best Regards Yang Xu > > - Eric
diff --git a/testcases/kernel/syscalls/statx/statx01.c b/testcases/kernel/syscalls/statx/statx01.c index f9c2748d2..08b26f77b 100644 --- a/testcases/kernel/syscalls/statx/statx01.c +++ b/testcases/kernel/syscalls/statx/statx01.c @@ -49,20 +49,25 @@ static int file_fd = -1; -#ifdef HAVE_STRUCT_STATX_STX_MNT_ID static void test_mnt_id(struct statx *buf) { FILE *file; char line[PATH_MAX]; int pid, flag = 0; unsigned int line_mjr, line_mnr; - uint64_t mnt_id; + uint64_t mnt_id, stx_mnt_id; if (!(buf->stx_mask & STATX_MNT_ID)) { tst_res(TCONF, "stx_mnt_id is not supported until linux 5.8"); return; } +#if defined(HAVE_STRUCT_STATX) && !defined(HAVE_STRUCT_STATX_STX_MNT_ID) + stx_mnt_id = buf->__spare2[0]; +#else + stx_mnt_id = buf->stx_mnt_id; +#endif + file = SAFE_FOPEN("/proc/self/mountinfo", "r"); while (fgets(line, sizeof(line), file)) { @@ -70,12 +75,12 @@ static void test_mnt_id(struct statx *buf) continue; if (line_mjr == buf->stx_dev_major && line_mnr == buf->stx_dev_minor) { - if (buf->stx_mnt_id == mnt_id) { + if (stx_mnt_id == mnt_id) { flag = 1; break; } tst_res(TINFO, "%s doesn't contain %"PRIu64" %d:%d", - line, (uint64_t)buf->stx_mnt_id, buf->stx_dev_major, buf->stx_dev_minor); + line, (uint64_t)stx_mnt_id, buf->stx_dev_major, buf->stx_dev_minor); } } @@ -88,13 +93,12 @@ static void test_mnt_id(struct statx *buf) else tst_res(TFAIL, "statx.stx_mnt_id(%"PRIu64") doesn't exist in /proc/self/mountinfo", - (uint64_t)buf->stx_mnt_id); + (uint64_t)stx_mnt_id); pid = getpid(); snprintf(line, PATH_MAX, "/proc/%d/fdinfo/%d", pid, file_fd); - TST_ASSERT_FILE_INT(line, "mnt_id:", buf->stx_mnt_id); + TST_ASSERT_FILE_INT(line, "mnt_id:", stx_mnt_id); } -#endif static void test_normal_file(void) { @@ -147,11 +151,7 @@ static void test_normal_file(void) tst_res(TFAIL, "stx_nlink(%u) is different from expected(1)", buff.stx_nlink); -#ifdef HAVE_STRUCT_STATX_STX_MNT_ID test_mnt_id(&buff); -#else - tst_res(TCONF, "stx_mnt_id is not defined in struct statx"); -#endif } static void test_device_file(void)
Before this patch, we test stx_mnt_id only when glibc's statx struct has this member. Now, if glibc miss this filed, we will use __spare2[0], see url[1]. If glibc miss statx struct, we will use ltp owner definition. [1]https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=fa2fcf4f Signed-off-by: Yang Xu <xuyang2018.jy@fujitsu.com> --- testcases/kernel/syscalls/statx/statx01.c | 22 +++++++++++----------- 1 file changed, 11 insertions(+), 11 deletions(-)