[PATCH 0/7 v2] btrfs-progs: cleanup and preparatory around device scan

[PATCH 0/7 v2] btrfs-progs: cleanup and preparatory around device scan

[Anand Jain]

v2: I have separated preparatory and cleanups from the introduction of new
    features so that they can be easily modified with a smaller set of patches.

    Added missing git changelogs. (Looks like sshfs lost my last few changes,
    now fixed).

In an attempt to enable btrfstune to accept multiple devices from the
command line, this patch includes some cleanup around the related
preparatory work around the device scan code.

Patches 1 to 5 primarily consist of cleanups. Patches 6 and 7 serve as
preparatory changes.

Anand Jain (7):
  btrfs-progs: check_mounted_where: declare is_btrfs as bool
  btrfs-progs: check_mounted_where: pack varibles type by size
  btrfs-progs: rename struct open_ctree_flags to open_ctree_args
  btrfs-progs: device_list_add: optimize arguments drop devid
  btrfs-progs: btrfs_scan_one_device: drop local variable ret
  btrfs-progs: factor out btrfs_scan_stdin_devices
  btrfs-progs: refactor check_where_mounted with noscan argument

 btrfs-find-root.c        |  2 +-
 check/main.c             |  2 +-
 cmds/filesystem.c        |  2 +-
 cmds/inspect-dump-tree.c | 39 ++++-----------------------------------
 cmds/rescue.c            |  4 ++--
 cmds/restore.c           |  2 +-
 common/device-scan.c     | 39 +++++++++++++++++++++++++++++++++++++++
 common/device-scan.h     |  1 +
 common/open-utils.c      | 21 ++++++++++++---------
 common/open-utils.h      |  3 ++-
 common/utils.c           |  3 ++-
 image/main.c             |  4 ++--
 kernel-shared/disk-io.c  |  8 ++++----
 kernel-shared/disk-io.h  |  4 ++--
 kernel-shared/volumes.c  | 14 +++++---------
 mkfs/main.c              |  2 +-
 tune/main.c              |  2 +-
 17 files changed, 81 insertions(+), 71 deletions(-)

-- 
2.38.1

[PATCH 1/7] btrfs-progs: check_mounted_where: declare is_btrfs as bool

[Anand Jain]

The variable 'is_btrfs' is declared as an integer but could be a boolean
instead.

Signed-off-by: Anand Jain <anand.jain@oracle.com>
---
 common/open-utils.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/common/open-utils.c b/common/open-utils.c
