Message ID | 1470773576-18604-1-git-send-email-andreas.dilger@intel.com |
---|---|
State | Rejected, archived |
Headers | show |
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
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 -- To unsubscribe from this list: send the line "unsubscribe linux-ext4" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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;
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(-)