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

Richard Palethorpe
2022-11-08T10:20:39Z

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.