* [PATCH] xfsprogs: merge the duplicated condition in xfs_mkfs.c/validate_ag_geometry @ 2015-05-28 7:06 Wang Sheng-Hui 2015-05-28 14:55 ` Eric Sandeen 0 siblings, 1 reply; 3+ messages in thread From: Wang Sheng-Hui @ 2015-05-28 7:06 UTC (permalink / raw) To: xfs, dchinner, david, bfoster Some conditions are duplicated. Merge them. 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"), -- 1.8.3.2 _______________________________________________ xfs mailing list xfs@oss.sgi.com http://oss.sgi.com/mailman/listinfo/xfs ^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH] xfsprogs: merge the duplicated condition in xfs_mkfs.c/validate_ag_geometry 2015-05-28 7:06 [PATCH] xfsprogs: merge the duplicated condition in xfs_mkfs.c/validate_ag_geometry Wang Sheng-Hui @ 2015-05-28 14:55 ` Eric Sandeen 2015-05-29 0:05 ` Wang Sheng-Hui 0 siblings, 1 reply; 3+ messages in thread From: Eric Sandeen @ 2015-05-28 14:55 UTC (permalink / raw) To: Wang Sheng-Hui, xfs, dchinner, david, bfoster 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 ^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH] xfsprogs: merge the duplicated condition in xfs_mkfs.c/validate_ag_geometry 2015-05-28 14:55 ` Eric Sandeen @ 2015-05-29 0:05 ` Wang Sheng-Hui 0 siblings, 0 replies; 3+ messages in thread From: Wang Sheng-Hui @ 2015-05-29 0:05 UTC (permalink / raw) To: Eric Sandeen, xfs, dchinner, david, bfoster 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@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 ^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2015-05-29 0:05 UTC | newest] Thread overview: 3+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2015-05-28 7:06 [PATCH] xfsprogs: merge the duplicated condition in xfs_mkfs.c/validate_ag_geometry Wang Sheng-Hui 2015-05-28 14:55 ` Eric Sandeen 2015-05-29 0:05 ` Wang Sheng-Hui
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox