[LTP] [PATCH] lib/tst_device.c: Add exception handling of function tst_find_free_loopdev

[LTP] [PATCH] lib/tst_device.c: Add exception handling of function tst_find_free_loopdev

[Zhao Gongyi]

Because of race condition or abnormal env, set_dev_path may be
return 1. And when set_dev_path return 1 in tst_find_free_loopdev,
we should return Immediately.

Signed-off-by: Zhao Gongyi <zhaogongyi@huawei.com>
---
 lib/tst_device.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/lib/tst_device.c b/lib/tst_device.c
index c096b418b..51cf1ba7e 100644
--- a/lib/tst_device.c
+++ b/lib/tst_device.c
@@ -83,8 +83,13 @@ 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)
+				if (!set_dev_path(rc, path, path_len)) {
+					tst_resm(TINFO,
+						"loop device not exist");
+					return -1;
+				}
+
 			tst_resm(TINFO, "Found free device %d '%s'",
 				rc, path ?: "");
 			return rc;
--
2.17.1

[LTP] [PATCH] lib/tst_device.c: Add exception handling of function tst_find_free_loopdev

[Petr Vorel]

Hi Gongyi,

> Because of race condition or abnormal env, set_dev_path may be
> return 1. And when set_dev_path return 1 in tst_find_free_loopdev,
You mean return 0, right?

> we should return Immediately.

Not really sure about this patch, mainly question for others.
Could you please more describe your problem?

Kind regards,
Petr

[LTP] [PATCH] lib/tst_device.c: Add exception handling of function tst_find_free_loopdev

[Li Wang]

Hi Gongyi,

On Wed, Mar 10, 2021 at 4:11 PM Zhao Gongyi <zhaogongyi@huawei.com> wrote:

> Because of race condition or abnormal env, set_dev_path may be
> return 1. And when set_dev_path return 1 in tst_find_free_loopdev,
> we should return Immediately.
>

If there exists a race condition, I firstly think of the TST_RETRY_FUNC
macro
to try more times for the set_dev_path. That might help to get the function
success
in the follow-up tries.



>
> Signed-off-by: Zhao Gongyi <zhaogongyi@huawei.com>
> ---
>  lib/tst_device.c | 9 +++++++--
>  1 file changed, 7 insertions(+), 2 deletions(-)
>
> diff --git a/lib/tst_device.c b/lib/tst_device.c
> index c096b418b..51cf1ba7e 100644
> --- a/lib/tst_device.c
> +++ b/lib/tst_device.c
> @@ -83,8 +83,13 @@ 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)
> +                               if (!set_dev_path(rc, path, path_len)) {
> +                                       tst_resm(TINFO,
> +                                               "loop device not exist");
> +                                       return -1;
> +                               }
> +
>                         tst_resm(TINFO, "Found free device %d '%s'",
>                                 rc, path ?: "");
>                         return rc;
> --
> 2.17.1
>
>
> --
> Mailing list info: https://lists.linux.it/listinfo/ltp
>
>

-- 
Regards,
Li Wang
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.linux.it/pipermail/ltp/attachments/20210310/d3a354e5/attachment.htm>

[LTP] [PATCH] lib/tst_device.c: Add exception handling of function tst_find_free_loopdev

[Petr Vorel]

Hi,

> > Because of race condition or abnormal env, set_dev_path may be
> > return 1. And when set_dev_path return 1 in tst_find_free_loopdev,
> > we should return Immediately.


> If there exists a race condition, I firstly think of the TST_RETRY_FUNC
> macro
> to try more times for the set_dev_path. That might help to get the function
> success
> in the follow-up tries.

+1

Kind regards,
Petr

[LTP] [PATCH] lib/tst_device.c: Add exception handling of function tst_find_free_loopdev

[zhaogongyi]

Hi Petr, Li,

At first, it seems ok that using ST_RETRY_FUNC macro to try more times for race condition.

In addition, when "loopdevno = ioctl(fd, LOOP_CTL_GET_FREE)" get free loop device i, and 
the loop device file /dev/loop%i has been removed?set_dev_path will return 1 and set " /dev/block/loop%i " 
in path. It might happened in many Embedded Systems because the test process's id is root always. So we could 
also Add exception handling?

Thanks!

Best Regards,
Gongyi

