On 04/07/2017 at 02:03 PM, Bhupesh Sharma wrote:
On Fri, Apr 7, 2017 at 11:15 AM, Dave Young <dyoung(a)redhat.com>
wrote:
> On 04/07/17 at 09:46am, Bhupesh Sharma wrote:
>> Hi Dave,
>>
>> On Fri, Apr 7, 2017 at 8:04 AM, Dave Young <dyoung(a)redhat.com> wrote:
>>> On 04/06/17 at 07:59pm, Xunlei Pang wrote:
>>>> On 04/06/2017 at 06:01 PM, Bhupesh Sharma wrote:
>>>>> This patch introduces the 'force_no_rebuild' option
>>>>> inside the 'kdump.conf' and its handling inside the
'kdumpctl'
>>>>> script.
>>>>>
>>>>> There might be several use cases, where a system admin
>>>>> decides that he doesn't need to rebuild the kdump initrd
>>>>> and wants to use an existing version of the same. In such cases,
>>>>> he can set the 'force_no_rebuild' option inside
'kdump.conf'
>>>>> to 1, to force the 'kdumpctl' script not to rebuild the
kdump
>>>>> initrd.
>>>>>
>>>>> Signed-off-by: Bhupesh Sharma <bhsharma(a)redhat.com>
>>>>> ---
>>>>> Changes since v1:
>>>>> - Removed extra handling introduced for /boot being RO
>>>> This version feels way more better, thanks for the effort!
>>>>
>>>>> kdump.conf | 8 ++++++++
>>>>> kdump.conf.5 | 10 ++++++++++
>>>>> kdumpctl | 26 +++++++++++++++++++++++++-
>>>>> 3 files changed, 43 insertions(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/kdump.conf b/kdump.conf
>>>>> index ebf9587a6522..18bbd4ace971 100644
>>>>> --- a/kdump.conf
>>>>> +++ b/kdump.conf
>>>>> @@ -115,6 +115,13 @@
>>>>> # Specify 1 to force rebuilding kdump initrd every time
when kdump
>>>>> # service starts.
>>>>> #
>>>>> +# force_no_rebuild <0 | 1>
>>>>> +# - By default, kdump initrd will only be rebuilt when
necessary.
>>>> IMO, remove "only" sounds better?
>>>>
>>>>> +# Specify 1 to force no rebuild of kdump initrd.
>>>>> +#
>>>>> +# force_no_rebuild and force_rebuild options are
mutually exclusive and
>>>>> +# both should not be set to 1 simultaneously.
>>>>> +#
>>>>> # override_resettable <0 | 1>
>>>>> # - Usually an unresettable block device can't be a
dump target.
>>>>> # Specifying 1 when you want to dump even though the
block
>>>>> @@ -150,6 +157,7 @@ core_collector makedumpfile -l --message-level 1
-d 31
>>>>> #extra_modules gfs2
>>>>> #default shell
>>>>> #force_rebuild 1
>>>>> +#force_no_rebuild 1
>>>>> #dracut_args --omit-drivers "cfg80211 snd" --add-drivers
"ext2 ext3"
>>>>> #fence_kdump_args -p 7410 -f auto -c 0 -i 10
>>>>> #fence_kdump_nodes node1 node2
>>>>> diff --git a/kdump.conf.5 b/kdump.conf.5
>>>>> index ca427699be53..265a67f5d575 100644
>>>>> --- a/kdump.conf.5
>>>>> +++ b/kdump.conf.5
>>>>> @@ -166,6 +166,16 @@ By default, kdump initrd will only be rebuilt
when necessary.
>>>>> Specify 1 to force rebuilding kdump initrd every time when kdump
service starts.
>>>>> .RE
>>>>>
>>>>> +.B force_no_rebuild <0 | 1>
>>>>> +.RS
>>>>> +By default, kdump initrd will only be rebuilt when necessary.
>>>> ditto
>>>>
>>>>> +Specify 1 to force no rebuild of kdump initrd.
>>>>> +
>>>>> +.PP
>>>>> +force_no_rebuild and force_rebuild options are mutually exclusive
and
>>>>> +both should not be set to 1 simultaneously.
>>>>> +.RE
>>>>> +
>>>>> .B override_resettable <0 | 1>
>>>>> .RS
>>>>> Usually an unresettable block device can't be a dump target.
Specifying 1 means
>>>>> diff --git a/kdumpctl b/kdumpctl
>>>>> index 4d68be0c292d..63bc7adce422 100755
>>>>> --- a/kdumpctl
>>>>> +++ b/kdumpctl
>>>>> @@ -335,6 +335,8 @@ restore_default_initrd()
>>>>> check_config()
>>>>> {
>>>>> local nr
>>>>> + local force_rebuild=0
>>>>> + local force_no_rebuild=0
>>>>>
>>>>> nr=$(awk 'BEGIN{cnt=0}
/^raw|^ssh[[:blank:]]|^nfs|^ext[234]|^xfs|^btrfs|^minix|^dracut_args .*\-\-mount/{cnt++}
END{print cnt}' $KDUMP_CONFIG_FILE)
>>>>> [ $nr -gt 1 ] && {
>>>>> @@ -354,11 +356,18 @@ check_config()
>>>>> case "$config_opt" in
>>>>> \#* | "")
>>>>> ;;
>>>>> -
raw|ext2|ext3|ext4|minix|btrfs|xfs|nfs|ssh|sshkey|path|core_collector|kdump_post|kdump_pre|extra_bins|extra_modules|default|force_rebuild|dracut_args|fence_kdump_args|fence_kdump_nodes)
>>>>> +
raw|ext2|ext3|ext4|minix|btrfs|xfs|nfs|ssh|sshkey|path|core_collector|kdump_post|kdump_pre|extra_bins|extra_modules|default|force_rebuild|force_no_rebuild|dracut_args|fence_kdump_args|fence_kdump_nodes)
>>>>> [ -z "$config_val" ] && {
>>>>> echo "Invalid kdump config value for
option $config_opt."
>>>>> return 1;
>>>>> }
>>>>> +
>>>>> + [[ "$config_opt" ==
"force_no_rebuild" && "$config_val" == "1" ]]
&& force_no_rebuild=1
>>>>> + [[ "$config_opt" ==
"force_rebuild" && "$config_val" == "1" ]]
&& force_rebuild=1
>>>>> + [[ "$force_no_rebuild" == "1"
&& "$force_rebuild" == "1" ]] && {
>>>>> + echo "Error: both force_rebuild and
force_no_rebuild cannot be set in kdump.conf"
>>>>> + return 1;
>>>>> + }
>>>>> ;;
>>>>>
net|options|link_delay|disk_timeout|debug_mem_level|blacklist)
>>>>> echo "Deprecated kdump config option:
$config_opt. Refer to kdump.conf manpage for alternatives."
>>>>> @@ -609,6 +618,7 @@ check_rebuild()
>>>>> {
>>>>> local extra_modules
>>>>> local _force_rebuild force_rebuild="0"
>>>>> + local _force_no_rebuild force_no_rebuild="0"
>>>>> local ret system_modified="0"
>>>>>
>>>>> check_boot_dir
>>>>> @@ -625,6 +635,15 @@ check_rebuild()
>>>>> return 1
>>>>> fi
>>>>>
>>>>> + _force_no_rebuild=`grep ^force_no_rebuild $KDUMP_CONFIG_FILE
2>/dev/null`
>>>>> + if [ $? -eq 0 ]; then
>>>>> + force_no_rebuild=`echo $_force_no_rebuild | cut -d'
' -f2`
>>>>> + if [ "$force_no_rebuild" != "0" ]
&& [ "$force_no_rebuild" != "1" ];then
>>>>> + echo "Error: force_no_rebuild value is
invalid"
>>>>> + return 1
>>>>> + fi
>>>>> + fi
>>>>> +
>>>> How about move this chunk as well as the "force_rebuild" chunk
below to check_config()?
>>> The force_rebuild and force_no_rebuild config_val were already parsed in
>>> check_config, maybe set it as global vars at the beginning of kdumpctl
>>> and initialize them to 0, check_config will update the values, then
>>> just use the values here?
>> IMO its better to have the approach introduced in v1 (where wee did
>> this handling in check_config) or here (as done by v2) rather than
>> having this handling split across two functions using global
>> variables.
>>
>> I have no specific preference to either of the approaches used in v1
>> or v2, so I propose to continue with either rather than split it
>> across two functions.
> V2 looks better, but since these two options are bool type and we have get
> it in check_config, so it makes sense to reuse it instead of reparse the
> value. But I'm not sure what Xunlei thinks in his comment, let's wait
> for his comments.
Yes, I agree - let's wait for Xunlei's comments. Just to clarify, v1
had a similar approach where we handled the conflicting options and
their parsing only once in check_config function.
I prefer adding all the sanity check in check_config, we only check here, it's
consistent with the naming check_xxx
There are usually different preferences on handling such cases. While
one school of thought suggests to handle error cases as soon as
possible (which is done at v2) to avoid calling check_config later and
additional function calls altogether on the way.
On the other hand, others advice to avoid handling the same parsing
twice across two functions.
I have no specific preference to either one and can rework v3 as per
the suggestions.
I have no strong objection to this, just a personal preference, it's all up to you
:-)
Regards,
Xunlei
>> Regards,
>> Bhupesh
>>
>>>>> _force_rebuild=`grep ^force_rebuild $KDUMP_CONFIG_FILE
2>/dev/null`
>>>>> if [ $? -eq 0 ]; then
>>>>> force_rebuild=`echo $_force_rebuild | cut -d' '
-f2`
>>>>> @@ -634,6 +653,11 @@ check_rebuild()
>>>>> fi
>>>>> fi
>>>>>
>>>>> + # Will not rebuild kdump initrd
>>>>> + if [ "$force_no_rebuild" == "1" ]; then
>>>> Better to judge if [ -f "$TARGET_INITRD" ], if it does not
exist, just throw a warning message and bail out.
>>>>
>>>> Regards,
>>>> Xunlei
>>>>
>>>>> + return 0
>>>>> + fi
>>>>> +
>>>>> #will rebuild every time if extra_modules are specified
>>>>> extra_modules=`grep ^extra_modules $KDUMP_CONFIG_FILE`
>>>>> [ -n "$extra_modules" ] &&
force_rebuild="1"
>>>> _______________________________________________
>>>> kexec mailing list -- kexec(a)lists.fedoraproject.org
>>>> To unsubscribe send an email to kexec-leave(a)lists.fedoraproject.org
> Thanks
> Dave