* [PATCH] xfs_repair: don't use do_warn for normal log message
@ 2017-09-13 10:49 Masatake YAMATO
2017-09-13 21:48 ` Dave Chinner
0 siblings, 1 reply; 3+ messages in thread
From: Masatake YAMATO @ 2017-09-13 10:49 UTC (permalink / raw)
To: linux-xfs; +Cc: yamato
In some case, exit statuses of xfs_repair -n are different even for the
same file system when -v is specified or not. This patch fixes this
behavior.
If -v is specified, do_warn() is used in zero_log() for printing
a normal message. That makes the exit status to 1 though there
is no dirtiness in the file system.
The original zero_log():
error = xlog_find_tail(log, &head_blk, &tail_blk);
if (error) {...
} else {
if (verbose) {
do_warn(
_("zero_log: head block %" PRId64 " tail block %" PRId64 "\n"),
head_blk, tail_blkn);
}
do_warn() is used for the message, "zero_log:...". do_log
should be used instead because this log message is for just reporting the
values of head_blk, and tail_blk. do_log is for "just reporting".
Using do_warn unintentionally has an impact.
It causes the exit status of the command.
do_warn sets fs_is_dirty global variable.
That refers in main function to decide the exit status.
void
do_warn(char const *msg, ...)
{
va_list args;
fs_is_dirty = 1;
int
main(int argc, char **argv)
{
...
if (fs_is_dirty)
return(1);
---
repair/phase2.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/repair/phase2.c b/repair/phase2.c
index c21778b8..f7ea655f 100644
--- a/repair/phase2.c
+++ b/repair/phase2.c
@@ -75,7 +75,7 @@ zero_log(
*/
error = xlog_find_tail(log, &head_blk, &tail_blk);
if (error) {
- do_warn(
+ do_log(
_("zero_log: cannot find log head/tail (xlog_find_tail=%d)\n"),
error);
if (!no_modify && !zap_log)
--
2.13.5
^ permalink raw reply related [flat|nested] 3+ messages in thread* Re: [PATCH] xfs_repair: don't use do_warn for normal log message
2017-09-13 10:49 [PATCH] xfs_repair: don't use do_warn for normal log message Masatake YAMATO
@ 2017-09-13 21:48 ` Dave Chinner
2017-09-14 2:19 ` Masatake YAMATO
0 siblings, 1 reply; 3+ messages in thread
From: Dave Chinner @ 2017-09-13 21:48 UTC (permalink / raw)
To: Masatake YAMATO; +Cc: linux-xfs
On Wed, Sep 13, 2017 at 07:49:43PM +0900, Masatake YAMATO wrote:
> In some case, exit statuses of xfs_repair -n are different even for the
> same file system when -v is specified or not. This patch fixes this
> behavior.
>
> If -v is specified, do_warn() is used in zero_log() for printing
> a normal message. That makes the exit status to 1 though there
> is no dirtiness in the file system.
>
> The original zero_log():
>
> error = xlog_find_tail(log, &head_blk, &tail_blk);
> if (error) {...
> } else {
> if (verbose) {
> do_warn(
> _("zero_log: head block %" PRId64 " tail block %" PRId64 "\n"),
> head_blk, tail_blkn);
> }
>
> do_warn() is used for the message, "zero_log:...". do_log
> should be used instead because this log message is for just reporting the
> values of head_blk, and tail_blk. do_log is for "just reporting".
>
> Using do_warn unintentionally has an impact.
> It causes the exit status of the command.
> do_warn sets fs_is_dirty global variable.
> That refers in main function to decide the exit status.
>
> void
> do_warn(char const *msg, ...)
> {
> va_list args;
>
> fs_is_dirty = 1;
>
> int
> main(int argc, char **argv)
> {
> ...
>
> if (fs_is_dirty)
> return(1);
> ---
> repair/phase2.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/repair/phase2.c b/repair/phase2.c
> index c21778b8..f7ea655f 100644
> --- a/repair/phase2.c
> +++ b/repair/phase2.c
> @@ -75,7 +75,7 @@ zero_log(
> */
> error = xlog_find_tail(log, &head_blk, &tail_blk);
> if (error) {
> - do_warn(
> + do_log(
> _("zero_log: cannot find log head/tail (xlog_find_tail=%d)\n"),
> error);
> if (!no_modify && !zap_log)
The code change doesn't match the code in commit description. This
warning looks like a warning that needs to be issued and change the
exist status - it indicates that there is a problem with the log.
Indeed, the commit description does not even need the code in it;
reviewers are familiar with the difference between do_warn/do_log.
What the commit message needs to do is explain when the problem
occurs, what it's impact is and why it needs fixing. The code fix
should then be obvious and self explanitory....
Cheers,
Dave.
--
Dave Chinner
david@fromorbit.com
^ permalink raw reply [flat|nested] 3+ messages in thread* Re: [PATCH] xfs_repair: don't use do_warn for normal log message
2017-09-13 21:48 ` Dave Chinner
@ 2017-09-14 2:19 ` Masatake YAMATO
0 siblings, 0 replies; 3+ messages in thread
From: Masatake YAMATO @ 2017-09-14 2:19 UTC (permalink / raw)
To: david; +Cc: linux-xfs
On Thu, 14 Sep 2017 07:48:21 +1000, Dave Chinner <david@fromorbit.com> wrote:
> On Wed, Sep 13, 2017 at 07:49:43PM +0900, Masatake YAMATO wrote:
>> In some case, exit statuses of xfs_repair -n are different even for the
>> same file system when -v is specified or not. This patch fixes this
>> behavior.
>>
>> If -v is specified, do_warn() is used in zero_log() for printing
>> a normal message. That makes the exit status to 1 though there
>> is no dirtiness in the file system.
>>
>> The original zero_log():
>>
>> error = xlog_find_tail(log, &head_blk, &tail_blk);
>> if (error) {...
>> } else {
>> if (verbose) {
>> do_warn(
>> _("zero_log: head block %" PRId64 " tail block %" PRId64 "\n"),
>> head_blk, tail_blkn);
>> }
>>
>> do_warn() is used for the message, "zero_log:...". do_log
>> should be used instead because this log message is for just reporting the
>> values of head_blk, and tail_blk. do_log is for "just reporting".
>>
>> Using do_warn unintentionally has an impact.
>> It causes the exit status of the command.
>> do_warn sets fs_is_dirty global variable.
>> That refers in main function to decide the exit status.
>>
>> void
>> do_warn(char const *msg, ...)
>> {
>> va_list args;
>>
>> fs_is_dirty = 1;
>>
>> int
>> main(int argc, char **argv)
>> {
>> ...
>>
>> if (fs_is_dirty)
>> return(1);
>> ---
>> repair/phase2.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/repair/phase2.c b/repair/phase2.c
>> index c21778b8..f7ea655f 100644
>> --- a/repair/phase2.c
>> +++ b/repair/phase2.c
>> @@ -75,7 +75,7 @@ zero_log(
>> */
>> error = xlog_find_tail(log, &head_blk, &tail_blk);
>> if (error) {
>> - do_warn(
>> + do_log(
>> _("zero_log: cannot find log head/tail (xlog_find_tail=%d)\n"),
>> error);
>> if (!no_modify && !zap_log)
>
> The code change doesn't match the code in commit description. This
> warning looks like a warning that needs to be issued and change the
> exist status - it indicates that there is a problem with the log.
>
> Indeed, the commit description does not even need the code in it;
> reviewers are familiar with the difference between do_warn/do_log.
> What the commit message needs to do is explain when the problem
> occurs, what it's impact is and why it needs fixing. The code fix
> should then be obvious and self explanitory....
Thank you for comments.
How about [PATCH v2] I submited just after submitting
this [PATCH]? I think the change in [PATCH v2] makes sense.
If the commit log in [PATCH v2] is not clear enough, I will submit
v3.
Regards,
Masatake YAMATO
> Cheers,
>
> Dave.
> --
> Dave Chinner
> david@fromorbit.com
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2017-09-14 2:19 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2017-09-13 10:49 [PATCH] xfs_repair: don't use do_warn for normal log message Masatake YAMATO
2017-09-13 21:48 ` Dave Chinner
2017-09-14 2:19 ` Masatake YAMATO
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox