* [PATCH 0/3] xfsprogs: misc coverity fixes
@ 2018-03-27 22:04 Eric Sandeen
2018-03-27 22:08 ` [PATCH 1/3] xfs_scrub: synchronize error levels & logging Eric Sandeen
` (2 more replies)
0 siblings, 3 replies; 13+ messages in thread
From: Eric Sandeen @ 2018-03-27 22:04 UTC (permalink / raw)
To: linux-xfs
<EOM>
^ permalink raw reply [flat|nested] 13+ messages in thread* [PATCH 1/3] xfs_scrub: synchronize error levels & logging 2018-03-27 22:04 [PATCH 0/3] xfsprogs: misc coverity fixes Eric Sandeen @ 2018-03-27 22:08 ` Eric Sandeen 2018-03-27 22:14 ` Darrick J. Wong 2018-03-27 22:27 ` [PATCH 1/3 V2] " Eric Sandeen 2018-03-27 22:11 ` [PATCH 2/3] xfs_scrub: initialize movon in Eric Sandeen 2018-03-27 22:14 ` [PATCH 3/3] libfrog: enforce dir XOR blkdev in __fs_table_lookup_mount Eric Sandeen 2 siblings, 2 replies; 13+ messages in thread From: Eric Sandeen @ 2018-03-27 22:08 UTC (permalink / raw) To: Eric Sandeen, linux-xfs Having only a subset of the five error_levels present in the log_level[] array is asking for trouble when someone tries to __str_log(S_PREEN ...) and overruns the array. Tie it all together in a single structure that's initialized together to make the mapping more obvious and idiot-proof. Fixes-coverity-id: 1433618 Signed-off-by: Eric Sandeen <sandeen@redhat.com> --- diff --git a/scrub/common.c b/scrub/common.c index 5a37a98..02c6c9d 100644 --- a/scrub/common.c +++ b/scrub/common.c @@ -56,18 +56,15 @@ xfs_scrub_excessive_errors( return ret; } -static const char *err_str[] = { - [S_ERROR] = "Error", - [S_WARN] = "Warning", - [S_REPAIR] = "Repaired", - [S_INFO] = "Info", - [S_PREEN] = "Optimized", -}; - -static int log_level[] = { - [S_ERROR] = LOG_ERR, - [S_WARN] = LOG_WARNING, - [S_INFO] = LOG_INFO, +static struct { + const char *string; + int loglevel; +} err_levels[] = { + [S_ERROR] = { .string = "Error", .loglevel = LOG_ERR }, + [S_WARN] = { .string = "Warning", .loglevel = LOG_WARNING }, + [S_REPAIR] = { .string = "Repaired", .loglevel = LOG_WARNING }, + [S_INFO] = { .string = "Info", .loglevel = LOG_INFO }, + [S_PREEN] = { .string = "Optimized", .loglevel = LOG_INFO } }; /* If stream is a tty, clear to end of line to clean up progress bar. */ @@ -106,8 +103,8 @@ __str_out( if (level == S_PREEN && !debug && !verbose) goto out_record; - fprintf(stream, "%s%s: %s: ", stream_start(stream), _(err_str[level]), - descr); + fprintf(stream, "%s%s: %s: ", stream_start(stream), + err_levels[level].string, descr); if (error) { fprintf(stream, _("%s."), strerror_r(error, buf, DESCR_BUFSZ)); } else { @@ -152,10 +149,6 @@ __str_log( char buf[LOG_BUFSZ]; int sz; - /* We only want to hear about optimizing when in debug/verbose mode. */ - if (level == S_PREEN && !debug && !verbose) - return; - /* * Skip logging if we're being run as a service (presumably the * service will log stdout/stderr); if we're being run in a non @@ -168,11 +161,11 @@ __str_log( snprintf(logname, LOGNAME_BUFSZ, "%s@%s", progname, ctx->mntpoint); openlog(logname, LOG_PID, LOG_DAEMON); - sz = snprintf(buf, LOG_BUFSZ, "%s: ", _(err_str[level])); + sz = snprintf(buf, LOG_BUFSZ, "%s: ", _(err_levels[level].string)); va_start(args, format); vsnprintf(buf + sz, LOG_BUFSZ - sz, format, args); va_end(args); - syslog(log_level[level], "%s", buf); + syslog(err_levels[level].loglevel, "%s", buf); closelog(); } ^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH 1/3] xfs_scrub: synchronize error levels & logging 2018-03-27 22:08 ` [PATCH 1/3] xfs_scrub: synchronize error levels & logging Eric Sandeen @ 2018-03-27 22:14 ` Darrick J. Wong 2018-03-27 22:20 ` Eric Sandeen 2018-03-27 22:27 ` [PATCH 1/3 V2] " Eric Sandeen 1 sibling, 1 reply; 13+ messages in thread From: Darrick J. Wong @ 2018-03-27 22:14 UTC (permalink / raw) To: Eric Sandeen; +Cc: linux-xfs On Tue, Mar 27, 2018 at 05:08:54PM -0500, Eric Sandeen wrote: > Having only a subset of the five error_levels present in > the log_level[] array is asking for trouble when someone > tries to __str_log(S_PREEN ...) and overruns the array. > > Tie it all together in a single structure that's > initialized together to make the mapping more obvious and > idiot-proof. > > Fixes-coverity-id: 1433618 > Signed-off-by: Eric Sandeen <sandeen@redhat.com> > --- > > diff --git a/scrub/common.c b/scrub/common.c > index 5a37a98..02c6c9d 100644 > --- a/scrub/common.c > +++ b/scrub/common.c > @@ -56,18 +56,15 @@ xfs_scrub_excessive_errors( > return ret; > } > > -static const char *err_str[] = { > - [S_ERROR] = "Error", > - [S_WARN] = "Warning", > - [S_REPAIR] = "Repaired", > - [S_INFO] = "Info", > - [S_PREEN] = "Optimized", > -}; > - > -static int log_level[] = { > - [S_ERROR] = LOG_ERR, > - [S_WARN] = LOG_WARNING, > - [S_INFO] = LOG_INFO, > +static struct { > + const char *string; > + int loglevel; > +} err_levels[] = { > + [S_ERROR] = { .string = "Error", .loglevel = LOG_ERR }, > + [S_WARN] = { .string = "Warning", .loglevel = LOG_WARNING }, > + [S_REPAIR] = { .string = "Repaired", .loglevel = LOG_WARNING }, > + [S_INFO] = { .string = "Info", .loglevel = LOG_INFO }, > + [S_PREEN] = { .string = "Optimized", .loglevel = LOG_INFO } > }; > > /* If stream is a tty, clear to end of line to clean up progress bar. */ > @@ -106,8 +103,8 @@ __str_out( > if (level == S_PREEN && !debug && !verbose) > goto out_record; > > - fprintf(stream, "%s%s: %s: ", stream_start(stream), _(err_str[level]), > - descr); > + fprintf(stream, "%s%s: %s: ", stream_start(stream), > + err_levels[level].string, descr); _(err_levels[level]) here ^^^^^^^^^^^^ Otherwise we end up with: Error: 你吃饭了吗？你多吃一点。 > if (error) { > fprintf(stream, _("%s."), strerror_r(error, buf, DESCR_BUFSZ)); > } else { > @@ -152,10 +149,6 @@ __str_log( > char buf[LOG_BUFSZ]; > int sz; > > - /* We only want to hear about optimizing when in debug/verbose mode. */ > - if (level == S_PREEN && !debug && !verbose) > - return; Why is this being removed? --D > - > /* > * Skip logging if we're being run as a service (presumably the > * service will log stdout/stderr); if we're being run in a non > @@ -168,11 +161,11 @@ __str_log( > snprintf(logname, LOGNAME_BUFSZ, "%s@%s", progname, ctx->mntpoint); > openlog(logname, LOG_PID, LOG_DAEMON); > > - sz = snprintf(buf, LOG_BUFSZ, "%s: ", _(err_str[level])); > + sz = snprintf(buf, LOG_BUFSZ, "%s: ", _(err_levels[level].string)); > va_start(args, format); > vsnprintf(buf + sz, LOG_BUFSZ - sz, format, args); > va_end(args); > - syslog(log_level[level], "%s", buf); > + syslog(err_levels[level].loglevel, "%s", buf); > > closelog(); > } > -- > To unsubscribe from this list: send the line "unsubscribe linux-xfs" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 1/3] xfs_scrub: synchronize error levels & logging 2018-03-27 22:14 ` Darrick J. Wong @ 2018-03-27 22:20 ` Eric Sandeen 2018-03-27 22:22 ` Darrick J. Wong 0 siblings, 1 reply; 13+ messages in thread From: Eric Sandeen @ 2018-03-27 22:20 UTC (permalink / raw) To: Darrick J. Wong, Eric Sandeen; +Cc: linux-xfs On 3/27/18 5:14 PM, Darrick J. Wong wrote: > On Tue, Mar 27, 2018 at 05:08:54PM -0500, Eric Sandeen wrote: >> Having only a subset of the five error_levels present in >> the log_level[] array is asking for trouble when someone >> tries to __str_log(S_PREEN ...) and overruns the array. >> >> Tie it all together in a single structure that's >> initialized together to make the mapping more obvious and >> idiot-proof. >> >> Fixes-coverity-id: 1433618 >> Signed-off-by: Eric Sandeen <sandeen@redhat.com> >> --- >> >> diff --git a/scrub/common.c b/scrub/common.c >> index 5a37a98..02c6c9d 100644 >> --- a/scrub/common.c >> +++ b/scrub/common.c >> @@ -56,18 +56,15 @@ xfs_scrub_excessive_errors( >> return ret; >> } >> >> -static const char *err_str[] = { >> - [S_ERROR] = "Error", >> - [S_WARN] = "Warning", >> - [S_REPAIR] = "Repaired", >> - [S_INFO] = "Info", >> - [S_PREEN] = "Optimized", >> -}; >> - >> -static int log_level[] = { >> - [S_ERROR] = LOG_ERR, >> - [S_WARN] = LOG_WARNING, >> - [S_INFO] = LOG_INFO, >> +static struct { >> + const char *string; >> + int loglevel; >> +} err_levels[] = { >> + [S_ERROR] = { .string = "Error", .loglevel = LOG_ERR }, >> + [S_WARN] = { .string = "Warning", .loglevel = LOG_WARNING }, >> + [S_REPAIR] = { .string = "Repaired", .loglevel = LOG_WARNING }, >> + [S_INFO] = { .string = "Info", .loglevel = LOG_INFO }, >> + [S_PREEN] = { .string = "Optimized", .loglevel = LOG_INFO } >> }; >> >> /* If stream is a tty, clear to end of line to clean up progress bar. */ >> @@ -106,8 +103,8 @@ __str_out( >> if (level == S_PREEN && !debug && !verbose) >> goto out_record; >> >> - fprintf(stream, "%s%s: %s: ", stream_start(stream), _(err_str[level]), >> - descr); >> + fprintf(stream, "%s%s: %s: ", stream_start(stream), >> + err_levels[level].string, descr); > > _(err_levels[level]) here ^^^^^^^^^^^^ > > Otherwise we end up with: > > Error: 你吃饭了吗？你多吃一点。 oops but: _(err_levels[level].string) right ... > >> if (error) { >> fprintf(stream, _("%s."), strerror_r(error, buf, DESCR_BUFSZ)); >> } else { >> @@ -152,10 +149,6 @@ __str_log( >> char buf[LOG_BUFSZ]; >> int sz; >> >> - /* We only want to hear about optimizing when in debug/verbose mode. */ >> - if (level == S_PREEN && !debug && !verbose) >> - return; > > Why is this being removed? It seemed like a cut & paste error originally but ... it's a leftover, sorry. -Eric ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 1/3] xfs_scrub: synchronize error levels & logging 2018-03-27 22:20 ` Eric Sandeen @ 2018-03-27 22:22 ` Darrick J. Wong 0 siblings, 0 replies; 13+ messages in thread From: Darrick J. Wong @ 2018-03-27 22:22 UTC (permalink / raw) To: Eric Sandeen; +Cc: Eric Sandeen, linux-xfs On Tue, Mar 27, 2018 at 05:20:36PM -0500, Eric Sandeen wrote: > On 3/27/18 5:14 PM, Darrick J. Wong wrote: > > On Tue, Mar 27, 2018 at 05:08:54PM -0500, Eric Sandeen wrote: > >> Having only a subset of the five error_levels present in > >> the log_level[] array is asking for trouble when someone > >> tries to __str_log(S_PREEN ...) and overruns the array. > >> > >> Tie it all together in a single structure that's > >> initialized together to make the mapping more obvious and > >> idiot-proof. > >> > >> Fixes-coverity-id: 1433618 > >> Signed-off-by: Eric Sandeen <sandeen@redhat.com> > >> --- > >> > >> diff --git a/scrub/common.c b/scrub/common.c > >> index 5a37a98..02c6c9d 100644 > >> --- a/scrub/common.c > >> +++ b/scrub/common.c > >> @@ -56,18 +56,15 @@ xfs_scrub_excessive_errors( > >> return ret; > >> } > >> > >> -static const char *err_str[] = { > >> - [S_ERROR] = "Error", > >> - [S_WARN] = "Warning", > >> - [S_REPAIR] = "Repaired", > >> - [S_INFO] = "Info", > >> - [S_PREEN] = "Optimized", > >> -}; > >> - > >> -static int log_level[] = { > >> - [S_ERROR] = LOG_ERR, > >> - [S_WARN] = LOG_WARNING, > >> - [S_INFO] = LOG_INFO, > >> +static struct { > >> + const char *string; > >> + int loglevel; > >> +} err_levels[] = { > >> + [S_ERROR] = { .string = "Error", .loglevel = LOG_ERR }, > >> + [S_WARN] = { .string = "Warning", .loglevel = LOG_WARNING }, > >> + [S_REPAIR] = { .string = "Repaired", .loglevel = LOG_WARNING }, > >> + [S_INFO] = { .string = "Info", .loglevel = LOG_INFO }, > >> + [S_PREEN] = { .string = "Optimized", .loglevel = LOG_INFO } > >> }; > >> > >> /* If stream is a tty, clear to end of line to clean up progress bar. */ > >> @@ -106,8 +103,8 @@ __str_out( > >> if (level == S_PREEN && !debug && !verbose) > >> goto out_record; > >> > >> - fprintf(stream, "%s%s: %s: ", stream_start(stream), _(err_str[level]), > >> - descr); > >> + fprintf(stream, "%s%s: %s: ", stream_start(stream), > >> + err_levels[level].string, descr); > > > > _(err_levels[level]) here ^^^^^^^^^^^^ > > > > Otherwise we end up with: > > > > Error: 你吃饭了吗？你多吃一点。 > > oops > > but: _(err_levels[level].string) right ... Er, right. > > > >> if (error) { > >> fprintf(stream, _("%s."), strerror_r(error, buf, DESCR_BUFSZ)); > >> } else { > >> @@ -152,10 +149,6 @@ __str_log( > >> char buf[LOG_BUFSZ]; > >> int sz; > >> > >> - /* We only want to hear about optimizing when in debug/verbose mode. */ > >> - if (level == S_PREEN && !debug && !verbose) > >> - return; > > > > Why is this being removed? > > It seemed like a cut & paste error originally but ... it's a leftover, sorry. :) --D > -Eric > -- > To unsubscribe from this list: send the line "unsubscribe linux-xfs" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html ^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH 1/3 V2] xfs_scrub: synchronize error levels & logging 2018-03-27 22:08 ` [PATCH 1/3] xfs_scrub: synchronize error levels & logging Eric Sandeen 2018-03-27 22:14 ` Darrick J. Wong @ 2018-03-27 22:27 ` Eric Sandeen 2018-03-27 22:31 ` Darrick J. Wong 1 sibling, 1 reply; 13+ messages in thread From: Eric Sandeen @ 2018-03-27 22:27 UTC (permalink / raw) To: Eric Sandeen, linux-xfs Having only a subset of the five error_levels present in the log_level[] array is asking for trouble when someone tries to __str_log(S_PREEN ...) and overruns the array. Tie it all together in a single structure that's initialized together to make the mapping more obvious and idiot-proof. Fixes-coverity-id: 1433618 Signed-off-by: Eric Sandeen <sandeen@redhat.com> --- V2: i8n, keep test for PREEN in __str_log diff --git a/scrub/common.c b/scrub/common.c index 5a37a98..02c6c9d 100644 --- a/scrub/common.c +++ b/scrub/common.c @@ -56,18 +56,15 @@ xfs_scrub_excessive_errors( return ret; } -static const char *err_str[] = { - [S_ERROR] = "Error", - [S_WARN] = "Warning", - [S_REPAIR] = "Repaired", - [S_INFO] = "Info", - [S_PREEN] = "Optimized", -}; - -static int log_level[] = { - [S_ERROR] = LOG_ERR, - [S_WARN] = LOG_WARNING, - [S_INFO] = LOG_INFO, +static struct { + const char *string; + int loglevel; +} err_levels[] = { + [S_ERROR] = { .string = "Error", .loglevel = LOG_ERR }, + [S_WARN] = { .string = "Warning", .loglevel = LOG_WARNING }, + [S_REPAIR] = { .string = "Repaired", .loglevel = LOG_WARNING }, + [S_INFO] = { .string = "Info", .loglevel = LOG_INFO }, + [S_PREEN] = { .string = "Optimized", .loglevel = LOG_INFO } }; /* If stream is a tty, clear to end of line to clean up progress bar. */ @@ -106,8 +103,8 @@ __str_out( if (level == S_PREEN && !debug && !verbose) goto out_record; - fprintf(stream, "%s%s: %s: ", stream_start(stream), _(err_str[level]), - descr); + fprintf(stream, "%s%s: %s: ", stream_start(stream), + _(err_levels[level].string), descr); if (error) { fprintf(stream, _("%s."), strerror_r(error, buf, DESCR_BUFSZ)); } else { @@ -168,11 +161,11 @@ __str_log( snprintf(logname, LOGNAME_BUFSZ, "%s@%s", progname, ctx->mntpoint); openlog(logname, LOG_PID, LOG_DAEMON); - sz = snprintf(buf, LOG_BUFSZ, "%s: ", _(err_str[level])); + sz = snprintf(buf, LOG_BUFSZ, "%s: ", _(err_levels[level].string)); va_start(args, format); vsnprintf(buf + sz, LOG_BUFSZ - sz, format, args); va_end(args); - syslog(log_level[level], "%s", buf); + syslog(err_levels[level].loglevel, "%s", buf); closelog(); } ^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH 1/3 V2] xfs_scrub: synchronize error levels & logging 2018-03-27 22:27 ` [PATCH 1/3 V2] " Eric Sandeen @ 2018-03-27 22:31 ` Darrick J. Wong 0 siblings, 0 replies; 13+ messages in thread From: Darrick J. Wong @ 2018-03-27 22:31 UTC (permalink / raw) To: Eric Sandeen; +Cc: Eric Sandeen, linux-xfs On Tue, Mar 27, 2018 at 05:27:04PM -0500, Eric Sandeen wrote: > Having only a subset of the five error_levels present in > the log_level[] array is asking for trouble when someone > tries to __str_log(S_PREEN ...) and overruns the array. > > Tie it all together in a single structure that's > initialized together to make the mapping more obvious and > idiot-proof. > > Fixes-coverity-id: 1433618 > Signed-off-by: Eric Sandeen <sandeen@redhat.com> Looks ok, Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com> --D > --- > > V2: i8n, keep test for PREEN in __str_log > > diff --git a/scrub/common.c b/scrub/common.c > index 5a37a98..02c6c9d 100644 > --- a/scrub/common.c > +++ b/scrub/common.c > @@ -56,18 +56,15 @@ xfs_scrub_excessive_errors( > return ret; > } > > -static const char *err_str[] = { > - [S_ERROR] = "Error", > - [S_WARN] = "Warning", > - [S_REPAIR] = "Repaired", > - [S_INFO] = "Info", > - [S_PREEN] = "Optimized", > -}; > - > -static int log_level[] = { > - [S_ERROR] = LOG_ERR, > - [S_WARN] = LOG_WARNING, > - [S_INFO] = LOG_INFO, > +static struct { > + const char *string; > + int loglevel; > +} err_levels[] = { > + [S_ERROR] = { .string = "Error", .loglevel = LOG_ERR }, > + [S_WARN] = { .string = "Warning", .loglevel = LOG_WARNING }, > + [S_REPAIR] = { .string = "Repaired", .loglevel = LOG_WARNING }, > + [S_INFO] = { .string = "Info", .loglevel = LOG_INFO }, > + [S_PREEN] = { .string = "Optimized", .loglevel = LOG_INFO } > }; > > /* If stream is a tty, clear to end of line to clean up progress bar. */ > @@ -106,8 +103,8 @@ __str_out( > if (level == S_PREEN && !debug && !verbose) > goto out_record; > > - fprintf(stream, "%s%s: %s: ", stream_start(stream), _(err_str[level]), > - descr); > + fprintf(stream, "%s%s: %s: ", stream_start(stream), > + _(err_levels[level].string), descr); > if (error) { > fprintf(stream, _("%s."), strerror_r(error, buf, DESCR_BUFSZ)); > } else { > @@ -168,11 +161,11 @@ __str_log( > snprintf(logname, LOGNAME_BUFSZ, "%s@%s", progname, ctx->mntpoint); > openlog(logname, LOG_PID, LOG_DAEMON); > > - sz = snprintf(buf, LOG_BUFSZ, "%s: ", _(err_str[level])); > + sz = snprintf(buf, LOG_BUFSZ, "%s: ", _(err_levels[level].string)); > va_start(args, format); > vsnprintf(buf + sz, LOG_BUFSZ - sz, format, args); > va_end(args); > - syslog(log_level[level], "%s", buf); > + syslog(err_levels[level].loglevel, "%s", buf); > > closelog(); > } > -- > To unsubscribe from this list: send the line "unsubscribe linux-xfs" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html ^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH 2/3] xfs_scrub: initialize movon in 2018-03-27 22:04 [PATCH 0/3] xfsprogs: misc coverity fixes Eric Sandeen 2018-03-27 22:08 ` [PATCH 1/3] xfs_scrub: synchronize error levels & logging Eric Sandeen @ 2018-03-27 22:11 ` Eric Sandeen 2018-03-27 22:14 ` Darrick J. Wong 2018-03-27 22:14 ` [PATCH 3/3] libfrog: enforce dir XOR blkdev in __fs_table_lookup_mount Eric Sandeen 2 siblings, 1 reply; 13+ messages in thread From: Eric Sandeen @ 2018-03-27 22:11 UTC (permalink / raw) To: Eric Sandeen, linux-xfs Given the logic in xfs_scrub_connections, it's possible to fail all 3 tests and wind up checking an uninitialized moveon variable at the end. Start out with "true" to avoid this and move on even if all the conditions in the function are false. Fixes-coverity-id: 1433617 Signed-off-by: Eric Sandeen <sandeen@redhat.com> --- diff --git a/scrub/phase5.c b/scrub/phase5.c index 8e0a1be..5f2a1a7 100644 --- a/scrub/phase5.c +++ b/scrub/phase5.c @@ -244,7 +244,7 @@ xfs_scrub_connections( { bool *pmoveon = arg; char descr[DESCR_BUFSZ]; - bool moveon; + bool moveon = true; xfs_agnumber_t agno; xfs_agino_t agino; int fd = -1; ^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH 2/3] xfs_scrub: initialize movon in 2018-03-27 22:11 ` [PATCH 2/3] xfs_scrub: initialize movon in Eric Sandeen @ 2018-03-27 22:14 ` Darrick J. Wong 0 siblings, 0 replies; 13+ messages in thread From: Darrick J. Wong @ 2018-03-27 22:14 UTC (permalink / raw) To: Eric Sandeen; +Cc: linux-xfs On Tue, Mar 27, 2018 at 05:11:49PM -0500, Eric Sandeen wrote: > Given the logic in xfs_scrub_connections, it's possible to > fail all 3 tests and wind up checking an uninitialized moveon > variable at the end. Start out with "true" to avoid this and > move on even if all the conditions in the function are false. > > Fixes-coverity-id: 1433617 > Signed-off-by: Eric Sandeen <sandeen@redhat.com> Subject line needs typo fix, but otherwise: Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com> --D > --- > > diff --git a/scrub/phase5.c b/scrub/phase5.c > index 8e0a1be..5f2a1a7 100644 > --- a/scrub/phase5.c > +++ b/scrub/phase5.c > @@ -244,7 +244,7 @@ xfs_scrub_connections( > { > bool *pmoveon = arg; > char descr[DESCR_BUFSZ]; > - bool moveon; > + bool moveon = true; > xfs_agnumber_t agno; > xfs_agino_t agino; > int fd = -1; > > -- > To unsubscribe from this list: send the line "unsubscribe linux-xfs" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html ^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH 3/3] libfrog: enforce dir XOR blkdev in __fs_table_lookup_mount 2018-03-27 22:04 [PATCH 0/3] xfsprogs: misc coverity fixes Eric Sandeen 2018-03-27 22:08 ` [PATCH 1/3] xfs_scrub: synchronize error levels & logging Eric Sandeen 2018-03-27 22:11 ` [PATCH 2/3] xfs_scrub: initialize movon in Eric Sandeen @ 2018-03-27 22:14 ` Eric Sandeen 2018-03-27 22:19 ` Darrick J. Wong 2018-03-27 22:33 ` [PATCH 3/3 V2] libfrog: handle NULL dir && " Eric Sandeen 2 siblings, 2 replies; 13+ messages in thread From: Eric Sandeen @ 2018-03-27 22:14 UTC (permalink / raw) To: Eric Sandeen, linux-xfs It's not valid to call fs_table_lookup_mount with both dir and blkdev set, but one of them must be supplied. Doing otherwise is a programming error, so catch it with an ASSERT. Fixes-coverity-id: 1433615 Fixes-coverity-id: 1433616 Signed-off-by: Eric Sandeen <sandeen@redhat.com> --- diff --git a/libfrog/paths.c b/libfrog/paths.c index 318b48f..55cb284 100644 --- a/libfrog/paths.c +++ b/libfrog/paths.c @@ -98,6 +98,10 @@ __fs_table_lookup_mount( char rpath[PATH_MAX]; char dpath[PATH_MAX]; + /* There can be only one! */ + ASSERT(dir || blkdev); + ASSERT(!(dir && blkdev)); + if (dir && !realpath(dir, dpath)) return NULL; if (blkdev && !realpath(blkdev, dpath)) ^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH 3/3] libfrog: enforce dir XOR blkdev in __fs_table_lookup_mount 2018-03-27 22:14 ` [PATCH 3/3] libfrog: enforce dir XOR blkdev in __fs_table_lookup_mount Eric Sandeen @ 2018-03-27 22:19 ` Darrick J. Wong 2018-03-27 22:33 ` [PATCH 3/3 V2] libfrog: handle NULL dir && " Eric Sandeen 1 sibling, 0 replies; 13+ messages in thread From: Darrick J. Wong @ 2018-03-27 22:19 UTC (permalink / raw) To: Eric Sandeen; +Cc: linux-xfs On Tue, Mar 27, 2018 at 05:14:51PM -0500, Eric Sandeen wrote: > It's not valid to call fs_table_lookup_mount with both > dir and blkdev set, but one of them must be supplied. > Doing otherwise is a programming error, so catch it > with an ASSERT. > > Fixes-coverity-id: 1433615 > Fixes-coverity-id: 1433616 > Signed-off-by: Eric Sandeen <sandeen@redhat.com> Hmmm.... what /is/ the intended behavior if some jerk does fs_table_lookup_mount(NULL) and both arguments are NULL? AFAICT the non-static wrappers will prevent the case where both parameters are specified. How about: if (!dir && !blkdev) { ASSERT(0); return NULL; } just after the variable declarations? --D > --- > > diff --git a/libfrog/paths.c b/libfrog/paths.c > index 318b48f..55cb284 100644 > --- a/libfrog/paths.c > +++ b/libfrog/paths.c > @@ -98,6 +98,10 @@ __fs_table_lookup_mount( > char rpath[PATH_MAX]; > char dpath[PATH_MAX]; > > + /* There can be only one! */ > + ASSERT(dir || blkdev); > + ASSERT(!(dir && blkdev)); > + > if (dir && !realpath(dir, dpath)) > return NULL; > if (blkdev && !realpath(blkdev, dpath)) > > -- > To unsubscribe from this list: send the line "unsubscribe linux-xfs" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html ^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH 3/3 V2] libfrog: handle NULL dir && blkdev in __fs_table_lookup_mount 2018-03-27 22:14 ` [PATCH 3/3] libfrog: enforce dir XOR blkdev in __fs_table_lookup_mount Eric Sandeen 2018-03-27 22:19 ` Darrick J. Wong @ 2018-03-27 22:33 ` Eric Sandeen 2018-03-27 22:37 ` Darrick J. Wong 1 sibling, 1 reply; 13+ messages in thread From: Eric Sandeen @ 2018-03-27 22:33 UTC (permalink / raw) To: Eric Sandeen, linux-xfs If neither dir nor blkdev is set, dpath never gets set, and then gets used (uninitalized) later on. If we are asked where "nothing" is mounted, just return "nowhere." Fixes-coverity-id: 1433615 Fixes-coverity-id: 1433616 Signed-off-by: Eric Sandeen <sandeen@redhat.com> --- V2: wrappers prevent (sic) both being set, gracefully handle either of the sent-in paths being null. diff --git a/libfrog/paths.c b/libfrog/paths.c index 318b48f..c7895e9 100644 --- a/libfrog/paths.c +++ b/libfrog/paths.c @@ -98,6 +98,9 @@ __fs_table_lookup_mount( char rpath[PATH_MAX]; char dpath[PATH_MAX]; + if (!dir && !blkdev) + return NULL; + if (dir && !realpath(dir, dpath)) return NULL; if (blkdev && !realpath(blkdev, dpath)) ^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH 3/3 V2] libfrog: handle NULL dir && blkdev in __fs_table_lookup_mount 2018-03-27 22:33 ` [PATCH 3/3 V2] libfrog: handle NULL dir && " Eric Sandeen @ 2018-03-27 22:37 ` Darrick J. Wong 0 siblings, 0 replies; 13+ messages in thread From: Darrick J. Wong @ 2018-03-27 22:37 UTC (permalink / raw) To: Eric Sandeen; +Cc: Eric Sandeen, linux-xfs On Tue, Mar 27, 2018 at 05:33:11PM -0500, Eric Sandeen wrote: > If neither dir nor blkdev is set, dpath never gets set, > and then gets used (uninitalized) later on. > > If we are asked where "nothing" is mounted, just return > "nowhere." > > Fixes-coverity-id: 1433615 > Fixes-coverity-id: 1433616 > Signed-off-by: Eric Sandeen <sandeen@redhat.com> mmmmmm mmmm m m mmm m m mm mmmmm mmmmm m m #" m" "m ## ## m" " # # ## # "# # "# "m m" m# # # # ## # # mm #mmmm# # # #mmm#" #mmm#" "#" m" # # # "" # # # # # #mm# # # # ##mmmm #mm# # # "mmm" # # # # # # # Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com> --D > --- > > V2: wrappers prevent (sic) both being set, gracefully > handle either of the sent-in paths being null. > > diff --git a/libfrog/paths.c b/libfrog/paths.c > index 318b48f..c7895e9 100644 > --- a/libfrog/paths.c > +++ b/libfrog/paths.c > @@ -98,6 +98,9 @@ __fs_table_lookup_mount( > char rpath[PATH_MAX]; > char dpath[PATH_MAX]; > > + if (!dir && !blkdev) > + return NULL; > + > if (dir && !realpath(dir, dpath)) > return NULL; > if (blkdev && !realpath(blkdev, dpath)) > > -- > To unsubscribe from this list: send the line "unsubscribe linux-xfs" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html ^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2018-03-27 22:37 UTC | newest] Thread overview: 13+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2018-03-27 22:04 [PATCH 0/3] xfsprogs: misc coverity fixes Eric Sandeen 2018-03-27 22:08 ` [PATCH 1/3] xfs_scrub: synchronize error levels & logging Eric Sandeen 2018-03-27 22:14 ` Darrick J. Wong 2018-03-27 22:20 ` Eric Sandeen 2018-03-27 22:22 ` Darrick J. Wong 2018-03-27 22:27 ` [PATCH 1/3 V2] " Eric Sandeen 2018-03-27 22:31 ` Darrick J. Wong 2018-03-27 22:11 ` [PATCH 2/3] xfs_scrub: initialize movon in Eric Sandeen 2018-03-27 22:14 ` Darrick J. Wong 2018-03-27 22:14 ` [PATCH 3/3] libfrog: enforce dir XOR blkdev in __fs_table_lookup_mount Eric Sandeen 2018-03-27 22:19 ` Darrick J. Wong 2018-03-27 22:33 ` [PATCH 3/3 V2] libfrog: handle NULL dir && " Eric Sandeen 2018-03-27 22:37 ` Darrick J. Wong
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).