Re: [LTP] [PATCH v4] syscalls/mmap01: Rewrite the test using new LTP API

[Petr Vorel <pvorel@suse.cz>]

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