xfs
[Top] [All Lists]

Re: [PATCH] xfsprogs: merge the duplicated condition in xfs_mkfs.c/valid

To: Eric Sandeen <sandeen@xxxxxxxxxxx>, xfs@xxxxxxxxxxx, dchinner@xxxxxxxxxx, david@xxxxxxxxxxxxx, bfoster@xxxxxxxxxx
Subject: Re: [PATCH] xfsprogs: merge the duplicated condition in xfs_mkfs.c/validate_ag_geometry
From: Wang Sheng-Hui <shhuiw@xxxxxxxxxxx>
Date: Fri, 29 May 2015 08:05:03 +0800
Delivered-to: xfs@xxxxxxxxxxx
In-reply-to: <55672C4C.2010805@xxxxxxxxxxx>
References: <1432796808-862-1-git-send-email-shhuiw@xxxxxxxxxxx> <55672C4C.2010805@xxxxxxxxxxx>
User-agent: Mozilla/5.0 (X11; Linux i686; rv:24.0) Gecko/20100101 Thunderbird/24.2.0

On 2015å05æ28æ 22:55, Eric Sandeen wrote:
> On 5/28/15 2:06 AM, Wang Sheng-Hui wrote:
>> Some conditions are duplicated. Merge them.
> 
> While you are correct that they are duplicated, simply combining
> them doesn't mkae a lot of sense; now we'll get this sort of
> error message:
> 
>> agsize (%lld blocks) too small, need at least %lld blocks
>> too many allocation groups for size = %lld
>> need at most %lld allocation groups
> 
> and what should a user do with that information?  It's somewhat
> nonsensical.
> 
> The code as it is today, and as it has been since commit
> 1f1b8be7926480046ead7b98c9850ace7bcd82a3, doesn't make much sense,
> because we can never hit the second conditional.  But simply
> combining them doesn't look like the right answer to me.
> 
> If you look at the above commit, what it used to do was something
> like:
> 
> -               /*
> -                * If the specified agsize is too small, or too large,
> -                * complain.
> -                */
> -               if (agsize < XFS_AG_MIN_BLOCKS(blocklog)) {
> -                       fprintf(stderr,
> -               _("agsize (%lldb) too small, need at least %lld blocks\n"),
> -                               (long long)agsize,
> -                               (long long)XFS_AG_MIN_BLOCKS(blocklog));
> -                       usage();
> -               }
> 
> <snip>
> 
> -       /*
> -        * If the ag size is too small, complain if agcount/agsize was
> -        * specified, and fix it otherwise.
> -        */
> -       if (agsize < XFS_AG_MIN_BLOCKS(blocklog)) {
> -               if (daflag || dasize) {
> -                       fprintf(stderr,
> -                       _("too many allocation groups for size = %lld\n"),
> -                               (long long)agsize);
> -                       fprintf(stderr,
> -                               _("need at most %lld allocation groups\n"),
> -                               (long long)
> -                               (dblocks / XFS_AG_MIN_BLOCKS(blocklog) +
> -                               (dblocks % XFS_AG_MIN_BLOCKS(blocklog) != 
> 0)));
> 
> ...
> 
> I *think* the intent was to use different messages based on what
> was specified by the user; i.e. if agsize was specified, and is too
> small, then say:
> 
>> agsize (%lld blocks) too small, need at least %lld blocks
> 
> but if agcount was specified, and was too large, then say:
> 
>> too many allocation groups for size = %lld
>> need at most %lld allocation groups
> 
> but combining those two messages doesn't make sense to me.
> 

Got it, Eric.

Regards,
Sheng-Hui

> -Eric
> 
> 
>> Signed-off-by: Wang Sheng-Hui <shhuiw@xxxxxxxxxxx>
>> ---
>>  mkfs/xfs_mkfs.c | 24 ++++++++----------------
>>  1 file changed, 8 insertions(+), 16 deletions(-)
>>
>> diff --git a/mkfs/xfs_mkfs.c b/mkfs/xfs_mkfs.c
>> index e2a052d..7ec2556 100644
>> --- a/mkfs/xfs_mkfs.c
>> +++ b/mkfs/xfs_mkfs.c
>> @@ -740,22 +740,6 @@ validate_ag_geometry(
>>      __uint64_t      agsize,
>>      __uint64_t      agcount)
>>  {
>> -    if (agsize < XFS_AG_MIN_BLOCKS(blocklog)) {
>> -            fprintf(stderr,
>> -    _("agsize (%lld blocks) too small, need at least %lld blocks\n"),
>> -                    (long long)agsize,
>> -                    (long long)XFS_AG_MIN_BLOCKS(blocklog));
>> -            usage();
>> -    }
>> -
>> -    if (agsize > XFS_AG_MAX_BLOCKS(blocklog)) {
>> -            fprintf(stderr,
>> -    _("agsize (%lld blocks) too big, maximum is %lld blocks\n"),
>> -                    (long long)agsize,
>> -                    (long long)XFS_AG_MAX_BLOCKS(blocklog));
>> -            usage();
>> -    }
>> -
>>      if (agsize > dblocks) {
>>              fprintf(stderr,
>>      _("agsize (%lld blocks) too big, data area is %lld blocks\n"),
>> @@ -765,6 +749,10 @@ validate_ag_geometry(
>>  
>>      if (agsize < XFS_AG_MIN_BLOCKS(blocklog)) {
>>              fprintf(stderr,
>> +    _("agsize (%lld blocks) too small, need at least %lld blocks\n"),
>> +                    (long long)agsize,
>> +                    (long long)XFS_AG_MIN_BLOCKS(blocklog));
>> +            fprintf(stderr,
>>      _("too many allocation groups for size = %lld\n"),
>>                              (long long)agsize);
>>              fprintf(stderr, _("need at most %lld allocation groups\n"),
>> @@ -775,6 +763,10 @@ validate_ag_geometry(
>>  
>>      if (agsize > XFS_AG_MAX_BLOCKS(blocklog)) {
>>              fprintf(stderr,
>> +    _("agsize (%lld blocks) too big, maximum is %lld blocks\n"),
>> +                    (long long)agsize,
>> +                    (long long)XFS_AG_MAX_BLOCKS(blocklog));
>> +            fprintf(stderr,
>>      _("too few allocation groups for size = %lld\n"), (long long)agsize);
>>              fprintf(stderr,
>>      _("need at least %lld allocation groups\n"),
>>
> 
> 
> 


<Prev in Thread] Current Thread [Next in Thread>