-----Original Message-----
> From: crash-utility-bounces@xxxxxxxxxx <crash-utility-bounces@xxxxxxxxxx> On Behalf Of lijiang
> Sent: Friday, August 14, 2020 8:31 AM
> To: David Wysochanski <dwysocha@xxxxxxxxxx>
> Cc: Discussion list for crash utility usage, maintenance and development <crash-utility@xxxxxxxxxx>
> Subject: Re: 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@xxxxxxxxxx> wrote:
> >>
> >> 在 2020年08月13日 16:33, David Wysochanski 写道:
> >>> Hi Lianbo
> >>>
> >>> On Sat, Aug 8, 2020 at 10:46 PM lijiang <lijiang@xxxxxxxxxx> wrote:
> >>>>
> >>>> 在 2020年08月07日 00:00, crash-utility-request@xxxxxxxxxx 写道:
> >>>>> Message: 5
> >>>>> Date: Thu, 6 Aug 2020 09:30:22 -0400
> >>>>> From: Dave Wysochanski <dwysocha@xxxxxxxxxx>
> >>>>> To: crash-utility@xxxxxxxxxx
> >>>>> Subject: [PATCH v3] Fix "log" command when crash is
> >>>>> started with "--minimal" option
> >>>>> Message-ID: <20200806133022.2127538-1-dwysocha@xxxxxxxxxx>
> >>>>>
> >>>>> 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@xxxxxxxxxx>
> >>>>> Reviewed-by: Wang Long <w@xxxxxxxxxxxxx>
> >>>>> Tested-by: Mathias Krause <minipli@xxxxxxxxxxxxxx>
> >>>>> ---
> >>>>> 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.
No objection. I can ack a new one with the above change.
Thanks,
Kazu
> Otherwise Kazu can help to merge your last patch, because it can also work.
>
> Thanks.
> Lianbo
>
> --
> Crash-utility mailing list
> Crash-utility@xxxxxxxxxx
> https://www.redhat.com/mailman/listinfo/crash-utility
--
Crash-utility mailing list
Crash-utility@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/crash-utility
