| Message ID | 20230324002441.987778-5-edliaw@google.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | tst_device.c: Handle Android path for backing device | expand |
Hi Edward, Reviewed-by: Petr Vorel <pvorel@suse.cz> ... > - if (dev_name[0]) > - sprintf(dev, "/dev/%s", dev_name); > + if (!dev_name[0] || set_dev_path(dev_name, dev, dev_len) != 0) > + tst_brkm(TBROK, NULL, "Could not stat backing device %s", dev); nit: I'd use !set_dev_path() if (!dev_name[0] || set_dev_path(dev_name, dev, dev_len)) (can be changed be before merge). Kind regards, Petr > + > } else { > tst_brkm(TBROK, NULL, "uevent file (%s) access failed", uevent_path); > }
Hi! > ------------------------------------------------------------------------------- > #include "tst_test.h" > > -void tst_find_backing_dev(const char *path, char *dev); > +void tst_find_backing_dev(const char *path, char *dev, size_t dev_len); > ------------------------------------------------------------------------------- > > This function finds the block dev that this path belongs to, using uevent in sysfs. > diff --git a/include/tst_device.h b/include/tst_device.h > index 977427f1c..f0aff4473 100644 > --- a/include/tst_device.h > +++ b/include/tst_device.h > @@ -110,8 +110,9 @@ void tst_purge_dir(const char *path); > * Find the file or path belongs to which block dev > * @path Path to find the backing dev > * @dev The block dev > + * @dev_len The length of the dev string > */ > -void tst_find_backing_dev(const char *path, char *dev); > +void tst_find_backing_dev(const char *path, char *dev, size_t dev_len); ^ This would be a bit better as dev_size since the common usage is that strings have length and buffers have size. > /* > * Stat the device mounted on a given path. > diff --git a/lib/newlib_tests/tst_device.c b/lib/newlib_tests/tst_device.c > index 87cec3961..53099f9bc 100644 > --- a/lib/newlib_tests/tst_device.c > +++ b/lib/newlib_tests/tst_device.c > @@ -71,7 +71,7 @@ static void test_tst_find_backing_dev(void) > { > char block_dev[100]; > > - tst_find_backing_dev(mntpoint, block_dev); > + tst_find_backing_dev(mntpoint, block_dev, sizeof(block_dev)); > > if (!strcmp(tst_device->dev, block_dev)) > tst_res(TPASS, "%s belongs to %s block dev", mntpoint, > diff --git a/lib/tst_device.c b/lib/tst_device.c > index ba46b7613..74de307e7 100644 > --- a/lib/tst_device.c > +++ b/lib/tst_device.c > @@ -60,6 +60,11 @@ static const char *dev_loop_variants[] = { > "/dev/block/loop%i" > }; > > +static const char *dev_variants[] = { > + "/dev/%s", > + "/dev/block/%s" > +}; > + > static int set_dev_loop_path(int dev, char *path, size_t path_len) > { > unsigned int i; > @@ -75,6 +80,21 @@ static int set_dev_loop_path(int dev, char *path, size_t path_len) > return 1; > } > > +static int set_dev_path(char *dev, char *path, size_t path_len) > +{ > + unsigned int i; > + struct stat st; > + > + for (i = 0; i < ARRAY_SIZE(dev_variants); i++) { > + snprintf(path, path_len, dev_variants[i], dev); > + > + if (stat(path, &st) == 0 && S_ISBLK(st.st_mode)) > + return 0; > + } > + > + return 1; > +} > + > int tst_find_free_loopdev(char *path, size_t path_len) > { > int ctl_fd, dev_fd, rc, i; > @@ -511,7 +531,7 @@ unsigned long tst_dev_bytes_written(const char *dev) > } > > __attribute__((nonnull)) > -void tst_find_backing_dev(const char *path, char *dev) > +void tst_find_backing_dev(const char *path, char *dev, size_t dev_len) > { > struct stat buf; > struct btrfs_ioctl_fs_info_args args = {0}; > @@ -574,7 +594,7 @@ void tst_find_backing_dev(const char *path, char *dev) > sprintf(uevent_path, "%s/%s/uevent", > bdev_path, d->d_name); > } else { > - tst_brkm(TBROK | TERRNO, NULL, "No backining device found while looking in %s.", bdev_path); > + tst_brkm(TBROK | TERRNO, NULL, "No backing device found while looking in %s.", bdev_path); > } > > if (SAFE_READDIR(NULL, dir)) > @@ -590,17 +610,12 @@ void tst_find_backing_dev(const char *path, char *dev) > if (!access(uevent_path, R_OK)) { > FILE_LINES_SCANF(NULL, uevent_path, "DEVNAME=%s", dev_name); > > - if (dev_name[0]) > - sprintf(dev, "/dev/%s", dev_name); > + if (!dev_name[0] || set_dev_path(dev_name, dev, dev_len) != 0) ^ isn't this redundant? These two are the only minor issues I've found in the patchset, otherwise: Reviewed-by: Cyril Hrubis <chrubis@suse.cz>
diff --git a/doc/c-test-api.txt b/doc/c-test-api.txt index a7dd59dac..eddb5c893 100644 --- a/doc/c-test-api.txt +++ b/doc/c-test-api.txt @@ -1087,7 +1087,7 @@ is created for that intention. ------------------------------------------------------------------------------- #include "tst_test.h" -void tst_find_backing_dev(const char *path, char *dev); +void tst_find_backing_dev(const char *path, char *dev, size_t dev_len); ------------------------------------------------------------------------------- This function finds the block dev that this path belongs to, using uevent in sysfs. diff --git a/include/tst_device.h b/include/tst_device.h index 977427f1c..f0aff4473 100644 --- a/include/tst_device.h +++ b/include/tst_device.h @@ -110,8 +110,9 @@ void tst_purge_dir(const char *path); * Find the file or path belongs to which block dev * @path Path to find the backing dev * @dev The block dev + * @dev_len The length of the dev string */ -void tst_find_backing_dev(const char *path, char *dev); +void tst_find_backing_dev(const char *path, char *dev, size_t dev_len); /* * Stat the device mounted on a given path. diff --git a/lib/newlib_tests/tst_device.c b/lib/newlib_tests/tst_device.c index 87cec3961..53099f9bc 100644 --- a/lib/newlib_tests/tst_device.c +++ b/lib/newlib_tests/tst_device.c @@ -71,7 +71,7 @@ static void test_tst_find_backing_dev(void) { char block_dev[100]; - tst_find_backing_dev(mntpoint, block_dev); + tst_find_backing_dev(mntpoint, block_dev, sizeof(block_dev)); if (!strcmp(tst_device->dev, block_dev)) tst_res(TPASS, "%s belongs to %s block dev", mntpoint, diff --git a/lib/tst_device.c b/lib/tst_device.c index ba46b7613..74de307e7 100644 --- a/lib/tst_device.c +++ b/lib/tst_device.c @@ -60,6 +60,11 @@ static const char *dev_loop_variants[] = { "/dev/block/loop%i" }; +static const char *dev_variants[] = { + "/dev/%s", + "/dev/block/%s" +}; + static int set_dev_loop_path(int dev, char *path, size_t path_len) { unsigned int i; @@ -75,6 +80,21 @@ static int set_dev_loop_path(int dev, char *path, size_t path_len) return 1; } +static int set_dev_path(char *dev, char *path, size_t path_len) +{ + unsigned int i; + struct stat st; + + for (i = 0; i < ARRAY_SIZE(dev_variants); i++) { + snprintf(path, path_len, dev_variants[i], dev); + + if (stat(path, &st) == 0 && S_ISBLK(st.st_mode)) + return 0; + } + + return 1; +} + int tst_find_free_loopdev(char *path, size_t path_len) { int ctl_fd, dev_fd, rc, i; @@ -511,7 +531,7 @@ unsigned long tst_dev_bytes_written(const char *dev) } __attribute__((nonnull)) -void tst_find_backing_dev(const char *path, char *dev) +void tst_find_backing_dev(const char *path, char *dev, size_t dev_len) { struct stat buf; struct btrfs_ioctl_fs_info_args args = {0}; @@ -574,7 +594,7 @@ void tst_find_backing_dev(const char *path, char *dev) sprintf(uevent_path, "%s/%s/uevent", bdev_path, d->d_name); } else { - tst_brkm(TBROK | TERRNO, NULL, "No backining device found while looking in %s.", bdev_path); + tst_brkm(TBROK | TERRNO, NULL, "No backing device found while looking in %s.", bdev_path); } if (SAFE_READDIR(NULL, dir)) @@ -590,17 +610,12 @@ void tst_find_backing_dev(const char *path, char *dev) if (!access(uevent_path, R_OK)) { FILE_LINES_SCANF(NULL, uevent_path, "DEVNAME=%s", dev_name); - if (dev_name[0]) - sprintf(dev, "/dev/%s", dev_name); + if (!dev_name[0] || set_dev_path(dev_name, dev, dev_len) != 0) + tst_brkm(TBROK, NULL, "Could not stat backing device %s", dev); + } else { tst_brkm(TBROK, NULL, "uevent file (%s) access failed", uevent_path); } - - if (stat(dev, &buf) < 0) - tst_brkm(TWARN | TERRNO, NULL, "stat(%s) failed", dev); - - if (S_ISBLK(buf.st_mode) != 1) - tst_brkm(TCONF, NULL, "dev(%s) isn't a block dev", dev); } void tst_stat_mount_dev(const char *const mnt_path, struct stat *const st) @@ -643,7 +658,7 @@ int tst_dev_block_size(const char *path) int size; char dev_name[PATH_MAX]; - tst_find_backing_dev(path, dev_name); + tst_find_backing_dev(path, dev_name, sizeof(dev_name)); fd = SAFE_OPEN(NULL, dev_name, O_RDONLY); SAFE_IOCTL(NULL, fd, BLKSSZGET, &size); diff --git a/testcases/kernel/syscalls/ioctl/ioctl_loop05.c b/testcases/kernel/syscalls/ioctl/ioctl_loop05.c index b4427f331..3a5d5afef 100644 --- a/testcases/kernel/syscalls/ioctl/ioctl_loop05.c +++ b/testcases/kernel/syscalls/ioctl/ioctl_loop05.c @@ -125,7 +125,7 @@ static void setup(void) * needn't transform transfer. */ sprintf(backing_file_path, "%s/test.img", tst_get_tmpdir()); - tst_find_backing_dev(backing_file_path, bd_path); + tst_find_backing_dev(backing_file_path, bd_path, sizeof(bd_path)); block_devfd = SAFE_OPEN(bd_path, O_RDWR); SAFE_IOCTL(block_devfd, BLKSSZGET, &logical_block_size); tst_res(TINFO, "backing dev(%s) logical_block_size is %d", bd_path, logical_block_size);
Fixes: e1b1ae66b240 ("tst_find_backing_dev: Get dev name from /sys/dev/block/*/uevent") On Android, the backing devices are created in /dev/block/ and will not be found using the method added in e1b1ae66b240. Adds a check for /dev/block/%s as well as /dev/%s. Modified the function signature of tst_find_backing_dev to add the length of the dev path string. Updated the documentation and code that references it. Signed-off-by: Edward Liaw <edliaw@google.com> --- doc/c-test-api.txt | 2 +- include/tst_device.h | 3 +- lib/newlib_tests/tst_device.c | 2 +- lib/tst_device.c | 37 +++++++++++++------ .../kernel/syscalls/ioctl/ioctl_loop05.c | 2 +- 5 files changed, 31 insertions(+), 15 deletions(-)