All of lore.kernel.org
 help / color / mirror / Atom feed
From: ebiederm@xmission.com (Eric W. Biederman)
To: lijiang <lijiang@redhat.com>
Cc: "jgross@suse.com" <jgross@suse.com>,
	"Thomas.Lendacky@amd.com" <Thomas.Lendacky@amd.com>,
	"bhe@redhat.com" <bhe@redhat.com>,
	"dyoung@redhat.com" <dyoung@redhat.com>,
	"x86@kernel.org" <x86@kernel.org>,
	"kexec@lists.infradead.org" <kexec@lists.infradead.org>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
	"dhowells@redhat.com" <dhowells@redhat.com>,
	Simon Horman <horms@verge.net.au>, "bp@alien8.de" <bp@alien8.de>,
	"hpa@zytor.com" <hpa@zytor.com>,
	"tglx@linutronix.de" <tglx@linutronix.de>,
	"mingo@redhat.com" <mingo@redhat.com>,
	"d.hatayama@fujitsu.com" <d.hatayama@fujitsu.com>,
	"vgoyal@redhat.com" <vgoyal@redhat.com>
Subject: Re: [PATCH 1/2 v5] x86/kdump: always reserve the low 1MiB when the crashkernel option is specified
Date: Thu, 24 Oct 2019 22:39:59 -0500	[thread overview]
Message-ID: <875zkdtrw0.fsf@x220.int.ebiederm.org> (raw)
In-Reply-To: <2020bbf9-67b2-52e8-756f-b595414b4c02@redhat.com> (lijiang's message of "Fri, 25 Oct 2019 11:14:43 +0800")

lijiang <lijiang@redhat.com> writes:

>  * Returns the length of the argument (regardless of if it was
>  * truncated to fit in the buffer), or -1 on not found.
>  */
> static int
> __cmdline_find_option(const char *cmdline, int max_cmdline_size,
>                       const char *option, char *buffer, int bufsize)
>
>
> According to the above code comment, it should be better like this:
>
> +       if (cmdline_find_option(boot_command_line, "crashkernel",
> +                               NULL, 0) > 0) {
>
> After i test, i will post again.
>

This seems reasonable as we are dealing with x86 only code.

It wound be nice if someone could generalize cmdline_find_option to be
arch independent so that crash_core.c:parse_crashkernel could use it.
I don't think for this patchset, but it looks like an overdue cleanup.

We run the risk with parse_crashkernel using strstr and this using
another algorithm of having different kernel command line parsers
giving different results and disagreeing if "crashkernel=" is present
or not on the kernel command line.

Eric



_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

WARNING: multiple messages have this Message-ID (diff)
From: ebiederm@xmission.com (Eric W. Biederman)
To: lijiang <lijiang@redhat.com>
Cc: "d.hatayama\@fujitsu.com" <d.hatayama@fujitsu.com>,
	Simon Horman <horms@verge.net.au>,
	"linux-kernel\@vger.kernel.org" <linux-kernel@vger.kernel.org>,
	"jgross\@suse.com" <jgross@suse.com>,
	"Thomas.Lendacky\@amd.com" <Thomas.Lendacky@amd.com>,
	"bhe\@redhat.com" <bhe@redhat.com>,
	"x86\@kernel.org" <x86@kernel.org>,
	"kexec\@lists.infradead.org" <kexec@lists.infradead.org>,
	"dhowells\@redhat.com" <dhowells@redhat.com>,
	"mingo\@redhat.com" <mingo@redhat.com>,
	"bp\@alien8.de" <bp@alien8.de>, "hpa\@zytor.com" <hpa@zytor.com>,
	"tglx\@linutronix.de" <tglx@linutronix.de>,
	"dyoung\@redhat.com" <dyoung@redhat.com>,
	"vgoyal\@redhat.com" <vgoyal@redhat.com>
Subject: Re: [PATCH 1/2 v5] x86/kdump: always reserve the low 1MiB when the crashkernel option is specified
Date: Thu, 24 Oct 2019 22:39:59 -0500	[thread overview]
Message-ID: <875zkdtrw0.fsf@x220.int.ebiederm.org> (raw)
In-Reply-To: <2020bbf9-67b2-52e8-756f-b595414b4c02@redhat.com> (lijiang's message of "Fri, 25 Oct 2019 11:14:43 +0800")

lijiang <lijiang@redhat.com> writes:

>  * Returns the length of the argument (regardless of if it was
>  * truncated to fit in the buffer), or -1 on not found.
>  */
> static int
> __cmdline_find_option(const char *cmdline, int max_cmdline_size,
>                       const char *option, char *buffer, int bufsize)
>
>
> According to the above code comment, it should be better like this:
>
> +       if (cmdline_find_option(boot_command_line, "crashkernel",
> +                               NULL, 0) > 0) {
>
> After i test, i will post again.
>

This seems reasonable as we are dealing with x86 only code.

It wound be nice if someone could generalize cmdline_find_option to be
arch independent so that crash_core.c:parse_crashkernel could use it.
I don't think for this patchset, but it looks like an overdue cleanup.

We run the risk with parse_crashkernel using strstr and this using
another algorithm of having different kernel command line parsers
giving different results and disagreeing if "crashkernel=" is present
or not on the kernel command line.

Eric



  reply	other threads:[~2019-10-25  3:41 UTC|newest]

Thread overview: 20+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-10-23 14:19 [PATCH 0/2 v5] x86/kdump: Fix 'kmem -s' reported an invalid freepointer when SME was active Lianbo Jiang
2019-10-23 14:19 ` Lianbo Jiang
2019-10-23 14:19 ` [PATCH 1/2 v5] x86/kdump: always reserve the low 1MiB when the crashkernel option is specified Lianbo Jiang
2019-10-23 14:19   ` Lianbo Jiang
2019-10-24 10:07   ` Simon Horman
2019-10-24 10:07     ` Simon Horman
2019-10-24 11:33     ` lijiang
2019-10-24 11:33       ` lijiang
2019-10-25  1:31       ` lijiang
2019-10-25  1:31         ` lijiang
2019-10-25  1:38         ` d.hatayama
2019-10-25  1:38           ` d.hatayama
2019-10-25  3:14           ` lijiang
2019-10-25  3:14             ` lijiang
2019-10-25  3:39             ` Eric W. Biederman [this message]
2019-10-25  3:39               ` Eric W. Biederman
2019-10-25  8:52               ` lijiang
2019-10-25  8:52                 ` lijiang
2019-10-23 14:19 ` [PATCH 2/2 v5] x86/kdump: clean up all the code related to the backup region Lianbo Jiang
2019-10-23 14:19   ` Lianbo Jiang

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=875zkdtrw0.fsf@x220.int.ebiederm.org \
    --to=ebiederm@xmission.com \
    --cc=Thomas.Lendacky@amd.com \
    --cc=bhe@redhat.com \
    --cc=bp@alien8.de \
    --cc=d.hatayama@fujitsu.com \
    --cc=dhowells@redhat.com \
    --cc=dyoung@redhat.com \
    --cc=horms@verge.net.au \
    --cc=hpa@zytor.com \
    --cc=jgross@suse.com \
    --cc=kexec@lists.infradead.org \
    --cc=lijiang@redhat.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=mingo@redhat.com \
    --cc=tglx@linutronix.de \
    --cc=vgoyal@redhat.com \
    --cc=x86@kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body. 
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.