[Crash-utility] [PATCH v3] Fix "log" command when crash is, started with "--minimal" option
lijiang
lijiang at redhat.com
Sun Aug 16 02:32:43 UTC 2020
在 2020年08月16日 00:00, crash-utility-request at redhat.com 写道:
> Send Crash-utility mailing list submissions to
> crash-utility at redhat.com
>
> To subscribe or unsubscribe via the World Wide Web, visit
> https://www.redhat.com/mailman/listinfo/crash-utility
> or, via email, send a message with subject or body 'help' to
> crash-utility-request at redhat.com
>
> You can reach the person managing the list at
> crash-utility-owner at redhat.com
>
> When replying, please edit your Subject line so it is more specific
> than "Re: Contents of Crash-utility digest..."
>
>
> Today's Topics:
>
> 1. Re: Crash-utility Digest, Vol 179, Issue 4 (David Wysochanski)
>
>
> ----------------------------------------------------------------------
>
> Message: 1
> Date: Sat, 15 Aug 2020 04:45:49 -0400
> From: David Wysochanski <dwysocha at redhat.com>
> To: HAGIO KAZUHITO(?? ??) <k-hagio-ab at nec.com>
> Cc: lijiang <lijiang at redhat.com>, "Discussion list for crash utility
> usage, maintenance and development" <crash-utility at redhat.com>
> Subject: Re: [Crash-utility] Crash-utility Digest, Vol 179, Issue 4
> Message-ID:
> <CALF+zOkMQoyDn8cP5fBpMrrusvcqytMUW+tzs=npKUWwG=nH1g at mail.gmail.com>
> Content-Type: text/plain; charset="UTF-8"
>
> On Thu, Aug 13, 2020 at 8:47 PM HAGIO KAZUHITO(?????)
> <k-hagio-ab at nec.com> wrote:
>>
>> -----Original Message-----
>>> From: crash-utility-bounces at redhat.com <crash-utility-bounces at redhat.com> On Behalf Of lijiang
>>> Sent: Friday, August 14, 2020 8:31 AM
>>> To: David Wysochanski <dwysocha at redhat.com>
>>> Cc: Discussion list for crash utility usage, maintenance and development <crash-utility at 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 <lijiang at redhat.com> wrote:
>>>>>
>>>>> ? 2020?08?13? 16:33, David Wysochanski ??:
>>>>>> Hi Lianbo
>>>>>>
>>>>>> On Sat, Aug 8, 2020 at 10:46 PM lijiang <lijiang at redhat.com> wrote:
>>>>>>>
>>>>>>> ? 2020?08?07? 00:00, crash-utility-request at redhat.com ??:
>>>>>>>> Message: 5
>>>>>>>> Date: Thu, 6 Aug 2020 09:30:22 -0400
>>>>>>>> From: Dave Wysochanski <dwysocha at redhat.com>
>>>>>>>> To: crash-utility at redhat.com
>>>>>>>> Subject: [Crash-utility] [PATCH v3] Fix "log" command when crash is
>>>>>>>> started with "--minimal" option
>>>>>>>> Message-ID: <20200806133022.2127538-1-dwysocha at 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 <dwysocha at redhat.com>
>>>>>>>> Reviewed-by: Wang Long <w at laoqinren.net>
>>>>>>>> Tested-by: Mathias Krause <minipli at 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 at 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.
>
Hi, David
Sorry for the misunderstanding, I don't oppose to adding my signature, but, I just think
that, maybe it doesn't make sense to add my signature.
If you like the improvement suggestions, can you please post it again with or without
my signature?
Crash utility really needs the fix ASAP. :-)
Thanks.
Lianbo
> 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 at redhat.com
>>> https://www.redhat.com/mailman/listinfo/crash-utility
>
>
>
>
> ------------------------------
>
> --
> Crash-utility mailing list
> Crash-utility at redhat.com
> https://www.redhat.com/mailman/listinfo/crash-utility
>
> End of Crash-utility Digest, Vol 179, Issue 19
> **********************************************
>
More information about the Crash-utility
mailing list