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

[Eric Sandeen <sandeen@sandeen.net>]

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.

-Eric


> Signed-off-by: Wang Sheng-Hui <shhuiw@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"),
> 

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs