Re: [LTP] [PATCH v3 1/4] Hugetlb: Add new tst_test options for hugeltb test support

[Tarun Sahu <tsahu@linux.ibm.com>]

Hi Cyril,
Please find my comments inline.

On Oct 31 2022, Cyril Hrubis wrote:
> Hi!
> > 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},
> > };
> 
> I do not like that we have magic constants in the .hugepages that are
> not self describing. I would treat the hugetltbfs just as we treat
> devfs, that would be:
> 
> #define MNTPOINT "hugetlbfs/"
> #define HUGEFILE MNTPOINT "hugefile"
> 
> static int huge_fd;
> 
> static void setup(void)
> {
> 	huge_fd = tst_creat_unlinked(HUGEFILE);
> 	...
> }
> 
> static void cleanup(void)
> {
> 	if (huge_fd > 0)
> 		SAFE_CLOSE(huge_fd);
> }
> 
> static struct tst_test test = {
> 	...
> 	.mntpoint = MNTPOINT,
> 	.needs_hugetlbfs = 1,
> 	.setup = setup,
> 	.cleanup = cleanup,
> 	...
> }
> 
> 
> What do you think?
> 
My original idea behind putting it in tst_test struct instead of
tst_hugepages was based on below two reasoning-

1. tst_hugepages seems to have only hugepages related info. It would be
better to rename it to tst_hugetlb and rename <hugepages> field in tst_test
struct to <hugetlb>. If we rename it which will require changes in already
existing tests. and Moreover, there were similar field like needs_tmpdir
in tst_test so I put it(needs_unlinked_hugetlb_file) in tst_test.
2. There was already related fields in tst_test for mounting fs, like
needs_devfs, needs_rofs, So keeping all the needs_ABCfs at same structure.

> -- 
> Cyril Hrubis
> chrubis@suse.cz
> 
> -- 
> Mailing list info: https://lists.linux.it/listinfo/ltp

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