| Message ID | d6b012af9307b8ff71a3715e2e3d5cc58fafbee4.1686202417.git.anand.jain@oracle.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | btrfs-progs: cleanup and preparatory around device scan | expand |
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;
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
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
>> 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,
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;
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(-)