* Re: [PATCH] btrfs-progs: tidy up cmd_subvol_create() whitespace & returns [not found] <1378213291-1034-1-git-send-email-wangshilong1991@gmail.com> @ 2013-09-03 13:13 ` Wang Shilong 2013-09-03 15:50 ` Eric Sandeen 0 siblings, 1 reply; 5+ messages in thread From: Wang Shilong @ 2013-09-03 13:13 UTC (permalink / raw) To: Eric Sandeen Cc: linux-btrfs@vger.kernel.org Btrfs, dsterba@suse.cz Sterba, Miao Xie, Wang Shilong Hello Eric, Recently, i notice btrfs-progs's magic return value. For example, EACCESS return 12. Magic return value is confusing for code reviewers. However,at least we should guarantee all these conditions return the same value(for example 12 for this case) Or we can change all the magic number to 1. Miao reminded me that there may be some applications that have catched and checked these magic return values… Any ideas about this? Thanks, Wang > From: > Just whitespace fixes, and magical return value removal. > > Signed-off-by: Eric Sandeen <sandeen@redhat.com> > --- > cmds-subvolume.c | 18 +++++++++--------- > 1 file changed, 9 insertions(+), 9 deletions(-) > > diff --git a/cmds-subvolume.c b/cmds-subvolume.c > index 01b982c..a9999ac 100644 > --- a/cmds-subvolume.c > +++ b/cmds-subvolume.c > @@ -104,9 +104,9 @@ static int cmd_subvol_create(int argc, char **argv) > dst = argv[optind]; > > res = test_isdir(dst); > - if(res >= 0 ){ > + if (res >= 0) { > fprintf(stderr, "ERROR: '%s' exists\n", dst); > - return 12; > + return 1; > } > > newname = strdup(dst); > @@ -114,24 +114,24 @@ static int cmd_subvol_create(int argc, char **argv) > dstdir = strdup(dst); > dstdir = dirname(dstdir); > > - if( !strcmp(newname,".") || !strcmp(newname,"..") || > + if (!strcmp(newname, ".") || !strcmp(newname, "..") || > strchr(newname, '/') ){ > fprintf(stderr, "ERROR: uncorrect subvolume name ('%s')\n", > newname); > - return 14; > + return 1; > } > > len = strlen(newname); > if (len == 0 || len >= BTRFS_VOL_NAME_MAX) { > fprintf(stderr, "ERROR: subvolume name too long ('%s)\n", > newname); > - return 14; > + return 1; > } > > fddst = open_file_or_dir(dstdir); > if (fddst < 0) { > fprintf(stderr, "ERROR: can't access to '%s'\n", dstdir); > - return 12; > + return 1; > } > > printf("Create subvolume '%s/%s'\n", dstdir, newname); > @@ -159,10 +159,10 @@ static int cmd_subvol_create(int argc, char **argv) > close(fddst); > free(inherit); > > - if(res < 0 ){ > - fprintf( stderr, "ERROR: cannot create subvolume - %s\n", > + if (res < 0) { > + fprintf(stderr, "ERROR: cannot create subvolume - %s\n", > strerror(e)); > - return 11; > + return 1; > } > > return 0; > -- > 1.7.11.7 > ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] btrfs-progs: tidy up cmd_subvol_create() whitespace & returns 2013-09-03 13:13 ` [PATCH] btrfs-progs: tidy up cmd_subvol_create() whitespace & returns Wang Shilong @ 2013-09-03 15:50 ` Eric Sandeen 2013-09-04 1:20 ` Wang Shilong 0 siblings, 1 reply; 5+ messages in thread From: Eric Sandeen @ 2013-09-03 15:50 UTC (permalink / raw) To: Wang Shilong Cc: linux-btrfs@vger.kernel.org Btrfs, dsterba@suse.cz Sterba, Miao Xie, Wang Shilong On 9/3/13 8:13 AM, Wang Shilong wrote: > Hello Eric, > > Recently, i notice btrfs-progs's magic return value. For example, EACCESS return 12. > Magic return value is confusing for code reviewers. However,at least we should guarantee > all these conditions return the same value(for example 12 for this case) > > Or we can change all the magic number to 1. Miao reminded me that > there may be some applications that have catched and checked these > magic return values… > > Any ideas about this? I think that if we want to do anything different from the standard, expected "return 1 and set errno" then it needs to be a careful design decision, documented, used consistently, and tested. As far as I can tell, what is in btrfs-progs today is undocumented, untested, and only occasionally/randomly used. Therefore I don't think it's useful as it stands today, and why I had started removing them. -Eric > Thanks, > Wang >> From: >> Just whitespace fixes, and magical return value removal. >> >> Signed-off-by: Eric Sandeen <sandeen@redhat.com> >> --- >> cmds-subvolume.c | 18 +++++++++--------- >> 1 file changed, 9 insertions(+), 9 deletions(-) >> >> diff --git a/cmds-subvolume.c b/cmds-subvolume.c >> index 01b982c..a9999ac 100644 >> --- a/cmds-subvolume.c >> +++ b/cmds-subvolume.c >> @@ -104,9 +104,9 @@ static int cmd_subvol_create(int argc, char **argv) >> dst = argv[optind]; >> >> res = test_isdir(dst); >> - if(res >= 0 ){ >> + if (res >= 0) { >> fprintf(stderr, "ERROR: '%s' exists\n", dst); >> - return 12; >> + return 1; >> } >> >> newname = strdup(dst); >> @@ -114,24 +114,24 @@ static int cmd_subvol_create(int argc, char **argv) >> dstdir = strdup(dst); >> dstdir = dirname(dstdir); >> >> - if( !strcmp(newname,".") || !strcmp(newname,"..") || >> + if (!strcmp(newname, ".") || !strcmp(newname, "..") || >> strchr(newname, '/') ){ >> fprintf(stderr, "ERROR: uncorrect subvolume name ('%s')\n", >> newname); >> - return 14; >> + return 1; >> } >> >> len = strlen(newname); >> if (len == 0 || len >= BTRFS_VOL_NAME_MAX) { >> fprintf(stderr, "ERROR: subvolume name too long ('%s)\n", >> newname); >> - return 14; >> + return 1; >> } >> >> fddst = open_file_or_dir(dstdir); >> if (fddst < 0) { >> fprintf(stderr, "ERROR: can't access to '%s'\n", dstdir); >> - return 12; >> + return 1; >> } >> >> printf("Create subvolume '%s/%s'\n", dstdir, newname); >> @@ -159,10 +159,10 @@ static int cmd_subvol_create(int argc, char **argv) >> close(fddst); >> free(inherit); >> >> - if(res < 0 ){ >> - fprintf( stderr, "ERROR: cannot create subvolume - %s\n", >> + if (res < 0) { >> + fprintf(stderr, "ERROR: cannot create subvolume - %s\n", >> strerror(e)); >> - return 11; >> + return 1; >> } >> >> return 0; >> -- >> 1.7.11.7 >> > ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] btrfs-progs: tidy up cmd_subvol_create() whitespace & returns 2013-09-03 15:50 ` Eric Sandeen @ 2013-09-04 1:20 ` Wang Shilong 2013-09-04 1:24 ` Eric Sandeen 0 siblings, 1 reply; 5+ messages in thread From: Wang Shilong @ 2013-09-04 1:20 UTC (permalink / raw) To: Eric Sandeen Cc: Wang Shilong, linux-btrfs@vger.kernel.org Btrfs, dsterba@suse.cz Sterba, Miao Xie On 09/03/2013 11:50 PM, Eric Sandeen wrote: > On 9/3/13 8:13 AM, Wang Shilong wrote: >> Hello Eric, >> >> Recently, i notice btrfs-progs's magic return value. For example, EACCESS return 12. >> Magic return value is confusing for code reviewers. However,at least we should guarantee >> all these conditions return the same value(for example 12 for this case) >> >> Or we can change all the magic number to 1. Miao reminded me that >> there may be some applications that have catched and checked these >> magic return values… >> >> Any ideas about this? > I think that if we want to do anything different from the standard, expected > "return 1 and set errno" then it needs to be a careful design decision, documented, > used consistently, and tested. I'd prefer to use standard approach.There are still many magic return values unfixed.If there are no objections, i would do like to correct the magic return value following the rule: 0 No errors 1 Usage or syntax error Exceptions come from btrfs scrub/fsck,getting such operations' status etc.It is meaningful to differ these return values. Thanks, Wang > > As far as I can tell, what is in btrfs-progs today is undocumented, > untested, and only occasionally/randomly used. Therefore I don't > think it's useful as it stands today, and why I had started removing > them. > > -Eric > >> Thanks, >> Wang >>> From: >>> Just whitespace fixes, and magical return value removal. >>> >>> Signed-off-by: Eric Sandeen <sandeen@redhat.com> >>> --- >>> cmds-subvolume.c | 18 +++++++++--------- >>> 1 file changed, 9 insertions(+), 9 deletions(-) >>> >>> diff --git a/cmds-subvolume.c b/cmds-subvolume.c >>> index 01b982c..a9999ac 100644 >>> --- a/cmds-subvolume.c >>> +++ b/cmds-subvolume.c >>> @@ -104,9 +104,9 @@ static int cmd_subvol_create(int argc, char **argv) >>> dst = argv[optind]; >>> >>> res = test_isdir(dst); >>> - if(res >= 0 ){ >>> + if (res >= 0) { >>> fprintf(stderr, "ERROR: '%s' exists\n", dst); >>> - return 12; >>> + return 1; >>> } >>> >>> newname = strdup(dst); >>> @@ -114,24 +114,24 @@ static int cmd_subvol_create(int argc, char **argv) >>> dstdir = strdup(dst); >>> dstdir = dirname(dstdir); >>> >>> - if( !strcmp(newname,".") || !strcmp(newname,"..") || >>> + if (!strcmp(newname, ".") || !strcmp(newname, "..") || >>> strchr(newname, '/') ){ >>> fprintf(stderr, "ERROR: uncorrect subvolume name ('%s')\n", >>> newname); >>> - return 14; >>> + return 1; >>> } >>> >>> len = strlen(newname); >>> if (len == 0 || len >= BTRFS_VOL_NAME_MAX) { >>> fprintf(stderr, "ERROR: subvolume name too long ('%s)\n", >>> newname); >>> - return 14; >>> + return 1; >>> } >>> >>> fddst = open_file_or_dir(dstdir); >>> if (fddst < 0) { >>> fprintf(stderr, "ERROR: can't access to '%s'\n", dstdir); >>> - return 12; >>> + return 1; >>> } >>> >>> printf("Create subvolume '%s/%s'\n", dstdir, newname); >>> @@ -159,10 +159,10 @@ static int cmd_subvol_create(int argc, char **argv) >>> close(fddst); >>> free(inherit); >>> >>> - if(res < 0 ){ >>> - fprintf( stderr, "ERROR: cannot create subvolume - %s\n", >>> + if (res < 0) { >>> + fprintf(stderr, "ERROR: cannot create subvolume - %s\n", >>> strerror(e)); >>> - return 11; >>> + return 1; >>> } >>> >>> return 0; >>> -- >>> 1.7.11.7 >>> > ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] btrfs-progs: tidy up cmd_subvol_create() whitespace & returns 2013-09-04 1:20 ` Wang Shilong @ 2013-09-04 1:24 ` Eric Sandeen 2013-09-04 1:40 ` Wang Shilong 0 siblings, 1 reply; 5+ messages in thread From: Eric Sandeen @ 2013-09-04 1:24 UTC (permalink / raw) To: Wang Shilong Cc: Eric Sandeen, Wang Shilong, linux-btrfs@vger.kernel.org Btrfs, dsterba@suse.cz Sterba, Miao Xie [-- Warning: decoded text below may be mangled, UTF-8 assumed --] [-- Attachment #1: Type: text/plain; charset=utf-8, Size: 4103 bytes --] MOn Sep 3, 2013, at 8:22 PM, Wang Shilong <wangsl.fnst@cn.fujitsu.com> wrote: > On 09/03/2013 11:50 PM, Eric Sandeen wrote: >> On 9/3/13 8:13 AM, Wang Shilong wrote: >>> Hello Eric, >>> >>> Recently, i notice btrfs-progs's magic return value. For example, EACCESS return 12. >>> Magic return value is confusing for code reviewers. However,at least we should guarantee >>> all these conditions return the same value(for example 12 for this case) >>> >>> Or we can change all the magic number to 1. Miao reminded me that >>> there may be some applications that have catched and checked these >>> magic return values… >>> >>> Any ideas about this? >> I think that if we want to do anything different from the standard, expected >> "return 1 and set errno" then it needs to be a careful design decision, documented, >> used consistently, and tested. > I'd prefer to use standard approach.There are still many magic return > values unfixed.If > there are no objections, i would do like to correct the magic return > value following the rule: > > 0 No errors > 1 Usage or syntax error > > Exceptions come from btrfs scrub/fsck,getting such operations' status > etc.It is meaningful to > differ these return values. > Fsck in particular has some semi-standard, documented return values I think. Eric > Thanks, > Wang >> >> As far as I can tell, what is in btrfs-progs today is undocumented, >> untested, and only occasionally/randomly used. Therefore I don't >> think it's useful as it stands today, and why I had started removing >> them. >> >> -Eric >> >>> Thanks, >>> Wang >>>> From: >>>> Just whitespace fixes, and magical return value removal. >>>> >>>> Signed-off-by: Eric Sandeen <sandeen@redhat.com> >>>> --- >>>> cmds-subvolume.c | 18 +++++++++--------- >>>> 1 file changed, 9 insertions(+), 9 deletions(-) >>>> >>>> diff --git a/cmds-subvolume.c b/cmds-subvolume.c >>>> index 01b982c..a9999ac 100644 >>>> --- a/cmds-subvolume.c >>>> +++ b/cmds-subvolume.c >>>> @@ -104,9 +104,9 @@ static int cmd_subvol_create(int argc, char **argv) >>>> dst = argv[optind]; >>>> >>>> res = test_isdir(dst); >>>> - if(res >= 0 ){ >>>> + if (res >= 0) { >>>> fprintf(stderr, "ERROR: '%s' exists\n", dst); >>>> - return 12; >>>> + return 1; >>>> } >>>> >>>> newname = strdup(dst); >>>> @@ -114,24 +114,24 @@ static int cmd_subvol_create(int argc, char **argv) >>>> dstdir = strdup(dst); >>>> dstdir = dirname(dstdir); >>>> >>>> - if( !strcmp(newname,".") || !strcmp(newname,"..") || >>>> + if (!strcmp(newname, ".") || !strcmp(newname, "..") || >>>> strchr(newname, '/') ){ >>>> fprintf(stderr, "ERROR: uncorrect subvolume name ('%s')\n", >>>> newname); >>>> - return 14; >>>> + return 1; >>>> } >>>> >>>> len = strlen(newname); >>>> if (len == 0 || len >= BTRFS_VOL_NAME_MAX) { >>>> fprintf(stderr, "ERROR: subvolume name too long ('%s)\n", >>>> newname); >>>> - return 14; >>>> + return 1; >>>> } >>>> >>>> fddst = open_file_or_dir(dstdir); >>>> if (fddst < 0) { >>>> fprintf(stderr, "ERROR: can't access to '%s'\n", dstdir); >>>> - return 12; >>>> + return 1; >>>> } >>>> >>>> printf("Create subvolume '%s/%s'\n", dstdir, newname); >>>> @@ -159,10 +159,10 @@ static int cmd_subvol_create(int argc, char **argv) >>>> close(fddst); >>>> free(inherit); >>>> >>>> - if(res < 0 ){ >>>> - fprintf( stderr, "ERROR: cannot create subvolume - %s\n", >>>> + if (res < 0) { >>>> + fprintf(stderr, "ERROR: cannot create subvolume - %s\n", >>>> strerror(e)); >>>> - return 11; >>>> + return 1; >>>> } >>>> >>>> return 0; >>>> -- >>>> 1.7.11.7 > > -- > 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 ÿôèº{.nÇ+‰·Ÿ®‰†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±ý»k~ÏâžØ^n‡r¡ö¦zË\x1aëh™¨èÚ&£ûàz¿äz¹Þ—ú+€Ê+zf£¢·hšˆ§~††Ûiÿÿïêÿ‘êçz_è®\x0fæj:+v‰¨þ)ߣøm ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] btrfs-progs: tidy up cmd_subvol_create() whitespace & returns 2013-09-04 1:24 ` Eric Sandeen @ 2013-09-04 1:40 ` Wang Shilong 0 siblings, 0 replies; 5+ messages in thread From: Wang Shilong @ 2013-09-04 1:40 UTC (permalink / raw) To: Eric Sandeen Cc: Eric Sandeen, Wang Shilong, linux-btrfs@vger.kernel.org Btrfs, dsterba@suse.cz Sterba, Miao Xie On 09/04/2013 09:24 AM, Eric Sandeen wrote: > MOn Sep 3, 2013, at 8:22 PM, Wang Shilong <wangsl.fnst@cn.fujitsu.com> wrote: > >> On 09/03/2013 11:50 PM, Eric Sandeen wrote: >>> On 9/3/13 8:13 AM, Wang Shilong wrote: >>>> Hello Eric, >>>> >>>> Recently, i notice btrfs-progs's magic return value. For example, EACCESS return 12. >>>> Magic return value is confusing for code reviewers. However,at least we should guarantee >>>> all these conditions return the same value(for example 12 for this case) >>>> >>>> Or we can change all the magic number to 1. Miao reminded me that >>>> there may be some applications that have catched and checked these >>>> magic return values… >>>> >>>> Any ideas about this? >>> I think that if we want to do anything different from the standard, expected >>> "return 1 and set errno" then it needs to be a careful design decision, documented, >>> used consistently, and tested. >> I'd prefer to use standard approach.There are still many magic return >> values unfixed.If >> there are no objections, i would do like to correct the magic return >> value following the rule: >> >> 0 No errors >> 1 Usage or syntax error >> >> Exceptions come from btrfs scrub/fsck,getting such operations' status >> etc.It is meaningful to >> differ these return values. >> > Fsck in particular has some semi-standard, documented return values I think See it, thanks for reminding^_^ Thanks, Wang > > Eric > >> Thanks, >> Wang >>> As far as I can tell, what is in btrfs-progs today is undocumented, >>> untested, and only occasionally/randomly used. Therefore I don't >>> think it's useful as it stands today, and why I had started removing >>> them. >>> >>> -Eric >>> >>>> Thanks, >>>> Wang >>>>> From: >>>>> Just whitespace fixes, and magical return value removal. >>>>> >>>>> Signed-off-by: Eric Sandeen <sandeen@redhat.com> >>>>> --- >>>>> cmds-subvolume.c | 18 +++++++++--------- >>>>> 1 file changed, 9 insertions(+), 9 deletions(-) >>>>> >>>>> diff --git a/cmds-subvolume.c b/cmds-subvolume.c >>>>> index 01b982c..a9999ac 100644 >>>>> --- a/cmds-subvolume.c >>>>> +++ b/cmds-subvolume.c >>>>> @@ -104,9 +104,9 @@ static int cmd_subvol_create(int argc, char **argv) >>>>> dst = argv[optind]; >>>>> >>>>> res = test_isdir(dst); >>>>> - if(res >= 0 ){ >>>>> + if (res >= 0) { >>>>> fprintf(stderr, "ERROR: '%s' exists\n", dst); >>>>> - return 12; >>>>> + return 1; >>>>> } >>>>> >>>>> newname = strdup(dst); >>>>> @@ -114,24 +114,24 @@ static int cmd_subvol_create(int argc, char **argv) >>>>> dstdir = strdup(dst); >>>>> dstdir = dirname(dstdir); >>>>> >>>>> - if( !strcmp(newname,".") || !strcmp(newname,"..") || >>>>> + if (!strcmp(newname, ".") || !strcmp(newname, "..") || >>>>> strchr(newname, '/') ){ >>>>> fprintf(stderr, "ERROR: uncorrect subvolume name ('%s')\n", >>>>> newname); >>>>> - return 14; >>>>> + return 1; >>>>> } >>>>> >>>>> len = strlen(newname); >>>>> if (len == 0 || len >= BTRFS_VOL_NAME_MAX) { >>>>> fprintf(stderr, "ERROR: subvolume name too long ('%s)\n", >>>>> newname); >>>>> - return 14; >>>>> + return 1; >>>>> } >>>>> >>>>> fddst = open_file_or_dir(dstdir); >>>>> if (fddst < 0) { >>>>> fprintf(stderr, "ERROR: can't access to '%s'\n", dstdir); >>>>> - return 12; >>>>> + return 1; >>>>> } >>>>> >>>>> printf("Create subvolume '%s/%s'\n", dstdir, newname); >>>>> @@ -159,10 +159,10 @@ static int cmd_subvol_create(int argc, char **argv) >>>>> close(fddst); >>>>> free(inherit); >>>>> >>>>> - if(res < 0 ){ >>>>> - fprintf( stderr, "ERROR: cannot create subvolume - %s\n", >>>>> + if (res < 0) { >>>>> + fprintf(stderr, "ERROR: cannot create subvolume - %s\n", >>>>> strerror(e)); >>>>> - return 11; >>>>> + return 1; >>>>> } >>>>> >>>>> return 0; >>>>> -- >>>>> 1.7.11.7 >> -- >> 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 ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2013-09-04 1:43 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <1378213291-1034-1-git-send-email-wangshilong1991@gmail.com>
2013-09-03 13:13 ` [PATCH] btrfs-progs: tidy up cmd_subvol_create() whitespace & returns Wang Shilong
2013-09-03 15:50 ` Eric Sandeen
2013-09-04 1:20 ` Wang Shilong
2013-09-04 1:24 ` Eric Sandeen
2013-09-04 1:40 ` Wang Shilong
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).