Re: [LTP] [PATCH v3 3/3] tst_find_backing_dev(): add support for overlayfs

[Petr Vorel <pvorel@suse.cz>]

Hi Jeff,
> > LGTM, thanks for a very nice work!

> > Reviewed-by: Petr Vorel <pvorel@suse.cz>

> > I'd prefer others have look into it before merging.

> Sure, any review is appreciated.

> > Again, I'll apply minor formatting changes before merge (using SAFE_STAT(),
> > moving else branch after break to it's own and checkpatch.pl fixes).

> It looks fine, but I will make a couple of observations.

> > @@ -634,11 +633,11 @@ static char *overlay_get_upperdir(char *mountpoint)
> >  			upperdir = calloc(optend - optstart + 1, 1);
> >  			memcpy(upperdir, optstart, optend - optstart);
> >  			break;
> > -		} else {
> > -			tst_brkm(TBROK, NULL,
> > -				 "mount point %s does not contain an upperdir",
> > -				 mountpoint);
> >  		}
> > +
> > +		tst_brkm(TBROK, NULL,
> > +			 "mount point %s does not contain an upperdir",
> > +			 mountpoint);

> This is technically different, but I don't think it matters.  All
> overlay mount points need an upperdir, so it is valid to error out here.

FYI my point here was to change:

if (...) {
	foo ...
	break;
} else {
	bar ...
}

to:

if (...) {
	foo ...
	break;
}

bar ...

(IMHO slightly readable + checkpatch.pl prefers it.)
Did I overlook something?

> >  	}
> >  	endmntent(mntf);

> > @@ -679,26 +678,21 @@ static char *overlay_get_upperdir(char *mountpoint)
> >   */
> >  static void overlay_get_uevent_path(char *tmp_path, char *uevent_path)
> >  {
> > -	int ret;
> >  	struct stat st;
> >  	char *mountpoint, *upperdir;

> >  	tst_resm(TINFO, "Use OVERLAYFS specific strategy");

> > -	ret = stat(tmp_path, &st);
> > -	if (ret)
> > -		tst_brkm(TBROK | TERRNO, NULL, "stat failed");
> > +	SAFE_STAT(NULL, tmp_path, &st);

> Sorry for not using SAFE_STAT.  I don't know how I missed that.  Thanks
> again for the review and for fixing up these issues.

Nah, not a big deal. The patchset is very nice, thanks for that!

Kind regards,
Petr

> Cheers,
> Jeff


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