[LTP] [PATCH v3 0/4] tst_device.c: Handle Android path for backing device

[LTP] [PATCH v3 0/4] tst_device.c: Handle Android path for backing device

[Edward Liaw via ltp]

e1b1ae66b240 ("tst_find_backing_dev: Get dev name from
/sys/dev/block/*/uevent") added a hardcoded path to check for the
backing device in /dev.  On Android, it needs to check /dev/block.

The set_dev_path function was renamed to set_dev_loop_path and its
return value was changed from 1 on success to 0 on success to be more
consistent with other functions.  A check was added to
tst_find_free_loopdev in the event that the loop device is not ready
yet, which appears to happen occasionally on Android.

v1->v2:
Changed the function signature of tst_find_backing_dev to add the length
of the path string.  Updated all references for this function to include
the added parameter.

v2->v3:
Split the function rename, return value change, and the return value
check into 3 commits.  Fixed logic for non-NULL path check.  Added Fixes
tag for e1b1ae66b240.

Edward Liaw (4):
  set_dev_path: Rename to set_dev_loop_path
  set_dev_loop_path: Change return value to zero on success
  tst_find_free_loopdev: Check return value of set_dev_loop_path
  tst_find_backing_dev: Also check /dev/block/ for backing device

 doc/c-test-api.txt                            |  2 +-
 include/tst_device.h                          |  3 +-
 lib/newlib_tests/tst_device.c                 |  2 +-
 lib/tst_device.c                              | 51 ++++++++++++-------
 .../kernel/syscalls/ioctl/ioctl_loop05.c      |  2 +-
 5 files changed, 38 insertions(+), 22 deletions(-)

-- 
2.40.0.348.gf938b09366-goog


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

[LTP] [PATCH v3 1/4] set_dev_path: Rename to set_dev_loop_path

[Edward Liaw via ltp]

Rename set_dev_path to set_dev_loop_path and the array dev_variants to
dev_loop_variants.  This will differentiate it from adding a similar
function that is checking a more generic dev path.

Signed-off-by: Edward Liaw <edliaw@google.com>
---
 lib/tst_device.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/lib/tst_device.c b/lib/tst_device.c
index a61c5a748..5d057570c 100644
--- a/lib/tst_device.c
+++ b/lib/tst_device.c
@@ -54,19 +54,19 @@ static char dev_path[PATH_MAX];
 static int device_acquired;
 static unsigned long prev_dev_sec_write;
 
-static const char *dev_variants[] = {
+static const char *dev_loop_variants[] = {
 	"/dev/loop%i",
 	"/dev/loop/%i",
 	"/dev/block/loop%i"
 };
 
-static int set_dev_path(int dev, char *path, size_t path_len)
+static int set_dev_loop_path(int 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);
+	for (i = 0; i < ARRAY_SIZE(dev_loop_variants); i++) {
+		snprintf(path, path_len, dev_loop_variants[i], dev);
 
 		if (stat(path, &st) == 0 && S_ISBLK(st.st_mode))
 			return 1;
@@ -89,7 +89,7 @@ int tst_find_free_loopdev(char *path, size_t path_len)
 		close(ctl_fd);
 		if (rc >= 0) {
 			if (path)
-				set_dev_path(rc, path, path_len);
+				set_dev_loop_path(rc, path, path_len);
 			tst_resm(TINFO, "Found free device %d '%s'",
 				rc, path ?: "");
 			return rc;
@@ -116,7 +116,7 @@ int tst_find_free_loopdev(char *path, size_t path_len)
 	 */
 	for (i = 0; i < 256; i++) {
 
-		if (!set_dev_path(i, buf, sizeof(buf)))
+		if (!set_dev_loop_path(i, buf, sizeof(buf)))
 			continue;
 
 		dev_fd = open(buf, O_RDONLY);
-- 
2.40.0.348.gf938b09366-goog


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

Re: [LTP] [PATCH v3 1/4] set_dev_path: Rename to set_dev_loop_path

[Petr Vorel]

Hi Edward,

Reviewed-by: Petr Vorel <pvorel@suse.cz>

Kind regards,
Petr

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

[LTP] [PATCH v3 2/4] set_dev_loop_path: Change return value to zero on success

[Edward Liaw via ltp]

set_dev_loop_path was returning 1 on success and 0 on error, which is
inconsistent with the other functions in this file.  Change it to 0 on
success and 1 on error.

Signed-off-by: Edward Liaw <edliaw@google.com>
---
 lib/tst_device.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/lib/tst_device.c b/lib/tst_device.c
index 5d057570c..2c83fb764 100644
--- a/lib/tst_device.c
+++ b/lib/tst_device.c
@@ -69,10 +69,10 @@ static int set_dev_loop_path(int dev, char *path, size_t path_len)
 		snprintf(path, path_len, dev_loop_variants[i], dev);
 
 		if (stat(path, &st) == 0 && S_ISBLK(st.st_mode))
-			return 1;
+			return 0;
 	}
 
-	return 0;
+	return 1;
 }
 
 int tst_find_free_loopdev(char *path, size_t path_len)
