[PATCH 0/3] cleanup couple of device-related functions' retval

[PATCH 0/3] cleanup couple of device-related functions' retval

[Nikolay Borisov]

Currently btrfs_find_device_by_path, btrfs_find_device_missing_or_by_path and 
btrfs_find_device_by_devspec are called in a chain and they all return an 
integer value to signal error and at the same time use one of their parameters
as an output. This patch set refactors those functions starting from the 
bottom, gradually making them return a pointer to btrfs_device. This is 
sufficient to convey an error when it occurs as well as return the actual 
device we are looking for. One added benefit is that the ioctl-specific positive
return value BTRFS_ERROR_DEV_MISSING_NOT_FOUND is now returned from 
btrfs_rm_device rather than from some internal function. 

Additionally I'll be sending a patch to progs, adding a test ensuring that 
BTRFS_ERROR_MISSING_NOT_FOUND is returned appropriately. 

This survived both my btrfs-progs test as well as xfstest run. No functional 
changes.


Nikolay Borisov (3):
  btrfs: Make btrfs_find_device_by_path return struct btrfs_device
  btrfs: Make btrfs_find_device_missing_or_by_path return directly a
    device
  btrfs: Make btrfs_find_device_by_devspec return btrfs_device directly

 fs/btrfs/dev-replace.c |  8 ++---
 fs/btrfs/volumes.c     | 73 ++++++++++++++++++++++--------------------
 fs/btrfs/volumes.h     |  9 ++----
 3 files changed, 45 insertions(+), 45 deletions(-)

-- 
2.17.1

[PATCH 1/3] btrfs: Make btrfs_find_device_by_path return struct btrfs_device

[Nikolay Borisov]

Currently this function returns an error code as well as uses one of
its arguments as a return value for struct btrfs_device. Change the
function so that it returns btrfs_device directly and use the usual
"encode error in pointer" mechanics if something goes wrong. No
functional changes.

Signed-off-by: Nikolay Borisov <nborisov@suse.com>
---
 fs/btrfs/volumes.c | 26 ++++++++++++++------------
 1 file changed, 14 insertions(+), 12 deletions(-)

diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
index da86706123ff..715ea45c6c28 100644
--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c
@@ -2096,9 +2096,9 @@ void btrfs_destroy_dev_replace_tgtdev(struct btrfs_device *tgtdev)
 	call_rcu(&tgtdev->rcu, free_device_rcu);
 }
 
