Re: [LTP] [PATCH 3/3] fanotify10: Make evictable marks tests more reliable

[Petr Vorel <pvorel@suse.cz>]

Hi all,

...
> >  static void setup(void)
> >  {
> > +	int i;
> > +
> >  	exec_events_unsupported = fanotify_events_supported_by_kernel(FAN_OPEN_EXEC,
> >  								      FAN_CLASS_CONTENT, 0);
> >  	filesystem_mark_unsupported = fanotify_mark_supported_by_kernel(FAN_MARK_FILESYSTEM);
> > @@ -880,7 +894,24 @@ static void setup(void)
> >  	SAFE_MKDIR(DIR_PATH, 0755);
> >  	SAFE_MKDIR(SUBDIR_PATH, 0755);
> >  	SAFE_FILE_PRINTF(FILE_PATH, "1");
> > -	SAFE_FILE_PRINTF(FILE2_PATH, "1");
> > +	for (i = 0; i < (int)(sizeof(tcases)/sizeof(tcases[0])); i++) {
>                                   ^
> 				  We do have the standard ARRAY_SIZE()
> 				  macro defined in LTP

With this being fixed before merge
Reviewed-by: Petr Vorel <pvorel@suse.cz>

> > +		if (tcases[i].mark_path_cnt > max_file_multi)
> > +			max_file_multi = tcases[i].mark_path_cnt;
> > +		if (tcases[i].ignore_path_cnt > max_file_multi)
> > +			max_file_multi = tcases[i].ignore_path_cnt;
> > +		if (tcases[i].event_path_cnt > max_file_multi)
> > +			max_file_multi = tcases[i].event_path_cnt;
> > +	}
> > +	for (i = 0; i < max_file_multi; i++) {
> > +		char path[PATH_MAX];
> > +
> > +		sprintf(path, FILE_PATH_MULTI, i);
> > +		SAFE_FILE_PRINTF(path, "1");
> > +		sprintf(path, DIR_PATH_MULTI, i);
> > +		SAFE_MKDIR(path, 0755);
> > +		sprintf(path, FILE_PATH_MULTIDIR, i);
> > +		SAFE_FILE_PRINTF(path, "1");
> > +	}

> >  	SAFE_CP(TEST_APP, FILE_EXEC_PATH);
> >  	SAFE_CP(TEST_APP, FILE2_EXEC_PATH);
> > @@ -896,6 +927,8 @@ static void setup(void)

> >  static void cleanup(void)
> >  {
> > +	int i;
> > +
> >  	cleanup_fanotify_groups();

> >  	if (bind_mount_created)
> > @@ -903,8 +936,17 @@ static void cleanup(void)

> >  	SAFE_FILE_PRINTF(CACHE_PRESSURE_FILE, "%d", old_cache_pressure);

> > +	for (i = 0; i < max_file_multi; i++) {
> > +		char path[PATH_MAX];
> > +
> > +		sprintf(path, FILE_PATH_MULTIDIR, i);
> > +		SAFE_UNLINK(path);
> > +		sprintf(path, DIR_PATH_MULTI, i);
> > +		SAFE_RMDIR(path);
> > +		sprintf(path, FILE_PATH_MULTI, i);
> > +		SAFE_UNLINK(path);
> > +	}
> >  	SAFE_UNLINK(FILE_PATH);
> > -	SAFE_UNLINK(FILE2_PATH);
> >  	SAFE_RMDIR(SUBDIR_PATH);
> >  	SAFE_RMDIR(DIR_PATH);
> >  	SAFE_RMDIR(MNT2_PATH);

> Do we have to unlink anything at all?

> As far as I can tell we create these files on a device that is
> reformatted after the test anyways.

It looks it's not cleared, because tmpdir is removed in do_cleanup(),
which is called just before the end of testing, not for each filesystem:

https://github.com/linux-test-project/ltp/tree/master/lib

Files created for testing in test's setup() are called for each filesystem
(do_test_setup), therefore files would remain created:

fanotify10.c:894: TBROK: mkdir(fs_mnt/testdir, 0755) failed: EEXIST (17)

Summary:
passed   492
failed   0
broken   1
skipped  3
warnings 0

And even if we disable just SAFE_UNLINK(FILE_PATH); we get errors
due our rmdir() implementation wanting directory being empty:

fanotify10.c:953: TWARN: rmdir(fs_mnt/testdir) failed: ENOTEMPTY (39)
fanotify10.c:894: TBROK: mkdir(fs_mnt/testdir, 0755) failed: EEXIST (17)
fanotify10.c:952: TWARN: rmdir(fs_mnt/testdir/testdir2) failed: ENOENT (2)
fanotify10.c:953: TWARN: rmdir(fs_mnt/testdir) failed: ENOTEMPTY (39)
fanotify10.c:954: TWARN: rmdir(mntpoint) failed: ENOENT (2)

IMHO it's safer to remove these files instead of creating them just for first
filesystem. Having unique path for each filesystem (e.g. use filesystem name in
the directory path would result would solve the need to remove files but
probably not worth of implementing.

Kind regards,
Petr

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