[LTP] [PATCH v2 1/2] OVL_MNT: add setup_overlay helper

[Murphy Zhou]

Hi Petr,

On Wed, May 15, 2019 at 03:42:45PM +0200, Petr Vorel wrote:
> Hi Murphy,
> 
> > ...
> > > +int setup_overlay(int mountovl)
> > > +{
> > > +	int ret;
> > > +
> > > +	/* Setup an overlay mount with lower dir and file */
> > > +	SAFE_MKDIR(OVL_LOWER, 0755);
> > > +	SAFE_MKDIR(OVL_UPPER, 0755);
> > > +	SAFE_MKDIR(OVL_WORK, 0755);
> > > +	SAFE_MKDIR(OVL_MNT, 0755);
> > > +
> > > +	/* Only create dirs, do not mount */
> > > +	if (mountovl == 0)
> > > +		return 0;
> > Instead of having int parameter, there could be create_overlay_dirs()
> > and mount_overlay(), which would call create_overlay_dirs().
> > (no need to lookup meaning of parameter).
> 
> > > +
> > > +	ret = mount("overlay", OVL_MNT, "overlay", 0, "lowerdir="OVL_LOWER
> > > +		    ",upperdir="OVL_UPPER",workdir="OVL_WORK);
> > > +	if (ret < 0) {
> > > +		if (errno == ENODEV) {
> > > +			tst_res(TINFO,
> > > +				"overlayfs is not configured in this kernel.");
> > > +			return 1;
> > First I thought we'd implement it as a test flag (member of struct tst_test).
> > When we have it as separate function I wonder whether we could TCONF on ENODEV
> > instead of TINFO and return. Maybe there could be here for int strict parameter,
> > where 1 would be force safe (i.e. TCONF), otherwise only TINFO.
> > This could also to have SAFE_MOUNT_OVERLAY() macro which would use mount_overlay().
> > Similar approach as SAFE_SEND() and safe_send().
> 
> From next patch:
> > +++ b/testcases/kernel/syscalls/inotify/inotify07.c
> > -#define OVL_MNT "ovl"
> >  #define DIR_NAME "test_dir"
> >  #define DIR_PATH OVL_MNT"/"DIR_NAME
> >  #define FILE_NAME "test_file"
> >  #define FILE_PATH OVL_MNT"/"DIR_NAME"/"FILE_NAME
> 
> >  static int ovl_mounted;
> > +static const char mntpoint[] = OVL_BASE_MNTPOINT;
> 
> >  static struct event_t event_set[EVENT_MAX];
> 
> > @@ -164,14 +164,11 @@ static void setup(void)
> >  	int ret;
> 
> >  	/* Setup an overlay mount with lower dir and file */
> > -	SAFE_MKDIR("lower", 0755);
> > -	SAFE_MKDIR("lower/"DIR_NAME, 0755);
> > -	SAFE_TOUCH("lower/"DIR_NAME"/"FILE_NAME, 0644, NULL);
> > -	SAFE_MKDIR("upper", 0755);
> > -	SAFE_MKDIR("work", 0755);
> > -	SAFE_MKDIR(OVL_MNT, 0755);
> > -	ret = mount("overlay", OVL_MNT, "overlay", 0,
> > -		    "lowerdir=lower,upperdir=upper,workdir=work");
> > +	setup_overlay(0);
> > +	SAFE_MKDIR(OVL_LOWER"/"DIR_NAME, 0755);
> > +	SAFE_TOUCH(OVL_LOWER"/"DIR_NAME"/"FILE_NAME, 0644, NULL);
> > +	ret = mount("overlay", OVL_MNT, "overlay", 0, "lowerdir="OVL_LOWER
> > +		    ",upperdir="OVL_UPPER",workdir="OVL_WORK);
> >  	if (ret < 0) {
> >  		if (errno == ENODEV) {
> >  			tst_brk(TCONF,
> In here in inotify07.c and in inotify08.c you create dirs (0 parameter) for because you it's
> needed to create more dirs. Than the rest (mount, TCONF on ENODEV, TBROK
> otherwise) is still copy pasted. I wonder how to move everything into
> setup_overlay() helper. Maybe struct with files or directories and permissions

If we define a struct to put names amd modes in it then pass to helper, we
still need to write all these OVL macros in the testcase to defile the struct.
So we need to write all the _same_ macros in every testcase where needed.

In this case, it's against my intention of this patch: dedupe duplicated lines.

Your struct idea is great when handling the different files that need to be
created in different testcases. However I'd like to do it in a simpler way.
Only make necessary dirs in the helper, let the testcases to create the other
dirs they want themselves.

Thanks,
Murphy

> to for touch/mkdir? With this, int mountovl dir create_overlay_dirs() might not
> be needed.
> 
> 
> Kind regards,
> Petr