| Message ID | 27568376033263288027ccb60dbbc0d9f78c4744.1690985783.git.anand.jain@oracle.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | fixes and preparatory related to metadata_uuid | expand |
On Thu, Aug 03, 2023 at 07:29:46AM +0800, Anand Jain wrote: > The upcoming "--device" option requires memory to parse devices, which > should be freed before returning from the main() function. As a > preparation for adding the "--device" option to the "btrfstune" command, > provide a consolidated error return exit from the main function with a > "goto" labeled "free_out". The label "free_out" may not make sense > currently, but it will when the "--device" option is added. > > There are several return statements within the main function, and > changing all of them in the main "--device" feature patch would deviate > from the actual for the feature changes. Hence, it made sense to create > a preparatory patch. > > The return value for any failure remains the same as in the original code. > > Signed-off-by: Anand Jain <anand.jain@oracle.com> > --- > tune/main.c | 27 +++++++++++++++++---------- > 1 file changed, 17 insertions(+), 10 deletions(-) > > diff --git a/tune/main.c b/tune/main.c > index d6c01bb75261..7951fa15b59d 100644 > --- a/tune/main.c > +++ b/tune/main.c > @@ -145,7 +145,7 @@ int BOX_MAIN(btrfstune)(int argc, char *argv[]) > bool to_fst = false; > int csum_type = -1; > char *new_fsid_str = NULL; > - int ret; > + int ret = 1; The pattern I'd like to use is to keep ret either uninitialized or 0 and let each jump to error set the value explicitly, so it's clear without going back to the initialization. > u64 super_flags = 0; > int fd = -1; > > @@ -233,18 +233,18 @@ int BOX_MAIN(btrfstune)(int argc, char *argv[]) > set_argv0(argv); > device = argv[optind]; > if (check_argc_exact(argc - optind, 1)) > - return 1; > + goto free_out; > > if (random_fsid && new_fsid_str) { > error("random fsid can't be used with specified fsid"); > - return 1; ie. ret = 1; > + goto free_out; > } > if (!super_flags && !seeding_flag && !(random_fsid || new_fsid_str) && > !change_metadata_uuid && csum_type == -1 && !to_bg_tree && > !to_extent_tree && !to_fst) { > error("at least one option should be specified"); > usage(&tune_cmd, 1); > - return 1; > + goto free_out;
On 12/8/23 01:47, David Sterba wrote: > On Thu, Aug 03, 2023 at 07:29:46AM +0800, Anand Jain wrote: >> The upcoming "--device" option requires memory to parse devices, which >> should be freed before returning from the main() function. As a >> preparation for adding the "--device" option to the "btrfstune" command, >> provide a consolidated error return exit from the main function with a >> "goto" labeled "free_out". The label "free_out" may not make sense >> currently, but it will when the "--device" option is added. >> >> There are several return statements within the main function, and >> changing all of them in the main "--device" feature patch would deviate >> from the actual for the feature changes. Hence, it made sense to create >> a preparatory patch. >> >> The return value for any failure remains the same as in the original code. >> >> Signed-off-by: Anand Jain <anand.jain@oracle.com> >> --- >> tune/main.c | 27 +++++++++++++++++---------- >> 1 file changed, 17 insertions(+), 10 deletions(-) >> >> diff --git a/tune/main.c b/tune/main.c >> index d6c01bb75261..7951fa15b59d 100644 >> --- a/tune/main.c >> +++ b/tune/main.c >> @@ -145,7 +145,7 @@ int BOX_MAIN(btrfstune)(int argc, char *argv[]) >> bool to_fst = false; >> int csum_type = -1; >> char *new_fsid_str = NULL; >> - int ret; >> + int ret = 1; > > The pattern I'd like to use is to keep ret either uninitialized or 0 and > let each jump to error set the value explicitly, so it's clear without > going back to the initialization. Understood. However, if uninitialized, older GCC versions might give false positive warnings for uninitialized variable usage, though it's not an issue here. > >> u64 super_flags = 0; >> int fd = -1; >> >> @@ -233,18 +233,18 @@ int BOX_MAIN(btrfstune)(int argc, char *argv[]) >> set_argv0(argv); >> device = argv[optind]; >> if (check_argc_exact(argc - optind, 1)) >> - return 1; >> + goto free_out; >> >> if (random_fsid && new_fsid_str) { >> error("random fsid can't be used with specified fsid"); >> - return 1; > > ie. ret = 1; Code in the devel looks good. Thanks. > >> + goto free_out; >> } >> if (!super_flags && !seeding_flag && !(random_fsid || new_fsid_str) && >> !change_metadata_uuid && csum_type == -1 && !to_bg_tree && >> !to_extent_tree && !to_fst) { >> error("at least one option should be specified"); >> usage(&tune_cmd, 1); >> - return 1; >> + goto free_out;
diff --git a/tune/main.c b/tune/main.c index d6c01bb75261..7951fa15b59d 100644 --- a/tune/main.c +++ b/tune/main.c @@ -145,7 +145,7 @@ int BOX_MAIN(btrfstune)(int argc, char *argv[]) bool to_fst = false; int csum_type = -1; char *new_fsid_str = NULL; - int ret; + int ret = 1; u64 super_flags = 0; int fd = -1; @@ -233,18 +233,18 @@ int BOX_MAIN(btrfstune)(int argc, char *argv[]) set_argv0(argv); device = argv[optind]; if (check_argc_exact(argc - optind, 1)) - return 1; + goto free_out; if (random_fsid && new_fsid_str) { error("random fsid can't be used with specified fsid"); - return 1; + goto free_out; } if (!super_flags && !seeding_flag && !(random_fsid || new_fsid_str) && !change_metadata_uuid && csum_type == -1 && !to_bg_tree && !to_extent_tree && !to_fst) { error("at least one option should be specified"); usage(&tune_cmd, 1); - return 1; + goto free_out; } if (new_fsid_str) { @@ -253,18 +253,21 @@ int BOX_MAIN(btrfstune)(int argc, char *argv[]) ret = uuid_parse(new_fsid_str, tmp); if (ret < 0) { error("could not parse UUID: %s", new_fsid_str); - return 1; + ret = 1; + goto free_out; } if (!test_uuid_unique(new_fsid_str)) { error("fsid %s is not unique", new_fsid_str); - return 1; + ret = 1; + goto free_out; } } fd = open(device, O_RDWR); if (fd < 0) { error("mount check: cannot open %s: %m", device); - return 1; + ret = 1; + goto free_out; } ret = check_mounted_where(fd, device, NULL, 0, NULL, @@ -273,18 +276,21 @@ int BOX_MAIN(btrfstune)(int argc, char *argv[]) errno = -ret; error("could not check mount status of %s: %m", device); close(fd); - return 1; + ret = 1; + goto free_out; } else if (ret) { error("%s is mounted", device); close(fd); - return 1; + ret = 1; + goto free_out; } root = open_ctree_fd(fd, device, 0, ctree_flags); if (!root) { error("open ctree failed"); - return 1; + ret = 1; + goto free_out; } /* @@ -450,5 +456,6 @@ out: close_ctree(root); btrfs_close_all_devices(); +free_out: return ret; }
The upcoming "--device" option requires memory to parse devices, which should be freed before returning from the main() function. As a preparation for adding the "--device" option to the "btrfstune" command, provide a consolidated error return exit from the main function with a "goto" labeled "free_out". The label "free_out" may not make sense currently, but it will when the "--device" option is added. There are several return statements within the main function, and changing all of them in the main "--device" feature patch would deviate from the actual for the feature changes. Hence, it made sense to create a preparatory patch. The return value for any failure remains the same as in the original code. Signed-off-by: Anand Jain <anand.jain@oracle.com> --- tune/main.c | 27 +++++++++++++++++---------- 1 file changed, 17 insertions(+), 10 deletions(-)