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@xxxxxxxxxx>
>>> ---
>>> 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@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
