Hi Tarun, This version is much better, comments are inline below. On Sat, Oct 29, 2022 at 3:14 PM Tarun Sahu wrote: > Most of libhugetlbfs test require mounted hugetlbfs and random opened > unlinked file for follow-up test actions. > > Here, this patch adds two new field in tst_test struct(include/tst_test.h) > which user can set if the test requires mounted hugetlbfs and other one > for if test requires opened unlinked file. > > Signed-off-by: Tarun Sahu > --- > include/tst_test.h | 20 +++++++++++++++++- > lib/tst_test.c | 51 +++++++++++++++++++++++++++++++++++++++++++--- > 2 files changed, 67 insertions(+), 4 deletions(-) > > diff --git a/include/tst_test.h b/include/tst_test.h > index a69965b95..f36382ae9 100644 > --- a/include/tst_test.h > +++ b/include/tst_test.h > @@ -131,7 +131,7 @@ struct tst_tag { > }; > > extern unsigned int tst_variant; > - > +extern int tst_hugetlb_fd; > #define TST_NO_HUGEPAGES ((unsigned long)-1) > > #define TST_UNLIMITED_RUNTIME (-1) > @@ -176,6 +176,18 @@ struct tst_test { > int all_filesystems:1; > int skip_in_lockdown:1; > int skip_in_compat:1; > + /* > + * If set, the test function will create a hugetlbfs mount point > + * at /tmp/xxxxxx, where xxxxxx is a random string. > + */ > + int needs_hugetlbfs:1; > + /* > + * If set, the test function will create and open a random file > + * under mounted hugetlbfs. To use this option, needs_hugetlbfs > must > + * be set. The file descriptior will be set in tst_hugetlb_fd. > + * The close(tst_hugetlb_fd) will be called on test exit(cleanup). > + */ > + int needs_unlinked_hugetlb_file:1; > Why not consider encapsulating these two new fields in 'struct tst_hugepage' ? Then the tst_test in the case can simply initialize to: .... static struct tst_test test = { .needs_root = 1, .taint_check = TST_TAINT_D | TST_TAINT_W, .setup = setup, .test_all = run_test, .hugepages = {1, TST_NEEDS, 1, 1}, }; > > /* > * The skip_filesystems is a NULL terminated list of filesystems > the > @@ -357,6 +369,12 @@ unsigned int tst_remaining_runtime(void); > */ > void tst_set_max_runtime(int max_runtime); > > +/* > + * Create and open a random file inside the given dir path. > + * It unlinks the file after opening and return file descriptor. > + */ > +int tst_create_unlinked_file(const char *path); > what about renaming this function to tst_'get|create'_unlinked_file_fd? I guess the 'fd' part should be emphasized here. > + > /* > * Returns path to the test temporary directory in a newly allocated > buffer. > */ > diff --git a/lib/tst_test.c b/lib/tst_test.c > index 8ccde1629..43cba1004 100644 > --- a/lib/tst_test.c > +++ b/lib/tst_test.c > @@ -925,7 +925,8 @@ static int needs_tmpdir(void) > tst_test->needs_device || > tst_test->mntpoint || > tst_test->resource_files || > - tst_test->needs_checkpoints; > + tst_test->needs_checkpoints || > + tst_test->needs_hugetlbfs; > } > > static void copy_resources(void) > @@ -1021,6 +1022,30 @@ static void prepare_and_mount_dev_fs(const char > *mntpoint) > } > } > > +static void prepare_and_mount_hugetlb_fs(void) > +{ > + tst_test->mntpoint = tst_get_tmpdir(); > + SAFE_MOUNT("none", tst_test->mntpoint, "hugetlbfs", 0, NULL); > + mntpoint_mounted = 1; > +} > + > +int tst_create_unlinked_file(const char *path) > +{ > + char template[PATH_MAX]; > + int fd; > + > + snprintf(template, PATH_MAX, "%s/ltp_%.3sXXXXXX", > + path, TCID); > + > + fd = mkstemp(template); > + if (fd < 0) > + tst_brk(TBROK | TERRNO, > + "%s: mkstemp(%s) failed", __func__, template); > + > + SAFE_UNLINK(template); > + return fd; > +} > + > static const char *limit_tmpfs_mount_size(const char *mnt_data, > char *buf, size_t buf_size, const char *fs_type) > { > @@ -1094,6 +1119,8 @@ static void do_cgroup_requires(void) > tst_cg_init(); > } > > +int tst_hugetlb_fd = -1; > + > static void do_setup(int argc, char *argv[]) > { > if (!tst_test) > @@ -1217,6 +1244,17 @@ static void do_setup(int argc, char *argv[]) > } > } > > + if (tst_test->needs_hugetlbfs) > + prepare_and_mount_hugetlb_fs(); > + > + if (tst_test->needs_unlinked_hugetlb_file) { > + if (!(tst_test->needs_hugetlbfs)) { > + tst_brk(TBROK, "Option needs_unlinked_hugetlb_file > " > + "requires option needs_hugetlbfs"); > + } > + tst_hugetlb_fd = > tst_create_unlinked_file(tst_test->mntpoint); > + } > + > Seems we have to add a confliction check[1] to avoid multiple mounting at 'tst_test->mntpoint'. Or maybe go another method to move all the hugetlbfs operations into tst_hugetlb.c to isolate details from the tst_test library, but this will require more changes for all preexisting hugetlb tests. [1] something like this: @@ -1224,9 +1224,9 @@ static void do_setup(int argc, char *argv[]) } if (!!tst_test->needs_rofs + !!tst_test->needs_devfs + - !!tst_test->needs_device > 1) { + !!tst_test->needs_device + !!tst_test->needs_hugetlbfs > 1) { tst_brk(TBROK, - "Two or more of needs_{rofs, devfs, device} are set"); + "Two or more of needs_{rofs, devfs, device, hugetlb} are set"); } if (tst_test->needs_device && !mntpoint_mounted) { > tdev.dev = tst_acquire_device_(NULL, > tst_test->dev_min_size); > @@ -1299,8 +1337,15 @@ static void do_cleanup(void) > if (ovl_mounted) > SAFE_UMOUNT(OVL_MNT); > > - if (mntpoint_mounted) > - tst_umount(tst_test->mntpoint); > + if (tst_hugetlb_fd >= 0) > + SAFE_CLOSE(tst_hugetlb_fd); > + > + if (mntpoint_mounted) { > + if (tst_test->needs_hugetlbfs) > + SAFE_UMOUNT(tst_test->mntpoint); > + else > + tst_umount(tst_test->mntpoint); > + } > > if (tst_test->needs_device && tdev.dev) > tst_release_device(tdev.dev); > -- > 2.31.1 > > > -- > Mailing list info: https://lists.linux.it/listinfo/ltp > > -- Regards, Li Wang