Re: [LTP] [PATCH v4 1/7] Hugetlb: Add new argument flags in tst_creat_unlinked

Tarun Sahu
2022-11-18T08:27:59Z

On Nov 17 2022, Tarun Sahu wrote:
> Hi,
> Thanks for reviewing the patch.
> On Nov 16 2022, Cyril Hrubis wrote:
> > Hi!
> > > Some test requires custom flags for file opened by tst_creat_unlinked
> > > along with O_CREAT|O_EXCL|O_RDWR. This patch creates support to pass
> > > custom flags in tst_creat_unlinked.
> > >
> > > Signed-off-by: Tarun Sahu <tsahu@linux.ibm.com>
> > > ---
> > > include/tst_test.h | 2 +-
> > > lib/tst_test.c | 19 ++++++++++++++-----
> > > .../kernel/mem/hugetlb/hugemmap/hugemmap07.c | 2 +-
> > > .../kernel/mem/hugetlb/hugemmap/hugemmap08.c | 2 +-
> > > .../kernel/mem/hugetlb/hugemmap/hugemmap09.c | 2 +-
> > > 5 files changed, 18 insertions(+), 9 deletions(-)
> > >
> > > diff --git a/include/tst_test.h b/include/tst_test.h
> > > index acf2421de..a62515bfe 100644
> > > --- a/include/tst_test.h
> > > +++ b/include/tst_test.h
> > > @@ -365,7 +365,7 @@ 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_creat_unlinked(const char *path);
> > > +int tst_creat_unlinked(const char *path, int flags);
> > >
> > > /*
> > > * 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 b225ba082..d17b15ee8 100644
> > > --- a/lib/tst_test.c
> > > +++ b/lib/tst_test.c
> > > @@ -1027,18 +1027,27 @@ static void prepare_and_mount_hugetlb_fs(void)
> > > mntpoint_mounted = 1;
> > > }
> > >
> > > -int tst_creat_unlinked(const char *path)
> > > +int tst_creat_unlinked(const char *path, int flags)
> > > {
> > > char template[PATH_MAX];
> > > + int len, c, range;
> > > int fd;
> > > + int start[3] = {'0', 'a', 'A'};
> > >
> > > snprintf(template, PATH_MAX, "%s/ltp_%.3sXXXXXX",
> > > path, tid);
> > > + len = strlen(template) - 1;
> > > +
> > > + srand(time(NULL));
> > > + while (template[len] == 'X') {
> > > + c = rand() % 3;
> > > + range = start[c] == '0' ? 10 : 26;
> > > + c = start[c] + (rand() % range);
> > > + template[len--] = (char)c;
> > > + }
> > >
> > > - fd = mkstemp(template);
> > > - if (fd < 0)
> > > - tst_brk(TBROK | TERRNO, "mkstemp(%s) failed", template);
> > > -
> > > + flags |= O_CREAT|O_EXCL|O_RDWR;
> > > + fd = SAFE_OPEN(template, flags);
> >
> > I wonder if it would be easier to add the O_DIRECT flag with F_GETFL and
> > F_SETFL fcntl(), but I guess that this is fine as it is.
> >
> > The only potential problem I see is that setting the random seed may
> > interfere with anything that would print what seed has been used for
> > random data in order to be able to reproduce the same random sequence if
> > needed. So maybe I wouldn't attempt to set the seed in this function at
> > all.
> >
> Ok. can remove it.
>
> One more concern, I have, it wont be thread safe. Is it acceptable?
> Though, an alternative is to use mkostemp, but it is only defined in
> __GNU_SOURCE, defining it for the whole library tst_test.c doesnt seem good.
>
> instead using rand_r will require _thread static seed to make it
> thread safe. but doesn't seem to be worth the effort, if we can simply use,
> mkostemp or fcntl to set flags manually later.
>
> What do you think?
>
I missed it,
I could make use of mktemp. will replace all this. and update it in next
version.

> > > SAFE_UNLINK(template);
> > > return fd;
> > > }
> > > diff --git a/testcases/kernel/mem/hugetlb/hugemmap/hugemmap07.c b/testcases/kernel/mem/hugetlb/hugemmap/hugemmap07.c
> > > index bd0fb440a..3122d5b9d 100644
> > > --- a/testcases/kernel/mem/hugetlb/hugemmap/hugemmap07.c
> > > +++ b/testcases/kernel/mem/hugetlb/hugemmap/hugemmap07.c
> > > @@ -113,7 +113,7 @@ cleanup:
> > > static void setup(void)
> > > {
> > > hpage_size = SAFE_READ_MEMINFO(MEMINFO_HPAGE_SIZE)*1024;
> > > - huge_fd = tst_creat_unlinked(MNTPOINT);
> > > + huge_fd = tst_creat_unlinked(MNTPOINT, 0);
> > > }
> > >
> > > static void cleanup(void)
> > > diff --git a/testcases/kernel/mem/hugetlb/hugemmap/hugemmap08.c b/testcases/kernel/mem/hugetlb/hugemmap/hugemmap08.c
> > > index ce40e7b69..f66b331dc 100644
> > > --- a/testcases/kernel/mem/hugetlb/hugemmap/hugemmap08.c
> > > +++ b/testcases/kernel/mem/hugetlb/hugemmap/hugemmap08.c
> > > @@ -118,7 +118,7 @@ static void run_test(unsigned int test_type)
> > > static void setup(void)
> > > {
> > > hpage_size = SAFE_READ_MEMINFO(MEMINFO_HPAGE_SIZE)*1024;
> > > - huge_fd = tst_creat_unlinked(MNTPOINT);
> > > + huge_fd = tst_creat_unlinked(MNTPOINT, 0);
> > > }
> > >
> > > static void cleanup(void)
> > > diff --git a/testcases/kernel/mem/hugetlb/hugemmap/hugemmap09.c b/testcases/kernel/mem/hugetlb/hugemmap/hugemmap09.c
> > > index 1008395a4..ceb0f64a1 100644
> > > --- a/testcases/kernel/mem/hugetlb/hugemmap/hugemmap09.c
> > > +++ b/testcases/kernel/mem/hugetlb/hugemmap/hugemmap09.c
> > > @@ -60,7 +60,7 @@ static void run_test(void)
> > > static void setup(void)
> > > {
> > > hpage_size = SAFE_READ_MEMINFO(MEMINFO_HPAGE_SIZE)*1024;
> > > - huge_fd = tst_creat_unlinked(MNTPOINT);
> > > + huge_fd = tst_creat_unlinked(MNTPOINT, 0);
> > > }
> > >
> > > static void cleanup(void)
> > > --
> > > 2.31.1
> > >
> >
> > --
> > Cyril Hrubis
> > chrubis@suse.cz
> >
> > --
> > Mailing list info: https://lists.linux.it/listinfo/ltp
>
> --
> Mailing list info: https://lists.linux.it/listinfo/ltp