[v2,2/3] set_dev_loop_path: Refactor set_dev_path and check return value

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

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

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".

> 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