Re: [PATCH v5 2/2] powerpc/fadump: update documentation about 'fadump_append=' parameter

[Hari Bathini <hbathini@linux.vnet.ibm.com>]

Hi Michal,


Thanks for the review..


On Monday 26 June 2017 05:45 PM, Michal Suchánek wrote:
> Hello,
>
> On Tue, 20 Jun 2017 21:14:08 +0530
> Hari Bathini <hbathini@linux.vnet.ibm.com> wrote:
>
>> On Friday 09 June 2017 05:34 PM, Michal Suchánek wrote:
>>> On Thu, 8 Jun 2017 23:30:37 +0530
>>> Hari Bathini <hbathini@linux.vnet.ibm.com> wrote:
>>>> On Monday 15 May 2017 02:59 PM, Michal Suchánek wrote:
>>>>> On Mon, 15 May 2017 12:59:46 +0530
>>>>> Hari Bathini <hbathini@linux.vnet.ibm.com> wrote:
>>>>>> On Friday 12 May 2017 09:12 PM, Michal Suchánek wrote:
>>>>>>> On Fri, 12 May 2017 15:15:33 +0530
>>>>>>> Hari Bathini <hbathini@linux.vnet.ibm.com> wrote:
>>>>>>>> On Thursday 11 May 2017 06:46 PM, Michal Suchánek wrote:
>>>>>>>>> On Thu, 11 May 2017 02:00:11 +0530
>>>>>>>>> Hari Bathini <hbathini@linux.vnet.ibm.com> wrote:
>>>>>>>>>> On Wednesday 10 May 2017 09:31 PM, Michal Suchánek wrote:
>>>>>>>>>>> On Wed, 03 May 2017 23:52:52 +0530
>>>>>>>>>>> Hari Bathini <hbathini@linux.vnet.ibm.com> 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 <hbathini@linux.vnet.ibm.com>
>>>>>>>>>>>> ---
>>>>>>>>>>>>        
>>>>>>>>>>>>               
>>>>>>>>>>> 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.
>

I would prefer documenting over a complex implementation. Actually, I
am considering a simple approach of replacing every occurrence of
"fadump_extra_args=" with "fadump_extra_args " in fadump capture
kernel. The cmdline

   "root=/dev/sda2 ro fadump_extra_args="a b c" crashkernel=512M 
fadump_extra_args=d"

becomes

   "root=/dev/sda2 ro fadump_extra_args "a b c" crashkernel=512M 
fadump_extra_args d"

in fadump capture kernel. This must take care of the pitfalls with the 
current approach and also,
doesn't rely on parse_args() which was not designed for this scenario to 
start with..?

Will report a bug for yast handling of kernel parameters with quotes.

Thanks
Hari