From: Petr Vorel <pvorel@suse.cz>
To: Avinesh Kumar <akumar@suse.de>
Cc: ltp@lists.linux.it
Subject: Re: [LTP] [PATCH v4] syscalls/mmap01: Rewrite the test using new LTP API
Date: Wed, 20 Mar 2024 17:02:08 +0100 [thread overview]
Message-ID: <20240320160208.GA489473@pevik> (raw)
In-Reply-To: <2673972.lGaqSPkdTl@localhost>
Hi Avinesh,
> Hi Cyril, Petr,
> Thank you for the review.
...
> > > - /* Creat a temporary file used for mapping */
> > > - if ((fildes = open(TEMPFILE, O_RDWR | O_CREAT, 0666)) < 0) {
> > > - tst_brkm(TFAIL, cleanup, "opening %s failed", TEMPFILE);
> > > - }
> > > + addr[file_sz] = 'X';
> > > + addr[file_sz + 1] = 'Y';
> > > + addr[file_sz + 2] = 'Z';
> > > - /* Write some data into temporary file */
> > > - if (write(fildes, write_buf, strlen(write_buf)) !=
> > > (long)strlen(write_buf)) { - tst_brkm(TFAIL, cleanup, "writing to %s",
> > > TEMPFILE);
> > > - }
> > > + SAFE_MSYNC(addr, page_sz, MS_SYNC);
> > > - /* Get the size of temporary file */
> > > - if (stat(TEMPFILE, &stat_buf) < 0) {
> > > - tst_brkm(TFAIL | TERRNO, cleanup, "stat of %s failed",
> > > - TEMPFILE);
> > > - }
> > > - file_sz = stat_buf.st_size;
> > > + SAFE_FILE_SCANF(TEMPFILE, "%s", buf);
> > Hmm, why do we SAFE_LSEEK() the fd if we are not using it for reading?
> I guess I can remove the SAFE_LSEEK() in setup(), as we want to read the
> complete file contents without knowing it's size, hence SAFE_FILE_SCANF().
I'm not sure if any lseek() is needed.
> Please correct me if this is not the right approach.
I guess Cyril means by SAFE_READ() to read just that 3 bytes
changed or strlen(write_buf) (whole string).
> > This could be just simple SAFE_READ() instead.
> > > - page_sz = getpagesize();
> > > + if (strcmp(write_buf, buf))
> > > + tst_res(TFAIL, "File data has changed");
> > > + else
> > > + tst_res(TPASS, "mmap() functionality successful");
> > ^
> > "Data after file end were not written out"
> > It's kind of pointless to print message that just means "success".
> > > - /* Allocate and initialize dummy string of system page size bytes */
> > > - if ((dummy = calloc(page_sz, sizeof(char))) == NULL) {
> > > - tst_brkm(TFAIL, cleanup, "calloc failed (dummy)");
> > > - }
> > > + SAFE_LSEEK(fd, 0, SEEK_SET);
> > > + memset(&addr[file_sz], 0, 3);
> > I was wondering why this is needed, seems like for tmpfs we will read
> > back the data after the end of the file on a subsequent runs of the
> > test, i.e. with -i 2.
> > I wonder if that is expected or not, it's a bit strange that we can
> > expand the file size that way.
> > And it seems to happen for FUSE as well, that actually does sound like a
> > bug.
> Thanks for pointing this out, I was overlooking this issue. I verified that we
> read back the data written past eof in further iteration of the test only in
> tmpfs and fuse.ntfs. How would you suggest to confirm if this is indeed a bug
> with these filesystems.
Interesting. Feel free to Cc LTP ML if you report to mainline developers (not
sure if mainline kernel or SUSE kernel is affected).
Kind regards,
Petr
> Regards,
> Avinesh
--
Mailing list info: https://lists.linux.it/listinfo/ltp
prev parent reply other threads:[~2024-03-20 16:02 UTC|newest]
Thread overview: 5+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-01-30 12:23 [LTP] [PATCH v4] syscalls/mmap01: Rewrite the test using new LTP API Avinesh Kumar
2024-03-05 22:00 ` Petr Vorel
2024-03-06 14:55 ` Cyril Hrubis
2024-03-20 10:56 ` Avinesh Kumar
2024-03-20 16:02 ` Petr Vorel [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20240320160208.GA489473@pevik \
--to=pvorel@suse.cz \
--cc=akumar@suse.de \
--cc=ltp@lists.linux.it \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).