> 
> Hi,
> 
> > > Because of race condition or abnormal env, set_dev_path may be
> > > return 1. And when set_dev_path return 1 in tst_find_free_loopdev,
> > > we should return Immediately.
> 
> 
> > If there exists a race condition, I firstly think of the
> > TST_RETRY_FUNC macro to try more times for the set_dev_path. That
> > might help to get the function success in the follow-up tries.
> 
> +1
> 
> Kind regards,
> Petr

[LTP] [PATCH] lib/tst_device.c: Add exception handling of function tst_find_free_loopdev

[zhaogongyi]

Hi!
> In addition, when "loopdevno = ioctl(fd, LOOP_CTL_GET_FREE)" get free
> loop device i, and the loop device file /dev/loop%i has been removed?
> set_dev_path will return 1 and set " /dev/block/loop%i "
> in path. It might happened in many Embedded Systems because the test
> process's id is root always. So we could also Add exception handling?

Or maybe we can try to create it when node doesn't exist?
	mknod("/dev/loop%i", S_IFBLK|0644, makedev(7, i))

-----------------------------------------------------------
> 
> Hi Petr, Li,
> 
> At first, it seems ok that using ST_RETRY_FUNC macro to try more times for
> race condition.
> 
> In addition, when "loopdevno = ioctl(fd, LOOP_CTL_GET_FREE)" get free
> loop device i, and the loop device file /dev/loop%i has been removed?
> set_dev_path will return 1 and set " /dev/block/loop%i "
> in path. It might happened in many Embedded Systems because the test
> process's id is root always. So we could also Add exception handling?
> 
> Thanks!
> 
> Best Regards,
> Gongyi
> 
> >
> > Hi,
> >
> > > > Because of race condition or abnormal env, set_dev_path may be
> > > > return 1. And when set_dev_path return 1 in tst_find_free_loopdev,
> > > > we should return Immediately.
> >
> >
> > > If there exists a race condition, I firstly think of the
> > > TST_RETRY_FUNC macro to try more times for the set_dev_path. That
> > > might help to get the function success in the follow-up tries.
> >
> > +1
> >
> > Kind regards,
> > Petr

[LTP] [PATCH] lib/tst_device.c: Add exception handling of function tst_find_free_loopdev

[Cyril Hrubis]

Hi!
> > In addition, when "loopdevno = ioctl(fd, LOOP_CTL_GET_FREE)" get free
> > loop device i, and the loop device file /dev/loop%i has been removed??
> > set_dev_path will return 1 and set " /dev/block/loop%i "
> > in path. It might happened in many Embedded Systems because the test
> > process's id is root always. So we could also Add exception handling?
> 
> Or maybe we can try to create it when node doesn't exist?
> 	mknod("/dev/loop%i", S_IFBLK|0644, makedev(7, i))

I do not really get what happens on your system. It looks like dev fs is
not properly populated, which would be bug in your system rather than in
the test library.

What is the state of /dev/loop* and /dev/block/loop* before you attempt
to run the test?

-- 
Cyril Hrubis
chrubis@suse.cz

[LTP] [PATCH] lib/tst_device.c: Add exception handling of function tst_find_free_loopdev

[zhaogongyi]

Hi Cyril,

I mean that /dev/loopX maybe removed by mistake after we get a free loop dev /dev/loopX. And in this case, we could try to create it or report error info immediately?

Actually?it seems there has no Sufficient reason to do it.

Thanks so much!

Best Regards,
Gongyi

> 
> Hi!
> > > In addition, when "loopdevno = ioctl(fd, LOOP_CTL_GET_FREE)" get
> > > free loop device i, and the loop device file /dev/loop%i has been
> removed??
> > > set_dev_path will return 1 and set " /dev/block/loop%i "
> > > in path. It might happened in many Embedded Systems because the
> test
> > > process's id is root always. So we could also Add exception handling?
> >
> > Or maybe we can try to create it when node doesn't exist?
> > 	mknod("/dev/loop%i", S_IFBLK|0644, makedev(7, i))
> 
> I do not really get what happens on your system. It looks like dev fs is not
> properly populated, which would be bug in your system rather than in the
> test library.
> 
> What is the state of /dev/loop* and /dev/block/loop* before you attempt
> to run the test?
> 
> --
> Cyril Hrubis
> chrubis@suse.cz