| Message ID | 1419185238-4254-2-git-send-email-kreijack@inwind.it (mailing list archive) |
|---|---|
| State | Not Applicable |
| Headers | show |
On 2014/12/22 3:07, Goffredo Baroncelli wrote: > Change a spagetti-style code (there are some "interlaced" gotos) to > a more modern style... > > This patch removes also some #define from utils.h, which define > constants used only in cmds-filesystems.c . Instead an enum > is used locally in cmds-filesystems.c . I'm happy if you have a regression test for this patch since such kind of change often cause regression. Thanks, Satoru > --- > cmds-filesystem.c | 106 ++++++++++++++++++++++++++++-------------------------- > utils.h | 3 -- > 2 files changed, 55 insertions(+), 54 deletions(-) > > diff --git a/cmds-filesystem.c b/cmds-filesystem.c > index 7eaccb9..08ddb5d 100644 > --- a/cmds-filesystem.c > +++ b/cmds-filesystem.c > @@ -40,6 +40,11 @@ > #include "list_sort.h" > #include "disk-io.h" > > +enum filesystem_show_scan_method { > + BTRFS_SCAN_ANY, > + BTRFS_SCAN_MOUNTED, > + BTRFS_SCAN_LBLKID > +}; > > /* > * for btrfs fi show, we maintain a hash of fsids we've already printed. > @@ -853,7 +858,7 @@ static int cmd_show(int argc, char **argv) > char *search = NULL; > int ret; > /* default, search both kernel and udev */ > - int where = -1; > + int where = BTRFS_SCAN_ANY; > int type = 0; > char mp[BTRFS_PATH_NAME_MAX + 1]; > char path[PATH_MAX]; > @@ -930,61 +935,60 @@ static int cmd_show(int argc, char **argv) > uuid_unparse(fsid, uuid_buf); > search = uuid_buf; > type = BTRFS_ARG_UUID; > - goto devs_only; > + where = BTRFS_SCAN_LBLKID; > } > } > } > > - if (where == BTRFS_SCAN_LBLKID) > - goto devs_only; > - > - /* show mounted btrfs */ > - ret = btrfs_scan_kernel(search); > - if (search && !ret) { > - /* since search is found we are done */ > - goto out; > - } > - > - /* shows mounted only */ > - if (where == BTRFS_SCAN_MOUNTED) > - goto out; > - > -devs_only: > - ret = btrfs_scan_lblkid(); > - > - if (ret) { > - fprintf(stderr, "ERROR: %d while scanning\n", ret); > - return 1; > - } > - > - found = search_umounted_fs_uuids(&all_uuids, search); > - if (found < 0) { > - fprintf(stderr, > - "ERROR: %d while searching target device\n", ret); > - return 1; > - } > - > - /* > - * The seed/sprout mapping are not detected yet, > - * do mapping build for all umounted fs > - */ > - ret = map_seed_devices(&all_uuids); > - if (ret) { > - fprintf(stderr, > - "ERROR: %d while mapping seed devices\n", ret); > - return 1; > + if (where == BTRFS_SCAN_MOUNTED || where == BTRFS_SCAN_ANY) { > + > + /* show mounted btrfs */ > + ret = btrfs_scan_kernel(search); > + if (search && !ret) { > + /* since search is found we are done */ > + goto out; > + } > + > } > - > - list_for_each_entry(fs_devices, &all_uuids, list) > - print_one_uuid(fs_devices); > - > - if (search && !found) > - ret = 1; > - > - while (!list_empty(&all_uuids)) { > - fs_devices = list_entry(all_uuids.next, > - struct btrfs_fs_devices, list); > - free_fs_devices(fs_devices); > + > + if (where == BTRFS_SCAN_LBLKID || where == BTRFS_SCAN_ANY) { > + > + ret = btrfs_scan_lblkid(); > + > + if (ret) { > + fprintf(stderr, "ERROR: %d while scanning\n", ret); > + return 1; > + } > + > + found = search_umounted_fs_uuids(&all_uuids, search); > + if (found < 0) { > + fprintf(stderr, > + "ERROR: %d while searching target device\n", ret); > + return 1; > + } > + > + /* > + * The seed/sprout mapping are not detected yet, > + * do mapping build for all umounted fs > + */ > + ret = map_seed_devices(&all_uuids); > + if (ret) { > + fprintf(stderr, > + "ERROR: %d while mapping seed devices\n", ret); > + return 1; > + } > + > + list_for_each_entry(fs_devices, &all_uuids, list) > + print_one_uuid(fs_devices); > + > + if (search && !found) > + ret = 1; > + > + while (!list_empty(&all_uuids)) { > + fs_devices = list_entry(all_uuids.next, > + struct btrfs_fs_devices, list); > + free_fs_devices(fs_devices); > + } > } > out: > printf("%s\n", BTRFS_BUILD_VERSION); > diff --git a/utils.h b/utils.h > index 0464c2d..603cdfb 100644 > --- a/utils.h > +++ b/utils.h > @@ -26,9 +26,6 @@ > #define BTRFS_MKFS_SYSTEM_GROUP_SIZE (4 * 1024 * 1024) > #define BTRFS_MKFS_SMALL_VOLUME_SIZE (1024 * 1024 * 1024) > > -#define BTRFS_SCAN_MOUNTED (1ULL << 0) > -#define BTRFS_SCAN_LBLKID (1ULL << 1) > - > #define BTRFS_UPDATE_KERNEL 1 > > #define BTRFS_ARG_UNKNOWN 0 > -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
... Subject: [PATCH][BTRFS-PROGS][CLEANUP] Remove gotos You're being too creative with the subjects and lack the preferred formatting. If you're going to submit more patches, please get it right so I don't have to fix it repeatedly manually. There's a wiki page https://btrfs.wiki.kernel.org/index.php/Developer's_FAQ that should cover the topic. On Sun, Dec 21, 2014 at 07:07:18PM +0100, Goffredo Baroncelli wrote: > Change a spagetti-style code (there are some "interlaced" gotos) to > a more modern style... > > This patch removes also some #define from utils.h, which define > constants used only in cmds-filesystems.c . Instead an enum > is used locally in cmds-filesystems.c . That's an unrelated change, please put it into a separate patch. -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 12/22/2014 12:34 PM, Satoru Takeuchi wrote: > On 2014/12/22 3:07, Goffredo Baroncelli wrote: >> Change a spagetti-style code (there are some "interlaced" gotos) to >> a more modern style... >> >> This patch removes also some #define from utils.h, which define >> constants used only in cmds-filesystems.c . Instead an enum >> is used locally in cmds-filesystems.c . > > I'm happy if you have a regression test for this patch > since such kind of change often cause regression. I am impressed, this is the first time that I received this kind of request on btrfs ml.... In which form you want this "regression test" ? I see a directory tests/ under btrfs-progs; but also I saw reference to xfstest... > > Thanks, > Satoru > >> --- >> cmds-filesystem.c | 106 ++++++++++++++++++++++++++++-------------------------- >> utils.h | 3 -- >> 2 files changed, 55 insertions(+), 54 deletions(-) >> >> diff --git a/cmds-filesystem.c b/cmds-filesystem.c >> index 7eaccb9..08ddb5d 100644 >> --- a/cmds-filesystem.c >> +++ b/cmds-filesystem.c >> @@ -40,6 +40,11 @@ >> #include "list_sort.h" >> #include "disk-io.h" >> >> +enum filesystem_show_scan_method { >> + BTRFS_SCAN_ANY, >> + BTRFS_SCAN_MOUNTED, >> + BTRFS_SCAN_LBLKID >> +}; >> >> /* >> * for btrfs fi show, we maintain a hash of fsids we've already printed. >> @@ -853,7 +858,7 @@ static int cmd_show(int argc, char **argv) >> char *search = NULL; >> int ret; >> /* default, search both kernel and udev */ >> - int where = -1; >> + int where = BTRFS_SCAN_ANY; >> int type = 0; >> char mp[BTRFS_PATH_NAME_MAX + 1]; >> char path[PATH_MAX]; >> @@ -930,61 +935,60 @@ static int cmd_show(int argc, char **argv) >> uuid_unparse(fsid, uuid_buf); >> search = uuid_buf; >> type = BTRFS_ARG_UUID; >> - goto devs_only; >> + where = BTRFS_SCAN_LBLKID; >> } >> } >> } >> >> - if (where == BTRFS_SCAN_LBLKID) >> - goto devs_only; >> - >> - /* show mounted btrfs */ >> - ret = btrfs_scan_kernel(search); >> - if (search && !ret) { >> - /* since search is found we are done */ >> - goto out; >> - } >> - >> - /* shows mounted only */ >> - if (where == BTRFS_SCAN_MOUNTED) >> - goto out; >> - >> -devs_only: >> - ret = btrfs_scan_lblkid(); >> - >> - if (ret) { >> - fprintf(stderr, "ERROR: %d while scanning\n", ret); >> - return 1; >> - } >> - >> - found = search_umounted_fs_uuids(&all_uuids, search); >> - if (found < 0) { >> - fprintf(stderr, >> - "ERROR: %d while searching target device\n", ret); >> - return 1; >> - } >> - >> - /* >> - * The seed/sprout mapping are not detected yet, >> - * do mapping build for all umounted fs >> - */ >> - ret = map_seed_devices(&all_uuids); >> - if (ret) { >> - fprintf(stderr, >> - "ERROR: %d while mapping seed devices\n", ret); >> - return 1; >> + if (where == BTRFS_SCAN_MOUNTED || where == BTRFS_SCAN_ANY) { >> + >> + /* show mounted btrfs */ >> + ret = btrfs_scan_kernel(search); >> + if (search && !ret) { >> + /* since search is found we are done */ >> + goto out; >> + } >> + >> } >> - >> - list_for_each_entry(fs_devices, &all_uuids, list) >> - print_one_uuid(fs_devices); >> - >> - if (search && !found) >> - ret = 1; >> - >> - while (!list_empty(&all_uuids)) { >> - fs_devices = list_entry(all_uuids.next, >> - struct btrfs_fs_devices, list); >> - free_fs_devices(fs_devices); >> + >> + if (where == BTRFS_SCAN_LBLKID || where == BTRFS_SCAN_ANY) { >> + >> + ret = btrfs_scan_lblkid(); >> + >> + if (ret) { >> + fprintf(stderr, "ERROR: %d while scanning\n", ret); >> + return 1; >> + } >> + >> + found = search_umounted_fs_uuids(&all_uuids, search); >> + if (found < 0) { >> + fprintf(stderr, >> + "ERROR: %d while searching target device\n", ret); >> + return 1; >> + } >> + >> + /* >> + * The seed/sprout mapping are not detected yet, >> + * do mapping build for all umounted fs >> + */ >> + ret = map_seed_devices(&all_uuids); >> + if (ret) { >> + fprintf(stderr, >> + "ERROR: %d while mapping seed devices\n", ret); >> + return 1; >> + } >> + >> + list_for_each_entry(fs_devices, &all_uuids, list) >> + print_one_uuid(fs_devices); >> + >> + if (search && !found) >> + ret = 1; >> + >> + while (!list_empty(&all_uuids)) { >> + fs_devices = list_entry(all_uuids.next, >> + struct btrfs_fs_devices, list); >> + free_fs_devices(fs_devices); >> + } >> } >> out: >> printf("%s\n", BTRFS_BUILD_VERSION); >> diff --git a/utils.h b/utils.h >> index 0464c2d..603cdfb 100644 >> --- a/utils.h >> +++ b/utils.h >> @@ -26,9 +26,6 @@ >> #define BTRFS_MKFS_SYSTEM_GROUP_SIZE (4 * 1024 * 1024) >> #define BTRFS_MKFS_SMALL_VOLUME_SIZE (1024 * 1024 * 1024) >> >> -#define BTRFS_SCAN_MOUNTED (1ULL << 0) >> -#define BTRFS_SCAN_LBLKID (1ULL << 1) >> - >> #define BTRFS_UPDATE_KERNEL 1 >> >> #define BTRFS_ARG_UNKNOWN 0 >> > >
Hi Goffredo, On 2014/12/23 4:07, Goffredo Baroncelli wrote: > On 12/22/2014 12:34 PM, Satoru Takeuchi wrote: >> On 2014/12/22 3:07, Goffredo Baroncelli wrote: >>> Change a spagetti-style code (there are some "interlaced" gotos) to >>> a more modern style... >>> >>> This patch removes also some #define from utils.h, which define >>> constants used only in cmds-filesystems.c . Instead an enum >>> is used locally in cmds-filesystems.c . >> >> I'm happy if you have a regression test for this patch >> since such kind of change often cause regression. > > I am impressed, this is the first time that I received this kind > of request on btrfs ml.... It's because I'd like to contribute to Btrfs not only by submitting patches, but also by reviewing/testing other guy's patches to improve its quality better. > > In which form you want this "regression test" ? I see a > directory tests/ under btrfs-progs; but also I saw > reference to xfstest... Any format is OK, for example a simple shell script. Thanks, Satoru > > > >> >> Thanks, >> Satoru >> >>> --- >>> cmds-filesystem.c | 106 ++++++++++++++++++++++++++++-------------------------- >>> utils.h | 3 -- >>> 2 files changed, 55 insertions(+), 54 deletions(-) >>> >>> diff --git a/cmds-filesystem.c b/cmds-filesystem.c >>> index 7eaccb9..08ddb5d 100644 >>> --- a/cmds-filesystem.c >>> +++ b/cmds-filesystem.c >>> @@ -40,6 +40,11 @@ >>> #include "list_sort.h" >>> #include "disk-io.h" >>> >>> +enum filesystem_show_scan_method { >>> + BTRFS_SCAN_ANY, >>> + BTRFS_SCAN_MOUNTED, >>> + BTRFS_SCAN_LBLKID >>> +}; >>> >>> /* >>> * for btrfs fi show, we maintain a hash of fsids we've already printed. >>> @@ -853,7 +858,7 @@ static int cmd_show(int argc, char **argv) >>> char *search = NULL; >>> int ret; >>> /* default, search both kernel and udev */ >>> - int where = -1; >>> + int where = BTRFS_SCAN_ANY; >>> int type = 0; >>> char mp[BTRFS_PATH_NAME_MAX + 1]; >>> char path[PATH_MAX]; >>> @@ -930,61 +935,60 @@ static int cmd_show(int argc, char **argv) >>> uuid_unparse(fsid, uuid_buf); >>> search = uuid_buf; >>> type = BTRFS_ARG_UUID; >>> - goto devs_only; >>> + where = BTRFS_SCAN_LBLKID; >>> } >>> } >>> } >>> >>> - if (where == BTRFS_SCAN_LBLKID) >>> - goto devs_only; >>> - >>> - /* show mounted btrfs */ >>> - ret = btrfs_scan_kernel(search); >>> - if (search && !ret) { >>> - /* since search is found we are done */ >>> - goto out; >>> - } >>> - >>> - /* shows mounted only */ >>> - if (where == BTRFS_SCAN_MOUNTED) >>> - goto out; >>> - >>> -devs_only: >>> - ret = btrfs_scan_lblkid(); >>> - >>> - if (ret) { >>> - fprintf(stderr, "ERROR: %d while scanning\n", ret); >>> - return 1; >>> - } >>> - >>> - found = search_umounted_fs_uuids(&all_uuids, search); >>> - if (found < 0) { >>> - fprintf(stderr, >>> - "ERROR: %d while searching target device\n", ret); >>> - return 1; >>> - } >>> - >>> - /* >>> - * The seed/sprout mapping are not detected yet, >>> - * do mapping build for all umounted fs >>> - */ >>> - ret = map_seed_devices(&all_uuids); >>> - if (ret) { >>> - fprintf(stderr, >>> - "ERROR: %d while mapping seed devices\n", ret); >>> - return 1; >>> + if (where == BTRFS_SCAN_MOUNTED || where == BTRFS_SCAN_ANY) { >>> + >>> + /* show mounted btrfs */ >>> + ret = btrfs_scan_kernel(search); >>> + if (search && !ret) { >>> + /* since search is found we are done */ >>> + goto out; >>> + } >>> + >>> } >>> - >>> - list_for_each_entry(fs_devices, &all_uuids, list) >>> - print_one_uuid(fs_devices); >>> - >>> - if (search && !found) >>> - ret = 1; >>> - >>> - while (!list_empty(&all_uuids)) { >>> - fs_devices = list_entry(all_uuids.next, >>> - struct btrfs_fs_devices, list); >>> - free_fs_devices(fs_devices); >>> + >>> + if (where == BTRFS_SCAN_LBLKID || where == BTRFS_SCAN_ANY) { >>> + >>> + ret = btrfs_scan_lblkid(); >>> + >>> + if (ret) { >>> + fprintf(stderr, "ERROR: %d while scanning\n", ret); >>> + return 1; >>> + } >>> + >>> + found = search_umounted_fs_uuids(&all_uuids, search); >>> + if (found < 0) { >>> + fprintf(stderr, >>> + "ERROR: %d while searching target device\n", ret); >>> + return 1; >>> + } >>> + >>> + /* >>> + * The seed/sprout mapping are not detected yet, >>> + * do mapping build for all umounted fs >>> + */ >>> + ret = map_seed_devices(&all_uuids); >>> + if (ret) { >>> + fprintf(stderr, >>> + "ERROR: %d while mapping seed devices\n", ret); >>> + return 1; >>> + } >>> + >>> + list_for_each_entry(fs_devices, &all_uuids, list) >>> + print_one_uuid(fs_devices); >>> + >>> + if (search && !found) >>> + ret = 1; >>> + >>> + while (!list_empty(&all_uuids)) { >>> + fs_devices = list_entry(all_uuids.next, >>> + struct btrfs_fs_devices, list); >>> + free_fs_devices(fs_devices); >>> + } >>> } >>> out: >>> printf("%s\n", BTRFS_BUILD_VERSION); >>> diff --git a/utils.h b/utils.h >>> index 0464c2d..603cdfb 100644 >>> --- a/utils.h >>> +++ b/utils.h >>> @@ -26,9 +26,6 @@ >>> #define BTRFS_MKFS_SYSTEM_GROUP_SIZE (4 * 1024 * 1024) >>> #define BTRFS_MKFS_SMALL_VOLUME_SIZE (1024 * 1024 * 1024) >>> >>> -#define BTRFS_SCAN_MOUNTED (1ULL << 0) >>> -#define BTRFS_SCAN_LBLKID (1ULL << 1) >>> - >>> #define BTRFS_UPDATE_KERNEL 1 >>> >>> #define BTRFS_ARG_UNKNOWN 0 >>> >> >> > > -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/cmds-filesystem.c b/cmds-filesystem.c index 7eaccb9..08ddb5d 100644 --- a/cmds-filesystem.c +++ b/cmds-filesystem.c @@ -40,6 +40,11 @@ #include "list_sort.h" #include "disk-io.h" +enum filesystem_show_scan_method { + BTRFS_SCAN_ANY, + BTRFS_SCAN_MOUNTED, + BTRFS_SCAN_LBLKID +}; /* * for btrfs fi show, we maintain a hash of fsids we've already printed. @@ -853,7 +858,7 @@ static int cmd_show(int argc, char **argv) char *search = NULL; int ret; /* default, search both kernel and udev */ - int where = -1; + int where = BTRFS_SCAN_ANY; int type = 0; char mp[BTRFS_PATH_NAME_MAX + 1]; char path[PATH_MAX]; @@ -930,61 +935,60 @@ static int cmd_show(int argc, char **argv) uuid_unparse(fsid, uuid_buf); search = uuid_buf; type = BTRFS_ARG_UUID; - goto devs_only; + where = BTRFS_SCAN_LBLKID; } } } - if (where == BTRFS_SCAN_LBLKID) - goto devs_only; - - /* show mounted btrfs */ - ret = btrfs_scan_kernel(search); - if (search && !ret) { - /* since search is found we are done */ - goto out; - } - - /* shows mounted only */ - if (where == BTRFS_SCAN_MOUNTED) - goto out; - -devs_only: - ret = btrfs_scan_lblkid(); - - if (ret) { - fprintf(stderr, "ERROR: %d while scanning\n", ret); - return 1; - } - - found = search_umounted_fs_uuids(&all_uuids, search); - if (found < 0) { - fprintf(stderr, - "ERROR: %d while searching target device\n", ret); - return 1; - } - - /* - * The seed/sprout mapping are not detected yet, - * do mapping build for all umounted fs - */ - ret = map_seed_devices(&all_uuids); - if (ret) { - fprintf(stderr, - "ERROR: %d while mapping seed devices\n", ret); - return 1; + if (where == BTRFS_SCAN_MOUNTED || where == BTRFS_SCAN_ANY) { + + /* show mounted btrfs */ + ret = btrfs_scan_kernel(search); + if (search && !ret) { + /* since search is found we are done */ + goto out; + } + } - - list_for_each_entry(fs_devices, &all_uuids, list) - print_one_uuid(fs_devices); - - if (search && !found) - ret = 1; - - while (!list_empty(&all_uuids)) { - fs_devices = list_entry(all_uuids.next, - struct btrfs_fs_devices, list); - free_fs_devices(fs_devices); + + if (where == BTRFS_SCAN_LBLKID || where == BTRFS_SCAN_ANY) { + + ret = btrfs_scan_lblkid(); + + if (ret) { + fprintf(stderr, "ERROR: %d while scanning\n", ret); + return 1; + } + + found = search_umounted_fs_uuids(&all_uuids, search); + if (found < 0) { + fprintf(stderr, + "ERROR: %d while searching target device\n", ret); + return 1; + } + + /* + * The seed/sprout mapping are not detected yet, + * do mapping build for all umounted fs + */ + ret = map_seed_devices(&all_uuids); + if (ret) { + fprintf(stderr, + "ERROR: %d while mapping seed devices\n", ret); + return 1; + } + + list_for_each_entry(fs_devices, &all_uuids, list) + print_one_uuid(fs_devices); + + if (search && !found) + ret = 1; + + while (!list_empty(&all_uuids)) { + fs_devices = list_entry(all_uuids.next, + struct btrfs_fs_devices, list); + free_fs_devices(fs_devices); + } } out: printf("%s\n", BTRFS_BUILD_VERSION); diff --git a/utils.h b/utils.h index 0464c2d..603cdfb 100644 --- a/utils.h +++ b/utils.h @@ -26,9 +26,6 @@ #define BTRFS_MKFS_SYSTEM_GROUP_SIZE (4 * 1024 * 1024) #define BTRFS_MKFS_SMALL_VOLUME_SIZE (1024 * 1024 * 1024) -#define BTRFS_SCAN_MOUNTED (1ULL << 0) -#define BTRFS_SCAN_LBLKID (1ULL << 1) - #define BTRFS_UPDATE_KERNEL 1 #define BTRFS_ARG_UNKNOWN 0