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

[Richard Palethorpe <rpalethorpe@suse.de>]

Hello,

I'd like to merge, but discovered some more issues that requuire more
than a fixup before merge.

Also please use the -v flag in git format-patch to version the patches
after the first revision. I don't mind which version you start at now.

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

> In some minimal Linux, the /dev/root can be missing. The consequence of
> this is that mountinfo doesn't contain the correct information. btrfs
> file systems are yet another point of trouble for this function.
>
> The unevent file in sysfs is another method to retrieve device info
> using the sysfs.
>
> btrfs file systems are special from the device name retrieval, and in
> place of use of the minor/major they are approached by using the uuid.
> In the end, btrfs strategy is a slightly modified version of the same
> unevent strategy.
>
> Non btrfs look in "/sys/dev/block/%d:%d/uevent" major, minor
> btrfs look in /sys/fs/btrfs/%s/devices/%s/uevent, uuid, devname
>
> The btrfs handling requires BTRFS specific ioctl for finding the
> file system uuid, and for this reason, btrfs/ioctl.h is needed.
>
> Signed-off-by: Richard Palethorpe <rpalethorpe@suse.com>
> Signed-off-by: Alessandro Carminati <alessandro.carminati@gmail.com>
> ---
>  lib/tst_device.c | 91 ++++++++++++++++++++++++++++++++----------------
>  1 file changed, 61 insertions(+), 30 deletions(-)
>
> diff --git a/lib/tst_device.c b/lib/tst_device.c
> index 8419b80c3..054e39bcd 100644
> --- a/lib/tst_device.c
> +++ b/lib/tst_device.c
> @@ -33,6 +33,9 @@
>  #include <stdint.h>
>  #include <inttypes.h>
>  #include <sys/sysmacros.h>
> +#include <linux/btrfs.h>
> +#include <linux/limits.h>
> +#include <dirent.h>
>  #include "lapi/syscalls.h"
>  #include "test.h"
>  #include "safe_macros.h"
> @@ -45,6 +48,8 @@
>  
>  #define DEV_FILE "test_dev.img"
>  #define DEV_SIZE_MB 300u
> +#define UUID_STR_SZ 37
> +#define UUID_FMT "%02x%02x%02x%02x-%02x%02x-%02x%02x-%02x%02x-%02x%02x%02x%02x%02x%02x"
>  
>  static char dev_path[1024];
>  static int device_acquired;
> @@ -519,48 +524,74 @@ static int count_match_len(const char *first, const char *second)
>  void tst_find_backing_dev(const char *path, char *dev)
>  {
>  	struct stat buf;
> -	FILE *file;
> -	char line[PATH_MAX];
> -	char *pre = NULL;
> -	char *next = NULL;
> -	unsigned int dev_major, dev_minor, line_mjr, line_mnr;
> -	unsigned int len, best_match_len = 1;
> -	char mnt_point[PATH_MAX];
> +	struct btrfs_ioctl_fs_info_args args = {0};
> +	struct dirent *d;
> +	char uevent_path[PATH_MAX];
> +	char dev_name[NAME_MAX];
> +	char bdev_path[PATH_MAX];
> +	char btrfs_uuid_str[UUID_STR_SZ];
> +	DIR *dir;
> +	unsigned int dev_major, dev_minor;
> +	int fd;
>  
>  	if (stat(path, &buf) < 0)
>  		tst_brkm(TWARN | TERRNO, NULL, "stat() failed");
>  
>  	dev_major = major(buf.st_dev);
>  	dev_minor = minor(buf.st_dev);
> -	file = SAFE_FOPEN(NULL, "/proc/self/mountinfo", "r");
>  	*dev = '\0';
>  
> -	while (fgets(line, sizeof(line), file)) {
> -		if (sscanf(line, "%*d %*d %d:%d %*s %s",
> -			&line_mjr, &line_mnr, mnt_point) != 3)
> -			continue;
> -
> -		pre = strstr(line, " - ");
> -		pre = strtok_r(pre, " ", &next);
> -		pre = strtok_r(NULL, " ", &next);
> -		pre = strtok_r(NULL, " ", &next);
> -
> -		if (line_mjr == dev_major && line_mnr == dev_minor) {
> -			strcpy(dev, pre);
> -			break;
> -		}
> -
> -		len = count_match_len(path, mnt_point);
> -		if (len > best_match_len) {
> -			strcpy(dev, pre);
> -			best_match_len = len;
> +	if (dev_major == 0) {
> +		tst_resm(TINFO, "Use BTRFS specific strategy");
> +
> +		fd = SAFE_OPEN(NULL, dirname(path), O_DIRECTORY);

There are two problems here. One is simple and that dirname can modify
path, but path is a const pointer (compiler should warn about dropping
const modifiers). The simple solution is just to copy path into a buffer.

Secondly ioctl_loop05 passes the path to an image, but the self test in
/lib/newlib_tests/tst_device.c passes the mount point. So unless I am
mistaken dirname will return the dir below the mount point which is
wrong.

One option is to try opening path as a dir first and if that fails, use
dirname to get the containing folder. Changeing ioctl_loop05 would also
be valid.

> +		if (!ioctl(fd, BTRFS_IOC_FS_INFO, &args)) {
> +			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);
> +		} else {
> +			tst_brkm(TBROK, NULL, "BTRFS ioctl failed. Is %s
> on a tmpfs?", path);
Need TERRNO here and/or check that the errorno is ENOTTY otherwise the
hint makes no sense.
> +			}
> +		SAFE_CLOSE(NULL, fd);
> +		dir = SAFE_OPENDIR(NULL, bdev_path);
> +		while (d = SAFE_READDIR(NULL, dir)) {
> +			if (d->d_name[0]!='.')

There are a few formatting errors like the missing spaces around !=.

Run make check-tst_device in the lib dir and see the kernel style
guidelines.

> +				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");

Still need to print some information about where we are looking (bdev_path).

> +			}
> +		if (SAFE_READDIR(NULL, dir))
> +			tst_resm(TINFO, "Warning: used first of multiple backing device.");
> +		SAFE_CLOSEDIR(NULL, dir);
> +	} else {
> +
> +		tst_resm(TINFO, "Use uevent strategy");
> +		sprintf(uevent_path,
> +			"/sys/dev/block/%d:%d/uevent", dev_major, dev_minor);
>  	}
>  
> -	SAFE_FCLOSE(NULL, file);
> +	if (!access(uevent_path, R_OK)) {
> +		FILE_LINES_SCANF(NULL, uevent_path, "DEVNAME=%s", dev_name);
>  
> -	if (!*dev)
> -		tst_brkm(TBROK, NULL, "Cannot find block device for %s", path);
> +		if (dev_name[0])
> +			sprintf(dev, "/dev/%s", dev_name);
> +	} else {
> +		tst_brkm(TBROK, NULL, "uevent file (%s) access failed",
> uevent_path);

Also we can use (TBROK | TERRNO) here as access sets that.

> +		}

make check somehow missing this. The } is indented too far.

>  
>  	if (stat(dev, &buf) < 0)
>  		tst_brkm(TWARN | TERRNO, NULL, "stat(%s) failed", dev);



-- 
Thank you,
Richard.

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