Re: [LTP] [PATCH v2 2/3] set_dev_loop_path: Refactor set_dev_path and check return value

[Petr Vorel <pvorel@suse.cz>]

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