Re: [Crash-utility] Crash-utility Digest, Vol 179, Issue 4

From: crash-utility-bounces(a)redhat.com <crash-utility-bounces(a)redhat.com&gt; On Behalf Of lijiang Sent: Friday, August 14, 2020 8:31 AM To: David Wysochanski <dwysocha(a)redhat.com&gt; Cc: Discussion list for crash utility usage, maintenance and development <crash-utility(a)redhat.com&gt; Subject: Re: [Crash-utility] Crash-utility Digest, Vol 179, Issue 4 在 2020年08月13日 22:58, David Wysochanski 写道: > On Thu, Aug 13, 2020 at 9:08 AM lijiang <lijiang(a)redhat.com&gt; wrote: >> >> 在 2020年08月13日 16:33, David Wysochanski 写道: >>> Hi Lianbo >>> >>> On Sat, Aug 8, 2020 at 10:46 PM lijiang <lijiang(a)redhat.com&gt; wrote: >>>> >>>> 在 2020年08月07日 00:00, crash-utility-request(a)redhat.com 写道: >>>>> Message: 5 >>>>> Date: Thu, 6 Aug 2020 09:30:22 -0400 >>>>> From: Dave Wysochanski <dwysocha(a)redhat.com&gt; >>>>> To: crash-utility(a)redhat.com >>>>> Subject: [Crash-utility] [PATCH v3] Fix "log" command when crash is >>>>> started with "--minimal" option >>>>> Message-ID: <20200806133022.2127538-1-dwysocha(a)redhat.com&gt; >>>>> >>>>> Commit c86250bce29f introduced the useful '-T' option to print the >>>>> log timestamp in human-readable form. However, this option does >>>>> not work when crash is invoked with '--minimal' mode, and if tried, >>>>> crash will spin at 100% and continuously crash at a divide by 0 >>>>> because machdep->hz == 0. >>>>> >>>>> Fix this by disallowing this option in minimal mode. In addition, >>>>> only calculate the logic to calculate kt->boot_date.tv_sec >>>>> when this option is enabled. >>>>> >>>> Hi, Dave Wysochanski >>>> >>>> Thank you for the patch. >>>> >>>>> Fixes: c86250bce29f ("Introduction of the "log -T" option...") >>>>> Signed-off-by: Dave Wysochanski <dwysocha(a)redhat.com&gt; >>>>> Reviewed-by: Wang Long <w(a)laoqinren.net&gt; >>>>> Tested-by: Mathias Krause <minipli(a)grsecurity.net&gt; >>>>> --- >>>>> kernel.c | 5 ++++- >>>>> 1 file changed, 4 insertions(+), 1 deletion(-) >>>>> >>>>> diff --git a/kernel.c b/kernel.c >>>>> index 5ed6021..95119f3 100644 >>>>> --- a/kernel.c >>>>> +++ b/kernel.c >>>>> @@ -4939,7 +4939,10 @@ cmd_log(void) >>>>> if (argerrs) >>>>> cmd_usage(pc->curcmd, SYNOPSIS); >>>>> >>>>> - if (kt->boot_date.tv_sec == 0) { >>>>> + if (msg_flags & SHOW_LOG_CTIME && pc->flags & MINIMAL_MODE) >>>>> + error(FATAL, "log: option 'T' not available in minimal mode\n"); >>>>> + >>>>> + if (msg_flags & SHOW_LOG_CTIME && kt->boot_date.tv_sec == 0) { >>>> >>>> The above two 'if' statements have the same checking condition, would you mind putting them together >>>> as a statement block? E.g: >>>> >>> Sure I can resubmit a fixup of v4 patch once there are no more changes needed. >>> >>>> + if (msg_flags & SHOW_LOG_CTIME) { >>>> + if (pc->flags & MINIMAL_MODE) { >>>> + error(WARNING, "the option '-T' not available in minimal mode\n"); >>>> + return; >>>> + } >>>> + >>>> + if (kt->boot_date.tv_sec == 0) { >>>> ... >>>> + } >>>> } >>>> >>>> In addition, might it be more reasonable to issue a warning instead of a fatal error? >>>> >>> >>> If you use WARNING it will not fix the infinite loop / CPU spin at >>> 100%. You have to CTRL-C the crash program to get the prompt back. >>> So I do not think this is a good idea. >>> >> How did you reproduce it? Can you help to confirm if you have applied the correct patch >> as below? >> >> [root@intel-sharkbay-mb-03 crash]# git diff kernel.c >> diff --git a/kernel.c b/kernel.c >> index 5ed6021..6375b24 100644 >> --- a/kernel.c >> +++ b/kernel.c >> @@ -4939,13 +4939,20 @@ cmd_log(void) >> if (argerrs) >> cmd_usage(pc->curcmd, SYNOPSIS); >> >> - if (kt->boot_date.tv_sec == 0) { >> - ulonglong uptime_jiffies; >> - ulong uptime_sec; >> - get_uptime(NULL, &uptime_jiffies); >> - uptime_sec = (uptime_jiffies)/(ulonglong)machdep->hz; >> - kt->boot_date.tv_sec = kt->date.tv_sec - uptime_sec; >> - kt->boot_date.tv_nsec = 0; >> + if (msg_flags & SHOW_LOG_CTIME) { >> + if (pc->flags & MINIMAL_MODE) { >> + error(WARNING, "the option '-T' not available in minimal mode\n"); >> + return; >> + } >> + >> + if (kt->boot_date.tv_sec == 0) { >> + ulonglong uptime_jiffies; >> + ulong uptime_sec; >> + get_uptime(NULL, &uptime_jiffies); >> + uptime_sec = (uptime_jiffies)/(ulonglong)machdep->hz; >> + kt->boot_date.tv_sec = kt->date.tv_sec - uptime_sec; >> + kt->boot_date.tv_nsec = 0; >> + } >> } >> >> if (msg_flags & SHOW_LOG_AUDIT) { >> >> >> I didn't see any problems, it's strange, this is my test steps. >> > > You are right - I missed the 'return;' in your patch. The WARNING is fine. > Thanks for your confirmation. > How do you want to handle this? Do you want to take the original header > and add your signed-off-by line and commit your patch? Or do you want > me to resubmit with review-by or signed-off-by lines? > No, please do not add my signed-off-by and review-by line. If you and Kazu have no objection, you could post it again with the above changes.

Otherwise Kazu can help to merge your last patch, because it can also work. Thanks. Lianbo -- Crash-utility mailing list Crash-utility(a)redhat.com https://www.redhat.com/mailman/listinfo/crash-utility