On Thu, Aug 13, 2020 at 8:47 PM HAGIO KAZUHITO(萩尾 一仁)
<k-hagio...@nec.com> wrote:
>
> -----Original Message-----
> > From: crash-utility-boun...@redhat.com <crash-utility-boun...@redhat.com>
> > On Behalf Of lijiang
> > Sent: Friday, August 14, 2020 8:31 AM
> > To: David Wysochanski <dwyso...@redhat.com>
> > Cc: Discussion list for crash utility usage, maintenance and development
> > <crash-utility@redhat.com>
> > 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 <liji...@redhat.com> wrote:
> > >>
> > >> 在 2020年08月13日 16:33, David Wysochanski 写道:
> > >>> Hi Lianbo
> > >>>
> > >>> On Sat, Aug 8, 2020 at 10:46 PM lijiang <liji...@redhat.com> wrote:
> > >>>>
> > >>>> 在 2020年08月07日 00:00, crash-utility-requ...@redhat.com 写道:
> > >>>>> Message: 5
> > >>>>> Date: Thu, 6 Aug 2020 09:30:22 -0400
> > >>>>> From: Dave Wysochanski <dwyso...@redhat.com>
> > >>>>> To: crash-utility@redhat.com
> > >>>>> Subject: [Crash-utility] [PATCH v3] Fix "log" command when crash is
> > >>>>> started with "--minimal" option
> > >>>>> Message-ID: <20200806133022.2127538-1-dwyso...@redhat.com>
> > >>>>>
> > >>>>> 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 <dwyso...@redhat.com>
> > >>>>> Reviewed-by: Wang Long <w...@laoqinren.net>
> > >>>>> Tested-by: Mathias Krause <mini...@grsecurity.net>
> > >>>>> ---
> > >>>>> 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.
>
I would suggest taking the v3 patch as is because Lianbo has said not
to add his signed-off-by line because I did not write that portion. I'm not
going to modify something written by someone else and omit where it
came from.
Thanks!
> Thanks,
> Kazu
>
> > Otherwise Kazu can help to merge your last patch, because it can also work.
> >
> > Thanks.
> > Lianbo
> >
> > --
> > Crash-utility mailing list
> > Crash-utility@redhat.com
> > https://www.redhat.com/mailman/listinfo/crash-utility
--
Crash-utility mailing list
Crash-utility@redhat.com
https://www.redhat.com/mailman/listinfo/crash-utility
