On Tue, Oct 10, 2023 at 12:25 AM Philipp Rudo <prudo(a)redhat.com> wrote:
Hi Tao,
as said earlier I don't think this is the right way to go. Here are
some more details why this approach is bad in my opinion.
On Thu, 28 Sep 2023 21:20:26 +0800
Tao Liu <ltao(a)redhat.com> wrote:
> There are some functions which are common for mkdumprd and kdumpctl
> test/test-check. For example, mkdumprd will check the target fs size
> and warn if size is less than total memory. This involves remote fs mount,
> same as test-check which will also mount remote fs to check if the test
> timestamp id exist.
>
> Also, this patch makes modifications on functions such as
> "handle_default_dump_target" "check_user_configured_target",
which receives a
> function as argument, so they can be used for both mkdumprd and kdumpctl
> test/test-check.
>
> Signed-off-by: Tao Liu <ltao(a)redhat.com>
> ---
> kdump-lib.sh | 138 +++++++++++++++++++++++++++++++++++++++++++++++++++
> mkdumprd | 127 ++++-------------------------------------------
> 2 files changed, 149 insertions(+), 116 deletions(-)
>
> diff --git a/kdump-lib.sh b/kdump-lib.sh
> index 042ac87..0eb5f03 100755
> --- a/kdump-lib.sh
> +++ b/kdump-lib.sh
> @@ -1089,3 +1089,141 @@ get_all_kdump_crypt_dev()
> get_luks_crypt_dev "$(kdump_get_maj_min "$_dev")"
> done
> }
> +
> +SAVE_PATH=$(get_save_path)
This is the same like ${OPT[path]}.
> +handle_tmp_dir()
> +{
> + TMPDIR="$(mktemp -d -t mkdumprd.XXXXXX)"
> + [ -d "$TMPDIR" ] || perror_exit "dracut: mktemp -p -d -t
dracut.XXXXXX failed."
> + TMPMNT="$TMPDIR/target"
> +
> + trap '
> + ret=$?;
> + is_mounted $TMPMNT && umount -f $TMPMNT;
> + [[ -d $TMPDIR ]] && rm --one-file-system -rf --
"$TMPDIR";
> + exit $ret;
> + ' EXIT
> +}
This will overwrite the trap handler set up at the beginning of
kdumpctl. Ultimately this will cause $KDUMP_TMPDIR to not be deleted.
Sure, I don't like this part either, will implement it in a better way.
> +check_save_path_fs()
> +{
> + local _path=$1
> +
> + if [[ ! -d $_path ]]; then
> + perror_exit "Dump path $_path does not exist."
> + fi
> +}
> +
> +#handle the case user does not specify the dump target explicitly
> +# $1: function
> +# ${2..}: arguments for the function
> +handle_default_dump_target()
> +{
> + local _target
> + local _mntpoint
> + local _func=$1
> +
> + shift 1
> + is_user_configured_dump_target && return
> +
> + check_save_path_fs "$SAVE_PATH"
> +
> + _save_path=$(get_bind_mount_source "$SAVE_PATH")
> + _target=$(get_target_from_path "$_save_path")
> + _mntpoint=$(get_mntpoint_from_target "$_target")
> +
> + SAVE_PATH=${_save_path##"$_mntpoint"}
> + $_func "$_target" "$_save_path" $@
> +}
> +
> +mount_failure()
> +{
> + local _target=$1
> + local _mnt=$2
> + local _fstype=$3
> + local msg="Failed to mount $_target"
> +
> + if [[ -n $_mnt ]]; then
> + msg="$msg on $_mnt"
> + fi
> +
> + msg="$msg for kdump preflight check."
> +
> + if [[ $_fstype == "nfs" ]]; then
> + msg="$msg Please make sure nfs-utils has been installed."
> + fi
> +
> + perror_exit "$msg"
> +}
> +
> +# $1: fs target
> +# $2: fs type
> +# $3: function
> +# ${4..}: arguments for the function
> +check_user_configured_target()
> +{
> + local _target=$1 _cfg_fs_type=$2 _mounted _func=$3
> + local _mnt _opt _fstype _ret
> +
> + _mnt=$(get_mntpoint_from_target "$_target")
> + _opt=$(get_mntopt_from_target "$_target")
> + _fstype=$(get_fs_type_from_target "$_target")
> +
> + if [[ -n $_fstype ]]; then
> + # In case of nfs4, nfs should be used instead, nfs* options is
deprecated in kdump.conf
> + [[ $_fstype == "nfs"* ]] && _fstype=nfs
> +
> + if [[ -n $_cfg_fs_type ]] && [[ $_fstype !=
"$_cfg_fs_type" ]]; then
> + perror_exit "\"$_target\" have a wrong type
config \"$_cfg_fs_type\", expected \"$_fstype\""
> + fi
> + else
> + _fstype="$_cfg_fs_type"
> + _fstype="$_cfg_fs_type"
Double line.
> + fi
> +
> + # For noauto mount, mount it inplace with default value.
> + # Else use the temporary target directory
> + if [[ -n $_mnt ]]; then
> + if ! is_mounted "$_mnt"; then
> + if [[ $_opt == *",noauto"* ]]; then
> + mount "$_mnt" || mount_failure
"$_target" "$_mnt" "$_fstype"
> + _mounted=$_mnt
> + else
> + perror_exit "Dump target \"$_target\"
is neither mounted nor configured as \"noauto\""
> + fi
> + fi
> + else
> + handle_tmp_dir
> + _mnt=$TMPMNT
> + mkdir -p "$_mnt"
> + mount "$_target" "$_mnt" -t "$_fstype"
-o defaults || mount_failure "$_target" "" "$_fstype"
> + _mounted=$_mnt
> + fi
> +
> + # For user configured target, use $SAVE_PATH as the dump path within the
target
> + if [[ ! -d "$_mnt/$SAVE_PATH" ]]; then
> + perror_exit "Dump path \"$SAVE_PATH\" does not exist in
dump target \"$_target\""
> + fi
> +
> + shift 3
this needs to be at the beginning of the function where you process
its arguments. Otherwise someone will add a new argument and will
forget to update this line as well. Personally I prefer something like
this
local _target _cfg_fs_type _mounted _func
_target=$1; shift
_cfg_fs_type=$1; shift
_func=$1; shift
as that makes it easiest to expand later.
Yeah, that indeed is better, thanks!
> + $_func "$_target" "$_mnt/$SAVE_PATH" $@
> + _ret=$?
> +
> + # Unmount it early, if function is interrupted and didn't reach here, the
shell trap will clear it up anyway
> + if [[ -n $_mounted ]]; then
> + umount -f -- "$_mounted"
> + fi
> +
> + return $_ret
> +}
> +
> +get_ssh_key_location()
> +{
> + SSH_KEY_LOCATION=$DEFAULT_SSHKEY
> + keyfile=$(kdump_get_conf_val sshkey)
> + if [[ -f $keyfile ]]; then
> + # canonicalize the path
> + SSH_KEY_LOCATION=$(/usr/bin/readlink -m "$keyfile")
> + fi
> +}
This is already done in parse_config. You can simply use ${OPT[sshkey]}.
I will check it out, thanks for pointing it out.
Thanks
Philipp