index d4fdb2b01c7f..01d747d8ac43 100644
--- a/common/open-utils.c
+++ b/common/open-utils.c
@@ -57,7 +57,7 @@ int check_mounted_where(int fd, const char *file, char *where, int size,
 {
 	int ret;
 	u64 total_devs = 1;
-	int is_btrfs;
+	bool is_btrfs;
 	struct btrfs_fs_devices *fs_devices_mnt = NULL;
 	FILE *f;
 	struct mntent *mnt;
-- 
2.38.1

Re: [PATCH 1/7] btrfs-progs: check_mounted_where: declare is_btrfs as bool

[Qu Wenruo]

On 2023/6/8 14:00, Anand Jain wrote:
> The variable 'is_btrfs' is declared as an integer but could be a boolean
> instead.
>
> Signed-off-by: Anand Jain <anand.jain@oracle.com>

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

Thanks,
Qu

> ---
>   common/open-utils.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/common/open-utils.c b/common/open-utils.c
> index d4fdb2b01c7f..01d747d8ac43 100644
> --- a/common/open-utils.c
> +++ b/common/open-utils.c
> @@ -57,7 +57,7 @@ int check_mounted_where(int fd, const char *file, char *where, int size,
>   {
>   	int ret;
>   	u64 total_devs = 1;
> -	int is_btrfs;
> +	bool is_btrfs;
>   	struct btrfs_fs_devices *fs_devices_mnt = NULL;
>   	FILE *f;
>   	struct mntent *mnt;

[PATCH 2/7] btrfs-progs: check_mounted_where: pack varibles type by size

[Anand Jain]

Pack variables by their type; it may save some space. Also, fixes a line
indentation.

Signed-off-by: Anand Jain <anand.jain@oracle.com>
---
 common/open-utils.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/common/open-utils.c b/common/open-utils.c
index 01d747d8ac43..1e18fa905b51 100644
--- a/common/open-utils.c
+++ b/common/open-utils.c
@@ -55,16 +55,16 @@ static int blk_file_in_dev_list(struct btrfs_fs_devices* fs_devices,
 int check_mounted_where(int fd, const char *file, char *where, int size,
 			struct btrfs_fs_devices **fs_dev_ret, unsigned sbflags)
 {
-	int ret;
-	u64 total_devs = 1;
-	bool is_btrfs;
 	struct btrfs_fs_devices *fs_devices_mnt = NULL;
-	FILE *f;
 	struct mntent *mnt;
+	u64 total_devs = 1;
+	FILE *f;
+	int ret;
+	bool is_btrfs;
 
 	/* scan the initial device */
-	ret = btrfs_scan_one_device(fd, file, &fs_devices_mnt,
-		    &total_devs, BTRFS_SUPER_INFO_OFFSET, sbflags);
+	ret = btrfs_scan_one_device(fd, file, &fs_devices_mnt, &total_devs,
+				    BTRFS_SUPER_INFO_OFFSET, sbflags);
 	is_btrfs = (ret >= 0);
 
 	/* scan other devices */
-- 
2.38.1

Re: [PATCH 2/7] btrfs-progs: check_mounted_where: pack varibles type by size

[Qu Wenruo]

On 2023/6/8 14:00, Anand Jain wrote:
> Pack variables by their type; it may save some space. Also, fixes a line
> indentation.
>
> Signed-off-by: Anand Jain <anand.jain@oracle.com>

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

Thanks,
Qu
> ---
>   common/open-utils.c | 12 ++++++------
>   1 file changed, 6 insertions(+), 6 deletions(-)
>
> diff --git a/common/open-utils.c b/common/open-utils.c
> index 01d747d8ac43..1e18fa905b51 100644
> --- a/common/open-utils.c
> +++ b/common/open-utils.c
> @@ -55,16 +55,16 @@ static int blk_file_in_dev_list(struct btrfs_fs_devices* fs_devices,
>   int check_mounted_where(int fd, const char *file, char *where, int size,
>   			struct btrfs_fs_devices **fs_dev_ret, unsigned sbflags)
>   {
> -	int ret;
> -	u64 total_devs = 1;
> -	bool is_btrfs;
>   	struct btrfs_fs_devices *fs_devices_mnt = NULL;
> -	FILE *f;
>   	struct mntent *mnt;
> +	u64 total_devs = 1;
> +	FILE *f;
> +	int ret;
> +	bool is_btrfs;
>
>   	/* scan the initial device */
> -	ret = btrfs_scan_one_device(fd, file, &fs_devices_mnt,
> -		    &total_devs, BTRFS_SUPER_INFO_OFFSET, sbflags);
> +	ret = btrfs_scan_one_device(fd, file, &fs_devices_mnt, &total_devs,
> +				    BTRFS_SUPER_INFO_OFFSET, sbflags);
>   	is_btrfs = (ret >= 0);
>
>   	/* scan other devices */

Re: [PATCH 2/7] btrfs-progs: check_mounted_where: pack varibles type by size

[David Sterba]

On Thu, Jun 08, 2023 at 02:00:59PM +0800, Anand Jain wrote:
> Pack variables by their type; it may save some space.

AFAIK compiler does not stick to the order defined in the sources on
stack and is free to reorder variables or completely optimize them out
so I don't see point in doing such changes.

> Also, fixes a line
> indentation.

And we don't want whitespace-only patches unless it's something that
really annoys us. Argument formatting is mixed and as long as it's
readable it should not be changed. Acceptable styles are one or two tabs
or align under the openeing ( .

Re: [PATCH 2/7] btrfs-progs: check_mounted_where: pack varibles type by size

[Anand Jain]

On 08/06/2023 20:38, David Sterba wrote:
> On Thu, Jun 08, 2023 at 02:00:59PM +0800, Anand Jain wrote:
>> Pack variables by their type; it may save some space.
> 
> AFAIK compiler does not stick to the order defined in the sources on
> stack and is free to reorder variables or completely optimize them out
> so I don't see point in doing such changes.
> 

  Oh. I don't see any point in packing them.

>> Also, fixes a line
>> indentation.
> 
> And we don't want whitespace-only patches unless it's something that
> really annoys us. Argument formatting is mixed and as long as it's
> readable it should not be changed. Acceptable styles are one or two tabs
> or align under the openeing ( .


  Got it.

  I will drop this patch in the next reroll.

Thanks.

[PATCH 3/7] btrfs-progs: rename struct open_ctree_flags to open_ctree_args

[Anand Jain]

The struct open_ctree_flags currently holds arguments for
open_ctree_fs_info(), it can be confusing when mixed with a local variable
named open_ctree_flags as below in the function cmd_inspect_dump_tree().

	cmd_inspect_dump_tree()
	::
	struct open_ctree_flags ocf = { 0 };
	::
	unsigned open_ctree_flags;

So rename struct open_ctree_flags to struct open_ctree_args.

Signed-off-by: Anand Jain <anand.jain@oracle.com>
---
 btrfs-find-root.c        | 2 +-
 check/main.c             | 2 +-
 cmds/filesystem.c        | 2 +-
 cmds/inspect-dump-tree.c | 2 +-
 cmds/rescue.c            | 4 ++--
 cmds/restore.c           | 2 +-
 image/main.c             | 4 ++--
 kernel-shared/disk-io.c  | 8 ++++----
 kernel-shared/disk-io.h  | 4 ++--
 mkfs/main.c              | 2 +-
 10 files changed, 16 insertions(+), 16 deletions(-)

diff --git a/btrfs-find-root.c b/btrfs-find-root.c
index 398d7f216ee7..52041d82c182 100644
--- a/btrfs-find-root.c
+++ b/btrfs-find-root.c
@@ -335,7 +335,7 @@ int main(int argc, char **argv)
 	struct btrfs_find_root_filter filter = {0};
 	struct cache_tree result;
 	struct cache_extent *found;
-	struct open_ctree_flags ocf = { 0 };
+	struct open_ctree_args ocf = { 0 };
 	int ret;
 
 	/* Default to search root tree */
diff --git a/check/main.c b/check/main.c
index 7542b49f44f5..f7a2d446370a 100644
--- a/check/main.c
+++ b/check/main.c
@@ -9983,7 +9983,7 @@ static int cmd_check(const struct cmd_struct *cmd, int argc, char **argv)
 {
 	struct cache_tree root_cache;
 	struct btrfs_root *root;
-	struct open_ctree_flags ocf = { 0 };
+	struct open_ctree_args ocf = { 0 };
 	u64 bytenr = 0;
 	u64 subvolid = 0;
 	u64 tree_root_bytenr = 0;
diff --git a/cmds/filesystem.c b/cmds/filesystem.c
index 47fd2377f5f4..c9e641b2fa9a 100644
--- a/cmds/filesystem.c
+++ b/cmds/filesystem.c
@@ -636,7 +636,7 @@ static int map_seed_devices(struct list_head *all_uuids)
 	fs_uuids = btrfs_scanned_uuids();
 
 	list_for_each_entry(cur_fs, all_uuids, list) {
-		struct open_ctree_flags ocf = { 0 };
+		struct open_ctree_args ocf = { 0 };
 
 		device = list_first_entry(&cur_fs->devices,
 						struct btrfs_device, dev_list);
diff --git a/cmds/inspect-dump-tree.c b/cmds/inspect-dump-tree.c
index bfc0fff148dd..35920d14b7e9 100644
--- a/cmds/inspect-dump-tree.c
+++ b/cmds/inspect-dump-tree.c
@@ -317,7 +317,7 @@ static int cmd_inspect_dump_tree(const struct cmd_struct *cmd,
 	struct btrfs_disk_key disk_key;
 	struct btrfs_key found_key;
 	struct cache_tree block_root;	/* for multiple --block parameters */
-	struct open_ctree_flags ocf = { 0 };
+	struct open_ctree_args ocf = { 0 };
 	char uuidbuf[BTRFS_UUID_UNPARSED_SIZE];
 	int ret = 0;
 	int slot;
diff --git a/cmds/rescue.c b/cmds/rescue.c
index 5551374d4b75..aee5446e66d0 100644
--- a/cmds/rescue.c
+++ b/cmds/rescue.c
@@ -233,7 +233,7 @@ static int cmd_rescue_fix_device_size(const struct cmd_struct *cmd,
 				      int argc, char **argv)
 {
 	struct btrfs_fs_info *fs_info;
-	struct open_ctree_flags ocf = { 0 };
+	struct open_ctree_args ocf = { 0 };
 	char *devname;
 	int ret;
 
@@ -368,7 +368,7 @@ static int cmd_rescue_clear_uuid_tree(const struct cmd_struct *cmd,
 				      int argc, char **argv)
 {
 	struct btrfs_fs_info *fs_info;
-	struct open_ctree_flags ocf = {};
+	struct open_ctree_args ocf = {};
 	char *devname;
 	int ret;
 
diff --git a/cmds/restore.c b/cmds/restore.c
index 9fe7b4d2d07d..aa78d0799c4a 100644
--- a/cmds/restore.c
+++ b/cmds/restore.c
@@ -1216,7 +1216,7 @@ static struct btrfs_root *open_fs(const char *dev, u64 root_location,
 {
 	struct btrfs_fs_info *fs_info = NULL;
 	struct btrfs_root *root = NULL;
-	struct open_ctree_flags ocf = { 0 };
+	struct open_ctree_args ocf = { 0 };
 	u64 bytenr;
 	int i;
 
diff --git a/image/main.c b/image/main.c
index 50c3f2ca7db5..9e460e7076e7 100644
--- a/image/main.c
+++ b/image/main.c
@@ -2795,7 +2795,7 @@ static int restore_metadump(const char *input, FILE *out, int old_restore,
 
 	/* NOTE: open with write mode */
 	if (fixup_offset) {
-		struct open_ctree_flags ocf = { 0 };
+		struct open_ctree_args ocf = { 0 };
 
 		ocf.filename = target;
 		ocf.flags = OPEN_CTREE_WRITES | OPEN_CTREE_RESTORE |
@@ -3223,7 +3223,7 @@ int BOX_MAIN(image)(int argc, char *argv[])
 
 	 /* extended support for multiple devices */
 	if (!create && multi_devices) {
-		struct open_ctree_flags ocf = { 0 };
+		struct open_ctree_args ocf = { 0 };
 		struct btrfs_fs_info *info;
 		u64 total_devs;
 		int i;
diff --git a/kernel-shared/disk-io.c b/kernel-shared/disk-io.c
index 442d3af8bc01..3b709b2c0f7f 100644
--- a/kernel-shared/disk-io.c
+++ b/kernel-shared/disk-io.c
@@ -1437,7 +1437,7 @@ int btrfs_setup_chunk_tree_and_device_map(struct btrfs_fs_info *fs_info,
 	return 0;
 }
 
-static struct btrfs_fs_info *__open_ctree_fd(int fp, struct open_ctree_flags *ocf)
+static struct btrfs_fs_info *__open_ctree_fd(int fp, struct open_ctree_args *ocf)
 {
 	struct btrfs_fs_info *fs_info;
 	struct btrfs_super_block *disk_super;
@@ -1608,7 +1608,7 @@ out:
 	return NULL;
 }
 
-struct btrfs_fs_info *open_ctree_fs_info(struct open_ctree_flags *ocf)
+struct btrfs_fs_info *open_ctree_fs_info(struct open_ctree_args *ocf)
 {
 	int fp;
 	int ret;
@@ -1646,7 +1646,7 @@ struct btrfs_root *open_ctree(const char *filename, u64 sb_bytenr,
 			      unsigned flags)
 {
 	struct btrfs_fs_info *info;
-	struct open_ctree_flags ocf = { 0 };
+	struct open_ctree_args ocf = { 0 };
 
 	/* This flags may not return fs_info with any valid root */
 	BUG_ON(flags & OPEN_CTREE_IGNORE_CHUNK_TREE_ERROR);
@@ -1665,7 +1665,7 @@ struct btrfs_root *open_ctree_fd(int fp, const char *path, u64 sb_bytenr,
 				 unsigned flags)
 {
 	struct btrfs_fs_info *info;
-	struct open_ctree_flags ocf = { 0 };
+	struct open_ctree_args ocf = { 0 };
 
 	/* This flags may not return fs_info with any valid root */
 	if (flags & OPEN_CTREE_IGNORE_CHUNK_TREE_ERROR) {
diff --git a/kernel-shared/disk-io.h b/kernel-shared/disk-io.h
index 3a31667967cc..93572c4297ad 100644
--- a/kernel-shared/disk-io.h
+++ b/kernel-shared/disk-io.h
@@ -175,7 +175,7 @@ struct btrfs_root *open_ctree(const char *filename, u64 sb_bytenr,
 			      unsigned flags);
 struct btrfs_root *open_ctree_fd(int fp, const char *path, u64 sb_bytenr,
 				 unsigned flags);
-struct open_ctree_flags {
+struct open_ctree_args {
 	const char *filename;
 	u64 sb_bytenr;
 	u64 root_tree_bytenr;
@@ -183,7 +183,7 @@ struct open_ctree_flags {
 	unsigned flags;
 };
 
-struct btrfs_fs_info *open_ctree_fs_info(struct open_ctree_flags *ocf);
+struct btrfs_fs_info *open_ctree_fs_info(struct open_ctree_args *ctree_args);
 int close_ctree_fs_info(struct btrfs_fs_info *fs_info);
 static inline int close_ctree(struct btrfs_root *root)
 {
diff --git a/mkfs/main.c b/mkfs/main.c
index a31b32c644c9..23db58b7186d 100644
--- a/mkfs/main.c
+++ b/mkfs/main.c
@@ -990,7 +990,7 @@ int BOX_MAIN(mkfs)(int argc, char **argv)
 	struct btrfs_root *root;
 	struct btrfs_fs_info *fs_info;
 	struct btrfs_trans_handle *trans;
-	struct open_ctree_flags ocf = { 0 };
+	struct open_ctree_args ocf = { 0 };
 	int ret = 0;
 	int close_ret;
 	int i;
-- 
2.38.1

Re: [PATCH 3/7] btrfs-progs: rename struct open_ctree_flags to open_ctree_args

[Qu Wenruo]

On 2023/6/8 14:01, Anand Jain wrote:
> The struct open_ctree_flags currently holds arguments for
> open_ctree_fs_info(), it can be confusing when mixed with a local variable
> named open_ctree_flags as below in the function cmd_inspect_dump_tree().
>
> 	cmd_inspect_dump_tree()
> 	::
> 	struct open_ctree_flags ocf = { 0 };
> 	::
> 	unsigned open_ctree_flags;
>
> So rename struct open_ctree_flags to struct open_ctree_args.

I don't think this is a big deal.

Any LSP server and compiler can handle it correct.

Furthermore the rename would make a lot of @ocf variables loses its
meaning. (The patch doesn't rename it to oca).

To me, the better solution would be remove local variable
open_ctree_flags completely, and do all the flags setting using
ocf.flags instead.

Thanks,
Qu
>
> Signed-off-by: Anand Jain <anand.jain@oracle.com>
> ---
>   btrfs-find-root.c        | 2 +-
>   check/main.c             | 2 +-
>   cmds/filesystem.c        | 2 +-
>   cmds/inspect-dump-tree.c | 2 +-
>   cmds/rescue.c            | 4 ++--
>   cmds/restore.c           | 2 +-
>   image/main.c             | 4 ++--
>   kernel-shared/disk-io.c  | 8 ++++----
>   kernel-shared/disk-io.h  | 4 ++--
>   mkfs/main.c              | 2 +-
>   10 files changed, 16 insertions(+), 16 deletions(-)
>
> diff --git a/btrfs-find-root.c b/btrfs-find-root.c
> index 398d7f216ee7..52041d82c182 100644
> --- a/btrfs-find-root.c
> +++ b/btrfs-find-root.c
> @@ -335,7 +335,7 @@ int main(int argc, char **argv)
>   	struct btrfs_find_root_filter filter = {0};
>   	struct cache_tree result;
>   	struct cache_extent *found;
> -	struct open_ctree_flags ocf = { 0 };
> +	struct open_ctree_args ocf = { 0 };
>   	int ret;
>
>   	/* Default to search root tree */
> diff --git a/check/main.c b/check/main.c
> index 7542b49f44f5..f7a2d446370a 100644
> --- a/check/main.c
> +++ b/check/main.c
> @@ -9983,7 +9983,7 @@ static int cmd_check(const struct cmd_struct *cmd, int argc, char **argv)
>   {
>   	struct cache_tree root_cache;
>   	struct btrfs_root *root;
> -	struct open_ctree_flags ocf = { 0 };
> +	struct open_ctree_args ocf = { 0 };
>   	u64 bytenr = 0;
>   	u64 subvolid = 0;
>   	u64 tree_root_bytenr = 0;
> diff --git a/cmds/filesystem.c b/cmds/filesystem.c
> index 47fd2377f5f4..c9e641b2fa9a 100644
> --- a/cmds/filesystem.c
> +++ b/cmds/filesystem.c
> @@ -636,7 +636,7 @@ static int map_seed_devices(struct list_head *all_uuids)
>   	fs_uuids = btrfs_scanned_uuids();
>
>   	list_for_each_entry(cur_fs, all_uuids, list) {
> -		struct open_ctree_flags ocf = { 0 };
> +		struct open_ctree_args ocf = { 0 };
>
>   		device = list_first_entry(&cur_fs->devices,
>   						struct btrfs_device, dev_list);
> diff --git a/cmds/inspect-dump-tree.c b/cmds/inspect-dump-tree.c
> index bfc0fff148dd..35920d14b7e9 100644
> --- a/cmds/inspect-dump-tree.c
> +++ b/cmds/inspect-dump-tree.c
> @@ -317,7 +317,7 @@ static int cmd_inspect_dump_tree(const struct cmd_struct *cmd,
>   	struct btrfs_disk_key disk_key;
>   	struct btrfs_key found_key;
>   	struct cache_tree block_root;	/* for multiple --block parameters */
> -	struct open_ctree_flags ocf = { 0 };
> +	struct open_ctree_args ocf = { 0 };
>   	char uuidbuf[BTRFS_UUID_UNPARSED_SIZE];
>   	int ret = 0;
>   	int slot;
> diff --git a/cmds/rescue.c b/cmds/rescue.c
> index 5551374d4b75..aee5446e66d0 100644
> --- a/cmds/rescue.c
> +++ b/cmds/rescue.c
> @@ -233,7 +233,7 @@ static int cmd_rescue_fix_device_size(const struct cmd_struct *cmd,
>   				      int argc, char **argv)
>   {
>   	struct btrfs_fs_info *fs_info;
> -	struct open_ctree_flags ocf = { 0 };
> +	struct open_ctree_args ocf = { 0 };
>   	char *devname;
>   	int ret;
>
> @@ -368,7 +368,7 @@ static int cmd_rescue_clear_uuid_tree(const struct cmd_struct *cmd,
>   				      int argc, char **argv)
>   {
>   	struct btrfs_fs_info *fs_info;
> -	struct open_ctree_flags ocf = {};
> +	struct open_ctree_args ocf = {};
>   	char *devname;
>   	int ret;
>
> diff --git a/cmds/restore.c b/cmds/restore.c
> index 9fe7b4d2d07d..aa78d0799c4a 100644
> --- a/cmds/restore.c
> +++ b/cmds/restore.c
> @@ -1216,7 +1216,7 @@ static struct btrfs_root *open_fs(const char *dev, u64 root_location,
>   {
>   	struct btrfs_fs_info *fs_info = NULL;
>   	struct btrfs_root *root = NULL;
> -	struct open_ctree_flags ocf = { 0 };
> +	struct open_ctree_args ocf = { 0 };
>   	u64 bytenr;
>   	int i;
>
> diff --git a/image/main.c b/image/main.c
> index 50c3f2ca7db5..9e460e7076e7 100644
> --- a/image/main.c
> +++ b/image/main.c
> @@ -2795,7 +2795,7 @@ static int restore_metadump(const char *input, FILE *out, int old_restore,
>
>   	/* NOTE: open with write mode */
>   	if (fixup_offset) {
> -		struct open_ctree_flags ocf = { 0 };
> +		struct open_ctree_args ocf = { 0 };
>
>   		ocf.filename = target;
>   		ocf.flags = OPEN_CTREE_WRITES | OPEN_CTREE_RESTORE |
> @@ -3223,7 +3223,7 @@ int BOX_MAIN(image)(int argc, char *argv[])
>
>   	 /* extended support for multiple devices */
>   	if (!create && multi_devices) {
> -		struct open_ctree_flags ocf = { 0 };
> +		struct open_ctree_args ocf = { 0 };
>   		struct btrfs_fs_info *info;
>   		u64 total_devs;
>   		int i;
> diff --git a/kernel-shared/disk-io.c b/kernel-shared/disk-io.c
> index 442d3af8bc01..3b709b2c0f7f 100644
> --- a/kernel-shared/disk-io.c
> +++ b/kernel-shared/disk-io.c
> @@ -1437,7 +1437,7 @@ int btrfs_setup_chunk_tree_and_device_map(struct btrfs_fs_info *fs_info,
>   	return 0;
>   }
>
> -static struct btrfs_fs_info *__open_ctree_fd(int fp, struct open_ctree_flags *ocf)
> +static struct btrfs_fs_info *__open_ctree_fd(int fp, struct open_ctree_args *ocf)
>   {
>   	struct btrfs_fs_info *fs_info;
>   	struct btrfs_super_block *disk_super;
> @@ -1608,7 +1608,7 @@ out:
>   	return NULL;
>   }
>
> -struct btrfs_fs_info *open_ctree_fs_info(struct open_ctree_flags *ocf)
> +struct btrfs_fs_info *open_ctree_fs_info(struct open_ctree_args *ocf)
>   {
>   	int fp;
>   	int ret;
> @@ -1646,7 +1646,7 @@ struct btrfs_root *open_ctree(const char *filename, u64 sb_bytenr,
>   			      unsigned flags)
>   {
>   	struct btrfs_fs_info *info;
> -	struct open_ctree_flags ocf = { 0 };
> +	struct open_ctree_args ocf = { 0 };
>
>   	/* This flags may not return fs_info with any valid root */
>   	BUG_ON(flags & OPEN_CTREE_IGNORE_CHUNK_TREE_ERROR);
> @@ -1665,7 +1665,7 @@ struct btrfs_root *open_ctree_fd(int fp, const char *path, u64 sb_bytenr,
>   				 unsigned flags)
>   {
>   	struct btrfs_fs_info *info;
> -	struct open_ctree_flags ocf = { 0 };
> +	struct open_ctree_args ocf = { 0 };
>
>   	/* This flags may not return fs_info with any valid root */
>   	if (flags & OPEN_CTREE_IGNORE_CHUNK_TREE_ERROR) {
> diff --git a/kernel-shared/disk-io.h b/kernel-shared/disk-io.h
> index 3a31667967cc..93572c4297ad 100644
> --- a/kernel-shared/disk-io.h
> +++ b/kernel-shared/disk-io.h
> @@ -175,7 +175,7 @@ struct btrfs_root *open_ctree(const char *filename, u64 sb_bytenr,
>   			      unsigned flags);
>   struct btrfs_root *open_ctree_fd(int fp, const char *path, u64 sb_bytenr,
>   				 unsigned flags);
> -struct open_ctree_flags {
> +struct open_ctree_args {
>   	const char *filename;
>   	u64 sb_bytenr;
>   	u64 root_tree_bytenr;
> @@ -183,7 +183,7 @@ struct open_ctree_flags {
>   	unsigned flags;
>   };
>
> -struct btrfs_fs_info *open_ctree_fs_info(struct open_ctree_flags *ocf);
> +struct btrfs_fs_info *open_ctree_fs_info(struct open_ctree_args *ctree_args);
>   int close_ctree_fs_info(struct btrfs_fs_info *fs_info);
>   static inline int close_ctree(struct btrfs_root *root)
>   {
> diff --git a/mkfs/main.c b/mkfs/main.c
> index a31b32c644c9..23db58b7186d 100644
> --- a/mkfs/main.c
> +++ b/mkfs/main.c
> @@ -990,7 +990,7 @@ int BOX_MAIN(mkfs)(int argc, char **argv)
>   	struct btrfs_root *root;
>   	struct btrfs_fs_info *fs_info;
>   	struct btrfs_trans_handle *trans;
> -	struct open_ctree_flags ocf = { 0 };
> +	struct open_ctree_args ocf = { 0 };
>   	int ret = 0;
>   	int close_ret;
>   	int i;

Re: [PATCH 3/7] btrfs-progs: rename struct open_ctree_flags to open_ctree_args

[Anand Jain]

On 08/06/2023 14:14, Qu Wenruo wrote:
> 
> 
> On 2023/6/8 14:01, Anand Jain wrote:
>> The struct open_ctree_flags currently holds arguments for
>> open_ctree_fs_info(), it can be confusing when mixed with a local 
>> variable
>> named open_ctree_flags as below in the function cmd_inspect_dump_tree().
>>
>>     cmd_inspect_dump_tree()
>>     ::
>>     struct open_ctree_flags ocf = { 0 };
>>     ::
>>     unsigned open_ctree_flags;
>>
>> So rename struct open_ctree_flags to struct open_ctree_args.
> 
> I don't think this is a big deal.
> 
> Any LSP server and compiler can handle it correct.
> 
> Furthermore the rename would make a lot of @ocf variables loses its
> meaning. (The patch doesn't rename it to oca).
> 
> To me, the better solution would be remove local variable
> open_ctree_flags completely, and do all the flags setting using
> ocf.flags instead.
s/open_ctree_flags/open_ctree_args makes sense as this struct is
not just about the flags.

struct open_ctree_args {
         const char *filename;
         u64 sb_bytenr;
         u64 root_tree_bytenr;
         u64 chunk_tree_bytenr;
         unsigned flags;
};


PS:
We also have
enum btrfs_open_ctree_flags {
::
}

Thanks, Anand

Re: [PATCH 3/7] btrfs-progs: rename struct open_ctree_flags to open_ctree_args

[Qu Wenruo]

On 2023/6/8 15:10, Anand Jain wrote:
>
>
> On 08/06/2023 14:14, Qu Wenruo wrote:
>>
>>
>> On 2023/6/8 14:01, Anand Jain wrote:
>>> The struct open_ctree_flags currently holds arguments for
>>> open_ctree_fs_info(), it can be confusing when mixed with a local
>>> variable
>>> named open_ctree_flags as below in the function cmd_inspect_dump_tree().
>>>
>>>     cmd_inspect_dump_tree()
>>>     ::
>>>     struct open_ctree_flags ocf = { 0 };
>>>     ::
>>>     unsigned open_ctree_flags;
>>>
>>> So rename struct open_ctree_flags to struct open_ctree_args.
>>
>> I don't think this is a big deal.
>>
>> Any LSP server and compiler can handle it correct.
>>
>> Furthermore the rename would make a lot of @ocf variables loses its
>> meaning. (The patch doesn't rename it to oca).
>>
>> To me, the better solution would be remove local variable
>> open_ctree_flags completely, and do all the flags setting using
>> ocf.flags instead.
> s/open_ctree_flags/open_ctree_args makes sense as this struct is
> not just about the flags.
>
> struct open_ctree_args {
>          const char *filename;
>          u64 sb_bytenr;
>          u64 root_tree_bytenr;
>          u64 chunk_tree_bytenr;
>          unsigned flags;
> };

OK, then you may want to also rename @ocf to @oca.

And since we're already here, removing @open_ctree_flags also makes
sense, as we can directly use oca.flags.

Thanks,
Qu
>
>
> PS:
> We also have
> enum btrfs_open_ctree_flags {
> ::
> }
>
> Thanks, Anand

Re: [PATCH 3/7] btrfs-progs: rename struct open_ctree_flags to open_ctree_args

[Anand Jain]

>> struct open_ctree_args {
>>          const char *filename;
>>          u64 sb_bytenr;
>>          u64 root_tree_bytenr;
>>          u64 chunk_tree_bytenr;
>>          unsigned flags;
>> };
> 
> OK, then you may want to also rename @ocf to @oca.
> 

Will do.

> And since we're already here, removing @open_ctree_flags also makes
> sense, as we can directly use oca.flags.

Included in the next reroll.

Thanks,

[PATCH 4/7] btrfs-progs: device_list_add: optimize arguments drop devid

[Anand Jain]

Drop the devid argument; instead, it can be fetched from the disk_super
argument.

Signed-off-by: Anand Jain <anand.jain@oracle.com>
---
 kernel-shared/volumes.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/kernel-shared/volumes.c b/kernel-shared/volumes.c
index 95d5930b95d8..81abda3f7d1c 100644
--- a/kernel-shared/volumes.c
+++ b/kernel-shared/volumes.c
@@ -334,11 +334,12 @@ static struct btrfs_fs_devices *find_fsid(u8 *fsid, u8 *metadata_uuid)
 
 static int device_list_add(const char *path,
 			   struct btrfs_super_block *disk_super,
-			   u64 devid, struct btrfs_fs_devices **fs_devices_ret)
+			   struct btrfs_fs_devices **fs_devices_ret)
 {
 	struct btrfs_device *device;
 	struct btrfs_fs_devices *fs_devices;
 	u64 found_transid = btrfs_super_generation(disk_super);
+	u64 devid = btrfs_stack_device_id(&disk_super->dev_item);
 	bool metadata_uuid = (btrfs_super_incompat_flags(disk_super) &
 		BTRFS_FEATURE_INCOMPAT_METADATA_UUID);
 
@@ -545,18 +546,17 @@ int btrfs_scan_one_device(int fd, const char *path,
 {
 	struct btrfs_super_block disk_super;
 	int ret;
-	u64 devid;
 
 	ret = btrfs_read_dev_super(fd, &disk_super, super_offset, sbflags);
 	if (ret < 0)
 		return -EIO;
-	devid = btrfs_stack_device_id(&disk_super.dev_item);
+
 	if (btrfs_super_flags(&disk_super) & BTRFS_SUPER_FLAG_METADUMP)
 		*total_devs = 1;
 	else
 		*total_devs = btrfs_super_num_devices(&disk_super);
 
-	ret = device_list_add(path, &disk_super, devid, fs_devices_ret);
+	ret = device_list_add(path, &disk_super, fs_devices_ret);
 
 	return ret;
 }
-- 
2.38.1

Re: [PATCH 4/7] btrfs-progs: device_list_add: optimize arguments drop devid

[Qu Wenruo]

On 2023/6/8 14:01, Anand Jain wrote:
> Drop the devid argument; instead, it can be fetched from the disk_super
> argument. >
> Signed-off-by: Anand Jain <anand.jain@oracle.com>

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

Thanks,
Qu

> ---
>   kernel-shared/volumes.c | 8 ++++----
>   1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/kernel-shared/volumes.c b/kernel-shared/volumes.c
> index 95d5930b95d8..81abda3f7d1c 100644
> --- a/kernel-shared/volumes.c
> +++ b/kernel-shared/volumes.c
> @@ -334,11 +334,12 @@ static struct btrfs_fs_devices *find_fsid(u8 *fsid, u8 *metadata_uuid)
>
>   static int device_list_add(const char *path,
>   			   struct btrfs_super_block *disk_super,
> -			   u64 devid, struct btrfs_fs_devices **fs_devices_ret)
> +			   struct btrfs_fs_devices **fs_devices_ret)
>   {
>   	struct btrfs_device *device;
>   	struct btrfs_fs_devices *fs_devices;
>   	u64 found_transid = btrfs_super_generation(disk_super);
> +	u64 devid = btrfs_stack_device_id(&disk_super->dev_item);
>   	bool metadata_uuid = (btrfs_super_incompat_flags(disk_super) &
>   		BTRFS_FEATURE_INCOMPAT_METADATA_UUID);
>
> @@ -545,18 +546,17 @@ int btrfs_scan_one_device(int fd, const char *path,
>   {
>   	struct btrfs_super_block disk_super;
>   	int ret;
> -	u64 devid;
>
>   	ret = btrfs_read_dev_super(fd, &disk_super, super_offset, sbflags);
>   	if (ret < 0)
>   		return -EIO;
> -	devid = btrfs_stack_device_id(&disk_super.dev_item);
> +
>   	if (btrfs_super_flags(&disk_super) & BTRFS_SUPER_FLAG_METADUMP)
>   		*total_devs = 1;
>   	else
>   		*total_devs = btrfs_super_num_devices(&disk_super);
>
> -	ret = device_list_add(path, &disk_super, devid, fs_devices_ret);
> +	ret = device_list_add(path, &disk_super, fs_devices_ret);
>
>   	return ret;
>   }

[PATCH 5/7] btrfs-progs: btrfs_scan_one_device: drop local variable ret

[Anand Jain]

Local variable int ret is unnecessary, drop it.

Signed-off-by: Anand Jain <anand.jain@oracle.com>
---
 kernel-shared/volumes.c | 8 ++------
 1 file changed, 2 insertions(+), 6 deletions(-)

diff --git a/kernel-shared/volumes.c b/kernel-shared/volumes.c
index 81abda3f7d1c..c8053ae1c7f7 100644
--- a/kernel-shared/volumes.c
+++ b/kernel-shared/volumes.c
@@ -545,10 +545,8 @@ int btrfs_scan_one_device(int fd, const char *path,
 			  u64 *total_devs, u64 super_offset, unsigned sbflags)
 {
 	struct btrfs_super_block disk_super;
-	int ret;
 
-	ret = btrfs_read_dev_super(fd, &disk_super, super_offset, sbflags);
-	if (ret < 0)
+	if (btrfs_read_dev_super(fd, &disk_super, super_offset, sbflags) < 0)
 		return -EIO;
 
 	if (btrfs_super_flags(&disk_super) & BTRFS_SUPER_FLAG_METADUMP)
@@ -556,9 +554,7 @@ int btrfs_scan_one_device(int fd, const char *path,
 	else
 		*total_devs = btrfs_super_num_devices(&disk_super);
 
-	ret = device_list_add(path, &disk_super, fs_devices_ret);
-
-	return ret;
+	return device_list_add(path, &disk_super, fs_devices_ret);
 }
 
 static u64 dev_extent_search_start(struct btrfs_device *device, u64 start)
-- 
2.38.1

Re: [PATCH 5/7] btrfs-progs: btrfs_scan_one_device: drop local variable ret

[Qu Wenruo]

On 2023/6/8 14:01, Anand Jain wrote:
> Local variable int ret is unnecessary, drop it.
>
> Signed-off-by: Anand Jain <anand.jain@oracle.com>

I'm not sure if this change is really necessary.

To me it's more nature to go "ret = what_ever();" then handling @ret.
And compiler is definitely clever enough for those optimization.

Thanks,
Qu
> ---
>   kernel-shared/volumes.c | 8 ++------
>   1 file changed, 2 insertions(+), 6 deletions(-)
>
> diff --git a/kernel-shared/volumes.c b/kernel-shared/volumes.c
> index 81abda3f7d1c..c8053ae1c7f7 100644
> --- a/kernel-shared/volumes.c
> +++ b/kernel-shared/volumes.c
> @@ -545,10 +545,8 @@ int btrfs_scan_one_device(int fd, const char *path,
>   			  u64 *total_devs, u64 super_offset, unsigned sbflags)
>   {
>   	struct btrfs_super_block disk_super;
> -	int ret;
>
> -	ret = btrfs_read_dev_super(fd, &disk_super, super_offset, sbflags);
> -	if (ret < 0)
> +	if (btrfs_read_dev_super(fd, &disk_super, super_offset, sbflags) < 0)
>   		return -EIO;
>
>   	if (btrfs_super_flags(&disk_super) & BTRFS_SUPER_FLAG_METADUMP)
> @@ -556,9 +554,7 @@ int btrfs_scan_one_device(int fd, const char *path,
>   	else
>   		*total_devs = btrfs_super_num_devices(&disk_super);
>
> -	ret = device_list_add(path, &disk_super, fs_devices_ret);
> -
> -	return ret;
> +	return device_list_add(path, &disk_super, fs_devices_ret);
>   }
>
>   static u64 dev_extent_search_start(struct btrfs_device *device, u64 start)

Re: [PATCH 5/7] btrfs-progs: btrfs_scan_one_device: drop local variable ret

[David Sterba]

On Thu, Jun 08, 2023 at 02:22:35PM +0800, Qu Wenruo wrote:
> 
> 
> On 2023/6/8 14:01, Anand Jain wrote:
> > Local variable int ret is unnecessary, drop it.
> >
> > Signed-off-by: Anand Jain <anand.jain@oracle.com>
> 
> I'm not sure if this change is really necessary.
> 
> To me it's more nature to go "ret = what_ever();" then handling @ret.
> And compiler is definitely clever enough for those optimization.

Yeah we do

	ret = function(...)
	if (ret < 0)
		...

almost everywhere. I find it clear, there's something happening is
on a separate line and the condtion check should have as little side
effects as possible.

Re: [PATCH 5/7] btrfs-progs: btrfs_scan_one_device: drop local variable ret

[Anand Jain]

On 08/06/2023 20:42, David Sterba wrote:
> On Thu, Jun 08, 2023 at 02:22:35PM +0800, Qu Wenruo wrote:
>>
>>
>> On 2023/6/8 14:01, Anand Jain wrote:
>>> Local variable int ret is unnecessary, drop it.
>>>
>>> Signed-off-by: Anand Jain <anand.jain@oracle.com>
>>
>> I'm not sure if this change is really necessary.
>>
>> To me it's more nature to go "ret = what_ever();" then handling @ret.
>> And compiler is definitely clever enough for those optimization.
> 
> Yeah we do
> 
> 	ret = function(...)
> 	if (ret < 0)
> 		...
> 
> almost everywhere. I find it clear, there's something happening is
> on a separate line and the condtion check should have as little side
> effects as possible.


I noticed that the partner will leave the ret to stay.
I am okay with dropping this patch in the next reroll.

I assume you also meant the following is not needed?

@@ -556,9 +554,7 @@ int btrfs_scan_one_device(int fd, const char *path,
  	else
  		*total_devs = btrfs_super_num_devices(&disk_super);

-	ret = device_list_add(path, &disk_super, fs_devices_ret);
-
-	return ret;
+	return device_list_add(path, &disk_super, fs_devices_ret);


Thanks

[PATCH 6/7] btrfs-progs: factor out btrfs_scan_stdin_devices

[Anand Jain]

To prepare for handling command line given devices factor out
btrfs_scan_stdin_devices().

Signed-off-by: Anand Jain <anand.jain@oracle.com>
---
 cmds/inspect-dump-tree.c | 37 +++----------------------------------
 common/device-scan.c     | 39 +++++++++++++++++++++++++++++++++++++++
 common/device-scan.h     |  1 +
 3 files changed, 43 insertions(+), 34 deletions(-)

diff --git a/cmds/inspect-dump-tree.c b/cmds/inspect-dump-tree.c
index 35920d14b7e9..311dfbfddab6 100644
--- a/cmds/inspect-dump-tree.c
+++ b/cmds/inspect-dump-tree.c
@@ -327,7 +327,6 @@ static int cmd_inspect_dump_tree(const struct cmd_struct *cmd,
 	bool roots_only = false;
 	bool root_backups = false;
 	int traverse = BTRFS_PRINT_TREE_DEFAULT;
-	int dev_optind;
 	unsigned open_ctree_flags;
 	u64 block_bytenr;
 	struct btrfs_root *tree_root_scan;
@@ -456,39 +455,9 @@ static int cmd_inspect_dump_tree(const struct cmd_struct *cmd,
 	if (check_argc_min(argc - optind, 1))
 		return 1;
 
-	dev_optind = optind;
-	while (dev_optind < argc) {
-		int fd;
-		struct btrfs_fs_devices *fs_devices;
-		u64 num_devices;
-
-		ret = check_arg_type(argv[optind]);
-		if (ret != BTRFS_ARG_BLKDEV && ret != BTRFS_ARG_REG) {
-			if (ret < 0) {
-				errno = -ret;
-				error("invalid argument %s: %m", argv[dev_optind]);
-			} else {
-				error("not a block device or regular file: %s",
-				       argv[dev_optind]);
-			}
-		}
-		fd = open(argv[dev_optind], O_RDONLY);
-		if (fd < 0) {
-			error("cannot open %s: %m", argv[dev_optind]);
-			return -EINVAL;
-		}
-		ret = btrfs_scan_one_device(fd, argv[dev_optind], &fs_devices,
-					    &num_devices,
-					    BTRFS_SUPER_INFO_OFFSET,
-					    SBREAD_DEFAULT);
-		close(fd);
-		if (ret < 0) {
-			errno = -ret;
-			error("device scan %s: %m", argv[dev_optind]);
-			return ret;
-		}
-		dev_optind++;
-	}
+	ret = btrfs_scan_stdin_devices(optind, argc, argv);
+	if (ret)
+		return ret;
 
 	pr_verbose(LOG_DEFAULT, "%s\n", PACKAGE_STRING);
 
diff --git a/common/device-scan.c b/common/device-scan.c
index 515481a6efa9..38f986df810f 100644
--- a/common/device-scan.c
+++ b/common/device-scan.c
@@ -500,3 +500,42 @@ int btrfs_scan_devices(int verbose)
 	return 0;
 }
 
+int btrfs_scan_stdin_devices(int dev_optind, int argc, char **argv)
+{
+	int ret;
+
+	while (dev_optind < argc) {
+		int fd;
+		u64 num_devices;
+		struct btrfs_fs_devices *fs_devices;
+
+		ret = check_arg_type(argv[optind]);
+		if (ret != BTRFS_ARG_BLKDEV && ret != BTRFS_ARG_REG) {
+			if (ret < 0) {
+				errno = -ret;
+				error("invalid argument %s: %m", argv[dev_optind]);
+			} else {
+				error("not a block device or regular file: %s",
+				       argv[dev_optind]);
+			}
+		}
+		fd = open(argv[dev_optind], O_RDONLY);
+		if (fd < 0) {
+			error("cannot open %s: %m", argv[dev_optind]);
+			return -EINVAL;
+		}
+		ret = btrfs_scan_one_device(fd, argv[dev_optind], &fs_devices,
+					    &num_devices,
+					    BTRFS_SUPER_INFO_OFFSET,
+					    SBREAD_DEFAULT);
+		close(fd);
+		if (ret < 0) {
+			errno = -ret;
+			error("device scan %s: %m", argv[dev_optind]);
+			return ret;
+		}
+		dev_optind++;
+	}
+
+	return 0;
+}
diff --git a/common/device-scan.h b/common/device-scan.h
index f805b489f595..e2480d3eb168 100644
--- a/common/device-scan.h
+++ b/common/device-scan.h
@@ -58,5 +58,6 @@ int add_seen_fsid(u8 *fsid, struct seen_fsid *seen_fsid_hash[],
 		int fd, DIR *dirstream);
 void free_seen_fsid(struct seen_fsid *seen_fsid_hash[]);
 int test_uuid_unique(const char *uuid_str);
+int btrfs_scan_stdin_devices(int dev_optind, int argc, char **argv);
 
 #endif
-- 
2.38.1

Re: [PATCH 6/7] btrfs-progs: factor out btrfs_scan_stdin_devices

[Qu Wenruo]

On 2023/6/8 14:01, Anand Jain wrote:
> To prepare for handling command line given devices factor out
> btrfs_scan_stdin_devices().
>
> Signed-off-by: Anand Jain <anand.jain@oracle.com>
> ---
>   cmds/inspect-dump-tree.c | 37 +++----------------------------------
>   common/device-scan.c     | 39 +++++++++++++++++++++++++++++++++++++++
>   common/device-scan.h     |  1 +
>   3 files changed, 43 insertions(+), 34 deletions(-)
>
> diff --git a/cmds/inspect-dump-tree.c b/cmds/inspect-dump-tree.c
> index 35920d14b7e9..311dfbfddab6 100644
> --- a/cmds/inspect-dump-tree.c
> +++ b/cmds/inspect-dump-tree.c
> @@ -327,7 +327,6 @@ static int cmd_inspect_dump_tree(const struct cmd_struct *cmd,
>   	bool roots_only = false;
>   	bool root_backups = false;
>   	int traverse = BTRFS_PRINT_TREE_DEFAULT;
> -	int dev_optind;
>   	unsigned open_ctree_flags;
>   	u64 block_bytenr;
>   	struct btrfs_root *tree_root_scan;
> @@ -456,39 +455,9 @@ static int cmd_inspect_dump_tree(const struct cmd_struct *cmd,
>   	if (check_argc_min(argc - optind, 1))
>   		return 1;
>
> -	dev_optind = optind;
> -	while (dev_optind < argc) {
> -		int fd;
> -		struct btrfs_fs_devices *fs_devices;
> -		u64 num_devices;
> -
> -		ret = check_arg_type(argv[optind]);
> -		if (ret != BTRFS_ARG_BLKDEV && ret != BTRFS_ARG_REG) {
> -			if (ret < 0) {
> -				errno = -ret;
> -				error("invalid argument %s: %m", argv[dev_optind]);
> -			} else {
> -				error("not a block device or regular file: %s",
> -				       argv[dev_optind]);
> -			}
> -		}
> -		fd = open(argv[dev_optind], O_RDONLY);
> -		if (fd < 0) {
> -			error("cannot open %s: %m", argv[dev_optind]);
> -			return -EINVAL;
> -		}
> -		ret = btrfs_scan_one_device(fd, argv[dev_optind], &fs_devices,
> -					    &num_devices,
> -					    BTRFS_SUPER_INFO_OFFSET,
> -					    SBREAD_DEFAULT);
> -		close(fd);
> -		if (ret < 0) {
> -			errno = -ret;
> -			error("device scan %s: %m", argv[dev_optind]);
> -			return ret;
> -		}
> -		dev_optind++;
> -	}
> +	ret = btrfs_scan_stdin_devices(optind, argc, argv);
> +	if (ret)
> +		return ret;
>
>   	pr_verbose(LOG_DEFAULT, "%s\n", PACKAGE_STRING);
>
> diff --git a/common/device-scan.c b/common/device-scan.c
> index 515481a6efa9..38f986df810f 100644
> --- a/common/device-scan.c
> +++ b/common/device-scan.c
> @@ -500,3 +500,42 @@ int btrfs_scan_devices(int verbose)
>   	return 0;
>   }
>
> +int btrfs_scan_stdin_devices(int dev_optind, int argc, char **argv)
> +{
> +	int ret;
> +
> +	while (dev_optind < argc) {

In this case, I believe "--device dev1 --device dev2" would be much
easier to parse, and would not lead to any confusion between the device
scan and target fs.

Thanks,
Qu
> +		int fd;
> +		u64 num_devices;
> +		struct btrfs_fs_devices *fs_devices;
> +
> +		ret = check_arg_type(argv[optind]);
> +		if (ret != BTRFS_ARG_BLKDEV && ret != BTRFS_ARG_REG) {
> +			if (ret < 0) {
> +				errno = -ret;
> +				error("invalid argument %s: %m", argv[dev_optind]);
> +			} else {
> +				error("not a block device or regular file: %s",
> +				       argv[dev_optind]);
> +			}
> +		}
> +		fd = open(argv[dev_optind], O_RDONLY);
> +		if (fd < 0) {
> +			error("cannot open %s: %m", argv[dev_optind]);
> +			return -EINVAL;
> +		}
> +		ret = btrfs_scan_one_device(fd, argv[dev_optind], &fs_devices,
> +					    &num_devices,
> +					    BTRFS_SUPER_INFO_OFFSET,
> +					    SBREAD_DEFAULT);
> +		close(fd);
> +		if (ret < 0) {
> +			errno = -ret;
> +			error("device scan %s: %m", argv[dev_optind]);
> +			return ret;
> +		}
> +		dev_optind++;
> +	}
> +
> +	return 0;
> +}
> diff --git a/common/device-scan.h b/common/device-scan.h
> index f805b489f595..e2480d3eb168 100644
> --- a/common/device-scan.h
> +++ b/common/device-scan.h
> @@ -58,5 +58,6 @@ int add_seen_fsid(u8 *fsid, struct seen_fsid *seen_fsid_hash[],
>   		int fd, DIR *dirstream);
>   void free_seen_fsid(struct seen_fsid *seen_fsid_hash[]);
>   int test_uuid_unique(const char *uuid_str);
> +int btrfs_scan_stdin_devices(int dev_optind, int argc, char **argv);
>
>   #endif

Re: [PATCH 6/7] btrfs-progs: factor out btrfs_scan_stdin_devices

[David Sterba]

On Thu, Jun 08, 2023 at 02:01:03PM +0800, Anand Jain wrote:
> To prepare for handling command line given devices factor out
> btrfs_scan_stdin_devices().

I find using 'stdin' in this context confusion, the tool is not
taking the devices from stdin, but command line arguments. You can use
'argv' for that.

[PATCH 7/7] btrfs-progs: refactor check_where_mounted with noscan argument

[Anand Jain]

The function check_where_mounted() scans the system for all other btrfs
devices, which is necessary for its operation.

However, in certain cases, devices remained in the scanned state is
undesirable.

So introduces the 'noscan' argument to make devices unscanned before
return.

Signed-off-by: Anand Jain <anand.jain@oracle.com>
---
 common/open-utils.c | 9 ++++++---
 common/open-utils.h | 3 ++-
 common/utils.c      | 3 ++-
 tune/main.c         | 2 +-
 4 files changed, 11 insertions(+), 6 deletions(-)

diff --git a/common/open-utils.c b/common/open-utils.c
index 1e18fa905b51..e34abee97f60 100644
--- a/common/open-utils.c
+++ b/common/open-utils.c
@@ -53,7 +53,8 @@ static int blk_file_in_dev_list(struct btrfs_fs_devices* fs_devices,
 }
 
 int check_mounted_where(int fd, const char *file, char *where, int size,
-			struct btrfs_fs_devices **fs_dev_ret, unsigned sbflags)
+			struct btrfs_fs_devices **fs_dev_ret, unsigned sbflags,
+			bool noscan)
 {
 	struct btrfs_fs_devices *fs_devices_mnt = NULL;
 	struct mntent *mnt;
@@ -108,6 +109,8 @@ int check_mounted_where(int fd, const char *file, char *where, int size,
 	}
 	if (fs_dev_ret)
 		*fs_dev_ret = fs_devices_mnt;
+	else if (noscan)
+		btrfs_close_all_devices();
 
 	ret = (mnt != NULL);
 
@@ -132,7 +135,7 @@ int check_mounted(const char* file)
 		return -errno;
 	}
 
-	ret =  check_mounted_where(fd, file, NULL, 0, NULL, SBREAD_DEFAULT);
+	ret =  check_mounted_where(fd, file, NULL, 0, NULL, SBREAD_DEFAULT, false);
 	close(fd);
 
 	return ret;
@@ -168,7 +171,7 @@ int get_btrfs_mount(const char *dev, char *mp, size_t mp_size)
 		goto out;
 	}
 
-	ret = check_mounted_where(fd, dev, mp, mp_size, NULL, SBREAD_DEFAULT);
+	ret = check_mounted_where(fd, dev, mp, mp_size, NULL, SBREAD_DEFAULT, false);
 	if (!ret) {
 		ret = -EINVAL;
 	} else { /* mounted, all good */
diff --git a/common/open-utils.h b/common/open-utils.h
index 3924be36e2ea..27000cdbd626 100644
--- a/common/open-utils.h
+++ b/common/open-utils.h
@@ -23,7 +23,8 @@
 struct btrfs_fs_devices;
 
 int check_mounted_where(int fd, const char *file, char *where, int size,
-			struct btrfs_fs_devices **fs_dev_ret, unsigned sbflags);
+			struct btrfs_fs_devices **fs_dev_ret, unsigned sbflags,
+			bool noscan);
 int check_mounted(const char* file);
 int get_btrfs_mount(const char *dev, char *mp, size_t mp_size);
 int open_path_or_dev_mnt(const char *path, DIR **dirstream, int verbose);
diff --git a/common/utils.c b/common/utils.c
index 436ff8c2a827..b62f9f04ad5a 100644
--- a/common/utils.c
+++ b/common/utils.c
@@ -230,7 +230,8 @@ int get_fs_info(const char *path, struct btrfs_ioctl_fs_info_args *fi_args,
 			goto out;
 		}
 		ret = check_mounted_where(fd, path, mp, sizeof(mp),
-					  &fs_devices_mnt, SBREAD_DEFAULT);
+					  &fs_devices_mnt, SBREAD_DEFAULT,
+					  false);
 		if (!ret) {
 			ret = -EINVAL;
 			goto out;
diff --git a/tune/main.c b/tune/main.c
index e38c1f6d3729..0ca1e01282c9 100644
--- a/tune/main.c
+++ b/tune/main.c
@@ -268,7 +268,7 @@ int BOX_MAIN(btrfstune)(int argc, char *argv[])
 	}
 
 	ret = check_mounted_where(fd, device, NULL, 0, NULL,
-			SBREAD_IGNORE_FSID_MISMATCH);
+				  SBREAD_IGNORE_FSID_MISMATCH, false);
 	if (ret < 0) {
 		errno = -ret;
 		error("could not check mount status of %s: %m", device);
-- 
2.38.1