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

[Wang Sheng-Hui]

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 at foxmail.com>
>> ---
>>  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"),
>>
> 
> 
>