-static int btrfs_find_device_by_path(struct btrfs_fs_info *fs_info,
-				     const char *device_path,
-				     struct btrfs_device **device)
+static struct btrfs_device *
+btrfs_find_device_by_path(struct btrfs_fs_info *fs_info,
+			  const char *device_path)
 {
 	int ret = 0;
 	struct btrfs_super_block *disk_super;
@@ -2106,21 +2106,21 @@ static int btrfs_find_device_by_path(struct btrfs_fs_info *fs_info,
 	u8 *dev_uuid;
 	struct block_device *bdev;
 	struct buffer_head *bh;
+	struct btrfs_device *device;
 
-	*device = NULL;
 	ret = btrfs_get_bdev_and_sb(device_path, FMODE_READ,
 				    fs_info->bdev_holder, 0, &bdev, &bh);
 	if (ret)
-		return ret;
+		return ERR_PTR(ret);
 	disk_super = (struct btrfs_super_block *)bh->b_data;
 	devid = btrfs_stack_device_id(&disk_super->dev_item);
 	dev_uuid = disk_super->dev_item.uuid;
-	*device = btrfs_find_device(fs_info, devid, dev_uuid, disk_super->fsid);
+	device = btrfs_find_device(fs_info, devid, dev_uuid, disk_super->fsid);
 	brelse(bh);
-	if (!*device)
-		ret = -ENOENT;
+	if (!device)
+		device = ERR_PTR(-ENOENT);
 	blkdev_put(bdev, FMODE_READ);
-	return ret;
+	return device;
 }
 
 int btrfs_find_device_missing_or_by_path(struct btrfs_fs_info *fs_info,
@@ -2143,11 +2143,13 @@ int btrfs_find_device_missing_or_by_path(struct btrfs_fs_info *fs_info,
 
 		if (!*device)
 			return BTRFS_ERROR_DEV_MISSING_NOT_FOUND;
-
-		return 0;
 	} else {
-		return btrfs_find_device_by_path(fs_info, device_path, device);
+		*device = btrfs_find_device_by_path(fs_info, device_path);
+		if (IS_ERR(*device))
+			return PTR_ERR(*device);
 	}
+
+	return 0;
 }
 
 /*
-- 
2.17.1

[PATCH 2/3] btrfs: Make btrfs_find_device_missing_or_by_path return directly a device

[Nikolay Borisov]

This function returns a numeric error value and additionally the
device found in one of its input parameters. Simplify this by making
the function directly return a pointer to btrfs_device. Additionally
adjust the caller to handle the case when we want to remove the
'missing' device and ENOENT is returned to return the expected
positive error value, parsed by progs. Finally, unexport the function
since it's not called outside of volume.c. No functional changes.

Signed-off-by: Nikolay Borisov <nborisov@suse.com>
---
 fs/btrfs/volumes.c | 33 ++++++++++++++++++---------------
 fs/btrfs/volumes.h |  3 ---
 2 files changed, 18 insertions(+), 18 deletions(-)

diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
index 715ea45c6c28..6202aa15d0d7 100644
--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c
@@ -2123,11 +2123,11 @@ btrfs_find_device_by_path(struct btrfs_fs_info *fs_info,
 	return device;
 }
 
-int btrfs_find_device_missing_or_by_path(struct btrfs_fs_info *fs_info,
-					 const char *device_path,
-					 struct btrfs_device **device)
+static struct btrfs_device *
+btrfs_find_device_missing_or_by_path(struct btrfs_fs_info *fs_info,
+				     const char *device_path)
 {
-	*device = NULL;
+	struct btrfs_device *device = NULL;
 	if (strcmp(device_path, "missing") == 0) {
 		struct list_head *devices;
 		struct btrfs_device *tmp;
@@ -2136,20 +2136,18 @@ int btrfs_find_device_missing_or_by_path(struct btrfs_fs_info *fs_info,
 		list_for_each_entry(tmp, devices, dev_list) {
 			if (test_bit(BTRFS_DEV_STATE_IN_FS_METADATA,
 					&tmp->dev_state) && !tmp->bdev) {
-				*device = tmp;
+				device = tmp;
 				break;
 			}
 		}
 
-		if (!*device)
-			return BTRFS_ERROR_DEV_MISSING_NOT_FOUND;
+		if (!device)
+			return ERR_PTR(-ENOENT);
 	} else {
-		*device = btrfs_find_device_by_path(fs_info, device_path);
-		if (IS_ERR(*device))
-			return PTR_ERR(*device);
+		device = btrfs_find_device_by_path(fs_info, device_path);
 	}
 
-	return 0;
+	return device;
 }
 
 /*
@@ -2159,10 +2157,9 @@ int btrfs_find_device_by_devspec(struct btrfs_fs_info *fs_info, u64 devid,
 				 const char *devpath,
 				 struct btrfs_device **device)
 {
-	int ret;
+	int ret = 0;
 
 	if (devid) {
-		ret = 0;
 		*device = btrfs_find_device(fs_info, devid, NULL, NULL);
 		if (!*device)
 			ret = -ENOENT;
@@ -2170,8 +2167,14 @@ int btrfs_find_device_by_devspec(struct btrfs_fs_info *fs_info, u64 devid,
 		if (!devpath || !devpath[0])
 			return -EINVAL;
 
-		ret = btrfs_find_device_missing_or_by_path(fs_info, devpath,
-							   device);
+		*device = btrfs_find_device_missing_or_by_path(fs_info, devpath);
+		if (IS_ERR(*device)) {
+			if (PTR_ERR(*device) == -ENOENT &&
+			    strcmp(devpath, "missing") == 0)
+				ret = BTRFS_ERROR_DEV_MISSING_NOT_FOUND;
+			else
+				ret = PTR_ERR(*device);
+		}
 	}
 	return ret;
 }
diff --git a/fs/btrfs/volumes.h b/fs/btrfs/volumes.h
index 23e9285d88de..e7811473024d 100644
--- a/fs/btrfs/volumes.h
+++ b/fs/btrfs/volumes.h
@@ -410,9 +410,6 @@ int btrfs_close_devices(struct btrfs_fs_devices *fs_devices);
 void btrfs_free_extra_devids(struct btrfs_fs_devices *fs_devices, int step);
 void btrfs_assign_next_active_device(struct btrfs_device *device,
 				     struct btrfs_device *this_dev);
-int btrfs_find_device_missing_or_by_path(struct btrfs_fs_info *fs_info,
-					 const char *device_path,
-					 struct btrfs_device **device);
 int btrfs_find_device_by_devspec(struct btrfs_fs_info *fs_info, u64 devid,
 					 const char *devpath,
 					 struct btrfs_device **device);
-- 
2.17.1

[PATCH 3/3] btrfs: Make btrfs_find_device_by_devspec return btrfs_device directly

[Nikolay Borisov]

Instead of returning an error value and using one of the parameters for
returning the actual object we are interested in just refactor the
function to directly return btrfs_device *. Also bubble up the error
handling for the special BTRFS_ERROR_DEV_MISSING_NOT_FOUND value into
btrfs_rm_device. No functional changes.

Signed-off-by: Nikolay Borisov <nborisov@suse.com>
---
 fs/btrfs/dev-replace.c |  8 ++++----
 fs/btrfs/volumes.c     | 40 +++++++++++++++++++---------------------
 fs/btrfs/volumes.h     |  6 +++---
 3 files changed, 26 insertions(+), 28 deletions(-)

diff --git a/fs/btrfs/dev-replace.c b/fs/btrfs/dev-replace.c
index dec01970d8c5..4e2b67d06305 100644
--- a/fs/btrfs/dev-replace.c
+++ b/fs/btrfs/dev-replace.c
@@ -409,10 +409,10 @@ int btrfs_dev_replace_start(struct btrfs_fs_info *fs_info,
 	struct btrfs_device *tgt_device = NULL;
 	struct btrfs_device *src_device = NULL;
 
-	ret = btrfs_find_device_by_devspec(fs_info, srcdevid,
-					    srcdev_name, &src_device);
-	if (ret)
-		return ret;
+	src_device = btrfs_find_device_by_devspec(fs_info, srcdevid,
+						  srcdev_name);
+	if (IS_ERR(src_device))
+		return PTR_ERR(src_device);
 
 	ret = btrfs_init_dev_replace_tgtdev(fs_info, tgtdev_name,
 					    src_device, &tgt_device);
diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
index 6202aa15d0d7..ce336b39fa0f 100644
--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c
@@ -1877,10 +1877,16 @@ int btrfs_rm_device(struct btrfs_fs_info *fs_info, const char *device_path,
 	if (ret)
 		goto out;
 
-	ret = btrfs_find_device_by_devspec(fs_info, devid, device_path,
-					   &device);
-	if (ret)
+	device = btrfs_find_device_by_devspec(fs_info, devid, device_path);
+
+	if (IS_ERR(device)) {
+		if (PTR_ERR(device) == -ENOENT &&
+		    strcmp(device_path, "missing") == 0)
+			ret = BTRFS_ERROR_DEV_MISSING_NOT_FOUND;
+		else
+			ret = PTR_ERR(device);
 		goto out;
+	}
 
 	if (test_bit(BTRFS_DEV_STATE_REPLACE_TGT, &device->dev_state)) {
 		ret = BTRFS_ERROR_DEV_TGT_REPLACE;
@@ -2153,30 +2159,22 @@ btrfs_find_device_missing_or_by_path(struct btrfs_fs_info *fs_info,
 /*
  * Lookup a device given by device id, or the path if the id is 0.
  */
-int btrfs_find_device_by_devspec(struct btrfs_fs_info *fs_info, u64 devid,
-				 const char *devpath,
-				 struct btrfs_device **device)
+struct btrfs_device *
+btrfs_find_device_by_devspec(struct btrfs_fs_info *fs_info, u64 devid,
+			     const char *devpath)
 {
-	int ret = 0;
+	struct btrfs_device *device;
 
 	if (devid) {
-		*device = btrfs_find_device(fs_info, devid, NULL, NULL);
-		if (!*device)
-			ret = -ENOENT;
+		device = btrfs_find_device(fs_info, devid, NULL, NULL);
+		if (!device)
+			return ERR_PTR(-ENOENT);
 	} else {
 		if (!devpath || !devpath[0])
-			return -EINVAL;
-
-		*device = btrfs_find_device_missing_or_by_path(fs_info, devpath);
-		if (IS_ERR(*device)) {
-			if (PTR_ERR(*device) == -ENOENT &&
-			    strcmp(devpath, "missing") == 0)
-				ret = BTRFS_ERROR_DEV_MISSING_NOT_FOUND;
-			else
-				ret = PTR_ERR(*device);
-		}
+			return ERR_PTR(-EINVAL);
+		device = btrfs_find_device_missing_or_by_path(fs_info, devpath);
 	}
-	return ret;
+	return device;
 }
 
 /*
diff --git a/fs/btrfs/volumes.h b/fs/btrfs/volumes.h
index e7811473024d..aefce895e994 100644
--- a/fs/btrfs/volumes.h
+++ b/fs/btrfs/volumes.h
@@ -410,9 +410,9 @@ int btrfs_close_devices(struct btrfs_fs_devices *fs_devices);
 void btrfs_free_extra_devids(struct btrfs_fs_devices *fs_devices, int step);
 void btrfs_assign_next_active_device(struct btrfs_device *device,
 				     struct btrfs_device *this_dev);
-int btrfs_find_device_by_devspec(struct btrfs_fs_info *fs_info, u64 devid,
-					 const char *devpath,
-					 struct btrfs_device **device);
+struct btrfs_device *btrfs_find_device_by_devspec(struct btrfs_fs_info *fs_info,
+						  u64 devid,
+						  const char *devpath);
 struct btrfs_device *btrfs_alloc_device(struct btrfs_fs_info *fs_info,
 					const u64 *devid,
 					const u8 *uuid);
-- 
2.17.1

[PATCH] btrfs-progs: tests: Add test for missing device delete error value

[Nikolay Borisov]

Add a test which ensures the kernel returns the correct error value
when missing device removal is requested. This test verifies that kernel
refactoring didn't broken the return value.

Signed-off-by: Nikolay Borisov <nborisov@suse.com>
---
 tests/misc-tests/011-delete-missing-device/test.sh | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/tests/misc-tests/011-delete-missing-device/test.sh b/tests/misc-tests/011-delete-missing-device/test.sh
index 4c976421c091..b799a25c201d 100755
--- a/tests/misc-tests/011-delete-missing-device/test.sh
+++ b/tests/misc-tests/011-delete-missing-device/test.sh
@@ -44,6 +44,21 @@ test_delete_missing()
 	run_check_umount_test_dev
 }
 
+test_missing_error()
+{
+	run_check $SUDO_HELPER "$TOP/mkfs.btrfs" -f "$TEST_DEV"
+	run_check_mount_test_dev
+	local out
+	out=$(run_mustfail_stdout "Unexpected success" \
+		$SUDO_HELPER "$TOP/btrfs" device remove missing "$TEST_MNT")
+
+	if ! echo "$out" | grep -q "no missing devices found to remove"; then
+		_fail "IOCTL returned unexpected error value"
+	fi
+
+	run_check_umount_test_dev
+}
+
 setup_loopdevs 4
 prepare_loopdevs
 dev1=${loopdevs[1]}
@@ -53,5 +68,7 @@ TEST_DEV=$dev1
 test_do_mkfs -m raid1 -d raid1
 test_wipefs
 test_delete_missing
+test_missing_error
+
 
 cleanup_loopdevs
-- 
2.7.4

Re: [PATCH 1/3] btrfs: Make btrfs_find_device_by_path return struct btrfs_device

[Qu Wenruo]

On 2018/9/3 下午5:46, Nikolay Borisov wrote:
> Currently this function returns an error code as well as uses one of
> its arguments as a return value for struct btrfs_device. Change the
> function so that it returns btrfs_device directly and use the usual
> "encode error in pointer" mechanics if something goes wrong. No
> functional changes.
> 
> Signed-off-by: Nikolay Borisov <nborisov@suse.com>

Reviewed-by: Qu Wenruo <wqu@suse.com>

Thanks,
Qu

> ---
>  fs/btrfs/volumes.c | 26 ++++++++++++++------------
>  1 file changed, 14 insertions(+), 12 deletions(-)
> 
> diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
> index da86706123ff..715ea45c6c28 100644
> --- a/fs/btrfs/volumes.c
> +++ b/fs/btrfs/volumes.c
> @@ -2096,9 +2096,9 @@ void btrfs_destroy_dev_replace_tgtdev(struct btrfs_device *tgtdev)
>  	call_rcu(&tgtdev->rcu, free_device_rcu);
>  }
>  
> -static int btrfs_find_device_by_path(struct btrfs_fs_info *fs_info,
> -				     const char *device_path,
> -				     struct btrfs_device **device)
> +static struct btrfs_device *
> +btrfs_find_device_by_path(struct btrfs_fs_info *fs_info,
> +			  const char *device_path)
>  {
>  	int ret = 0;
>  	struct btrfs_super_block *disk_super;
> @@ -2106,21 +2106,21 @@ static int btrfs_find_device_by_path(struct btrfs_fs_info *fs_info,
>  	u8 *dev_uuid;
>  	struct block_device *bdev;
>  	struct buffer_head *bh;
> +	struct btrfs_device *device;
>  
> -	*device = NULL;
>  	ret = btrfs_get_bdev_and_sb(device_path, FMODE_READ,
>  				    fs_info->bdev_holder, 0, &bdev, &bh);
>  	if (ret)
> -		return ret;
> +		return ERR_PTR(ret);
>  	disk_super = (struct btrfs_super_block *)bh->b_data;
>  	devid = btrfs_stack_device_id(&disk_super->dev_item);
>  	dev_uuid = disk_super->dev_item.uuid;
> -	*device = btrfs_find_device(fs_info, devid, dev_uuid, disk_super->fsid);
> +	device = btrfs_find_device(fs_info, devid, dev_uuid, disk_super->fsid);
>  	brelse(bh);
> -	if (!*device)
> -		ret = -ENOENT;
> +	if (!device)
> +		device = ERR_PTR(-ENOENT);
>  	blkdev_put(bdev, FMODE_READ);
> -	return ret;
> +	return device;
>  }
>  
>  int btrfs_find_device_missing_or_by_path(struct btrfs_fs_info *fs_info,
> @@ -2143,11 +2143,13 @@ int btrfs_find_device_missing_or_by_path(struct btrfs_fs_info *fs_info,
>  
>  		if (!*device)
>  			return BTRFS_ERROR_DEV_MISSING_NOT_FOUND;
> -
> -		return 0;
>  	} else {
> -		return btrfs_find_device_by_path(fs_info, device_path, device);
> +		*device = btrfs_find_device_by_path(fs_info, device_path);
> +		if (IS_ERR(*device))
> +			return PTR_ERR(*device);
>  	}
> +
> +	return 0;
>  }
>  
>  /*
> 

Re: [PATCH 2/3] btrfs: Make btrfs_find_device_missing_or_by_path return directly a device

[Qu Wenruo]

On 2018/9/3 下午5:46, Nikolay Borisov wrote:
> This function returns a numeric error value and additionally the
> device found in one of its input parameters. Simplify this by making
> the function directly return a pointer to btrfs_device. Additionally
> adjust the caller to handle the case when we want to remove the
> 'missing' device and ENOENT is returned to return the expected
> positive error value, parsed by progs. Finally, unexport the function
> since it's not called outside of volume.c. No functional changes.
> 
> Signed-off-by: Nikolay Borisov <nborisov@suse.com>

Reviewed-by: Qu Wenruo <wqu@suse.com>

Just changed when BTRFS_ERROR_DEV_MISSING_NOT_FOUND is returned, and
since btrfs_find_device_missing_or_by_path() is only called in
btrfs_find_device_by_devspec(), there is indeed no functional change.

Thanks,
Qu

> ---
>  fs/btrfs/volumes.c | 33 ++++++++++++++++++---------------
>  fs/btrfs/volumes.h |  3 ---
>  2 files changed, 18 insertions(+), 18 deletions(-)
> 
> diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
> index 715ea45c6c28..6202aa15d0d7 100644
> --- a/fs/btrfs/volumes.c
> +++ b/fs/btrfs/volumes.c
> @@ -2123,11 +2123,11 @@ btrfs_find_device_by_path(struct btrfs_fs_info *fs_info,
>  	return device;
>  }
>  
> -int btrfs_find_device_missing_or_by_path(struct btrfs_fs_info *fs_info,
> -					 const char *device_path,
> -					 struct btrfs_device **device)
> +static struct btrfs_device *
> +btrfs_find_device_missing_or_by_path(struct btrfs_fs_info *fs_info,
> +				     const char *device_path)
>  {
> -	*device = NULL;
> +	struct btrfs_device *device = NULL;
>  	if (strcmp(device_path, "missing") == 0) {
>  		struct list_head *devices;
>  		struct btrfs_device *tmp;
> @@ -2136,20 +2136,18 @@ int btrfs_find_device_missing_or_by_path(struct btrfs_fs_info *fs_info,
>  		list_for_each_entry(tmp, devices, dev_list) {
>  			if (test_bit(BTRFS_DEV_STATE_IN_FS_METADATA,
>  					&tmp->dev_state) && !tmp->bdev) {
> -				*device = tmp;
> +				device = tmp;
>  				break;
>  			}
>  		}
>  
> -		if (!*device)
> -			return BTRFS_ERROR_DEV_MISSING_NOT_FOUND;
> +		if (!device)
> +			return ERR_PTR(-ENOENT);
>  	} else {
> -		*device = btrfs_find_device_by_path(fs_info, device_path);
> -		if (IS_ERR(*device))
> -			return PTR_ERR(*device);
> +		device = btrfs_find_device_by_path(fs_info, device_path);
>  	}
>  
> -	return 0;
> +	return device;
>  }
>  
>  /*
> @@ -2159,10 +2157,9 @@ int btrfs_find_device_by_devspec(struct btrfs_fs_info *fs_info, u64 devid,
>  				 const char *devpath,
>  				 struct btrfs_device **device)
>  {
> -	int ret;
> +	int ret = 0;
>  
>  	if (devid) {
> -		ret = 0;
>  		*device = btrfs_find_device(fs_info, devid, NULL, NULL);
>  		if (!*device)
>  			ret = -ENOENT;
> @@ -2170,8 +2167,14 @@ int btrfs_find_device_by_devspec(struct btrfs_fs_info *fs_info, u64 devid,
>  		if (!devpath || !devpath[0])
>  			return -EINVAL;
>  
> -		ret = btrfs_find_device_missing_or_by_path(fs_info, devpath,
> -							   device);
> +		*device = btrfs_find_device_missing_or_by_path(fs_info, devpath);
> +		if (IS_ERR(*device)) {
> +			if (PTR_ERR(*device) == -ENOENT &&
> +			    strcmp(devpath, "missing") == 0)
> +				ret = BTRFS_ERROR_DEV_MISSING_NOT_FOUND;
> +			else
> +				ret = PTR_ERR(*device);
> +		}
>  	}
>  	return ret;
>  }
> diff --git a/fs/btrfs/volumes.h b/fs/btrfs/volumes.h
> index 23e9285d88de..e7811473024d 100644
> --- a/fs/btrfs/volumes.h
> +++ b/fs/btrfs/volumes.h
> @@ -410,9 +410,6 @@ int btrfs_close_devices(struct btrfs_fs_devices *fs_devices);
>  void btrfs_free_extra_devids(struct btrfs_fs_devices *fs_devices, int step);
>  void btrfs_assign_next_active_device(struct btrfs_device *device,
>  				     struct btrfs_device *this_dev);
> -int btrfs_find_device_missing_or_by_path(struct btrfs_fs_info *fs_info,
> -					 const char *device_path,
> -					 struct btrfs_device **device);
>  int btrfs_find_device_by_devspec(struct btrfs_fs_info *fs_info, u64 devid,
>  					 const char *devpath,
>  					 struct btrfs_device **device);
> 

Re: [PATCH 3/3] btrfs: Make btrfs_find_device_by_devspec return btrfs_device directly

[Qu Wenruo]

On 2018/9/3 下午5:46, Nikolay Borisov wrote:
> Instead of returning an error value and using one of the parameters for
> returning the actual object we are interested in just refactor the
> function to directly return btrfs_device *. Also bubble up the error
> handling for the special BTRFS_ERROR_DEV_MISSING_NOT_FOUND value into
> btrfs_rm_device. No functional changes.
> 
> Signed-off-by: Nikolay Borisov <nborisov@suse.com>

Reviewed-by: Qu Wenruo <wqu@suse.com>

Thanks,
Qu

> ---
>  fs/btrfs/dev-replace.c |  8 ++++----
>  fs/btrfs/volumes.c     | 40 +++++++++++++++++++---------------------
>  fs/btrfs/volumes.h     |  6 +++---
>  3 files changed, 26 insertions(+), 28 deletions(-)
> 
> diff --git a/fs/btrfs/dev-replace.c b/fs/btrfs/dev-replace.c
> index dec01970d8c5..4e2b67d06305 100644
> --- a/fs/btrfs/dev-replace.c
> +++ b/fs/btrfs/dev-replace.c
> @@ -409,10 +409,10 @@ int btrfs_dev_replace_start(struct btrfs_fs_info *fs_info,
>  	struct btrfs_device *tgt_device = NULL;
>  	struct btrfs_device *src_device = NULL;
>  
> -	ret = btrfs_find_device_by_devspec(fs_info, srcdevid,
> -					    srcdev_name, &src_device);
> -	if (ret)
> -		return ret;
> +	src_device = btrfs_find_device_by_devspec(fs_info, srcdevid,
> +						  srcdev_name);
> +	if (IS_ERR(src_device))
> +		return PTR_ERR(src_device);
>  
>  	ret = btrfs_init_dev_replace_tgtdev(fs_info, tgtdev_name,
>  					    src_device, &tgt_device);
> diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
> index 6202aa15d0d7..ce336b39fa0f 100644
> --- a/fs/btrfs/volumes.c
> +++ b/fs/btrfs/volumes.c
> @@ -1877,10 +1877,16 @@ int btrfs_rm_device(struct btrfs_fs_info *fs_info, const char *device_path,
>  	if (ret)
>  		goto out;
>  
> -	ret = btrfs_find_device_by_devspec(fs_info, devid, device_path,
> -					   &device);
> -	if (ret)
> +	device = btrfs_find_device_by_devspec(fs_info, devid, device_path);
> +
> +	if (IS_ERR(device)) {
> +		if (PTR_ERR(device) == -ENOENT &&
> +		    strcmp(device_path, "missing") == 0)
> +			ret = BTRFS_ERROR_DEV_MISSING_NOT_FOUND;
> +		else
> +			ret = PTR_ERR(device);
>  		goto out;
> +	}
>  
>  	if (test_bit(BTRFS_DEV_STATE_REPLACE_TGT, &device->dev_state)) {
>  		ret = BTRFS_ERROR_DEV_TGT_REPLACE;
> @@ -2153,30 +2159,22 @@ btrfs_find_device_missing_or_by_path(struct btrfs_fs_info *fs_info,
>  /*
>   * Lookup a device given by device id, or the path if the id is 0.
>   */
> -int btrfs_find_device_by_devspec(struct btrfs_fs_info *fs_info, u64 devid,
> -				 const char *devpath,
> -				 struct btrfs_device **device)
> +struct btrfs_device *
> +btrfs_find_device_by_devspec(struct btrfs_fs_info *fs_info, u64 devid,
> +			     const char *devpath)
>  {
> -	int ret = 0;
> +	struct btrfs_device *device;
>  
>  	if (devid) {
> -		*device = btrfs_find_device(fs_info, devid, NULL, NULL);
> -		if (!*device)
> -			ret = -ENOENT;
> +		device = btrfs_find_device(fs_info, devid, NULL, NULL);
> +		if (!device)
> +			return ERR_PTR(-ENOENT);
>  	} else {
>  		if (!devpath || !devpath[0])
> -			return -EINVAL;
> -
> -		*device = btrfs_find_device_missing_or_by_path(fs_info, devpath);
> -		if (IS_ERR(*device)) {
> -			if (PTR_ERR(*device) == -ENOENT &&
> -			    strcmp(devpath, "missing") == 0)
> -				ret = BTRFS_ERROR_DEV_MISSING_NOT_FOUND;
> -			else
> -				ret = PTR_ERR(*device);
> -		}
> +			return ERR_PTR(-EINVAL);
> +		device = btrfs_find_device_missing_or_by_path(fs_info, devpath);
>  	}
> -	return ret;
> +	return device;
>  }
>  
>  /*
> diff --git a/fs/btrfs/volumes.h b/fs/btrfs/volumes.h
> index e7811473024d..aefce895e994 100644
> --- a/fs/btrfs/volumes.h
> +++ b/fs/btrfs/volumes.h
> @@ -410,9 +410,9 @@ int btrfs_close_devices(struct btrfs_fs_devices *fs_devices);
>  void btrfs_free_extra_devids(struct btrfs_fs_devices *fs_devices, int step);
>  void btrfs_assign_next_active_device(struct btrfs_device *device,
>  				     struct btrfs_device *this_dev);
> -int btrfs_find_device_by_devspec(struct btrfs_fs_info *fs_info, u64 devid,
> -					 const char *devpath,
> -					 struct btrfs_device **device);
> +struct btrfs_device *btrfs_find_device_by_devspec(struct btrfs_fs_info *fs_info,
> +						  u64 devid,
> +						  const char *devpath);
>  struct btrfs_device *btrfs_alloc_device(struct btrfs_fs_info *fs_info,
>  					const u64 *devid,
>  					const u8 *uuid);
> 

Re: [PATCH] btrfs-progs: tests: Add test for missing device delete error value

[David Sterba]

On Mon, Sep 03, 2018 at 01:02:57PM +0300, Nikolay Borisov wrote:
> Add a test which ensures the kernel returns the correct error value
> when missing device removal is requested. This test verifies that kernel
> refactoring didn't broken the return value.
> 
> Signed-off-by: Nikolay Borisov <nborisov@suse.com>
> ---
>  tests/misc-tests/011-delete-missing-device/test.sh | 17 +++++++++++++++++
>  1 file changed, 17 insertions(+)
> 
> diff --git a/tests/misc-tests/011-delete-missing-device/test.sh b/tests/misc-tests/011-delete-missing-device/test.sh
> index 4c976421c091..b799a25c201d 100755
> --- a/tests/misc-tests/011-delete-missing-device/test.sh
> +++ b/tests/misc-tests/011-delete-missing-device/test.sh
> @@ -44,6 +44,21 @@ test_delete_missing()
>  	run_check_umount_test_dev
>  }
>  
> +test_missing_error()
> +{
> +	run_check $SUDO_HELPER "$TOP/mkfs.btrfs" -f "$TEST_DEV"
> +	run_check_mount_test_dev
> +	local out
> +	out=$(run_mustfail_stdout "Unexpected success" \

Which would become:

  unexpected success: Unexpected success

You'd win the error message of the year :) The text is supposed to be
more specific what was not expected, eg. "missing device removed".

Re: [PATCH 1/3] btrfs: Make btrfs_find_device_by_path return struct btrfs_device

[David Sterba]

On Mon, Sep 03, 2018 at 12:46:12PM +0300, Nikolay Borisov wrote:
> Currently this function returns an error code as well as uses one of
> its arguments as a return value for struct btrfs_device. Change the
> function so that it returns btrfs_device directly and use the usual
> "encode error in pointer" mechanics if something goes wrong. No
> functional changes.
> 
> Signed-off-by: Nikolay Borisov <nborisov@suse.com>
> ---
>  fs/btrfs/volumes.c | 26 ++++++++++++++------------
>  1 file changed, 14 insertions(+), 12 deletions(-)
> 
> diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
> index da86706123ff..715ea45c6c28 100644
> --- a/fs/btrfs/volumes.c
> +++ b/fs/btrfs/volumes.c
> @@ -2096,9 +2096,9 @@ void btrfs_destroy_dev_replace_tgtdev(struct btrfs_device *tgtdev)
>  	call_rcu(&tgtdev->rcu, free_device_rcu);
>  }
>  
> -static int btrfs_find_device_by_path(struct btrfs_fs_info *fs_info,
> -				     const char *device_path,
> -				     struct btrfs_device **device)
> +static struct btrfs_device *
> +btrfs_find_device_by_path(struct btrfs_fs_info *fs_info,
> +			  const char *device_path)

Please don't split the type and function name, I'm going to fix that in
the remaining patches.

Re: [PATCH 0/3] cleanup couple of device-related functions' retval

[David Sterba]

On Mon, Sep 03, 2018 at 12:46:11PM +0300, Nikolay Borisov wrote:
> Currently btrfs_find_device_by_path, btrfs_find_device_missing_or_by_path and 
> btrfs_find_device_by_devspec are called in a chain and they all return an 
> integer value to signal error and at the same time use one of their parameters
> as an output. This patch set refactors those functions starting from the 
> bottom, gradually making them return a pointer to btrfs_device. This is 
> sufficient to convey an error when it occurs as well as return the actual 
> device we are looking for. One added benefit is that the ioctl-specific positive
> return value BTRFS_ERROR_DEV_MISSING_NOT_FOUND is now returned from 
> btrfs_rm_device rather than from some internal function. 
> 
> Additionally I'll be sending a patch to progs, adding a test ensuring that 
> BTRFS_ERROR_MISSING_NOT_FOUND is returned appropriately. 
> 
> This survived both my btrfs-progs test as well as xfstest run. No functional 
> changes.
> 
> 
> Nikolay Borisov (3):
>   btrfs: Make btrfs_find_device_by_path return struct btrfs_device
>   btrfs: Make btrfs_find_device_missing_or_by_path return directly a
>     device
>   btrfs: Make btrfs_find_device_by_devspec return btrfs_device directly

Added to misc-next, thanks.

Re: [PATCH] btrfs-progs: tests: Add test for missing device delete error value

[David Sterba]

On Mon, Sep 10, 2018 at 07:57:20PM +0200, David Sterba wrote:
> On Mon, Sep 03, 2018 at 01:02:57PM +0300, Nikolay Borisov wrote:
> > Add a test which ensures the kernel returns the correct error value
> > when missing device removal is requested. This test verifies that kernel
> > refactoring didn't broken the return value.
> > 
> > Signed-off-by: Nikolay Borisov <nborisov@suse.com>
> > ---
> >  tests/misc-tests/011-delete-missing-device/test.sh | 17 +++++++++++++++++
> >  1 file changed, 17 insertions(+)
> > 
> > diff --git a/tests/misc-tests/011-delete-missing-device/test.sh b/tests/misc-tests/011-delete-missing-device/test.sh
> > index 4c976421c091..b799a25c201d 100755
> > --- a/tests/misc-tests/011-delete-missing-device/test.sh
> > +++ b/tests/misc-tests/011-delete-missing-device/test.sh
> > @@ -44,6 +44,21 @@ test_delete_missing()
> >  	run_check_umount_test_dev
> >  }
> >  
> > +test_missing_error()
> > +{
> > +	run_check $SUDO_HELPER "$TOP/mkfs.btrfs" -f "$TEST_DEV"
> > +	run_check_mount_test_dev
> > +	local out
> > +	out=$(run_mustfail_stdout "Unexpected success" \
> 
> Which would become:
> 
>   unexpected success: Unexpected success
> 
> You'd win the error message of the year :) The text is supposed to be
> more specific what was not expected, eg. "missing device removed".

Updated and applied, thanks.