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

[Jeff Moyer <jmoyer@redhat.com>]

Hi, Cyril,

Thanks for the review.  Comments inline.

Cyril Hrubis <chrubis@suse.cz> writes:

>> +static char *overlay_mount_from_dev(dev_t dev)
>> +{
>> +	unsigned dev_major, dev_minor, mnt_major, mnt_minor;
>> +	FILE *fp;
>> +	char line[PATH_MAX];
>
> PATH_MAX does not really make any sense here. It's as good as any other
> number so I would just hardcode 4096 here.

Agreed.  Also, the theoretical max is beyond 4k, but it shouldn't be a
practical issue.  I did struggle with plucking a value out of thin air,
here.

>> +		if (ret != 3)
>> +			tst_brkm(TBROK, NULL,
>> +				 "failed to parse mountinfo line: \"%s\"",
>                                                                    ^
>                 We usually use ' instead of " inside of strings in LTP.

Ok.

>> +	if (!mountpoint)
>> +		tst_brkm(TBROK, NULL,
>> +			 "Unable to find mount entry for device %u:%u\n",
>                                                                       ^
> 							No newlines in
> 							tst_*()
> 							messages please.

Oops!

>> +	while ((mnt = getmntent(mntf)) != NULL) {
>> +		if (strncmp(mnt->mnt_dir, mountpoint, strlen(mountpoint)))
>> +			continue;
>
> Why strncmp() here? Isn't this possibly generating false positives in
> the case that we there is more mounts that have the same prefix that
> matches mountpoint?

Yes, good point.  Thanks!

>> +		if (strncmp(mnt->mnt_type, "overlay", strlen("overlay")))
>> +			tst_brkm(TBROK, NULL,
>> +				 "expected overlayfs on mount point \"%s\", but it is of type %s.",
>> +				 mountpoint, mnt->mnt_type);
>
> Here as well, I suppose that the probability of false positive here is
> close to zero, but I do not see the reason for strncmp() here either.

Agreed.

>> +		optstr = hasmntopt(mnt, "upperdir");
>> +		if (optstr) {
>> +			optstart = strchr(optstr, '=');
>> +			optstart++;
>> +			optend = strchrnul(optstr, ',');
>> +			upperdir = calloc(optend - optstart + 1, 1);
>> +			memcpy(upperdir, optstart, optend - optstart);
>
> Isn't this just a complicated way how to re-implement strndup()?

Yes.  :)  I'll fix that up.

>> +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");
>> +
>> +	mountpoint = overlay_mount_from_dev(st.st_dev);
>> +	upperdir = overlay_get_upperdir(mountpoint);
>> +	free(mountpoint);
>
> Since the mntpoint is only intermediate result, why can't we pass the
> st.dev to the overlay_get_upperdir() and call overlay_mount_from_dev()
> from there?

It makes more logical sense to me to pass a mount point to that
function.  Another argument against changing would be that
overlay_get_upperdir is already pretty large.  However, if you feel
strongly about it, I can certainly change it.

Thanks again for the thorough review!

Cheers,
Jeff


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