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

[Murphy Zhou]

Hi Petr,

On Fri, May 24, 2019 at 10:54:52AM +0200, Petr Vorel wrote:
> Hi Murphy,
> 
> > > > +	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,
> 
> > The return value is referenced in the testcase to determine whether to run
> > tests in overlayfs. It's needed.
> 
> > If this strict parameter is only for different wording on NODEV. Is it
> > necessary ?
> 
> I'll recap my suggestions:
> 1) I like having macros to help reduce some parameters, this does not block
> functions being flexible (which requires parameters).
> 2) Having helper function create_overlay_dirs() used separately, than parameter
> in single function (Amir [1] suggestion makes sense).
> 
> Something like this, just a suggestion:
> 
> int create_overlay_dirs()
> {
> 	SAFE_MKDIR(OVL_LOWER, 0755);
> 	...
> 	return ret;
> }
> 
> int mount_overlay(const char *file, const int lineno, int safe)
> {
> 	...
> 	if (create_overlay_dirs())
> 		tst_brk(TCONF, "...");
> 
> 	ret = mount("overlay", OVL_MNT, "overlay", 0, "lowerdir="OVL_LOWER
> 		    ",upperdir="OVL_UPPER",workdir="OVL_WORK);
> 	if (ret < 0) {
> 		if (errno == ENODEV) {
> 			/*
> 			 * TODO: maybe safe is confusing as we use tst_brk(TBROK anyway),
> 			 * + sometimes tst_res(TCONF, ..) would be preferred over
> 			 * tst_brk(TCONF, ...)
> 			 */
> 			if (safe)
> 				tst_brk(TCONF,
> 					"overlayfs is not configured in this kernel.");
> 			} else {
> 				tst_res(TINFO,
> 					"overlayfs is not configured in this kernel.");
> 				return 1;
> 			}
> 		}
> 		tst_brk(TBROK | TERRNO, "overlayfs mount failed");

Hmm.. This would changed "test logic" in the existing 4 testcases.

Murphy
> 	}
> }
> 
> #define SAFE_MOUNT_OVERLAY() \
> 	mount_overlay(__FILE__, __LINE__, 1);
> 
> #define TST_MOUNT_OVERLAY() \
> 	mount_overlay(__FILE__, __LINE__, 0);
> 
> Kind regards,
> Petr
> 
> [1] http://lists.linux.it/pipermail/ltp/2019-May/011983.html