@@ -116,7 +116,7 @@ int tst_find_free_loopdev(char *path, size_t path_len)
 	 */
 	for (i = 0; i < 256; i++) {
 
-		if (!set_dev_loop_path(i, buf, sizeof(buf)))
+		if (set_dev_loop_path(i, buf, sizeof(buf)))
 			continue;
 
 		dev_fd = open(buf, O_RDONLY);
-- 
2.40.0.348.gf938b09366-goog


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

Re: [LTP] [PATCH v3 2/4] set_dev_loop_path: Change return value to zero on success

[Petr Vorel]

Hi Edward,

Reviewed-by: Petr Vorel <pvorel@suse.cz>

Kind regards,
Petr

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

[LTP] [PATCH v3 3/4] tst_find_free_loopdev: Check return value of set_dev_loop_path

[Edward Liaw via ltp]

tst_find_free_loopdev does not check the return value of set_dev_loop_path
and will return the last attempted path even if it does not pass a stat
check.  Change it to return TBROK if it fails to acquire a loop device.

Signed-off-by: Edward Liaw <edliaw@google.com>
---
 lib/tst_device.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/lib/tst_device.c b/lib/tst_device.c
index 2c83fb764..ba46b7613 100644
--- a/lib/tst_device.c
+++ b/lib/tst_device.c
@@ -88,8 +88,8 @@ int tst_find_free_loopdev(char *path, size_t path_len)
 		rc = ioctl(ctl_fd, LOOP_CTL_GET_FREE);
 		close(ctl_fd);
 		if (rc >= 0) {
-			if (path)
-				set_dev_loop_path(rc, path, path_len);
+			if (path && set_dev_loop_path(rc, path, path_len))
+				tst_brkm(TBROK, NULL, "Could not stat loop device %i", rc);
 			tst_resm(TINFO, "Found free device %d '%s'",
 				rc, path ?: "");
 			return rc;
-- 
2.40.0.348.gf938b09366-goog


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

Re: [LTP] [PATCH v3 3/4] tst_find_free_loopdev: Check return value of set_dev_loop_path

[Petr Vorel]

Hi Edward,

Reviewed-by: Petr Vorel <pvorel@suse.cz>

Kind regards,
Petr

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

[LTP] [PATCH v3 4/4] tst_find_backing_dev: Also check /dev/block/ for backing device

[Edward Liaw via ltp]

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(-)

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);
-- 
2.40.0.348.gf938b09366-goog


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

Re: [LTP] [PATCH v3 4/4] tst_find_backing_dev: Also check /dev/block/ for backing device

[Petr Vorel]

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);
>  	}

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

Re: [LTP] [PATCH v3 4/4] tst_find_backing_dev: Also check /dev/block/ for backing device

[Cyril Hrubis]

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>

-- 
Cyril Hrubis
chrubis@suse.cz

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