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.
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