* [LTP] [PATCH v2 0/3] tst_device.c: Handle Android path for backing device
@ 2023-03-20 23:51 Edward Liaw via ltp
2023-03-20 23:51 ` [LTP] [PATCH v2 1/3] tst_device.c: Use PATH_MAX more consistently Edward Liaw via ltp
` (3 more replies)
0 siblings, 4 replies; 10+ messages in thread
From: Edward Liaw via ltp @ 2023-03-20 23:51 UTC (permalink / raw)
To: ltp; +Cc: kernel-team
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.
Edward Liaw (3):
tst_device.c: Use PATH_MAX more consistently
set_dev_loop_path: Refactor set_dev_path and check return value
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 | 59 ++++++++++++-------
.../kernel/syscalls/ioctl/ioctl_loop05.c | 2 +-
5 files changed, 42 insertions(+), 26 deletions(-)
--
2.40.0.rc1.284.g88254d51c5-goog
--
Mailing list info: https://lists.linux.it/listinfo/ltp
^ permalink raw reply [flat|nested] 10+ messages in thread
* [LTP] [PATCH v2 1/3] tst_device.c: Use PATH_MAX more consistently
2023-03-20 23:51 [LTP] [PATCH v2 0/3] tst_device.c: Handle Android path for backing device Edward Liaw via ltp
@ 2023-03-20 23:51 ` Edward Liaw via ltp
2023-03-23 8:13 ` Petr Vorel
2023-03-20 23:51 ` [LTP] [PATCH v2 2/3] set_dev_loop_path: Refactor set_dev_path and check return value Edward Liaw via ltp
` (2 subsequent siblings)
3 siblings, 1 reply; 10+ messages in thread
From: Edward Liaw via ltp @ 2023-03-20 23:51 UTC (permalink / raw)
To: ltp; +Cc: kernel-team
Replace hardcoded 1024 path sizes with PATH_MAX.
Signed-off-by: Edward Liaw <edliaw@google.com>
---
lib/tst_device.c | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/lib/tst_device.c b/lib/tst_device.c
index 48d7e3ab6..a61c5a748 100644
--- a/lib/tst_device.c
+++ b/lib/tst_device.c
@@ -50,7 +50,7 @@
#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 char dev_path[PATH_MAX];
static int device_acquired;
static unsigned long prev_dev_sec_write;
@@ -79,7 +79,7 @@ int tst_find_free_loopdev(char *path, size_t path_len)
{
int ctl_fd, dev_fd, rc, i;
struct loop_info loopinfo;
- char buf[1024];
+ char buf[PATH_MAX];
/* since Linux 3.1 */
ctl_fd = open(LOOP_CONTROL_FILE, O_RDWR);
@@ -489,7 +489,7 @@ int find_stat_file(const char *dev, char *path, size_t path_len)
unsigned long tst_dev_bytes_written(const char *dev)
{
unsigned long dev_sec_write = 0, dev_bytes_written, io_ticks = 0;
- char dev_stat_path[1024];
+ char dev_stat_path[PATH_MAX];
if (!find_stat_file(dev, dev_stat_path, sizeof(dev_stat_path)))
tst_brkm(TCONF, NULL, "Test device stat file: %s not found",
@@ -641,7 +641,7 @@ int tst_dev_block_size(const char *path)
{
int fd;
int size;
- char dev_name[1024];
+ char dev_name[PATH_MAX];
tst_find_backing_dev(path, dev_name);
--
2.40.0.rc1.284.g88254d51c5-goog
--
Mailing list info: https://lists.linux.it/listinfo/ltp
^ permalink raw reply related [flat|nested] 10+ messages in thread
* [LTP] [PATCH v2 2/3] set_dev_loop_path: Refactor set_dev_path and check return value
2023-03-20 23:51 [LTP] [PATCH v2 0/3] tst_device.c: Handle Android path for backing device Edward Liaw via ltp
2023-03-20 23:51 ` [LTP] [PATCH v2 1/3] tst_device.c: Use PATH_MAX more consistently Edward Liaw via ltp
@ 2023-03-20 23:51 ` Edward Liaw via ltp
2023-03-23 8:52 ` Petr Vorel
2023-03-20 23:51 ` [LTP] [PATCH v2 3/3] tst_find_backing_dev: Also check /dev/block/ for backing device Edward Liaw via ltp
2023-03-23 8:24 ` [LTP] [PATCH v2 0/3] tst_device.c: Handle Android path " Petr Vorel
3 siblings, 1 reply; 10+ messages in thread
From: Edward Liaw via ltp @ 2023-03-20 23:51 UTC (permalink / raw)
To: ltp; +Cc: kernel-team
tst_find_free_loopdev does not check the return value of set_dev_path
and will return the last attempted path even if it does not pass a stat
check. set_dev_path also has a return value that is not consistent with
the other functions in this file.
Renames the function to set_dev_loop_path, the const array to
dev_loop_variants and changes the return value to 0 on success and 1 on
failure. Check the return value when called in tst_find_free_loopdev
for failure to acquire a loop device.
Signed-off-by: Edward Liaw <edliaw@google.com>
---
lib/tst_device.c | 18 +++++++++---------
1 file changed, 9 insertions(+), 9 deletions(-)
diff --git a/lib/tst_device.c b/lib/tst_device.c
index a61c5a748..ae665f7b6 100644
--- a/lib/tst_device.c
+++ b/lib/tst_device.c
@@ -54,25 +54,25 @@ 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;
+ return 0;
}
- return 0;
+ return 1;
}
int tst_find_free_loopdev(char *path, size_t path_len)
@@ -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_path(rc, path, path_len);
+ if (!path || set_dev_loop_path(rc, path, path_len) != 0)
+ tst_brkm(TBROK, NULL, "Could not stat loop device %i", rc);
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)) != 0)
continue;
dev_fd = open(buf, O_RDONLY);
--
2.40.0.rc1.284.g88254d51c5-goog
--
Mailing list info: https://lists.linux.it/listinfo/ltp
^ permalink raw reply related [flat|nested] 10+ messages in thread
* [LTP] [PATCH v2 3/3] tst_find_backing_dev: Also check /dev/block/ for backing device
2023-03-20 23:51 [LTP] [PATCH v2 0/3] tst_device.c: Handle Android path for backing device Edward Liaw via ltp
2023-03-20 23:51 ` [LTP] [PATCH v2 1/3] tst_device.c: Use PATH_MAX more consistently Edward Liaw via ltp
2023-03-20 23:51 ` [LTP] [PATCH v2 2/3] set_dev_loop_path: Refactor set_dev_path and check return value Edward Liaw via ltp
@ 2023-03-20 23:51 ` Edward Liaw via ltp
2023-03-23 8:53 ` Petr Vorel
2023-03-23 8:24 ` [LTP] [PATCH v2 0/3] tst_device.c: Handle Android path " Petr Vorel
3 siblings, 1 reply; 10+ messages in thread
From: Edward Liaw via ltp @ 2023-03-20 23:51 UTC (permalink / raw)
To: ltp; +Cc: kernel-team
On Android, the backing devices are created in /dev/block/ and will not
be found using the method added in e1b1ae66b240 ("tst_find_backing_dev:
Get dev name from /sys/dev/block/*/uevent"). 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 ae665f7b6..dbeae4c03 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.rc1.284.g88254d51c5-goog
--
Mailing list info: https://lists.linux.it/listinfo/ltp
^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [LTP] [PATCH v2 1/3] tst_device.c: Use PATH_MAX more consistently
2023-03-20 23:51 ` [LTP] [PATCH v2 1/3] tst_device.c: Use PATH_MAX more consistently Edward Liaw via ltp
@ 2023-03-23 8:13 ` Petr Vorel
0 siblings, 0 replies; 10+ messages in thread
From: Petr Vorel @ 2023-03-23 8:13 UTC (permalink / raw)
To: Edward Liaw; +Cc: kernel-team, ltp
Hi Edward,
obviously correct, merged.
Kind regards,
Petr
--
Mailing list info: https://lists.linux.it/listinfo/ltp
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [LTP] [PATCH v2 0/3] tst_device.c: Handle Android path for backing device
2023-03-20 23:51 [LTP] [PATCH v2 0/3] tst_device.c: Handle Android path for backing device Edward Liaw via ltp
` (2 preceding siblings ...)
2023-03-20 23:51 ` [LTP] [PATCH v2 3/3] tst_find_backing_dev: Also check /dev/block/ for backing device Edward Liaw via ltp
@ 2023-03-23 8:24 ` Petr Vorel
3 siblings, 0 replies; 10+ messages in thread
From: Petr Vorel @ 2023-03-23 8:24 UTC (permalink / raw)
To: Edward Liaw; +Cc: kernel-team, Alessandro Carminati, 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.
Cc the original autor.
I guess both problems were unnoticed because they are kind of corner cases
(minimal Linux and Android). But sure we're happily accept patches to fix them.
Kind regards,
Petr
> 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.
> Edward Liaw (3):
> tst_device.c: Use PATH_MAX more consistently
> set_dev_loop_path: Refactor set_dev_path and check return value
> 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 | 59 ++++++++++++-------
> .../kernel/syscalls/ioctl/ioctl_loop05.c | 2 +-
> 5 files changed, 42 insertions(+), 26 deletions(-)
--
Mailing list info: https://lists.linux.it/listinfo/ltp
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [LTP] [PATCH v2 2/3] set_dev_loop_path: Refactor set_dev_path and check return value
2023-03-20 23:51 ` [LTP] [PATCH v2 2/3] set_dev_loop_path: Refactor set_dev_path and check return value Edward Liaw via ltp
@ 2023-03-23 8:52 ` Petr Vorel
2023-03-23 23:47 ` Edward Liaw via ltp
0 siblings, 1 reply; 10+ messages in thread
From: Petr Vorel @ 2023-03-23 8:52 UTC (permalink / raw)
To: Edward Liaw; +Cc: kernel-team, Alessandro Carminati, ltp
Hi Edward,
> tst_find_free_loopdev does not check the return value of set_dev_path
> and will return the last attempted path even if it does not pass a stat
> check. set_dev_path also has a return value that is not consistent with
> the other functions in this file.
This change and change of return is a bit burden in loop rename changes.
I'm ok it's in single patch, but it'd be more readable if it were separate.
> Renames the function to set_dev_loop_path, the const array to
> dev_loop_variants and changes the return value to 0 on success and 1 on
> failure. Check the return value when called in tst_find_free_loopdev
> for failure to acquire a loop device.
> Signed-off-by: Edward Liaw <edliaw@google.com>
> ---
> lib/tst_device.c | 18 +++++++++---------
> 1 file changed, 9 insertions(+), 9 deletions(-)
> diff --git a/lib/tst_device.c b/lib/tst_device.c
> index a61c5a748..ae665f7b6 100644
> --- a/lib/tst_device.c
> +++ b/lib/tst_device.c
> @@ -54,25 +54,25 @@ 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;
> + return 0;
> }
> - return 0;
> + return 1;
> }
> int tst_find_free_loopdev(char *path, size_t path_len)
> @@ -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_path(rc, path, path_len);
> + if (!path || set_dev_loop_path(rc, path, path_len) != 0)
> + tst_brkm(TBROK, NULL, "Could not stat loop device %i", rc);
set_dev_path() is going to be called only if non-NULL path
(see include/tst_device.h). I haven't found a test which uses it this way,
but shouldn't it be checking path, instead of !path?
if (path && set_dev_loop_path(rc, path, path_len) != 0)
Kind regards,
Petr
> 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)) != 0)
> continue;
> dev_fd = open(buf, O_RDONLY);
--
Mailing list info: https://lists.linux.it/listinfo/ltp
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [LTP] [PATCH v2 3/3] tst_find_backing_dev: Also check /dev/block/ for backing device
2023-03-20 23:51 ` [LTP] [PATCH v2 3/3] tst_find_backing_dev: Also check /dev/block/ for backing device Edward Liaw via ltp
@ 2023-03-23 8:53 ` Petr Vorel
0 siblings, 0 replies; 10+ messages in thread
From: Petr Vorel @ 2023-03-23 8:53 UTC (permalink / raw)
To: Edward Liaw; +Cc: kernel-team, ltp
Hi Edward,
> On Android, the backing devices are created in /dev/block/ and will not
> be found using the method added in e1b1ae66b240 ("tst_find_backing_dev:
> Get dev name from /sys/dev/block/*/uevent"). Adds a check for
> /dev/block/%s as well as /dev/%s.
Fixes: e1b1ae66b240 ("tst_find_backing_dev: Get dev name from /sys/dev/block/*/uevent")
Kind regards,
Petr
--
Mailing list info: https://lists.linux.it/listinfo/ltp
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [LTP] [PATCH v2 2/3] set_dev_loop_path: Refactor set_dev_path and check return value
2023-03-23 8:52 ` Petr Vorel
@ 2023-03-23 23:47 ` Edward Liaw via ltp
2023-03-24 6:12 ` Petr Vorel
0 siblings, 1 reply; 10+ messages in thread
From: Edward Liaw via ltp @ 2023-03-23 23:47 UTC (permalink / raw)
To: Petr Vorel; +Cc: kernel-team, Alessandro Carminati, ltp
On Thu, Mar 23, 2023 at 1:52 AM Petr Vorel <pvorel@suse.cz> wrote:
>
> Hi Edward,
>
> > tst_find_free_loopdev does not check the return value of set_dev_path
> > and will return the last attempted path even if it does not pass a stat
> > check. set_dev_path also has a return value that is not consistent with
> > the other functions in this file.
>
> This change and change of return is a bit burden in loop rename changes.
> I'm ok it's in single patch, but it'd be more readable if it were separate.
Not a problem, I will split it.
> set_dev_path() is going to be called only if non-NULL path
> (see include/tst_device.h). I haven't found a test which uses it this way,
> but shouldn't it be checking path, instead of !path?
>
> if (path && set_dev_loop_path(rc, path, path_len) != 0)
>
> Kind regards,
> Petr
Oops, I missed that in the comment and thought a NULL path should be
checked as an error. You are right, I will change it. Also, I wasn't
sure if I should be explicit with the "!= 0".
--
Mailing list info: https://lists.linux.it/listinfo/ltp
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [LTP] [PATCH v2 2/3] set_dev_loop_path: Refactor set_dev_path and check return value
2023-03-23 23:47 ` Edward Liaw via ltp
@ 2023-03-24 6:12 ` Petr Vorel
0 siblings, 0 replies; 10+ messages in thread
From: Petr Vorel @ 2023-03-24 6:12 UTC (permalink / raw)
To: Edward Liaw; +Cc: kernel-team, Alessandro Carminati, ltp
> On Thu, Mar 23, 2023 at 1:52 AM Petr Vorel <pvorel@suse.cz> wrote:
> > Hi Edward,
> > > tst_find_free_loopdev does not check the return value of set_dev_path
> > > and will return the last attempted path even if it does not pass a stat
> > > check. set_dev_path also has a return value that is not consistent with
> > > the other functions in this file.
> > This change and change of return is a bit burden in loop rename changes.
> > I'm ok it's in single patch, but it'd be more readable if it were separate.
> Not a problem, I will split it.
+1
Also, please rebase (I pushed some changes) which touch files you also modify.
> > set_dev_path() is going to be called only if non-NULL path
> > (see include/tst_device.h). I haven't found a test which uses it this way,
> > but shouldn't it be checking path, instead of !path?
> > if (path && set_dev_loop_path(rc, path, path_len) != 0)
> > Kind regards,
> > Petr
> Oops, I missed that in the comment and thought a NULL path should be
> checked as an error. You are right, I will change it. Also, I wasn't
> sure if I should be explicit with the "!= 0".
I guess we are quite ok with just "!". We try to be precise at syscalls testing
(to check if the return value on error is exactly -1, not just < 0, but with
normal non-testing code like this in tst_kernel.c it's not needed.
Kind regards,
Petr
--
Mailing list info: https://lists.linux.it/listinfo/ltp
^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2023-03-24 6:12 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-03-20 23:51 [LTP] [PATCH v2 0/3] tst_device.c: Handle Android path for backing device Edward Liaw via ltp
2023-03-20 23:51 ` [LTP] [PATCH v2 1/3] tst_device.c: Use PATH_MAX more consistently Edward Liaw via ltp
2023-03-23 8:13 ` Petr Vorel
2023-03-20 23:51 ` [LTP] [PATCH v2 2/3] set_dev_loop_path: Refactor set_dev_path and check return value Edward Liaw via ltp
2023-03-23 8:52 ` Petr Vorel
2023-03-23 23:47 ` Edward Liaw via ltp
2023-03-24 6:12 ` Petr Vorel
2023-03-20 23:51 ` [LTP] [PATCH v2 3/3] tst_find_backing_dev: Also check /dev/block/ for backing device Edward Liaw via ltp
2023-03-23 8:53 ` Petr Vorel
2023-03-23 8:24 ` [LTP] [PATCH v2 0/3] tst_device.c: Handle Android path " Petr Vorel
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).