Wang Shilong
2013-Sep-03 13:13 UTC
Re: [PATCH] btrfs-progs: tidy up cmd_subvol_create() whitespace & returns
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 >-- 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
Eric Sandeen
2013-Sep-03 15:50 UTC
Re: [PATCH] btrfs-progs: tidy up cmd_subvol_create() whitespace & returns
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 >> >-- 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
Wang Shilong
2013-Sep-04 01:20 UTC
Re: [PATCH] btrfs-progs: tidy up cmd_subvol_create() whitespace & returns
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 >>> >-- 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
Eric Sandeen
2013-Sep-04 01:24 UTC
Re: [PATCH] btrfs-progs: tidy up cmd_subvol_create() whitespace & returns
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
Wang Shilong
2013-Sep-04 01:40 UTC
Re: [PATCH] btrfs-progs: tidy up cmd_subvol_create() whitespace & returns
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 thinkSee 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-- 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