* [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
* [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 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 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
* 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 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 an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.