[PATCH] fsck: fix strange logic

[PATCH] fsck: fix strange logic

[Andreas Dilger]

llvm warns about the confusingly written comparison:

                              !strncmp(argv[i+1], "-", 1) == 0) {
    misc/fsck.c:1178 col 9: warning: logical not is only applied to
      the left hand side of comparison [-Wlogical-not-parentheses]
    misc/fsck.c:1178 col 9: note: add parentheses after the '!' to
      evaluate the comparison first
    misc/fsck.c:1178 col 9: note: add parentheses around left hand
      side expression to silence this warning

It makes sense to simplify this to a character comparison
rather than using strncmp() to check only one character.

Signed-off-by: Andreas Dilger <andreas.dilger@intel.com>
---
 misc/fsck.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/misc/fsck.c b/misc/fsck.c
index 826aaeb..4f918b7 100644
--- a/misc/fsck.c
+++ b/misc/fsck.c
@@ -1174,8 +1174,8 @@ static void PRS(int argc, char *argv[])
 						progress_fd = 0;
 					else
 						goto next_arg;
-				} else if ((i+1) < argc &&
-					   !strncmp(argv[i+1], "-", 1) == 0) {
+				} else if (argc > i + 1 &&
+					   argv[i + 1][0] == '-') {
 					progress_fd = string_to_int(argv[i]);
 					if (progress_fd < 0)
 						progress_fd = 0;
-- 
2.4.5

Re: [PATCH] fsck: fix strange logic

[Andreas Dilger]

[-- Attachment #1: Type: text/plain, Size: 1795 bytes --]

On Aug 9, 2016, at 2:12 PM, Andreas Dilger <andreas.dilger@intel.com> wrote:
> 
> llvm warns about the confusingly written comparison:
> 
>                              !strncmp(argv[i+1], "-", 1) == 0) {
>    misc/fsck.c:1178 col 9: warning: logical not is only applied to
>      the left hand side of comparison [-Wlogical-not-parentheses]
>    misc/fsck.c:1178 col 9: note: add parentheses after the '!' to
>      evaluate the comparison first
>    misc/fsck.c:1178 col 9: note: add parentheses around left hand
>      side expression to silence this warning
> 
> It makes sense to simplify this to a character comparison
> rather than using strncmp() to check only one character.
> 
> Signed-off-by: Andreas Dilger <andreas.dilger@intel.com>
> ---
> misc/fsck.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/misc/fsck.c b/misc/fsck.c
> index 826aaeb..4f918b7 100644
> --- a/misc/fsck.c
> +++ b/misc/fsck.c
> @@ -1174,8 +1174,8 @@ static void PRS(int argc, char *argv[])
> 						progress_fd = 0;
> 					else
> 						goto next_arg;
> -				} else if ((i+1) < argc &&
> -					   !strncmp(argv[i+1], "-", 1) == 0) {
> +				} else if (argc > i + 1 &&
> +					   argv[i + 1][0] == '-') {
> 					progress_fd = string_to_int(argv[i]);
> 					if (progress_fd < 0)
> 						progress_fd = 0;

Note that it isn't clear whether the original logic also contained a bug,
with both "!strncmp()" and the comparison with "== 0".  At first glance
it appeared that this was a bug to both negate and compare with 0, but
in further review I think that this should _not_ parse negative numbers
and use "-" as the fd.  Unfortunately, it isn't documented what "-" means.

I'll push a v2 patch that keeps the original logic, and Ted can choose
which one is correct.

Cheers, Andreas






[-- Attachment #2: Message signed with OpenPGP using GPGMail --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCHv2] fsck: fix strange logic

[Andreas Dilger]

llvm warns about the confusingly written comparison:

                              !strncmp(argv[i+1], "-", 1) == 0) {
    misc/fsck.c:1178 col 9: warning: logical not is only applied to
      the left hand side of comparison [-Wlogical-not-parentheses]
    misc/fsck.c:1178 col 9: note: add parentheses after the '!' to
      evaluate the comparison first
    misc/fsck.c:1178 col 9: note: add parentheses around left hand
      side expression to silence this warning

It makes sense to simplify this to a character comparison rather
than using strncmp() to check only one character.

Signed-off-by: Andreas Dilger <andreas.dilger@intel.com>
---
v1->v2: revert to original logic skipping fd '-'

 misc/fsck.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/misc/fsck.c b/misc/fsck.c
index 826aaeb..67e158a 100644
--- a/misc/fsck.c
+++ b/misc/fsck.c
@@ -1174,8 +1174,8 @@ static void PRS(int argc, char *argv[])
 						progress_fd = 0;
 					else
 						goto next_arg;
-				} else if ((i+1) < argc &&
-					   !strncmp(argv[i+1], "-", 1) == 0) {
+				} else if (argc > i + 1 &&
+					   argv[i + 1][0] != '-') {
 					progress_fd = string_to_int(argv[i]);
 					if (progress_fd < 0)
 						progress_fd = 0;
-- 
2.4.5

Re: [PATCH] fsck: fix strange logic

[Theodore Ts'o]

On Tue, Aug 09, 2016 at 02:26:21PM -0600, Andreas Dilger wrote:
> I'll push a v2 patch that keeps the original logic, and Ted can choose
> which one is correct.
>

The original logic is what's intended.  What's going on here is that
the argument to -C is optional (for backwards compatibility reasons),
and we know it's always going to be a positive integer, since it's a
file descriptor.  If the next argument begins with a '-', it must be
an option specifier, and the argument to -C was missing.  If the next
argument does not begin with a '-', then we try to parse it as an
integer, and we rely on the fact that in general the device specifier
to fsck generally begins with a '/', and if not, isn't going to be
parseable as a number.

						- Ted

Re: [PATCHv2] fsck: fix strange logic

[Theodore Ts'o]

On Tue, Aug 09, 2016 at 02:27:09PM -0600, Andreas Dilger wrote:
> llvm warns about the confusingly written comparison:
> 
>                               !strncmp(argv[i+1], "-", 1) == 0) {
>     misc/fsck.c:1178 col 9: warning: logical not is only applied to
>       the left hand side of comparison [-Wlogical-not-parentheses]
>     misc/fsck.c:1178 col 9: note: add parentheses after the '!' to
>       evaluate the comparison first
>     misc/fsck.c:1178 col 9: note: add parentheses around left hand
>       side expression to silence this warning
> 
> It makes sense to simplify this to a character comparison rather
> than using strncmp() to check only one character.
> 
> Signed-off-by: Andreas Dilger <andreas.dilger@intel.com>

Thanks, applied.

						- Ted