Hi Tao,
On Wed, 23 Feb 2022 18:12:45 +0800
Tao Liu <ltao(a)redhat.com> wrote:
Hi Philipp,
On Tue, Feb 22, 2022 at 03:17:39PM +0100, Philipp Rudo wrote:
> Hi Tao,
>
> On Tue, 22 Feb 2022 17:37:31 +0800
> Tao Liu <ltao(a)redhat.com> wrote:
>
> > Hi Philipp,
> >
> > I have some concerns about this patch.
> >
> > On Sat, Feb 5, 2022 at 1:36 AM Philipp Rudo <prudo(a)redhat.com> wrote:
> > >
> > > check_config and check_ssh_config both parse /etc/kdump.conf and are
> > > usually used together. The difference between both is that
> > > check_ssh_config does some extra checks on the format of the provided
> > > ssh destination but ignores invalid or deprecated options in the config.
> > > Thus merge check_ssh_config into check_config. Leave the additional
> > > checks on the ssh destination in check_ssh_config but treat it like the
> > > checks done for e.g. the failure_action.
> > >
> > > This slightly changes the behavior of 'kdumpctl propagate', which
now
> > > fails if kdump.conf contains an invalid value unrelated to ssh. This
> > > change in behavior isn't problematic because 'kdumpctl
propagate' always
> > > needs to be followed by a 'kdumpctl start' to have a working
kdump
> > > environment. For the situations where 'propagate' fails now the
'start'
> > > would have failed in the past. So the failure only moved one step ahead
> > > in the sequence.
> > >
> > > While at it drop check_ssh_target and call check_and_wait_network_ready
> > > directly.
> > >
> > > Signed-off-by: Philipp Rudo <prudo(a)redhat.com>
> > > ---
> > > kdumpctl | 71 ++++++++++++++++++++++++--------------------------------
> > > 1 file changed, 30 insertions(+), 41 deletions(-)
> > >
> > > diff --git a/kdumpctl b/kdumpctl
> > > index a311238..ebb634b 100755
> > > --- a/kdumpctl
> > > +++ b/kdumpctl
> > > @@ -204,10 +204,27 @@ check_config()
> > > fi
> > > config_opt=_target
> > > ;;
> > > - ext[234] | minix | btrfs | xfs | nfs | ssh)
> > > + ext[234] | minix | btrfs | xfs | nfs )
> > > config_opt=_target
> > > ;;
> > > - sshkey | path | core_collector | kdump_post | kdump_pre |
extra_bins | extra_modules | failure_action | default | final_action | force_rebuild |
force_no_rebuild | fence_kdump_args | fence_kdump_nodes | auto_reset_crashkernel) ;;
> > > + ssh)
> > > + config_opt=_target
> > > + DUMP_TARGET=$config_val
> > > + ;;
> > > + sshkey)
> > > + if [[ -z $config_val ]]; then
> > > + derror "Invalid kdump config value
for option '$config_opt'"
> > > + return 1
> > > + elif [[ -f $config_val ]]; then
> > > + SSH_KEY_LOCATION=$(/usr/bin/readlink -m
"$config_val")
> > > + else
> > > + dwarn "WARNING: '$config_val'
doesn't exist, using default value '$SSH_KEY_LOCATION'"
> > > + fi
> > > + ;;
> > > + path)
> > > + SAVE_PATH=$config_val
> > > + ;;
> > > + core_collector | kdump_post | kdump_pre | extra_bins |
extra_modules | failure_action | default | final_action | force_rebuild | force_no_rebuild
| fence_kdump_args | fence_kdump_nodes | auto_reset_crashkernel) ;;
> >
> > Personally I prefer the original way, which checks ssh specific
> > configs in check_ssh_config(),
> > and check_config() do the generic checking.
> >
> > >
> > > net | options | link_delay | disk_timeout |
debug_mem_level | blacklist)
> > > derror "Deprecated kdump config option:
$config_opt. Refer to kdump.conf manpage for alternatives."
> > > @@ -241,6 +258,7 @@ check_config()
> > > check_failure_action_config || return 1
> > > check_final_action_config || return 1
> > > check_fence_kdump_config || return 1
> > > + check_ssh_config || return 1
> > >
> > > return 0
> > > }
> > > @@ -664,29 +682,8 @@ check_ssh_config()
> > > {
> > > local target
> > >
> > > - while read -r config_opt config_val; do
> > > - case "$config_opt" in
> > > - sshkey)
> > > - # remove inline comments after the end of a
directive.
> > > - if [[ -f $config_val ]]; then
> > > - # canonicalize the path
> > > - SSH_KEY_LOCATION=$(/usr/bin/readlink -m
"$config_val")
> > > - else
> > > - dwarn "WARNING: '$config_val'
doesn't exist, using default value '$SSH_KEY_LOCATION'"
> > > - fi
> > > - ;;
> > > - path)
> > > - SAVE_PATH=$config_val
> > > - ;;
> > > - ssh)
> > > - DUMP_TARGET=$config_val
> > > - ;;
> > > - *) ;;
> > > -
> > > - esac
> > > - done <<< "$(kdump_read_conf)"
> >
> > If you want to emit the extra /etc/kdump.conf parsing,
> > how about using kdump_get_conf_val() to get values of
> > sshkey, path, ssh, just like how check_failure_action_config()
> > does, so we can keep the ssh specific checking in this function?
>
> omitting the extra parsing is the intermediate goal of this patch. The
> ultimate goal is to introduce the OPT array in the next patch. This
> allows simplifying kdumpctl a lot. Especially it allows to get rid of
> most calls to kdump_get_conf_val in kdumpctl and with it quite a lot of
> file accesses.
>
> Looking even further in the future the goal is to transform mkdumprd
> from a script to a library. It has a similar parser and thus could make
> use of the OPT array as well. I don't have it fully worked out yet,
> though.
Sorry I didn't look through all your patches. BTW, great design for
the OPT array, the config parser and setter, which making the code much
cleaner.
np and thanks.
>
> > > + [[ -n $DUMP_TARGET ]] || return 0
> > >
> > > - [[ -n $DUMP_TARGET ]] || return 1
> > > [[ $DUMP_TARGET =~ .*@.* ]] || return 1
> > > target=$(ssh -G "$DUMP_TARGET" | sed -n -e
"s/^hostname[[:space:]]\+\([^[:space:]]*\).*$/\1/p")
> > > if [[ ${DUMP_TARGET#*@} != "$target" ]]; then
> > > @@ -709,6 +706,8 @@ check_and_wait_network_ready()
> > > local retval
> > > local errmsg
> > >
> > > + [[ -n $DUMP_TARGET ]] || return 0
> > > +
> > > start_time=$(date +%s)
> > > while true; do
> > > errmsg=$(ssh -i "$SSH_KEY_LOCATION" -o
BatchMode=yes "$DUMP_TARGET" mkdir -p "$SAVE_PATH" 2>&1)
> > > @@ -747,16 +746,13 @@ check_and_wait_network_ready()
> > > return 1
> > > }
> > >
> > > -check_ssh_target()
> > > -{
> > > - check_and_wait_network_ready
> > > -}
> > > -
> > > propagate_ssh_key()
> > > {
> > > local SSH_USER SSH_SERVER
> > >
> > > - if ! check_ssh_config; then
> > > + check_config || return 1
> > > +
> >
> > Yes, just like what the commit message says,
> > it will change the behaviour of propagate, if any non-ssh
> > configuration error will fail ssh key propagate. It seems
> > not a good idea to me...
>
> For me the over all simplification was worth the change in behavior.
> Especially failing when encountering unknown or deprecating I think
> would be very helpful. Al least I think it would have prevented this
> bug [1].
>
I see...
> We could separate the parsing and verification steps into two
> functions. I.e. have one function that parses kdump.conf into the OPT
> array (only failing for unknown/deprecated options) and a second one
> that verifies the config options. Then propagate could only check the
> ssh options while the other function call the "check everything"
> function. Would that be ok with you?
>
Yes, thanks for the explanation, I'm OK with your desgin.
BTW, for all the patches:
Reviewed-by: Tao Liu <ltao(a)redhat.com>
Thanks for reviewing!
Philipp
Thanks,
Tao Liu
> Thanks
> Philipp
>
> [1]
https://bugzilla.redhat.com/show_bug.cgi?id=2049153
>
> > Thanks,
> > Tao Liu
> >
> >
> >
> > > + if [[ -z $DUMP_TARGET ]] ; then
> > > derror "No ssh destination defined in
$KDUMP_CONFIG_FILE."
> > > derror "Please verify that $KDUMP_CONFIG_FILE
contains 'ssh <user>@<host>' and that it is properly formatted."
> > > exit 1
> > > @@ -1039,11 +1035,9 @@ start()
> > > return 0
> > > fi
> > >
> > > - if check_ssh_config; then
> > > - if ! check_ssh_target; then
> > > - derror "Starting kdump: [FAILED]"
> > > - return 1
> > > - fi
> > > + if ! check_and_wait_network_ready; then
> > > + derror "Starting kdump: [FAILED]"
> > > + return 1
> > > fi
> > >
> > > if ! check_rebuild; then
> > > @@ -1159,12 +1153,7 @@ stop()
> > > rebuild()
> > > {
> > > check_config || return 1
> > > -
> > > - if check_ssh_config; then
> > > - if ! check_ssh_target; then
> > > - return 1
> > > - fi
> > > - fi
> > > + check_and_wait_network_ready || return 1
> > >
> > > setup_initrd || return 1
> > >
> > > --
> > > 2.34.1
> > > _______________________________________________
> > > kexec mailing list -- kexec(a)lists.fedoraproject.org
> > > To unsubscribe send an email to kexec-leave(a)lists.fedoraproject.org
> > > Fedora Code of Conduct:
https://docs.fedoraproject.org/en-US/project/code-of-conduct/
> > > List Guidelines:
https://fedoraproject.org/wiki/Mailing_list_guidelines
> > > List Archives:
https://lists.fedoraproject.org/archives/list/kexec@lists.fedoraproject.org
> > > Do not reply to spam on the list, report it:
https://pagure.io/fedora-infrastructure
> >
>