Re: [LTP] [PATCH 1/2] tst_find_backing_dev: Get dev name from /sys/*/uevent

[Richard Palethorpe <rpalethorpe@suse.de>]

Hello,

Just a couple of points.

Alessandro Carminati <alessandro.carminati@gmail.com> writes:

> +	if (dev_major == 0) {
> +		tst_resm(TINFO, "Use BTRFS specific strategy");
> +
> +		fd = SAFE_OPEN(NULL, dirname(path), O_DIRECTORY);
> +		if (!ioctl(fd, BTRFS_IOC_FS_INFO, &args)) {

If this is tmpfs, then this ioctl will fail with ENOTTY, but it is not
checked for.

> +			sprintf(btrfs_uuid_str,
> +				UUID_FMT,
> +				args.fsid[0], args.fsid[1],
> +				args.fsid[2], args.fsid[3],
> +				args.fsid[4], args.fsid[5],
> +				args.fsid[6], args.fsid[7],
> +				args.fsid[8], args.fsid[9],
> +				args.fsid[10], args.fsid[11],
> +				args.fsid[12], args.fsid[13],
> +				args.fsid[14], args.fsid[15]);
> +			sprintf(bdev_path,
> +				"/sys/fs/btrfs/%s/devices", btrfs_uuid_str);
>  		}
> -
> -		len = count_match_len(path, mnt_point);
> -		if (len > best_match_len) {
> -			strcpy(dev, pre);
> -			best_match_len = len;
> +		SAFE_CLOSE(NULL, fd);
> +		dir = SAFE_OPENDIR(NULL, bdev_path);

So then down here we get a failure with ENOENT or some similar confusing
message.

Ideally we want to tell the user that the FS has an anonymous backing
dev, but that it is not BTRFS. Maybe also hint that they need to add the
FS to tst_test.skip_filesystems if it is has no backing device
(e.g. tmpfs). As this is the most likely cause given ioctl_loop05's
history.

> +		while (d = SAFE_READDIR(NULL, dir)) {
> +			if (d->d_name[0]!='.')
> +				break;
>  		}
> +		uevent_path[0] = '\0';
> +		if (d) {
> +			sprintf(uevent_path, "%s/%s/uevent",
> +				bdev_path, d->d_name);
> +		} else {
> +			tst_brkm(TBROK, NULL, "No backining device
> found");

We can print bdev path here. Then the user knows where to look without
much effort.


Marking this as changes requested in patchwork.

-- 
Thank you,
Richard.

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