Hello,
On Tue, 20 Jun 2017 21:14:08 +0530
Hari Bathini <[email protected]> wrote:
On Friday 09 June 2017 05:34 PM, Michal Suchánek wrote:
On Thu, 8 Jun 2017 23:30:37 +0530
Hari Bathini <[email protected]> wrote:
On Monday 15 May 2017 02:59 PM, Michal Suchánek wrote:
On Mon, 15 May 2017 12:59:46 +0530
Hari Bathini <[email protected]> wrote:
On Friday 12 May 2017 09:12 PM, Michal Suchánek wrote:
On Fri, 12 May 2017 15:15:33 +0530
Hari Bathini <[email protected]> wrote:
On Thursday 11 May 2017 06:46 PM, Michal Suchánek wrote:
On Thu, 11 May 2017 02:00:11 +0530
Hari Bathini <[email protected]> wrote:
On Wednesday 10 May 2017 09:31 PM, Michal Suchánek wrote:
On Wed, 03 May 2017 23:52:52 +0530
Hari Bathini <[email protected]> wrote:
With the introduction of 'fadump_append=' parameter to pass
additional parameters to fadump (capture) kernel, update
documentation about it.
Signed-off-by: Hari Bathini <[email protected]>
---
Writing your own deficient parser for comma separated
arguments when perfectly fine parser for space separated
quoted arguments exists in the kernel and the bootloader
does not seem like a good idea to me.
Couple of things that prompted me for v5 are:
1. Using parse_early_options() limits the kind of
parameters that can be passed to fadump capture kernel.
Passing parameters like systemd.unit= & udev.childern.max=
has no effect with v4. Updating
boot_command_line parameter, when fadump is active,
seems a better alternative.
2. Passing space-separated quoted arguments is not
working as intended with lilo. Updating bootloader with the
below entry in /etc/lilo.conf file results in a missing
append entry in /etc/yaboot.conf file.
append = "quiet sysrq=1 insmod=sym53c8xx
insmod=ipr crashkernel=512M-:256M fadump_append=\"nr_cpus=1
numa=off udev.children-max=2\""
Meaning that a script that emulates LILO semantics on top of
yaboot which is completely capable of passing qouted space
separated arguments fails. IMHO it is more reasonable to fix
the script or whatever adaptation layer or use yaboot
directly than working around bug in said script by
introducing a new argument parser in the kernel.
The intention with this patch is to replace
"root=/dev/sda2 ro fadump_append=nr_cpus=1,numa=off
crashkernel=1024M"
with
"root=/dev/sda2 ro nr_cpus=1 numa=off crashkernel=1024M"
when kernel is booting for dump capture.
While parse_args() is making parsing relatively easy, the main
idea is to replace parameters as above when fadump capture kernel
is booting. The code is relatively complicated to replace
parameters using space-separated
quoted string even though parse_args() may parse them easily.
Comma-separated
list was easier to implement and seemed less error prone for
replacing parameters.
You can still do that relatively easily. parse_args() gives you the
content of fadump_capture argument. You should probably add a
variant that gives you also the position of the fadump_capture
argument in case user specified it multiple times - picking the
wrong one would cause errors.
There is no denying that this can be done with the use of
parse_args() function.
But my contention is a custom parser is no more error prone,
considering it only has to search/replace commas in
fadump_extra_args= till the first occurrence of space/nul, without
having to deal with the naunces of parse_args() function.
If you do that you
1) introduce new parser which is error-prone even for not very complex
parsers
2) does not handle quoting properly meaning that
a) the passed arguments cannot include a comma (which is the case for
many existing kernel parameters)
b) your parser, parse_args and any parser analyzing the commandline
before you replace the arguments do not agree on the content and
length of the fadump_extra_args argument
3) you introduce completely new syntax for arguments which the user
has to research and understand for no good reason
Also, to
get the last occurence, could use something like
get_last_crashkernel() instead of adding a variant to parse_args().
Why are you talking about last occurence all the time? Does that mean
that if user specifies fadump_extra_args in both the 'default' and
'extra' kernel arguments to the grub configuration script only the one
from 'extra' arguments will be expanded and the on from 'default'
arguments will be just left there? Why?
Then you can just replace the fadump_append="appended arguments"
with the dequoted "appended arguments" parse_args() gives you.
Dequating should shorten the arguments as would removing the
fadump_append= prefix so it should be quite possible in-place. It
is in fact even easier than the comma separated list since you do
not need to do any parsing yourself - only strlen, memset and
memcpy.
That said, if you replace the fadump_append argument the running
kernel will have extra arguments without knowing where they came
from making detection of this happening all the more difficult.
How about replacing "fadump_extra_args=x,y,z" with "fadump_extra_args
x y z"?
How does this handle x that includes a comma?
How does this handle x that includes a space?
How does it handle x that includes a double quote? - Well, presumably
it will just stay there with the fadump_extra_args expanded or not and
will cause equal breakage.
Presence of fadump_extra_args hints the user about the extra
parameters passed
to fadump capture kernel and also lets parsing/replacing be simple -
replace the first '=' and subsequent commas with ' ' in this
fadump_extra_args= parameter
and we are good to go. No additional handling. Even if we go with
space-separated
quoted string, simply replacing "fadump_extra_args=" with
"fadump_extra_args"
Leaving the empty fadump_extra_args argument might be good for
indication that the replacement possibly took place and comes for free.
should suffice, no need to worry about parse_args as well.
If you use parse_args it can give you the content of the every
fadump_extra_args argument and dequotes it for you. So in the case
fadump_extra_args does include double quotes you get them handled
correctly for you by existing parser.
And you want to make a variant of parse_args that tells you where the
argument starts and ends in the parsed commandline so you know which
part you should replace - which it certainly does know but no current
users need the information so it does not tell you.
When you do that users can read (or for the most part have already
read) about the kernel argument syntax and apply that to
fadump_extra_args. No additional specification and exceptions are
needed.
I prefer
comma-separated
list though, for it leaves no dependency on how bootloader handles
space-separated
quoted strings, considering my encounter with yast bootloader which
is not so
impressive in handling quoted strings. Also, the code change is not
so complicated.
If yast cannot handle kernel parameters correctly report a bug. The
quoting of arguments with spaces is part of the kernel parameter
specification so yast should handle it.
Want to replace fadump_append=, to avoid command line overflow
issues and also to not have to deal with special cases like --.
Actually, fadump_append=
seems to be confusing in that sense. How about
fadump_args=comma-separated-list-
of-params-for-capture-kernel ? Please share your thoughts.
The comma separated list still can contain a -- argument so you
still have to do something about that. Or not and just document
that people should not use it.
The user has the flexibility to use fadump_extra_args= to pass
special parameters
keeping in mind that "fadump_extra_args=x,y,z" will be replaced
in-place with
"fadump_extra_args x y z" and it happens only in fadump capture
kernel. No additional
code changes needed to handle this except for documentation talking
about it?
Presumably if y is "--" then z and any subsequent kernel arguments after
fadump_extra_args are passed to init when fadump_extra_args is
expanded and not so when left as is. This may be unexpected - the user
may expect that if fadump_extra_args contains a "--" as y then z is
appended after existing "--" in commandline or if none is present the
whole "-- z" is moved to the end. This would be clean way of handling
the extra arguments - no text only preprocessing allowing for dirty
tricks moving arguments from kernel to init by in-place text
replacement. Note that any unclosed quoting in arguments after
fadump_extra_args may make appending "-- z" quite difficult if you
wanted to do it correctly. If you do not want to bother (with either)
it is an understandable shortcoming in the implementation and should be
documented.
