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

[Jeff Moyer <jmoyer@redhat.com>]

Hi, Petr,

Petr Vorel <pvorel@suse.cz> writes:

> Hi Jeff,
>
>
>> Add checks for overlayfs in tst_find_backing_dev.  As with btrfs, only
>> a single device is checked (the upper one) and returned from
>> tst_find_backing_dev().
>
>> The implementation uses both /proc/self/mountinfo and /proc/self/mounts.
>> The former is used to map a device to a mountpoint, and the latter is
>> used to get the file system options for the mountpoint.  All of the
>> information is present in mountinfo, but the file format is more complex,
>> and there are no glibc helpers for parsing it.
>
>> The '#define _GNU_SOURCE' was added for the use of the strchrnul(3)
>> function.
>
>> Signed-off-by: Jeff Moyer <jmoyer@redhat.com>
>
>> ---
>> v3: Address review comments from Petr:
>>     - Don't split long strings across lines
>>     - use TST_OVERLAYFS_MAGIC instead of the kernel's definition
>>     Note that I did not use SAFE_SSCANF, as tst_device.c uses the old
>>     style safe macros, and that function is not covered.
>> v2: Don't use libmount.  Instead, map from device number to mount-point
>>     using /proc/self/mountinfo, and then use the mntent.h helpers to get
>>     the mount options for the mountpoint from /proc/self/mounts.
>
> 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.

>  	}
>  	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.

Cheers,
Jeff


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