Re: [LTP] [PATCH v3 2/2] mount03: Convert to new API

[Cyril Hrubis <chrubis@suse.cz>]

Hi!
> > > -/*
> > > - * DESCRIPTION
> > > - *	Check for basic mount(2) system call flags.
> > > +/*\
> > > + * [Description]
> > >   *
> > > - *	Verify that mount(2) syscall passes for each flag setting and validate
> > > - *	the flags
> > > - *	1) MS_RDONLY - mount read-only.
> > > - *	2) MS_NODEV - disallow access to device special files.
> > > - *	3) MS_NOEXEC - disallow program execution.
> > > - *	4) MS_SYNCHRONOUS - writes are synced at once.
> > > - *	5) MS_REMOUNT - alter flags of a mounted FS.
> > > - *	6) MS_NOSUID - ignore suid and sgid bits.
> > > - *	7) MS_NOATIME - do not update access times.
> > > + * Verify mount(2) for various flags.
> > >   */
> 
> > Can we please be a bit more verbose here?
> Sure, that was my change. Do you want me to put the original description or
> would be this enough?
> 
> Verify mount(2) run with various flags (e.g. MS_RDONLY, MS_NOEXEC).
> 
> => i.e. what are you missing? I'm not a big fan of listing all features tested,
> but if you prefer I'll put the original description.

I do not think that listing flags that are tested is pointless, at least
this is supposed to be documentation for the test that is shown on a web
page, it should be a bit more verbose than the single sentence you wrote
there.

> > > +static void test_synchronous(void)
> > > +{
> > > +	strcpy(wbuf, TEST_STR);
> > > +	snprintf(file, PATH_MAX, "%s/synchronous", MNTPOINT);
> > > +	otfd = SAFE_OPEN(file, O_RDWR | O_CREAT, FILE_MODE);
> > > +	SAFE_WRITE(1, otfd, wbuf, strlen(wbuf));
> > > +	SAFE_LSEEK(otfd, 0, SEEK_SET);
> > > +	SAFE_READ(0, otfd, rbuf, sizeof(rbuf));
> > > +	TST_EXP_EQ_STR(rbuf, wbuf);
> > > +}
> 
> > This is completely bogus check, this has to work regardless of the
> > MS_SYNCHRONOUS. The only way how to check MS_SYNCHRONOUS would be
> > pulling out the device just after write before page cache had a chance
> > to write out data but not before the disk flushes its caches.
> 
> > I guess that it may be possible to check this if create a loop device,
> > mount it MS_SYNCHRONOUS, write to a file on the loop device and check
> > that the data has been written to the underlying file. But that would
> > be completely different and quite complex test.
> 
> OK, I suggest to remove this test and put your suggestion for new to issues.
> 
> Also looking to the man page we're missing test for MS_LAZYTIME (since 4.O) and
> MS_NOSYMFOLLOW (5.10).

I guess that MS_NOSYMFOLLOW should be easy enough, we just need to
create a symlink to a file and then attempt to open it. We shouldn't end
up with a fd to the symlinked file in that case.

MS_LAZYTIME would be again complicated since that is about deffering
timestamps in memory so it would be similar to MS_SYNCHRONOUS in the
terms of complexity.

> And I'll drop TST_EXP_EQ_STR() unless you think it's useful.

I would follow the usuall, don't add anything that is not used.

-- 
Cyril Hrubis
chrubis@suse.cz

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