[PATCH 00/15] kdumpctl: various fixes and cleanups

[Philipp Rudo]

Hi,

while looking into transforming mkdumprd and mkfadumprd into library functions
I noticed various nits in kdumpctl. This series addresses them.

The series is made up of two parts:

Patches 1-8 are small independent cleanups and fixes.

Patches 9-15 tries to reduce the number of file accesses to /etc/kdump.conf. It
achieves this by only parsing kdump.conf once in check_config and storing the
parsed values in an array. Later accesses can then simply use the value stored
in the array instead of calling kdump_get_conf_val.

Thanks
Philipp

Philipp Rudo (15):
  kdump-capture.service: switch to journal for stdout
  kdumpctl: source dependencies before defining globals
  kdump-lib: fix typo in variable name
  kdumpctl: remove unnecessary uses of $?
  kdump-lib-initramfs: merge definitions for default ssh key
  kdumpctl: fix comment in check_and_wait_network_ready
  kdumpctl: forbid aliases from ssh config
  kdumpctl: simplify propagate_ssh_key
  kdumpctl: merge check_ssh_config into check_config
  kdumpctl: reduce file operations on kdump.conf
  kdumpctl: drop SAVE_PATH variable
  kdumpctl: drop SSH_KEY_LOCATION variable
  kdumpctl: drop DUMP_TARGET variable
  kdumpctl: remove kdump_get_conf_val in save_raw
  kdumpctl: simplify local_fs_dump_target

dracut-kdump-capture.service |   4 +-
 dracut-kdump.sh              |   2 +-
 kdump-lib-initramfs.sh       |   1 +
 kdump-lib.sh                 |  14 +-
 kdumpctl                     | 258 ++++++++++++++++-------------------
 mkdumprd                     |   2 +-
 6 files changed, 128 insertions(+), 153 deletions(-)

[PATCH 01/15] kdump-capture.service: switch to journal for stdout

[Philipp Rudo]

Using syslog for StandardOutput in a service file was deprecated in
systemd v246 with commit f3dc6af20f ("core: automatically update
StandardOuput=syslog to =journal (and similar for StandardError=)").
Thus the following warnings are printed in the crash kernel when
creating a dump.

systemd[1]: /usr/lib/systemd/system/kdump-capture.service:23: Standard output type syslog is obsolete, automatically updating to journal. Please update your unit file, and consider removing the setting altogether.
systemd[1]: /usr/lib/systemd/system/kdump-capture.service:24: Standard output type syslog+console is obsolete, automatically updating to journal+console. Please update your unit file, and consider removing the setting altogether.

Fix this by redirecting the stdout and stderr to the journal.

Signed-off-by: Philipp Rudo 
---
 dracut-kdump-capture.service | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/dracut-kdump-capture.service b/dracut-kdump-capture.service
index 3f20aba..7109169 100644
--- a/dracut-kdump-capture.service
+++ b/dracut-kdump-capture.service
@@ -20,8 +20,8 @@ Environment=NEWROOT=/sysroot
 Type=oneshot
 ExecStart=/bin/kdump.sh
 StandardInput=null
-StandardOutput=syslog
-StandardError=syslog+console
+StandardOutput=journal
+StandardError=journal+console
 KillMode=process
 RemainAfterExit=yes

[PATCH 02/15] kdumpctl: source dependencies before defining globals

[Philipp Rudo]

Allow the use of default values defined in kdump-lib-initramfs.sh and
kdump-lib.sh for global variables by moving their initialization before
the definition of the variables.

Signed-off-by: Philipp Rudo 
---
 kdumpctl | 33 +++++++++++++++++----------------
 1 file changed, 17 insertions(+), 16 deletions(-)

diff --git a/kdumpctl b/kdumpctl
index 3ccfa97..ed772a0 100755
--- a/kdumpctl
+++ b/kdumpctl
@@ -1,4 +1,20 @@
 #!/bin/bash
+
+if [[ -f /etc/sysconfig/kdump ]]; then
+	. /etc/sysconfig/kdump
+fi
+
+[[ $dracutbasedir ]] || dracutbasedir=/usr/lib/dracut
+. $dracutbasedir/dracut-functions.sh
+. /lib/kdump/kdump-lib.sh
+. /lib/kdump/kdump-logger.sh
+
+#initiate the kdump logger
+if ! dlog_init; then
+	echo "failed to initiate the kdump logger."
+	exit 1
+fi
+
 KEXEC=/sbin/kexec

KDUMP_KERNELVER=""
@@ -9,7 +25,7 @@ KDUMP_LOG_PATH="/var/log"
 MKDUMPRD="/sbin/mkdumprd -f"
 MKFADUMPRD="/sbin/mkfadumprd"
 DRACUT_MODULES_FILE="/usr/lib/dracut/modules.txt"
-SAVE_PATH=/var/crash
+SAVE_PATH=$DEFAULT_PATH
 SSH_KEY_LOCATION="/root/.ssh/kdump_id_rsa"
 INITRD_CHECKSUM_LOCATION="/boot/.fadump_initrd_checksum"
 DUMP_TARGET=""
@@ -27,21 +43,6 @@ standard_kexec_args="-d -p"
 # Some default values in case /etc/sysconfig/kdump doesn't include
 KDUMP_COMMANDLINE_REMOVE="hugepages hugepagesz slub_debug"

-if [[ -f /etc/sysconfig/kdump ]]; then
-	. /etc/sysconfig/kdump
-fi
-
-[[ $dracutbasedir ]] || dracutbasedir=/usr/lib/dracut
-. $dracutbasedir/dracut-functions.sh
-. /lib/kdump/kdump-lib.sh
-. /lib/kdump/kdump-logger.sh
-
-#initiate the kdump logger
-if ! dlog_init; then
-	echo "failed to initiate the kdump logger."
-	exit 1
-fi
-
 single_instance_lock()
 {
    local rc timeout=5

[PATCH 02/15] kdumpctl: source dependencies before defining globals

[Coiby Xu]

On Fri, Feb 04, 2022 at 06:34:16PM +0100, Philipp Rudo wrote:
> Allow the use of default values defined in kdump-lib-initramfs.sh and
> kdump-lib.sh for global variables by moving their initialization before
> the definition of the variables.
> Signed-off-by: Philipp Rudo
>
> kdumpctl | 33 +++++++++++++++++----------------
> 1 file changed, 17 insertions(+), 16 deletions(-)
>
> diff --git a/kdumpctl b/kdumpctl
> index 3ccfa97..ed772a0 100755
> --- a/kdumpctl
> +++ b/kdumpctl
> @@ -1,4 +1,20 @@
> #!/bin/bash
>
> +if [[ -f /etc/sysconfig/kdump ]]; then
> . /etc/sysconfig/kdump
>
> +fi
>
> +[[ $dracutbasedir ]] || dracutbasedir=/usr/lib/dracut
> +. $dracutbasedir/dracut-functions.sh
> +. /lib/kdump/kdump-lib.sh
> +. /lib/kdump/kdump-logger.sh
>
> +#initiate the kdump logger
> +if ! dlog_init; then
> echo "failed to initiate the kdump logger."
> exit 1
>
> +fi
>
> KEXEC=/sbin/kexec
>
> KDUMP_KERNELVER=""
> @@ -9,7 +25,7 @@ KDUMP_LOG_PATH="/var/log"
> MKDUMPRD="/sbin/mkdumprd -f"
> MKFADUMPRD="/sbin/mkfadumprd"
> DRACUT_MODULES_FILE="/usr/lib/dracut/modules.txt"
> -SAVE_PATH=/var/crash
> +SAVE_PATH=$DEFAULT_PATH
> SSH_KEY_LOCATION="/root/.ssh/kdump_id_rsa"
> INITRD_CHECKSUM_LOCATION="/boot/.fadump_initrd_checksum"
> DUMP_TARGET=""
> @@ -27,21 +43,6 @@ standard_kexec_args="-d -p"
> # Some default values in case /etc/sysconfig/kdump doesn't include
> KDUMP_COMMANDLINE_REMOVE="hugepages hugepagesz slub_debug"

One issue with sourcing /etc/sysconfig/kdump early is configurations like
KDUMP_KERNELVER, KDUMP_COMMANDLINE_REMOVE and etc. could be overwritten.
Maybe we simply move "SAVE_PATH=$DEFAULT_PATH" instead?
> -if [[ -f /etc/sysconfig/kdump ]]; then
> . /etc/sysconfig/kdump
>
> -fi
>
> -[[ $dracutbasedir ]] || dracutbasedir=/usr/lib/dracut
> -. $dracutbasedir/dracut-functions.sh
> -. /lib/kdump/kdump-lib.sh
> -. /lib/kdump/kdump-logger.sh
>
> -#initiate the kdump logger
> -if ! dlog_init; then
> echo "failed to initiate the kdump logger."
> exit 1
>
> -fi
>
> single_instance_lock()
> {
>    local rc timeout=5
> -- 
> 2.34.1
> _______________________________________________
> kexec mailing list -- [e-mail]
> To unsubscribe send an email to [e-mail]
> 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/[e-mail]
> Do not reply to spam on the list, report it: https://pagure.io/fedora-infrastructure

[PATCH 02/15] kdumpctl: source dependencies before defining globals

[Philipp Rudo]

Hi Coiby,

On Tue, 22 Feb 2022 14:44:51 +0800
Coiby Xu  wrote:
> On Fri, Feb 04, 2022 at 06:34:16PM +0100, Philipp Rudo wrote:
> > Allow the use of default values defined in kdump-lib-initramfs.sh and
> > kdump-lib.sh for global variables by moving their initialization before
> > the definition of the variables.
> > Signed-off-by: Philipp Rudo
> >
> > kdumpctl | 33 +++++++++++++++++----------------
> > 1 file changed, 17 insertions(+), 16 deletions(-)
> >
> > diff --git a/kdumpctl b/kdumpctl
> > index 3ccfa97..ed772a0 100755
> > --- a/kdumpctl
> > +++ b/kdumpctl
> > @@ -1,4 +1,20 @@
> > #!/bin/bash
> >
> > +if [[ -f /etc/sysconfig/kdump ]]; then
> > . /etc/sysconfig/kdump
> >
> > +fi
> >
> > +[[ $dracutbasedir ]] || dracutbasedir=/usr/lib/dracut
> > +. $dracutbasedir/dracut-functions.sh
> > +. /lib/kdump/kdump-lib.sh
> > +. /lib/kdump/kdump-logger.sh
> >
> > +#initiate the kdump logger
> > +if ! dlog_init; then
> > echo "failed to initiate the kdump logger."
> > exit 1
> >
> > +fi
> >
> > KEXEC=/sbin/kexec
> >
> > KDUMP_KERNELVER=""
> > @@ -9,7 +25,7 @@ KDUMP_LOG_PATH="/var/log"
> > MKDUMPRD="/sbin/mkdumprd -f"
> > MKFADUMPRD="/sbin/mkfadumprd"
> > DRACUT_MODULES_FILE="/usr/lib/dracut/modules.txt"
> > -SAVE_PATH=/var/crash
> > +SAVE_PATH=$DEFAULT_PATH
> > SSH_KEY_LOCATION="/root/.ssh/kdump_id_rsa"
> > INITRD_CHECKSUM_LOCATION="/boot/.fadump_initrd_checksum"
> > DUMP_TARGET=""
> > @@ -27,21 +43,6 @@ standard_kexec_args="-d -p"
> > # Some default values in case /etc/sysconfig/kdump doesn't include
> > KDUMP_COMMANDLINE_REMOVE="hugepages hugepagesz slub_debug"
>
> One issue with sourcing /etc/sysconfig/kdump early is configurations like
> KDUMP_KERNELVER, KDUMP_COMMANDLINE_REMOVE and etc. could be overwritten.
> Maybe we simply move "SAVE_PATH=$DEFAULT_PATH" instead?

you are right. With this patch the values in /etc/sysconfig/kdump get
overwritten again...

I took an other look and I think I will drop this patch. The only two
variables that benefit from this change are SAVE_PATH and
SSH_KEY_LOCATION (after patch 5). But both are dropped in later
patches. So there is no real benefit in keeping this patch. Especially
when it breaks stuff...

Thanks
Philipp
> > -if [[ -f /etc/sysconfig/kdump ]]; then
> > . /etc/sysconfig/kdump
> >
> > -fi
> >
> > -[[ $dracutbasedir ]] || dracutbasedir=/usr/lib/dracut
> > -. $dracutbasedir/dracut-functions.sh
> > -. /lib/kdump/kdump-lib.sh
> > -. /lib/kdump/kdump-logger.sh
> >
> > -#initiate the kdump logger
> > -if ! dlog_init; then
> > echo "failed to initiate the kdump logger."
> > exit 1
> >
> > -fi
> >
> > single_instance_lock()
> > {
> >    local rc timeout=5
> > -- 
> > 2.34.1
> > _______________________________________________
> > kexec mailing list -- [e-mail]
> > To unsubscribe send an email to [e-mail]
> > 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/[e-mail]
> > Do not reply to spam on the list, report it: https://pagure.io/fedora-infrastructure

[PATCH 03/15] kdump-lib: fix typo in variable name

[Philipp Rudo]

in prepare_kdump_bootinfo s/defaut/default/.

While at it declare it with the other local variables as local.

Signed-off-by: Philipp Rudo 
---
 kdump-lib.sh | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/kdump-lib.sh b/kdump-lib.sh
index 3e912cc..27d142a 100755
--- a/kdump-lib.sh
+++ b/kdump-lib.sh
@@ -640,7 +640,7 @@ prepare_kexec_args()
 prepare_kdump_bootinfo()
 {
    local boot_img boot_imglist boot_dirlist boot_initrdlist
-	local machine_id
+	local machine_id dir img default_initrd_base var_target_initrd_dir

if [[ -z $KDUMP_KERNELVER ]]; then
    	KDUMP_KERNELVER="$(uname -r)"
@@ -677,8 +677,8 @@ prepare_kdump_bootinfo()
    boot_initrdlist="initramfs-$KDUMP_KERNELVER.img initrd"
    for initrd in $boot_initrdlist; do
    	if [[ -f "$KDUMP_BOOTDIR/$initrd" ]]; then
-			defaut_initrd_base="$initrd"
-			DEFAULT_INITRD="$KDUMP_BOOTDIR/$defaut_initrd_base"
+			default_initrd_base="$initrd"
+			DEFAULT_INITRD="$KDUMP_BOOTDIR/$default_initrd_base"
    		break
    	fi
    done
@@ -686,12 +686,12 @@ prepare_kdump_bootinfo()
    # Create kdump initrd basename from default initrd basename
    # initramfs-5.7.9-200.fc32.x86_64.img => initramfs-5.7.9-200.fc32.x86_64kdump.img
    # initrd => initrdkdump
-	if [[ -z $defaut_initrd_base ]]; then
+	if [[ -z $default_initrd_base ]]; then
    	kdump_initrd_base=initramfs-${KDUMP_KERNELVER}kdump.img
-	elif [[ $defaut_initrd_base == *.* ]]; then
-		kdump_initrd_base=${defaut_initrd_base%.*}kdump.${DEFAULT_INITRD##*.}
+	elif [[ $default_initrd_base == *.* ]]; then
+		kdump_initrd_base=${default_initrd_base%.*}kdump.${DEFAULT_INITRD##*.}
    else
-		kdump_initrd_base=${defaut_initrd_base}kdump
+		kdump_initrd_base=${default_initrd_base}kdump
    fi

# Place kdump initrd in $(/var/lib/kdump) if $(KDUMP_BOOTDIR) not writable

[PATCH 04/15] kdumpctl: remove unnecessary uses of $?

[Philipp Rudo]

Signed-off-by: Philipp Rudo 
---
 kdumpctl | 16 ++--------------
 1 file changed, 2 insertions(+), 14 deletions(-)

diff --git a/kdumpctl b/kdumpctl
index ed772a0..d08b91c 100755
--- a/kdumpctl
+++ b/kdumpctl
@@ -86,7 +86,6 @@ rebuild_fadump_initrd()
 check_earlykdump_is_enabled()
 {
    grep -q -w "rd.earlykdump" /proc/cmdline
-	return $?
 }

rebuild_kdump_initrd()
@@ -116,8 +115,6 @@ rebuild_initrd()
    else
    	rebuild_kdump_initrd
    fi
-
-	return $?
 }

#$1: the files to be checked with IFS=' '
@@ -583,7 +580,6 @@ check_rebuild()

dinfo "Rebuilding $TARGET_INITRD"
    rebuild_initrd
-	return $?
 }

# On ppc64le LPARs, the keys trusted by firmware do not end up in
@@ -814,8 +810,6 @@ check_current_status()
    else
    	check_current_kdump_status
    fi
-
-	return $?
 }

save_raw()
@@ -944,7 +938,6 @@ check_dump_feasibility()
    fi

check_kdump_feasibility
-	return $?
 }

start_fadump()
@@ -966,8 +959,6 @@ start_dump()
    else
    	load_kdump
    fi
-
-	return $?
 }

check_failure_action_config()
@@ -1076,7 +1067,7 @@ reload()

if [[ $DEFAULT_DUMP_MODE == "fadump" ]]; then
    	reload_fadump
-		return $?
+		return
    else
    	if ! stop_kdump; then
    		derror "Stopping kdump: [FAILED]"
@@ -1140,7 +1131,7 @@ reload_fadump()
    	# to handle such scenario.
    	if stop_fadump; then
    		start_fadump
-			return $?
+			return
    	fi
    fi

@@ -1179,7 +1170,6 @@ rebuild()

dinfo "Rebuilding $TARGET_INITRD"
    rebuild_initrd
-	return $?
 }

do_estimate()
@@ -1711,5 +1701,3 @@ single_instance_lock
    exec 9<&-
    main "$@"
 )
-
-exit $?

[PATCH 04/15] kdumpctl: remove unnecessary uses of $?

[Coiby Xu]

On Fri, Feb 04, 2022 at 06:34:18PM +0100, Philipp Rudo wrote:
> Signed-off-by: Philipp Rudo
>
> kdumpctl | 16 ++--------------
> 1 file changed, 2 insertions(+), 14 deletions(-)
>
> diff --git a/kdumpctl b/kdumpctl
> index ed772a0..d08b91c 100755
> --- a/kdumpctl
> +++ b/kdumpctl
> @@ -86,7 +86,6 @@ rebuild_fadump_initrd()
> check_earlykdump_is_enabled()
> {
>    grep -q -w "rd.earlykdump" /proc/cmdline
> return $?
>
> }
>
> rebuild_kdump_initrd()
> @@ -116,8 +115,6 @@ rebuild_initrd()
>    else
>    	rebuild_kdump_initrd
>    fi
> return $?
>
> }
>
> #$1: the files to be checked with IFS=' '
> @@ -583,7 +580,6 @@ check_rebuild()
>
> dinfo "Rebuilding $TARGET_INITRD"
>    rebuild_initrd
> return $?
>
> }
>
> # On ppc64le LPARs, the keys trusted by firmware do not end up in
> @@ -814,8 +810,6 @@ check_current_status()
>    else
>    	check_current_kdump_status
>    fi
> return $?
>
> }
>
> save_raw()
> @@ -944,7 +938,6 @@ check_dump_feasibility()
>    fi
>
> check_kdump_feasibility
> return $?
>
> }
>
> start_fadump()
> @@ -966,8 +959,6 @@ start_dump()
>    else
>    	load_kdump
>    fi
> return $?
>
> }
>
> check_failure_action_config()
> @@ -1076,7 +1067,7 @@ reload()
>
> if [[ $DEFAULT_DUMP_MODE == "fadump" ]]; then
>    	reload_fadump
> return $?
> return
> else
> if ! stop_kdump; then
> 	derror "Stopping kdump: [FAILED]"
>
> @@ -1140,7 +1131,7 @@ reload_fadump()
>    	# to handle such scenario.
>    	if stop_fadump; then
>    		start_fadump
> 	return $?
> 	return

I'm curious for start_fadump and reload_fadump both of which may return 1,
why only return normal status?
> fi
>
> fi
>
> @@ -1179,7 +1170,6 @@ rebuild()
>
> dinfo "Rebuilding $TARGET_INITRD"
>    rebuild_initrd
> return $?
>
> }
>
> do_estimate()
> @@ -1711,5 +1701,3 @@ single_instance_lock
>    exec 9<&-
>    main "$@"
> )
> -exit $?
>
> 2.34.1
> _______________________________________________
> kexec mailing list -- [e-mail]
> To unsubscribe send an email to [e-mail]
> 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/[e-mail]
> Do not reply to spam on the list, report it: https://pagure.io/fedora-infrastructure

[PATCH 04/15] kdumpctl: remove unnecessary uses of $?

[Philipp Rudo]

Hi Coiby,

On Tue, 22 Feb 2022 15:07:12 +0800
Coiby Xu  wrote:
> On Fri, Feb 04, 2022 at 06:34:18PM +0100, Philipp Rudo wrote:
> > Signed-off-by: Philipp Rudo
> >
> > kdumpctl | 16 ++--------------
> > 1 file changed, 2 insertions(+), 14 deletions(-)
> >
> > diff --git a/kdumpctl b/kdumpctl
> > index ed772a0..d08b91c 100755
> > --- a/kdumpctl
> > +++ b/kdumpctl
> > @@ -86,7 +86,6 @@ rebuild_fadump_initrd()
> > check_earlykdump_is_enabled()
> > {
> >    grep -q -w "rd.earlykdump" /proc/cmdline
> > return $?
> >
> > }
> >
> > rebuild_kdump_initrd()
> > @@ -116,8 +115,6 @@ rebuild_initrd()
> >    else
> >    	rebuild_kdump_initrd
> >    fi
> > return $?
> >
> > }
> >
> > #$1: the files to be checked with IFS=' '
> > @@ -583,7 +580,6 @@ check_rebuild()
> >
> > dinfo "Rebuilding $TARGET_INITRD"
> >    rebuild_initrd
> > return $?
> >
> > }
> >
> > # On ppc64le LPARs, the keys trusted by firmware do not end up in
> > @@ -814,8 +810,6 @@ check_current_status()
> >    else
> >    	check_current_kdump_status
> >    fi
> > return $?
> >
> > }
> >
> > save_raw()
> > @@ -944,7 +938,6 @@ check_dump_feasibility()
> >    fi
> >
> > check_kdump_feasibility
> > return $?
> >
> > }
> >
> > start_fadump()
> > @@ -966,8 +959,6 @@ start_dump()
> >    else
> >    	load_kdump
> >    fi
> > return $?
> >
> > }
> >
> > check_failure_action_config()
> > @@ -1076,7 +1067,7 @@ reload()
> >
> > if [[ $DEFAULT_DUMP_MODE == "fadump" ]]; then
> >    	reload_fadump
> > return $?
> > return
> > else
> > if ! stop_kdump; then
> > 	derror "Stopping kdump: [FAILED]"
> >
> > @@ -1140,7 +1131,7 @@ reload_fadump()
> >    	# to handle such scenario.
> >    	if stop_fadump; then
> >    		start_fadump
> > 	return $?
> > 	return  
>
> I'm curious for start_fadump and reload_fadump both of which may return 1,
> why only return normal status?

There is no "normal status" you can return. When you omit the return
value 'return' will automatically return the status of the last
executed command. Thus 'return' and 'return $?' are identical.

Same for 'exit'

Thanks
Philipp
> > fi
> >
> > fi
> >
> > @@ -1179,7 +1170,6 @@ rebuild()
> >
> > dinfo "Rebuilding $TARGET_INITRD"
> >    rebuild_initrd
> > return $?
> >
> > }
> >
> > do_estimate()
> > @@ -1711,5 +1701,3 @@ single_instance_lock
> >    exec 9<&-
> >    main "$@"
> > )
> > -exit $?
> >
> > 2.34.1
> > _______________________________________________
> > kexec mailing list -- [e-mail]
> > To unsubscribe send an email to [e-mail]
> > 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/[e-mail]
> > Do not reply to spam on the list, report it: https://pagure.io/fedora-infrastructure

[PATCH 04/15] kdumpctl: remove unnecessary uses of $?

[Coiby Xu]

Hi Philipp,

On Tue, Feb 22, 2022 at 11:22:20AM +0100, Philipp Rudo wrote:
> Hi Coiby,
>
> On Tue, 22 Feb 2022 15:07:12 +0800
> Coiby Xu  wrote:

[...]
> > > start_fadump()
> > > @@ -966,8 +959,6 @@ start_dump()
> > >    else
> > >    	load_kdump
> > >    fi
> > > return $?
> > >
> > > }
> > >
> > > check_failure_action_config()
> > > @@ -1076,7 +1067,7 @@ reload()
> > >
> > > if [[ $DEFAULT_DUMP_MODE == "fadump" ]]; then
> > >    	reload_fadump
> > > return $?
> > > return
> > > else
> > > if ! stop_kdump; then
> > > 	derror "Stopping kdump: [FAILED]"
> > >
> > > @@ -1140,7 +1131,7 @@ reload_fadump()
> > >    	# to handle such scenario.
> > >    	if stop_fadump; then
> > >    		start_fadump
> > > 	return $?
> > > 	return
> >
> > I'm curious for start_fadump and reload_fadump both of which may return 1,
> > why only return normal status?
>
> There is no "normal status" you can return. When you omit the return
> value 'return' will automatically return the status of the last
> executed command. Thus 'return' and 'return $?' are identical.

Thanks for correcting my misunderstanding! But why not simply removing
"return $?" instead?
> Same for 'exit'
>
> Thanks
> Philipp
> > > fi
> > >
> > > fi
> > >
> > > @@ -1179,7 +1170,6 @@ rebuild()
> > >
> > > dinfo "Rebuilding $TARGET_INITRD"
> > >    rebuild_initrd
> > > return $?
> > >
> > > }
> > >
> > > do_estimate()
> > > @@ -1711,5 +1701,3 @@ single_instance_lock
> > >    exec 9<&-
> > >    main "$@"
> > > )
> > > -exit $?
> > >
> > > 2.34.1
> > > _______________________________________________
> > > kexec mailing list -- [e-mail]
> > > To unsubscribe send an email to [e-mail]
> > > 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/[e-mail]
> > > Do not reply to spam on the list, report it: https://pagure.io/fedora-infrastructure

[PATCH 04/15] kdumpctl: remove unnecessary uses of $?

[Philipp Rudo]

Hi Coiby,

On Wed, 23 Feb 2022 08:45:15 +0800
Coiby Xu  wrote:
> Hi Philipp,
>
> On Tue, Feb 22, 2022 at 11:22:20AM +0100, Philipp Rudo wrote:
> > Hi Coiby,
> >
> > On Tue, 22 Feb 2022 15:07:12 +0800
> > Coiby Xu  wrote:
>
> [...]
> > > > start_fadump()
> > > > @@ -966,8 +959,6 @@ start_dump()
> > > >    else
> > > >    	load_kdump
> > > >    fi
> > > > return $?
> > > >
> > > > }
> > > >
> > > > check_failure_action_config()
> > > > @@ -1076,7 +1067,7 @@ reload()
> > > >
> > > > if [[ $DEFAULT_DUMP_MODE == "fadump" ]]; then
> > > >    	reload_fadump
> > > > return $?
> > > > return
> > > > else
> > > > if ! stop_kdump; then
> > > > 	derror "Stopping kdump: [FAILED]"
> > > >
> > > > @@ -1140,7 +1131,7 @@ reload_fadump()
> > > >    	# to handle such scenario.
> > > >    	if stop_fadump; then
> > > >    		start_fadump
> > > > 	return $?
> > > > 	return  
> > >
> > > I'm curious for start_fadump and reload_fadump both of which may return 1,
> > > why only return normal status?
> >
> > There is no "normal status" you can return. When you omit the return
> > value 'return' will automatically return the status of the last
> > executed command. Thus 'return' and 'return $?' are identical.
>
> Thanks for correcting my misunderstanding! But why not simply removing
> "return $?" instead?

I don't fully understand your question. Do you mean doing something like
this?

diff --git a/kdumpctl b/kdumpctl
index 630e56f..24c266f 100755
--- a/kdumpctl
+++ b/kdumpctl
@@ -1121,11 +1121,10 @@ reload_fadump()
                # to handle such scenario.
                if stop_fadump; then
                        start_fadump
-                       return
                fi
        fi

-       return 1
+       return
 }

stop()

Unfortunately that won't work as expected. The problem is that in bash
control statements like 'if' or 'for' do have a return value. This is
usually the return value of the last command executed _or_ 0 if, e.g. no
test condition is true for 'if' (for a full list see 'Compound Commands'
section in the bash man page). Thus the above will change the return
value in the case that stop_fadump fails.

You can test that behavior with this snippet

if false; then
    false
fi && echo "return value is 0" || echo "return value is 1"

which will print "return value is 0". However, when you update
'if false;' to 'if true;' then it will print "return value is 1".

Thanks
Philipp

[PATCH 04/15] kdumpctl: remove unnecessary uses of $?

[Coiby Xu]

On Thu, Feb 24, 2022 at 05:49:33PM +0100, Philipp Rudo wrote:
> Hi Coiby,
> > Thanks for correcting my misunderstanding! But why not simply removing
> > "return $?" instead?
>
> I don't fully understand your question. Do you mean doing something like
> this?

Sorry for not being clear enough.
> diff --git a/kdumpctl b/kdumpctl
> index 630e56f..24c266f 100755
> --- a/kdumpctl
> +++ b/kdumpctl
> @@ -1121,11 +1121,10 @@ reload_fadump()
>                # to handle such scenario.
>                if stop_fadump; then
>                        start_fadump
>                   return

Although I only suggested removing this return originally (which obvious
is incorrect if I read the remaining code in reload_fadump),
>            fi
>    fi
>
>   return 1
>   return

actually it works with this return also removed.
> }
>
> stop()
>
> Unfortunately that won't work as expected.
> The problem is that in bash
> control statements like 'if' or 'for' do have a return value. This is
> usually the return value of the last command executed _or_ 0 if, e.g. no
> test condition is true for 'if' (for a full list see 'Compound Commands'
> section in the bash man page). Thus the above will change the return
> value in the case that stop_fadump fails.
>
> You can test that behavior with this snippet
>
> if false; then
>    false
> fi && echo "return value is 0" || echo "return value is 1"
>
> which will print "return value is 0". However, when you update
> 'if false;' to 'if true;' then it will print "return value is 1".

If we modify your example to the same if/else/fi structure as
reload_fadump,

if false; then
    false
else
     true
fi && echo "return value is 0" || echo "return value is 1"

this will print "return value is 0". And changing the only true to
false will print "return value is 1".
> Thanks
> Philipp

[PATCH 04/15] kdumpctl: remove unnecessary uses of $?

[Philipp Rudo]

Hi Coiby,

On Fri, 25 Feb 2022 14:45:27 +0800
Coiby Xu  wrote:
> On Thu, Feb 24, 2022 at 05:49:33PM +0100, Philipp Rudo wrote:
> > Hi Coiby,
> > > Thanks for correcting my misunderstanding! But why not simply removing
> > > "return $?" instead?
> >
> > I don't fully understand your question. Do you mean doing something like
> > this?
>
> Sorry for not being clear enough.
> > diff --git a/kdumpctl b/kdumpctl
> > index 630e56f..24c266f 100755
> > --- a/kdumpctl
> > +++ b/kdumpctl
> > @@ -1121,11 +1121,10 @@ reload_fadump()
> >                # to handle such scenario.
> >                if stop_fadump; then
> >                        start_fadump
> >                   return  
>
> Although I only suggested removing this return originally (which obvious
> is incorrect if I read the remaining code in reload_fadump),
> >            fi
> >    fi
> >
> >   return 1
> >   return  
>
> actually it works with this return also removed.

hmm... you are right that this 'return' is redundant as functions, like
'if' statements, automatically return the value of the last command
executed when they reach the end. However, dropping it still changes the
behavior compared to the original version.
> > }
> >
> > stop()
> >
> > Unfortunately that won't work as expected.
> > The problem is that in bash
> > control statements like 'if' or 'for' do have a return value. This is
> > usually the return value of the last command executed _or_ 0 if, e.g. no
> > test condition is true for 'if' (for a full list see 'Compound Commands'
> > section in the bash man page). Thus the above will change the return
> > value in the case that stop_fadump fails.
> >
> > You can test that behavior with this snippet
> >
> > if false; then
> >    false
> > fi && echo "return value is 0" || echo "return value is 1"
> >
> > which will print "return value is 0". However, when you update
> > 'if false;' to 'if true;' then it will print "return value is 1".
>
> If we modify your example to the same if/else/fi structure as
> reload_fadump,
>
> if false; then
>     false
> else
>      true
> fi && echo "return value is 0" || echo "return value is 1"
>
> this will print "return value is 0". And changing the only true to
> false will print "return value is 1".

That's absolutely right. But you're still missing that the original
code returned 'false' in that case. So dropping the return will change
the return value of the function for that case.

Let's look at  the full function. For the problematic case the original
code looked like this

f() {
    if false; then
    	echo "outer if"
    	return
    else
    	if false; then
    		echo "inner if"
    		return
    	fi
    fi
    return 1
}

For this case the return value of the function will always be 1 (aka.
false).
If we changed the last line of the function to a simple 'return' (or
drop the line altogether), then the return value will be determined by
the outer 'if' statement. For that case it will be the return value of
the last command executed in the 'else' block. But the 'else' block
only has one command which is the inner 'if' statement. For the inner
'if' none of the conditions are true so it returns 0 (aka. true). So
the return value of the function changed from 1 to 0.

Thanks
Philipp

[PATCH 04/15] kdumpctl: remove unnecessary uses of $?

[Coiby Xu]

Hi Philipp,

On Mon, Feb 28, 2022 at 04:24:33PM +0100, Philipp Rudo wrote:
> Hi Coiby,
>
> On Fri, 25 Feb 2022 14:45:27 +0800
> Coiby Xu  wrote:
> > On Thu, Feb 24, 2022 at 05:49:33PM +0100, Philipp Rudo wrote:
> > > Hi Coiby,
> > > > Thanks for correcting my misunderstanding! But why not simply removing
> > > > "return $?" instead?
> > >
> > > I don't fully understand your question. Do you mean doing something like
> > > this?
> >
> > Sorry for not being clear enough.
> > > diff --git a/kdumpctl b/kdumpctl
> > > index 630e56f..24c266f 100755
> > > --- a/kdumpctl
> > > +++ b/kdumpctl
> > > @@ -1121,11 +1121,10 @@ reload_fadump()
> > >                # to handle such scenario.
> > >                if stop_fadump; then
> > >                        start_fadump
> > >                   return
> >
> > Although I only suggested removing this return originally (which obvious
> > is incorrect if I read the remaining code in reload_fadump),
> > >            fi
> > >    fi
> > >
> > >   return 1
> > >   return
> >
> > actually it works with this return also removed.
>
> hmm... you are right that this 'return' is redundant as functions, like
> 'if' statements, automatically return the value of the last command
> executed when they reach the end. However, dropping it still changes the
> behavior compared to the original version.
> > > }
> > >
> > > stop()
> > >
> > > Unfortunately that won't work as expected.
> > > The problem is that in bash
> > > control statements like 'if' or 'for' do have a return value. This is
> > > usually the return value of the last command executed _or_ 0 if, e.g. no
> > > test condition is true for 'if' (for a full list see 'Compound Commands'
> > > section in the bash man page). Thus the above will change the return
> > > value in the case that stop_fadump fails.
> > >
> > > You can test that behavior with this snippet
> > >
> > > if false; then
> > >    false
> > > fi && echo "return value is 0" || echo "return value is 1"
> > >
> > > which will print "return value is 0". However, when you update
> > > 'if false;' to 'if true;' then it will print "return value is 1".
> >
> > If we modify your example to the same if/else/fi structure as
> > reload_fadump,
> >
> > if false; then
> >     false
> > else
> >      true
> > fi && echo "return value is 0" || echo "return value is 1"
> >
> > this will print "return value is 0". And changing the only true to
> > false will print "return value is 1".
>
> That's absolutely right. But you're still missing that the original
> code returned 'false' in that case. So dropping the return will change
> the return value of the function for that case.
>
> Let's look at  the full function. For the problematic case the original
> code looked like this
>
> f() {
>    if false; then
>    	echo "outer if"
>    	return
>    else
>    	if false; then
>    		echo "inner if"
>    		return
>    	fi
>    fi
>    return 1
> }
>
> For this case the return value of the function will always be 1 (aka.
> false).
> If we changed the last line of the function to a simple 'return' (or
> drop the line altogether), then the return value will be determined by
> the outer 'if' statement. For that case it will be the return value of
> the last command executed in the 'else' block. But the 'else' block
> only has one command which is the inner 'if' statement. For the inner
> 'if' none of the conditions are true so it returns 0 (aka. true). So
> the return value of the function changed from 1 to 0.

Thanks for explaining this case to me!
> Thanks
> Philipp

[PATCH 05/15] kdump-lib-initramfs: merge definitions for default ssh key

[Philipp Rudo]

There are currently three identical definitions for the default ssh key.
Combine them into one in kdump-lib-initramfs.sh.

Signed-off-by: Philipp Rudo 
---
 dracut-kdump.sh        | 2 +-
 kdump-lib-initramfs.sh | 1 +
 kdumpctl               | 2 +-
 mkdumprd               | 2 +-
 4 files changed, 4 insertions(+), 3 deletions(-)

diff --git a/dracut-kdump.sh b/dracut-kdump.sh
index b69bc98..b17455a 100755
--- a/dracut-kdump.sh
+++ b/dracut-kdump.sh
@@ -22,7 +22,7 @@ FAILURE_ACTION="systemctl reboot -f"
 DATEDIR=$(date +%Y-%m-%d-%T)
 HOST_IP='127.0.0.1'
 DUMP_INSTRUCTION=""
-SSH_KEY_LOCATION="/root/.ssh/kdump_id_rsa"
+SSH_KEY_LOCATION=$DEFAULT_SSHKEY
 DD_BLKSIZE=512
 FINAL_ACTION="systemctl reboot -f"
 KDUMP_PRE=""
diff --git a/kdump-lib-initramfs.sh b/kdump-lib-initramfs.sh
index 9be0fe9..84e6bf7 100755
--- a/kdump-lib-initramfs.sh
+++ b/kdump-lib-initramfs.sh
@@ -4,6 +4,7 @@
 # not be the default shell. Any code added must be POSIX compliant.

DEFAULT_PATH="/var/crash/"
+DEFAULT_SSHKEY="/root/.ssh/kdump_id_rsa"
 KDUMP_CONFIG_FILE="/etc/kdump.conf"
 FENCE_KDUMP_CONFIG_FILE="/etc/sysconfig/fence_kdump"
 FENCE_KDUMP_SEND="/usr/libexec/fence_kdump_send"
diff --git a/kdumpctl b/kdumpctl
index d08b91c..5d23f62 100755
--- a/kdumpctl
+++ b/kdumpctl
@@ -26,7 +26,7 @@ MKDUMPRD="/sbin/mkdumprd -f"
 MKFADUMPRD="/sbin/mkfadumprd"
 DRACUT_MODULES_FILE="/usr/lib/dracut/modules.txt"
 SAVE_PATH=$DEFAULT_PATH
-SSH_KEY_LOCATION="/root/.ssh/kdump_id_rsa"
+SSH_KEY_LOCATION=$DEFAULT_SSHKEY
 INITRD_CHECKSUM_LOCATION="/boot/.fadump_initrd_checksum"
 DUMP_TARGET=""
 DEFAULT_INITRD=""
diff --git a/mkdumprd b/mkdumprd
index 593ec77..cdff04f 100644
--- a/mkdumprd
+++ b/mkdumprd
@@ -22,7 +22,7 @@ if ! dlog_init; then
    exit 1
 fi

-SSH_KEY_LOCATION="/root/.ssh/kdump_id_rsa"
+SSH_KEY_LOCATION=$DEFAULT_SSHKEY
 SAVE_PATH=$(get_save_path)
 OVERRIDE_RESETTABLE=0

[PATCH 06/15] kdumpctl: fix comment in check_and_wait_network_ready

[Philipp Rudo]

The time out was increased to 180 seconds in 680c0d3 ("kdumpctl:
distinguish the failed reason of ssh"). Update the comment to reflect
that change.

Signed-off-by: Philipp Rudo 
---
 kdumpctl | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/kdumpctl b/kdumpctl
index 5d23f62..432186a 100755
--- a/kdumpctl
+++ b/kdumpctl
@@ -733,7 +733,7 @@ check_and_wait_network_ready()

cur=$(date +%s)
    	diff=$((cur - start_time))
-		# 60s time out
+		# time out after 180s
    	if [[ $diff -gt 180 ]]; then
    		break
    	fi

[PATCH 07/15] kdumpctl: forbid aliases from ssh config

[Philipp Rudo]

For ssh targets kdumpctl only verifies that the config value has the
correct <user>@<host> format itself. For all other tests, e.g. if the
destination can be reached, it relies on ssh. This allows users to
provide a <host> that isn't the proper hostname but an alias defined in
the ssh_config without failing the tests. If this is done
dracut-module-setup.sh:kdump_get_remote_ip will fail to obtain the
targets ip address. This failure is not detected and thus will not fail
the initramfs creation. The resulting initramfs however doesn't have the
necessary information for setting up the network and thus will fail to
boot.

Prevent the use of alias hostnames by verifying that the given hostname
is the same one ssh would use after parsing the ssh_config.

Note: Don't use getent ahosts to verify that the given host can be
resolved as this requires the network to be up which cannot be
guaranteed when the kdump.conf is parsed.

Signed-off-by: Philipp Rudo 
---
 kdumpctl | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/kdumpctl b/kdumpctl
index 432186a..4b6013f 100755
--- a/kdumpctl
+++ b/kdumpctl
@@ -662,7 +662,7 @@ load_kdump()

check_ssh_config()
 {
-	local SSH_TARGET
+	local target

while read -r config_opt config_val; do
    	case "$config_opt" in
@@ -686,11 +686,14 @@ check_ssh_config()
    	esac
    done <<< "$(kdump_read_conf)"

-	#make sure they've configured kdump.conf for ssh dumps
-	SSH_TARGET=$(echo -n "$DUMP_TARGET" | sed -n '/.*@/p')
-	if [[ -z $SSH_TARGET ]]; then
+	[[ -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
+		derror "Invalid ssh destination $DUMP_TARGET provided."
    	return 1
    fi
+
    return 0
 }

[PATCH 08/15] kdumpctl: simplify propagate_ssh_key

[Philipp Rudo]

The function has multiple problems:

1) SSH_{USER,SERVER} aren't defined local
2) Weird use of cut and sed to parse the DUMP_TARGET for the user and
   host although check_ssh_config guarantees that it has the format
   <user>@<host>.
3) Unnecessary use of a variable for the return value
4) Weird behavior to first unpack the DUMP_TARGET to SSH_USER and
   SSH_SERVER and then putting it back together again
5) Definition of variable errmsg that is only used once but breaks
   grep-ability of error message.
6) Wrong order when redirecting output of ssh-keygen, see SC2069 [1]

Fix them now.

While at it also improve the error messages in the function.

[1] https://www.shellcheck.net/wiki/SC2069

Signed-off-by: Philipp Rudo 
---
 kdumpctl | 21 +++++++++------------
 1 file changed, 9 insertions(+), 12 deletions(-)

diff --git a/kdumpctl b/kdumpctl
index 4b6013f..a311238 100755
--- a/kdumpctl
+++ b/kdumpctl
@@ -754,35 +754,32 @@ check_ssh_target()

propagate_ssh_key()
 {
+	local SSH_USER SSH_SERVER
+
    if ! check_ssh_config; then
-		derror "No ssh config specified in $KDUMP_CONFIG_FILE.  Can't propagate"
+		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
    fi

local KEYFILE=$SSH_KEY_LOCATION
-	local errmsg="Failed to propagate ssh key"

#Check to see if we already created key, if not, create it.
    if [[ -f $KEYFILE ]]; then
    	dinfo "Using existing keys..."
    else
    	dinfo "Generating new ssh keys... "
-		/usr/bin/ssh-keygen -t rsa -f "$KEYFILE" -N "" 2>&1 > /dev/null
+		/usr/bin/ssh-keygen -t rsa -f "$KEYFILE" -N "" &> /dev/null
    	dinfo "done."
    fi

-	#now find the target ssh user and server to contact.
-	SSH_USER=$(echo "$DUMP_TARGET" | cut -d@ -f1)
-	SSH_SERVER=$(echo "$DUMP_TARGET" | sed -e's/(.*@)(.*$)/\2/')
-
-	#now send the found key to the found server
-	ssh-copy-id -i "$KEYFILE" "$SSH_USER@$SSH_SERVER"
-	RET=$?
-	if [[ $RET == 0 ]]; then
+	SSH_USER=${DUMP_TARGET%@*}
+	SSH_SERVER=${DUMP_TARGET#*@}
+	if ssh-copy-id -i "$KEYFILE" "$DUMP_TARGET"; then
    	dinfo "$KEYFILE has been added to ~$SSH_USER/.ssh/authorized_keys on $SSH_SERVER"
    	return 0
    else
-		derror "$errmsg, $KEYFILE failed in transfer to $SSH_SERVER"
+		derror "Failed to propagate ssh key, could not transfer $KEYFILE to $SSH_SERVER"
    	exit 1
    fi
 }

[PATCH 09/15] kdumpctl: merge check_ssh_config into check_config

[Philipp Rudo]

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 
---
 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) ;;

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)"
+	[[ -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
+
+	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

[PATCH 09/15] kdumpctl: merge check_ssh_config into check_config

[Tao Liu]

Hi Philipp,

I have some concerns about this patch.

On Sat, Feb 5, 2022 at 1:36 AM Philipp Rudo  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
>
> 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?
>   [[ -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...

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 -- [e-mail]
> To unsubscribe send an email to [e-mail]
> 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/[e-mail]
> Do not reply to spam on the list, report it: https://pagure.io/fedora-infrastructure

[PATCH 09/15] kdumpctl: merge check_ssh_config into check_config

[Philipp Rudo]

Hi Tao,

On Tue, 22 Feb 2022 17:37:31 +0800
Tao Liu  wrote:
> Hi Philipp,
>
> I have some concerns about this patch.
>
> On Sat, Feb 5, 2022 at 1:36 AM Philipp Rudo  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
> >
> > 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.
> >   [[ -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].

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?

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 -- [e-mail]
> > To unsubscribe send an email to [e-mail]
> > 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/[e-mail]
> > Do not reply to spam on the list, report it: https://pagure.io/fedora-infrastructure

[PATCH 09/15] kdumpctl: merge check_ssh_config into check_config

[Tao Liu]

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  wrote:
> > Hi Philipp,
> >
> > I have some concerns about this patch.
> >
> > On Sat, Feb 5, 2022 at 1:36 AM Philipp Rudo  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
> > >
> > > 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.
> > >   [[ -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

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 -- [e-mail]
> > > To unsubscribe send an email to [e-mail]
> > > 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/[e-mail]
> > > Do not reply to spam on the list, report it: https://pagure.io/fedora-infrastructure

[PATCH 09/15] kdumpctl: merge check_ssh_config into check_config

[Philipp Rudo]

Hi Tao,

On Wed, 23 Feb 2022 18:12:45 +0800
Tao Liu  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  wrote:
> > > Hi Philipp,
> > >
> > > I have some concerns about this patch.
> > >
> > > On Sat, Feb 5, 2022 at 1:36 AM Philipp Rudo  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
> > > >
> > > > 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

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 -- [e-mail]
> > > > To unsubscribe send an email to [e-mail]
> > > > 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/[e-mail]
> > > > Do not reply to spam on the list, report it: https://pagure.io/fedora-infrastructure

[PATCH 10/15] kdumpctl: reduce file operations on kdump.conf

[Philipp Rudo]

Every call to kdump_get_conf_val parses kdump.conf although the file has
already been parsed in check_config. Thus store the values parsed in
check_config in an array and use them later instead of re-parsing the
file over and over again.

While at it rename check_config to parse_config.

Signed-off-by: Philipp Rudo 
---
 kdumpctl | 73 +++++++++++++++++++++++++++++++-------------------------
 1 file changed, 41 insertions(+), 32 deletions(-)

diff --git a/kdumpctl b/kdumpctl
index ebb634b..f386b0a 100755
--- a/kdumpctl
+++ b/kdumpctl
@@ -1,5 +1,7 @@
 #!/bin/bash

+declare -A OPT
+
 if [[ -f /etc/sysconfig/kdump ]]; then
    . /etc/sysconfig/kdump
 fi
@@ -184,9 +186,29 @@ restore_default_initrd()
    fi
 }

-check_config()
+_set_config()
+{
+	local opt=$1
+	local val=$2
+
+	if [[ -z $val ]]; then
+		derror "Invalid kdump config value for option '$opt'"
+		return 1
+	fi
+
+	if [[ -n ${OPT[$opt]} ]]; then
+		if [[ $opt == _target ]]; then
+			derror "More than one dump targets specified"
+		else
+			derror "Duplicated kdump config value of option $opt"
+		fi
+		return 1
+	fi
+	OPT[$opt]="$val"
+}
+
+parse_config()
 {
-	local -A _opt_rec
    while read -r config_opt config_val; do
    	case "$config_opt" in
    	dracut_args)
@@ -195,7 +217,7 @@ check_config()
    				derror 'Multiple mount targets specified in one "dracut_args".'
    				return 1
    			fi
-				config_opt=_target
+				_set_config _target "$(get_dracut_args_target "$config_val")" || return 1
    		fi
    		;;
    	raw)
@@ -239,20 +261,7 @@ check_config()
    		;;
    	esac

-		if [[ -z $config_val ]]; then
-			derror "Invalid kdump config value for option '$config_opt'"
-			return 1
-		fi
-
-		if [[ -n ${_opt_rec[$config_opt]} ]]; then
-			if [[ $config_opt == _target ]]; then
-				derror "More than one dump targets specified"
-			else
-				derror "Duplicated kdump config value of option $config_opt"
-			fi
-			return 1
-		fi
-		_opt_rec[$config_opt]="$config_val"
+		_set_config "$config_opt" "$config_val" || return 1
    done <<< "$(kdump_read_conf)"

check_failure_action_config || return 1
@@ -320,8 +329,8 @@ check_files_modified()
    #also rebuild when Pacemaker cluster conf is changed and fence kdump is enabled.
    modified_files=$(get_pcs_cluster_modified_files)

-	EXTRA_BINS=$(kdump_get_conf_val kdump_post)
-	CHECK_FILES=$(kdump_get_conf_val kdump_pre)
+	EXTRA_BINS=${OPT[kdump_post]}
+	CHECK_FILES=${OPT[kdump_pre]}
    HOOKS="/etc/kdump/post.d/ /etc/kdump/pre.d/"
    if [[ -d /etc/kdump/post.d ]]; then
    	for file in /etc/kdump/post.d/*; do
@@ -338,17 +347,17 @@ check_files_modified()
    	done
    fi
    HOOKS="$HOOKS $POST_FILES $PRE_FILES"
-	CORE_COLLECTOR=$(kdump_get_conf_val core_collector | awk '{print $1}')
+	CORE_COLLECTOR=$(echo "${OPT[core_collector]}" | awk '{print $1}')
    CORE_COLLECTOR=$(type -P "$CORE_COLLECTOR")
    # POST_FILES and PRE_FILES are already checked against executable, need not to check again.
    EXTRA_BINS="$EXTRA_BINS $CHECK_FILES"
-	CHECK_FILES=$(kdump_get_conf_val extra_bins)
+	CHECK_FILES=${OPT[extra_bins]}
    EXTRA_BINS="$EXTRA_BINS $CHECK_FILES"
    files="$KDUMP_CONFIG_FILE $KDUMP_KERNEL $EXTRA_BINS $CORE_COLLECTOR"
    [[ -e /etc/fstab ]] && files="$files /etc/fstab"

# Check for any updated extra module
-	EXTRA_MODULES="$(kdump_get_conf_val extra_modules)"
+	EXTRA_MODULES="${OPT[extra_modules]}"
    if [[ -n $EXTRA_MODULES ]]; then
    	if [[ -e /lib/modules/$KDUMP_KERNELVER/modules.dep ]]; then
    		files="$files /lib/modules/$KDUMP_KERNELVER/modules.dep"
@@ -540,14 +549,14 @@ check_rebuild()

setup_initrd || return 1

-	force_no_rebuild=$(kdump_get_conf_val force_no_rebuild)
+	force_no_rebuild=${OPT[force_no_rebuild]}
    force_no_rebuild=${force_no_rebuild:-0}
    if [[ $force_no_rebuild != "0" ]] && [[ $force_no_rebuild != "1" ]]; then
    	derror "Error: force_no_rebuild value is invalid"
    	return 1
    fi

-	force_rebuild=$(kdump_get_conf_val force_rebuild)
+	force_rebuild=${OPT[force_rebuild]}
    force_rebuild=${force_rebuild:-0}
    if [[ $force_rebuild != "0" ]] && [[ $force_rebuild != "1" ]]; then
    	derror "Error: force_rebuild value is invalid"
@@ -750,7 +759,7 @@ propagate_ssh_key()
 {
    local SSH_USER SSH_SERVER

-	check_config || return 1
+	parse_config || return 1

if [[ -z $DUMP_TARGET ]] ; then
    	derror "No ssh destination defined in $KDUMP_CONFIG_FILE."
@@ -824,7 +833,7 @@ save_raw()
    	dwarn "Warning: Detected '$check_fs' signature on $raw_target, data loss is expected."
    	return 0
    fi
-	kdump_dir=$(kdump_get_conf_val path)
+	kdump_dir=${OPT[path]}
    if [[ -z ${kdump_dir} ]]; then
    	coredir="/var/crash/$(date +"%Y-%m-%d-%H:%M")"
    else
@@ -910,7 +919,7 @@ check_fence_kdump_config()

hostname=$(hostname)
    ipaddrs=$(hostname -I)
-	nodes=$(kdump_get_conf_val "fence_kdump_nodes")
+	nodes=${OPT[fence_kdump_nodes]}

for node in $nodes; do
    	if [[ $node == "$hostname" ]]; then
@@ -963,8 +972,8 @@ check_failure_action_config()
    local failure_action
    local option="failure_action"

-	default_option=$(kdump_get_conf_val default)
-	failure_action=$(kdump_get_conf_val failure_action)
+	default_option=${OPT[default]}
+	failure_action=${OPT[failure_action]}

if [[ -z $failure_action ]] && [[ -z $default_option ]]; then
    	return 0
@@ -993,7 +1002,7 @@ check_final_action_config()
 {
    local final_action

-	final_action=$(kdump_get_conf_val final_action)
+	final_action=${OPT[final_action]}
    if [[ -z $final_action ]]; then
    	return 0
    else
@@ -1016,7 +1025,7 @@ start()
    	return 1
    fi

-	if ! check_config; then
+	if ! parse_config; then
    	derror "Starting kdump: [FAILED]"
    	return 1
    fi
@@ -1152,7 +1161,7 @@ stop()

rebuild()
 {
-	check_config || return 1
+	parse_config || return 1
    check_and_wait_network_ready || return 1

setup_initrd || return 1

[PATCH 11/15] kdumpctl: drop SAVE_PATH variable

[Philipp Rudo]

The variable is only used for ssh dump targets. Furthermore it is
identical to the value stored in ${OPT[path]}. Thus drop SAVE_PATH and
use ${OPT[path]} instead.

Also make sure that ${OPT[path]} is always set to the default value when
no entry in kdump.conf is found.

Signed-off-by: Philipp Rudo 
---
 kdumpctl | 24 ++++++++----------------
 1 file changed, 8 insertions(+), 16 deletions(-)

diff --git a/kdumpctl b/kdumpctl
index f386b0a..9f1b592 100755
--- a/kdumpctl
+++ b/kdumpctl
@@ -27,7 +27,6 @@ KDUMP_LOG_PATH="/var/log"
 MKDUMPRD="/sbin/mkdumprd -f"
 MKFADUMPRD="/sbin/mkfadumprd"
 DRACUT_MODULES_FILE="/usr/lib/dracut/modules.txt"
-SAVE_PATH=$DEFAULT_PATH
 SSH_KEY_LOCATION=$DEFAULT_SSHKEY
 INITRD_CHECKSUM_LOCATION="/boot/.fadump_initrd_checksum"
 DUMP_TARGET=""
@@ -243,10 +242,7 @@ parse_config()
    			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) ;;
+		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) ;;

net | options | link_delay | disk_timeout | debug_mem_level | blacklist)
    		derror "Deprecated kdump config option: $config_opt. Refer to kdump.conf manpage for alternatives."
@@ -264,6 +260,8 @@ parse_config()
    	_set_config "$config_opt" "$config_val" || return 1
    done <<< "$(kdump_read_conf)"

+	OPT[path]=${OPT[path]:-$DEFAULT_PATH}
+
    check_failure_action_config || return 1
    check_final_action_config || return 1
    check_fence_kdump_config || return 1
@@ -719,21 +717,21 @@ check_and_wait_network_ready()

start_time=$(date +%s)
    while true; do
-		errmsg=$(ssh -i "$SSH_KEY_LOCATION" -o BatchMode=yes "$DUMP_TARGET" mkdir -p "$SAVE_PATH" 2>&1)
+		errmsg=$(ssh -i "$SSH_KEY_LOCATION" -o BatchMode=yes "$DUMP_TARGET" mkdir -p "${OPT[path]}" 2>&1)
    	retval=$?

# ssh exits with the exit status of the remote command or with 255 if an error occurred
    	if [[ $retval -eq 0 ]]; then
    		return 0
    	elif [[ $retval -ne 255 ]]; then
-			derror "Could not create $DUMP_TARGET:$SAVE_PATH, you should check the privilege on server side"
+			derror "Could not create $DUMP_TARGET:${OPT[path]}, you should check the privilege on server side"
    		return 1
    	fi

# if server removes the authorized_keys or, no /root/.ssh/kdump_id_rsa
    	ddebug "$errmsg"
    	if echo "$errmsg" | grep -q "Permission denied|No such file or directory|Host key verification failed"; then
-			derror "Could not create $DUMP_TARGET:$SAVE_PATH, you probably need to run "kdumpctl propagate""
+			derror "Could not create $DUMP_TARGET:${OPT[path]}, you probably need to run "kdumpctl propagate""
    		return 1
    	fi

@@ -751,7 +749,7 @@ check_and_wait_network_ready()
    	sleep 1
    done

-	dinfo "Could not create $DUMP_TARGET:$SAVE_PATH, ipaddr is not ready yet. You should check network connection"
+	dinfo "Could not create $DUMP_TARGET:${OPT[path]}, ipaddr is not ready yet. You should check network connection"
    return 1
 }

@@ -819,7 +817,6 @@ check_current_status()

save_raw()
 {
-	local kdump_dir
    local raw_target

raw_target=$(kdump_get_conf_val raw)
@@ -833,13 +830,8 @@ save_raw()
    	dwarn "Warning: Detected '$check_fs' signature on $raw_target, data loss is expected."
    	return 0
    fi
-	kdump_dir=${OPT[path]}
-	if [[ -z ${kdump_dir} ]]; then
-		coredir="/var/crash/$(date +"%Y-%m-%d-%H:%M")"
-	else
-		coredir="${kdump_dir}/$(date +"%Y-%m-%d-%H:%M")"
-	fi

+	coredir="${OPT[path]}/$(date +"%Y-%m-%d-%H:%M")"
    mkdir -p "$coredir"
    [[ -d $coredir ]] || {
    	derror "failed to create $coredir"

[PATCH 12/15] kdumpctl: drop SSH_KEY_LOCATION variable

[Philipp Rudo]

The variable is only used for ssh dump targets. Furthermore it is
identical to the value stored in ${OPT[sshkey]}. Thus drop
SSH_KEY_LOCATION and use ${OPT[sshkey]} instead.

Signed-off-by: Philipp Rudo 
---
 kdumpctl | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/kdumpctl b/kdumpctl
index 9f1b592..e70b121 100755
--- a/kdumpctl
+++ b/kdumpctl
@@ -27,7 +27,6 @@ KDUMP_LOG_PATH="/var/log"
 MKDUMPRD="/sbin/mkdumprd -f"
 MKFADUMPRD="/sbin/mkfadumprd"
 DRACUT_MODULES_FILE="/usr/lib/dracut/modules.txt"
-SSH_KEY_LOCATION=$DEFAULT_SSHKEY
 INITRD_CHECKSUM_LOCATION="/boot/.fadump_initrd_checksum"
 DUMP_TARGET=""
 DEFAULT_INITRD=""
@@ -237,9 +236,10 @@ parse_config()
    			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")
+				config_val=$(/usr/bin/readlink -m "$config_val")
    		else
-				dwarn "WARNING: '$config_val' doesn't exist, using default value '$SSH_KEY_LOCATION'"
+				dwarn "WARNING: '$config_val' doesn't exist, using default value '$DEFAULT_SSHKEY'"
+				config_val=$DEFAULT_SSHKEY
    		fi
    		;;
    	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) ;;
@@ -261,6 +261,7 @@ parse_config()
    done <<< "$(kdump_read_conf)"

OPT[path]=${OPT[path]:-$DEFAULT_PATH}
+	OPT[sshkey]=${OPT[sshkey]:-$DEFAULT_SSHKEY}

check_failure_action_config || return 1
    check_final_action_config || return 1
@@ -717,7 +718,7 @@ check_and_wait_network_ready()

start_time=$(date +%s)
    while true; do
-		errmsg=$(ssh -i "$SSH_KEY_LOCATION" -o BatchMode=yes "$DUMP_TARGET" mkdir -p "${OPT[path]}" 2>&1)
+		errmsg=$(ssh -i "${OPT[sshkey]}" -o BatchMode=yes "$DUMP_TARGET" mkdir -p "${OPT[path]}" 2>&1)
    	retval=$?

# ssh exits with the exit status of the remote command or with 255 if an error occurred
@@ -765,7 +766,7 @@ propagate_ssh_key()
    	exit 1
    fi

-	local KEYFILE=$SSH_KEY_LOCATION
+	local KEYFILE=${OPT[sshkey]}

#Check to see if we already created key, if not, create it.
    if [[ -f $KEYFILE ]]; then

[PATCH 13/15] kdumpctl: drop DUMP_TARGET variable

[Philipp Rudo]

The variable is only used for ssh dump targets. Furthermore it is
identical to the value stored in ${OPT[_target]}. Thus drop DUMP_TARGET and
use ${OPT[_target]} instead.

In order to be able to distinguish between the different target types
introduce the internal ${OPT[_fstype]}.

Signed-off-by: Philipp Rudo 
---
 kdumpctl | 40 +++++++++++++++++++---------------------
 1 file changed, 19 insertions(+), 21 deletions(-)

diff --git a/kdumpctl b/kdumpctl
index e70b121..aaf753b 100755
--- a/kdumpctl
+++ b/kdumpctl
@@ -28,7 +28,6 @@ MKDUMPRD="/sbin/mkdumprd -f"
 MKFADUMPRD="/sbin/mkfadumprd"
 DRACUT_MODULES_FILE="/usr/lib/dracut/modules.txt"
 INITRD_CHECKSUM_LOCATION="/boot/.fadump_initrd_checksum"
-DUMP_TARGET=""
 DEFAULT_INITRD=""
 DEFAULT_INITRD_BAK=""
 KDUMP_INITRD=""
@@ -195,7 +194,7 @@ _set_config()
    fi

if [[ -n ${OPT[$opt]} ]]; then
-		if [[ $opt == _target ]]; then
+		if [[ $opt == _target ]] || [[ $opt == _fstype ]]; then
    		derror "More than one dump targets specified"
    	else
    		derror "Duplicated kdump config value of option $opt"
@@ -215,6 +214,7 @@ parse_config()
    				derror 'Multiple mount targets specified in one "dracut_args".'
    				return 1
    			fi
+				_set_config _fstype "$(get_dracut_args_fstype "$config_val")" || return 1
    			_set_config _target "$(get_dracut_args_target "$config_val")" || return 1
    		fi
    		;;
@@ -222,15 +222,13 @@ parse_config()
    		if [[ -d "/proc/device-tree/ibm,opal/dump" ]]; then
    			dwarn "WARNING: Won't capture opalcore when 'raw' dump target is used."
    		fi
+			_set_config _fstype "$config_opt" || return 1
    		config_opt=_target
    		;;
-		ext[234] | minix | btrfs | xfs | nfs )
+		ext[234] | minix | btrfs | xfs | nfs | ssh)
+			_set_config _fstype "$config_opt" || return 1
    		config_opt=_target
    		;;
-		ssh)
-			config_opt=_target
-			DUMP_TARGET=$config_val
-			;;
    	sshkey)
    		if [[ -z $config_val ]]; then
    			derror "Invalid kdump config value for option '$config_opt'"
@@ -690,12 +688,12 @@ check_ssh_config()
 {
    local target

-	[[ -n $DUMP_TARGET ]] || return 0
+	[[ "${OPT[_fstype]}" == ssh ]] || return 0

-	[[ $DUMP_TARGET =~ .*@.* ]] || return 1
-	target=$(ssh -G "$DUMP_TARGET" | sed  -n -e "s/^hostname[[:space:]]+([^[:space:]]*).*$/\1/p")
-	if [[ ${DUMP_TARGET#*@} != "$target" ]]; then
-		derror "Invalid ssh destination $DUMP_TARGET provided."
+	target=$(ssh -G "${OPT[_target]}" | sed  -n -e "s/^hostname[[:space:]]+([^[:space:]]*).*$/\1/p")
+	[[ ${OPT[_target]} =~ .*@.* ]] || return 1
+	if [[ ${OPT[_target]#*@} != "$target" ]]; then
+		derror "Invalid ssh destination ${OPT[_target]} provided."
    	return 1
    fi

@@ -714,25 +712,25 @@ check_and_wait_network_ready()
    local retval
    local errmsg

-	[[ -n $DUMP_TARGET ]] || return 0
+	[[ "${OPT[_fstype]}" != ssh ]] || return 0

start_time=$(date +%s)
    while true; do
-		errmsg=$(ssh -i "${OPT[sshkey]}" -o BatchMode=yes "$DUMP_TARGET" mkdir -p "${OPT[path]}" 2>&1)
+		errmsg=$(ssh -i "${OPT[sshkey]}" -o BatchMode=yes "${OPT[_target]}" mkdir -p "${OPT[path]}" 2>&1)
    	retval=$?

# ssh exits with the exit status of the remote command or with 255 if an error occurred
    	if [[ $retval -eq 0 ]]; then
    		return 0
    	elif [[ $retval -ne 255 ]]; then
-			derror "Could not create $DUMP_TARGET:${OPT[path]}, you should check the privilege on server side"
+			derror "Could not create ${OPT[_target]}:${OPT[path]}, you should check the privilege on server side"
    		return 1
    	fi

# if server removes the authorized_keys or, no /root/.ssh/kdump_id_rsa
    	ddebug "$errmsg"
    	if echo "$errmsg" | grep -q "Permission denied|No such file or directory|Host key verification failed"; then
-			derror "Could not create $DUMP_TARGET:${OPT[path]}, you probably need to run "kdumpctl propagate""
+			derror "Could not create ${OPT[_target]}:${OPT[path]}, you probably need to run "kdumpctl propagate""
    		return 1
    	fi

@@ -750,7 +748,7 @@ check_and_wait_network_ready()
    	sleep 1
    done

-	dinfo "Could not create $DUMP_TARGET:${OPT[path]}, ipaddr is not ready yet. You should check network connection"
+	dinfo "Could not create ${OPT[_target]}:${OPT[path]}, ipaddr is not ready yet. You should check network connection"
    return 1
 }

@@ -760,7 +758,7 @@ propagate_ssh_key()

parse_config || return 1

-	if [[ -z $DUMP_TARGET ]] ; then
+	if [[ ${OPT[_fstype]} != ssh ]] ; 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
@@ -777,9 +775,9 @@ propagate_ssh_key()
    	dinfo "done."
    fi

-	SSH_USER=${DUMP_TARGET%@*}
-	SSH_SERVER=${DUMP_TARGET#*@}
-	if ssh-copy-id -i "$KEYFILE" "$DUMP_TARGET"; then
+	SSH_USER=${OPT[_target]%@*}
+	SSH_SERVER=${OPT[_target]#*@}
+	if ssh-copy-id -i "$KEYFILE" "${OPT[_target]}"; then
    	dinfo "$KEYFILE has been added to ~$SSH_USER/.ssh/authorized_keys on $SSH_SERVER"
    	return 0
    else

[PATCH 13/15] kdumpctl: drop DUMP_TARGET variable

[Coiby Xu]

On Fri, Feb 04, 2022 at 06:34:27PM +0100, Philipp Rudo wrote:
> The variable is only used for ssh dump targets. Furthermore it is
> identical to the value stored in ${OPT[_target]}. Thus drop DUMP_TARGET and
> use ${OPT[_target]} instead.
>
> In order to be able to distinguish between the different target types
> introduce the internal ${OPT[_fstype]}.
> Signed-off-by: Philipp Rudo
>
> kdumpctl | 40 +++++++++++++++++++---------------------
> 1 file changed, 19 insertions(+), 21 deletions(-)
>
> diff --git a/kdumpctl b/kdumpctl
> index e70b121..aaf753b 100755
> --- a/kdumpctl
> +++ b/kdumpctl
> @@ -28,7 +28,6 @@ MKDUMPRD="/sbin/mkdumprd -f"
> MKFADUMPRD="/sbin/mkfadumprd"
> DRACUT_MODULES_FILE="/usr/lib/dracut/modules.txt"
> INITRD_CHECKSUM_LOCATION="/boot/.fadump_initrd_checksum"
> -DUMP_TARGET=""
> DEFAULT_INITRD=""
> DEFAULT_INITRD_BAK=""
> KDUMP_INITRD=""
> @@ -195,7 +194,7 @@ _set_config()
>    fi
>
> if [[ -n ${OPT[$opt]} ]]; then
> if [[ $opt == _target ]]; then
> if [[ $opt == _target ]] || [[ $opt == _fstype ]]; then
> derror "More than one dump targets specified"
> else
> 	derror "Duplicated kdump config value of option $opt"
>
> @@ -215,6 +214,7 @@ parse_config()
>    				derror 'Multiple mount targets specified in one "dracut_args".'
>    				return 1
>    			fi
> 		_set_config _fstype "$(get_dracut_args_fstype "$config_val")" || return 1
> 	_set_config _target "$(get_dracut_args_target "$config_val")" || return 1
> fi
> ;;
>
> @@ -222,15 +222,13 @@ parse_config()
>    		if [[ -d "/proc/device-tree/ibm,opal/dump" ]]; then
>    			dwarn "WARNING: Won't capture opalcore when 'raw' dump target is used."
>    		fi
> 	_set_config _fstype "$config_opt" || return 1
> config_opt=_target
> ;;
> ext[234] | minix | btrfs | xfs | nfs )
> ext[234] | minix | btrfs | xfs | nfs | ssh)
> 	_set_config _fstype "$config_opt" || return 1
> config_opt=_target
> ;;
> ssh)
> 	config_opt=_target
> 	DUMP_TARGET=$config_val
> 	;;
> sshkey)
> 	if [[ -z $config_val ]]; then
> 		derror "Invalid kdump config value for option '$config_opt'"
>
> @@ -690,12 +688,12 @@ check_ssh_config()
> {
>    local target
> [[ -n $DUMP_TARGET ]] || return 0
> [[ "${OPT[_fstype]}" == ssh ]] || return 0
> [[ $DUMP_TARGET =~ .*@.* ]] || return 1
> target=$(ssh -G "$DUMP_TARGET" | sed  -n -e "s/^hostname[[:space:]]+([^[:space:]]*).*$/\1/p")
> if [[ ${DUMP_TARGET#*@} != "$target" ]]; then
> derror "Invalid ssh destination $DUMP_TARGET provided."
> target=$(ssh -G "${OPT[_target]}" | sed  -n -e "s/^hostname[[:space:]]+([^[:space:]]*).*$/\1/p")
> [[ ${OPT[_target]} =~ .*@.* ]] || return 1
> if [[ ${OPT[_target]#*@} != "$target" ]]; then
> derror "Invalid ssh destination ${OPT[_target]} provided."
> return 1
> fi
>
> @@ -714,25 +712,25 @@ check_and_wait_network_ready()
>    local retval
>    local errmsg
> [[ -n $DUMP_TARGET ]] || return 0
> [[ "${OPT[_fstype]}" != ssh ]] || return 0

^^
                         s/!=/==

As caught by the kdump tests, this error would make kdump.service fail to
start when dump target is not ssh and thus all non-ssh tests fails,

$ RELEASE=34 make test-run
======== Test results ========
----------------
nfs-kdump:              TEST FAILED
----------------
nfs-early-kdump:                TEST FAILED
----------------
ssh-kdump:              TEST PASSED
----------------
local-kdump:            TEST FAILED

An unrelated question why no check_and_wait_network_ready for NFS dump
target since it also requires the network to be ready. Pinfang, do you
know the answer since you are author of
commit c1a0634 ("kdumpctl: wait a while for network ready if dump target is ssh")?
> start_time=$(date +%s)
>    while true; do
> errmsg=$(ssh -i "${OPT[sshkey]}" -o BatchMode=yes "$DUMP_TARGET" mkdir -p "${OPT[path]}" 2>&1)
> errmsg=$(ssh -i "${OPT[sshkey]}" -o BatchMode=yes "${OPT[_target]}" mkdir -p "${OPT[path]}" 2>&1)
>
> retval=$?
>
> # ssh exits with the exit status of the remote command or with 255 if an error occurred
> if [[ $retval -eq 0 ]]; then
> 	return 0
> elif [[ $retval -ne 255 ]]; then
> 	derror "Could not create $DUMP_TARGET:${OPT[path]}, you should check the privilege on server side"
> 	derror "Could not create ${OPT[_target]}:${OPT[path]}, you should check the privilege on server side"
> return 1
>
> fi
>
> # if server removes the authorized_keys or, no /root/.ssh/kdump_id_rsa
> ddebug "$errmsg"
> if echo "$errmsg" | grep -q "Permission denied|No such file or directory|Host key verification failed"; then
> 	derror "Could not create $DUMP_TARGET:${OPT[path]}, you probably need to run \"kdumpctl propagate\""
> 	derror "Could not create ${OPT[_target]}:${OPT[path]}, you probably need to run \"kdumpctl propagate\""
> return 1
> fi
>
> @@ -750,7 +748,7 @@ check_and_wait_network_ready()
>    	sleep 1
>    done
> dinfo "Could not create $DUMP_TARGET:${OPT[path]}, ipaddr is not ready yet. You should check network connection"
> dinfo "Could not create ${OPT[_target]}:${OPT[path]}, ipaddr is not ready yet. You should check network connection"
> return 1
>
> }
>
> @@ -760,7 +758,7 @@ propagate_ssh_key()
>
> parse_config || return 1
> if [[ -z $DUMP_TARGET ]] ; then
> if [[ ${OPT[_fstype]} != ssh ]] ; 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
>
> @@ -777,9 +775,9 @@ propagate_ssh_key()
>    	dinfo "done."
>    fi
> SSH_USER=${DUMP_TARGET%@*}
> SSH_SERVER=${DUMP_TARGET#*@}
> if ssh-copy-id -i "$KEYFILE" "$DUMP_TARGET"; then
> SSH_USER=${OPT[_target]%@*}
> SSH_SERVER=${OPT[_target]#*@}
> if ssh-copy-id -i "$KEYFILE" "${OPT[_target]}"; then
> dinfo "$KEYFILE has been added to ~$SSH_USER/.ssh/authorized_keys on $SSH_SERVER"
> return 0
> else
>
> -- 
> 2.34.1
> _______________________________________________
> kexec mailing list -- [e-mail]
> To unsubscribe send an email to [e-mail]
> 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/[e-mail]
> Do not reply to spam on the list, report it: https://pagure.io/fedora-infrastructure

[PATCH 13/15] kdumpctl: drop DUMP_TARGET variable

[Philipp Rudo]

Hi Coiby,

finally there's time to go through my fedora mails. Please excuse the
very late answer.

On Tue, 1 Mar 2022 12:33:32 +0800
Coiby Xu  wrote:
> On Fri, Feb 04, 2022 at 06:34:27PM +0100, Philipp Rudo wrote:
> > The variable is only used for ssh dump targets. Furthermore it is
> > identical to the value stored in ${OPT[_target]}. Thus drop DUMP_TARGET and
> > use ${OPT[_target]} instead.
> >
> > In order to be able to distinguish between the different target types
> > introduce the internal ${OPT[_fstype]}.
> > Signed-off-by: Philipp Rudo
> >
> > kdumpctl | 40 +++++++++++++++++++---------------------
> > 1 file changed, 19 insertions(+), 21 deletions(-)
> >
> > diff --git a/kdumpctl b/kdumpctl
> > index e70b121..aaf753b 100755
> > --- a/kdumpctl
> > +++ b/kdumpctl
> > @@ -28,7 +28,6 @@ MKDUMPRD="/sbin/mkdumprd -f"
> > MKFADUMPRD="/sbin/mkfadumprd"
> > DRACUT_MODULES_FILE="/usr/lib/dracut/modules.txt"
> > INITRD_CHECKSUM_LOCATION="/boot/.fadump_initrd_checksum"
> > -DUMP_TARGET=""
> > DEFAULT_INITRD=""
> > DEFAULT_INITRD_BAK=""
> > KDUMP_INITRD=""
> > @@ -195,7 +194,7 @@ _set_config()
> >    fi
> >
> > if [[ -n ${OPT[$opt]} ]]; then
> > if [[ $opt == _target ]]; then
> > if [[ $opt == _target ]] || [[ $opt == _fstype ]]; then
> > derror "More than one dump targets specified"
> > else
> > 	derror "Duplicated kdump config value of option $opt"
> >
> > @@ -215,6 +214,7 @@ parse_config()
> >    				derror 'Multiple mount targets specified in one "dracut_args".'
> >    				return 1
> >    			fi
> > 		_set_config _fstype "$(get_dracut_args_fstype "$config_val")" || return 1
> > 	_set_config _target "$(get_dracut_args_target "$config_val")" || return 1
> > fi
> > ;;
> >
> > @@ -222,15 +222,13 @@ parse_config()
> >    		if [[ -d "/proc/device-tree/ibm,opal/dump" ]]; then
> >    			dwarn "WARNING: Won't capture opalcore when 'raw' dump target is used."
> >    		fi
> > 	_set_config _fstype "$config_opt" || return 1
> > config_opt=_target
> > ;;
> > ext[234] | minix | btrfs | xfs | nfs )
> > ext[234] | minix | btrfs | xfs | nfs | ssh)
> > 	_set_config _fstype "$config_opt" || return 1
> > config_opt=_target
> > ;;
> > ssh)
> > 	config_opt=_target
> > 	DUMP_TARGET=$config_val
> > 	;;
> > sshkey)
> > 	if [[ -z $config_val ]]; then
> > 		derror "Invalid kdump config value for option '$config_opt'"
> >
> > @@ -690,12 +688,12 @@ check_ssh_config()
> > {
> >    local target
> > [[ -n $DUMP_TARGET ]] || return 0
> > [[ "${OPT[_fstype]}" == ssh ]] || return 0
> > [[ $DUMP_TARGET =~ .*@.* ]] || return 1
> > target=$(ssh -G "$DUMP_TARGET" | sed  -n -e "s/^hostname[[:space:]]+([^[:space:]]*).*$/\1/p")
> > if [[ ${DUMP_TARGET#*@} != "$target" ]]; then
> > derror "Invalid ssh destination $DUMP_TARGET provided."
> > target=$(ssh -G "${OPT[_target]}" | sed  -n -e "s/^hostname[[:space:]]+([^[:space:]]*).*$/\1/p")
> > [[ ${OPT[_target]} =~ .*@.* ]] || return 1
> > if [[ ${OPT[_target]#*@} != "$target" ]]; then
> > derror "Invalid ssh destination ${OPT[_target]} provided."
> > return 1
> > fi
> >
> > @@ -714,25 +712,25 @@ check_and_wait_network_ready()
> >    local retval
> >    local errmsg
> > [[ -n $DUMP_TARGET ]] || return 0
> > [[ "${OPT[_fstype]}" != ssh ]] || return 0
>                       ^^
>                      s/!=/==

Thanks! That's definitely not what it should be...
> As caught by the kdump tests, this error would make kdump.service fail to
> start when dump target is not ssh and thus all non-ssh tests fails,
>
> $ RELEASE=34 make test-run
> ======== Test results ========
> nfs-kdump:              TEST FAILED
> nfs-early-kdump:                TEST FAILED
> ssh-kdump:              TEST PASSED
>
> local-kdump:            TEST FAILED

I should take a look at your series. These tests seem to be very handy
;-)
> An unrelated question why no check_and_wait_network_ready for NFS dump
> target since it also requires the network to be ready. Pinfang, do you
> know the answer since you are author of
> commit c1a0634 ("kdumpctl: wait a while for network ready if dump target is ssh")?

Good question...
The kdump.serve has a 'After=remote-fs.target' which should be enough.
At least when the kdump.service is used and the dump target is auto
mounted.
Furthermore, if we wanted to use check_and_wait_network_ready for nfs we
need to rewrite it as it...
> > start_time=$(date +%s)
> >    while true; do
> > errmsg=$(ssh -i "${OPT[sshkey]}" -o BatchMode=yes "$DUMP_TARGET" mkdir -p "${OPT[path]}" 2>&1)
> > errmsg=$(ssh -i "${OPT[sshkey]}" -o BatchMode=yes "${OPT[_target]}" mkdir -p "${OPT[path]}" 2>&1)

... directly depends on ssh and requires the ssh key to be propagated.
That was a problem I ran into during this cleanup...

Thanks
Philipp
> > retval=$?
> >
> > # ssh exits with the exit status of the remote command or with 255 if an error occurred
> > if [[ $retval -eq 0 ]]; then
> > 	return 0
> > elif [[ $retval -ne 255 ]]; then
> > 	derror "Could not create $DUMP_TARGET:${OPT[path]}, you should check the privilege on server side"
> > 	derror "Could not create ${OPT[_target]}:${OPT[path]}, you should check the privilege on server side"
> > return 1
> >
> > fi
> >
> > # if server removes the authorized_keys or, no /root/.ssh/kdump_id_rsa
> > ddebug "$errmsg"
> > if echo "$errmsg" | grep -q "Permission denied|No such file or directory|Host key verification failed"; then
> > 	derror "Could not create $DUMP_TARGET:${OPT[path]}, you probably need to run \"kdumpctl propagate\""
> > 	derror "Could not create ${OPT[_target]}:${OPT[path]}, you probably need to run \"kdumpctl propagate\""
> > return 1
> > fi
> >
> > @@ -750,7 +748,7 @@ check_and_wait_network_ready()
> >    	sleep 1
> >    done
> > dinfo "Could not create $DUMP_TARGET:${OPT[path]}, ipaddr is not ready yet. You should check network connection"
> > dinfo "Could not create ${OPT[_target]}:${OPT[path]}, ipaddr is not ready yet. You should check network connection"
> > return 1
> >
> > }
> >
> > @@ -760,7 +758,7 @@ propagate_ssh_key()
> >
> > parse_config || return 1
> > if [[ -z $DUMP_TARGET ]] ; then
> > if [[ ${OPT[_fstype]} != ssh ]] ; 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
> >
> > @@ -777,9 +775,9 @@ propagate_ssh_key()
> >    	dinfo "done."
> >    fi
> > SSH_USER=${DUMP_TARGET%@*}
> > SSH_SERVER=${DUMP_TARGET#*@}
> > if ssh-copy-id -i "$KEYFILE" "$DUMP_TARGET"; then
> > SSH_USER=${OPT[_target]%@*}
> > SSH_SERVER=${OPT[_target]#*@}
> > if ssh-copy-id -i "$KEYFILE" "${OPT[_target]}"; then
> > dinfo "$KEYFILE has been added to ~$SSH_USER/.ssh/authorized_keys on $SSH_SERVER"
> > return 0
> > else
> >
> > -- 
> > 2.34.1
> > _______________________________________________
> > kexec mailing list -- [e-mail]
> > To unsubscribe send an email to [e-mail]
> > 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/[e-mail]
> > Do not reply to spam on the list, report it: https://pagure.io/fedora-infrastructure

[PATCH 14/15] kdumpctl: remove kdump_get_conf_val in save_raw

[Philipp Rudo]

With the introduction of ${OPT[fstype]} this call to kdump_get_conf_val
can be removed now as well.

Signed-off-by: Philipp Rudo 
---
 kdumpctl | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/kdumpctl b/kdumpctl
index aaf753b..23f26c9 100755
--- a/kdumpctl
+++ b/kdumpctl
@@ -818,8 +818,9 @@ save_raw()
 {
    local raw_target

-	raw_target=$(kdump_get_conf_val raw)
-	[[ -z $raw_target ]] && return 0
+	[[ ${OPT[_fstype]} == raw ]] || return 0
+
+	raw_target=${OPT[_target]}
    [[ -b $raw_target ]] || {
    	derror "raw partition $raw_target not found"
    	return 1

[PATCH 15/15] kdumpctl: simplify local_fs_dump_target

[Philipp Rudo]

Make use of the new ${OPT[]} array and simplify local_fs_dump_target to
remove one more file operations.

While at it rename the local_fs_dump_target to is_local_target

Signed-off-by: Philipp Rudo 
---
 kdumpctl | 15 +++++----------
 1 file changed, 5 insertions(+), 10 deletions(-)

diff --git a/kdumpctl b/kdumpctl
index 23f26c9..630e56f 100755
--- a/kdumpctl
+++ b/kdumpctl
@@ -849,27 +849,22 @@ save_raw()
    return 0
 }

-local_fs_dump_target()
+is_local_target()
 {
-	local _target
-
-	if _target=$(grep -E "^ext[234]|^xfs|^btrfs|^minix" /etc/kdump.conf); then
-		echo "$_target" | awk '{print $2}'
-	fi
+	[[ ${OPT[_fstype]} =~ ^ext[234]|^xfs|^btrfs|^minix ]]
 }

path_to_be_relabeled()
 {
-	local _path _target _mnt="/" _rmnt
+	local _path _mnt="/" _rmnt

if is_user_configured_dump_target; then
    	if is_mount_in_dracut_args; then
    		return
    	fi

-		_target=$(local_fs_dump_target)
-		if [[ -n $_target ]]; then
-			_mnt=$(get_mntpoint_from_target "$_target")
+		if is_local_target; then
+			_mnt=$(get_mntpoint_from_target "${OPT[_target]}")
    		if ! is_mounted "$_mnt"; then
    			return
    		fi

[Tao Liu]

Hi Philipp,

I noticed there are ^M characters after the lines which you modified
from this patch set.

For example in the first patch:

-StandardOutput=syslog
-StandardError=syslog+console
+StandardOutput=journal^M
+StandardError=journal+console^M

I think the ^M chars are unnecessary, and should be removed.

Btw, the checkpatch.pl tool gives the similar results on my machine:

ERROR: DOS line endings
#32: FILE: dracut-kdump-capture.service:23:
+StandardOutput=journal^M$

ERROR: DOS line endings
#33: FILE: dracut-kdump-capture.service:24:
+StandardError=journal+console^M$

I don't know if it is caused by your editor or my mutt program when downloading
the patches.

As for the other patches, I'm still reviewing them, please wait for a while.

Thanks,
Tao Liu

On Sat, Feb 5, 2022 at 1:34 AM Philipp Rudo  wrote:
> Hi,
>
> while looking into transforming mkdumprd and mkfadumprd into library functions
> I noticed various nits in kdumpctl. This series addresses them.
>
> The series is made up of two parts:
>
> Patches 1-8 are small independent cleanups and fixes.
>
> Patches 9-15 tries to reduce the number of file accesses to /etc/kdump.conf. It
> achieves this by only parsing kdump.conf once in check_config and storing the
> parsed values in an array. Later accesses can then simply use the value stored
> in the array instead of calling kdump_get_conf_val.
>
> Thanks
> Philipp
>
> Philipp Rudo (15):
>   kdump-capture.service: switch to journal for stdout
>   kdumpctl: source dependencies before defining globals
>   kdump-lib: fix typo in variable name
>   kdumpctl: remove unnecessary uses of $?
>   kdump-lib-initramfs: merge definitions for default ssh key
>   kdumpctl: fix comment in check_and_wait_network_ready
>   kdumpctl: forbid aliases from ssh config
>   kdumpctl: simplify propagate_ssh_key
>   kdumpctl: merge check_ssh_config into check_config
>   kdumpctl: reduce file operations on kdump.conf
>   kdumpctl: drop SAVE_PATH variable
>   kdumpctl: drop SSH_KEY_LOCATION variable
>   kdumpctl: drop DUMP_TARGET variable
>   kdumpctl: remove kdump_get_conf_val in save_raw
>   kdumpctl: simplify local_fs_dump_target
>
> dracut-kdump-capture.service |   4 +-
>  dracut-kdump.sh              |   2 +-
>  kdump-lib-initramfs.sh       |   1 +
>  kdump-lib.sh                 |  14 +-
>  kdumpctl                     | 258 ++++++++++++++++-------------------
>  mkdumprd                     |   2 +-
>  6 files changed, 128 insertions(+), 153 deletions(-)
>
> --
> 2.34.1
> _______________________________________________
> kexec mailing list -- [e-mail]
> To unsubscribe send an email to [e-mail]
> 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/[e-mail]
> Do not reply to spam on the list, report it: https://pagure.io/fedora-infrastructure

[Coiby Xu]

Hi Tao,

On Mon, Feb 21, 2022 at 03:09:24PM +0800, Tao Liu wrote:
> Hi Philipp,
>
> I noticed there are ^M characters after the lines which you modified
> from this patch set.
>
> For example in the first patch:
>
> -StandardOutput=syslog
> -StandardError=syslog+console
> +StandardOutput=journal^M
> +StandardError=journal+console^M
>
> I think the ^M chars are unnecessary, and should be removed.

This should be a know issue [1].

It would be good for Phillipp to push the changes to a remote repo
instead.

[1] https://pagure.io/fedora-infrastructure/issue/9831
> Btw, the checkpatch.pl tool gives the similar results on my machine:
>
> ERROR: DOS line endings
> #32: FILE: dracut-kdump-capture.service:23:
> +StandardOutput=journal^M$
>
> ERROR: DOS line endings
> #33: FILE: dracut-kdump-capture.service:24:
> +StandardError=journal+console^M$
>
> I don't know if it is caused by your editor or my mutt program when downloading
> the patches.
>
> As for the other patches, I'm still reviewing them, please wait for a while.
>
> Thanks,
> Tao Liu
>
> On Sat, Feb 5, 2022 at 1:34 AM Philipp Rudo  wrote:
> > Hi,
> >
> > while looking into transforming mkdumprd and mkfadumprd into library functions
> > I noticed various nits in kdumpctl. This series addresses them.
> >
> > The series is made up of two parts:
> >
> > Patches 1-8 are small independent cleanups and fixes.
> >
> > Patches 9-15 tries to reduce the number of file accesses to /etc/kdump.conf. It
> > achieves this by only parsing kdump.conf once in check_config and storing the
> > parsed values in an array. Later accesses can then simply use the value stored
> > in the array instead of calling kdump_get_conf_val.
> >
> > Thanks
> > Philipp
> >
> > Philipp Rudo (15):
> >   kdump-capture.service: switch to journal for stdout
> >   kdumpctl: source dependencies before defining globals
> >   kdump-lib: fix typo in variable name
> >   kdumpctl: remove unnecessary uses of $?
> >   kdump-lib-initramfs: merge definitions for default ssh key
> >   kdumpctl: fix comment in check_and_wait_network_ready
> >   kdumpctl: forbid aliases from ssh config
> >   kdumpctl: simplify propagate_ssh_key
> >   kdumpctl: merge check_ssh_config into check_config
> >   kdumpctl: reduce file operations on kdump.conf
> >   kdumpctl: drop SAVE_PATH variable
> >   kdumpctl: drop SSH_KEY_LOCATION variable
> >   kdumpctl: drop DUMP_TARGET variable
> >   kdumpctl: remove kdump_get_conf_val in save_raw
> >   kdumpctl: simplify local_fs_dump_target
> >
> > dracut-kdump-capture.service |   4 +-
> >  dracut-kdump.sh              |   2 +-
> >  kdump-lib-initramfs.sh       |   1 +
> >  kdump-lib.sh                 |  14 +-
> >  kdumpctl                     | 258 ++++++++++++++++-------------------
> >  mkdumprd                     |   2 +-
> >  6 files changed, 128 insertions(+), 153 deletions(-)
> >
> > --
> > 2.34.1
> > _______________________________________________
> > kexec mailing list -- [e-mail]
> > To unsubscribe send an email to [e-mail]
> > 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/[e-mail]
> > Do not reply to spam on the list, report it: https://pagure.io/fedora-infrastructure
>
> kexec mailing list -- [e-mail]
> To unsubscribe send an email to [e-mail]
> 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/[e-mail]
> Do not reply to spam on the list, report it: https://pagure.io/fedora-infrastructure

[Tao Liu]

Hi Coiby,

On Mon, Feb 21, 2022 at 3:41 PM Coiby Xu  wrote:
> Hi Tao,
>
> On Mon, Feb 21, 2022 at 03:09:24PM +0800, Tao Liu wrote:
> > Hi Philipp,
> >
> > I noticed there are ^M characters after the lines which you modified
> > from this patch set.
> >
> > For example in the first patch:
> >
> > -StandardOutput=syslog
> > -StandardError=syslog+console
> > +StandardOutput=journal^M
> > +StandardError=journal+console^M
> >
> > I think the ^M chars are unnecessary, and should be removed.
>
> This should be a know issue [1].

hmm... Interesting, it looks like an email-mutt issue right?

Just for curiosity, Will you merge the patches without any modification
on the patch set or will you use tools like dox2unix to strip these chars
before the merge?

I didn't use git am much, this will be the first time I encountered this, if
the patches are OK , then please ignore my comments...

Thanks,
Tao Liu
> It would be good for Phillipp to push the changes to a remote repo
> instead.
>
> [1] https://pagure.io/fedora-infrastructure/issue/9831
> > Btw, the checkpatch.pl tool gives the similar results on my machine:
> >
> > ERROR: DOS line endings
> > #32: FILE: dracut-kdump-capture.service:23:
> > +StandardOutput=journal^M$
> >
> > ERROR: DOS line endings
> > #33: FILE: dracut-kdump-capture.service:24:
> > +StandardError=journal+console^M$
> >
> > I don't know if it is caused by your editor or my mutt program when downloading
> > the patches.
> >
> > As for the other patches, I'm still reviewing them, please wait for a while.
> >
> > Thanks,
> > Tao Liu
> >
> > On Sat, Feb 5, 2022 at 1:34 AM Philipp Rudo  wrote:
> > > Hi,
> > >
> > > while looking into transforming mkdumprd and mkfadumprd into library functions
> > > I noticed various nits in kdumpctl. This series addresses them.
> > >
> > > The series is made up of two parts:
> > >
> > > Patches 1-8 are small independent cleanups and fixes.
> > >
> > > Patches 9-15 tries to reduce the number of file accesses to /etc/kdump.conf. It
> > > achieves this by only parsing kdump.conf once in check_config and storing the
> > > parsed values in an array. Later accesses can then simply use the value stored
> > > in the array instead of calling kdump_get_conf_val.
> > >
> > > Thanks
> > > Philipp
> > >
> > > Philipp Rudo (15):
> > >   kdump-capture.service: switch to journal for stdout
> > >   kdumpctl: source dependencies before defining globals
> > >   kdump-lib: fix typo in variable name
> > >   kdumpctl: remove unnecessary uses of $?
> > >   kdump-lib-initramfs: merge definitions for default ssh key
> > >   kdumpctl: fix comment in check_and_wait_network_ready
> > >   kdumpctl: forbid aliases from ssh config
> > >   kdumpctl: simplify propagate_ssh_key
> > >   kdumpctl: merge check_ssh_config into check_config
> > >   kdumpctl: reduce file operations on kdump.conf
> > >   kdumpctl: drop SAVE_PATH variable
> > >   kdumpctl: drop SSH_KEY_LOCATION variable
> > >   kdumpctl: drop DUMP_TARGET variable
> > >   kdumpctl: remove kdump_get_conf_val in save_raw
> > >   kdumpctl: simplify local_fs_dump_target
> > >
> > > dracut-kdump-capture.service |   4 +-
> > >  dracut-kdump.sh              |   2 +-
> > >  kdump-lib-initramfs.sh       |   1 +
> > >  kdump-lib.sh                 |  14 +-
> > >  kdumpctl                     | 258 ++++++++++++++++-------------------
> > >  mkdumprd                     |   2 +-
> > >  6 files changed, 128 insertions(+), 153 deletions(-)
> > >
> > > --
> > > 2.34.1
> > > _______________________________________________
> > > kexec mailing list -- [e-mail]
> > > To unsubscribe send an email to [e-mail]
> > > 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/[e-mail]
> > > Do not reply to spam on the list, report it: https://pagure.io/fedora-infrastructure
> >
> > kexec mailing list -- [e-mail]
> > To unsubscribe send an email to [e-mail]
> > 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/[e-mail]
> > Do not reply to spam on the list, report it: https://pagure.io/fedora-infrastructure
>
> --
> Best regards,
> Coiby

[Coiby Xu]

On Mon, Feb 21, 2022 at 04:19:22PM +0800, Tao Liu wrote:
> Hi Coiby,
>
> On Mon, Feb 21, 2022 at 3:41 PM Coiby Xu  wrote:
> > Hi Tao,
> >
> > On Mon, Feb 21, 2022 at 03:09:24PM +0800, Tao Liu wrote:
> > > Hi Philipp,
> > >
> > > I noticed there are ^M characters after the lines which you modified
> > > from this patch set.
> > >
> > > For example in the first patch:
> > >
> > > -StandardOutput=syslog
> > > -StandardError=syslog+console
> > > +StandardOutput=journal^M
> > > +StandardError=journal+console^M
> > >
> > > I think the ^M chars are unnecessary, and should be removed.
> >
> > This should be a know issue [1].
>
> hmm... Interesting, it looks like an email-mutt issue right?

It's likely an email relay server adds these trailing ^Ms.
> Just for curiosity, Will you merge the patches without any modification
> on the patch set or will you use tools like dox2unix to strip these chars
> before the merge?

I use a tool [2] from Kairui to remove trailing ^Ms before applying them.
But that tool can process one patch one time.

[2] https://gitlab.cee.redhat.com/kasong/dot-home/-/blob/master/misc/format-patc...
> I didn't use git am much, this will be the first time I encountered this, if
> the patches are OK , then please ignore my comments...
>
> Thanks,
> Tao Liu
> > It would be good for Phillipp to push the changes to a remote repo
> > instead.
> >
> > [1] https://pagure.io/fedora-infrastructure/issue/9831
> > > Btw, the checkpatch.pl tool gives the similar results on my machine:
> > >
> > > ERROR: DOS line endings
> > > #32: FILE: dracut-kdump-capture.service:23:
> > > +StandardOutput=journal^M$
> > >
> > > ERROR: DOS line endings
> > > #33: FILE: dracut-kdump-capture.service:24:
> > > +StandardError=journal+console^M$
> > >
> > > I don't know if it is caused by your editor or my mutt program when downloading
> > > the patches.
> > >
> > > As for the other patches, I'm still reviewing them, please wait for a while.
> > >
> > > Thanks,
> > > Tao Liu
> > >
> > > On Sat, Feb 5, 2022 at 1:34 AM Philipp Rudo  wrote:
> > > > Hi,
> > > >
> > > > while looking into transforming mkdumprd and mkfadumprd into library functions
> > > > I noticed various nits in kdumpctl. This series addresses them.
> > > >
> > > > The series is made up of two parts:
> > > >
> > > > Patches 1-8 are small independent cleanups and fixes.
> > > >
> > > > Patches 9-15 tries to reduce the number of file accesses to /etc/kdump.conf. It
> > > > achieves this by only parsing kdump.conf once in check_config and storing the
> > > > parsed values in an array. Later accesses can then simply use the value stored
> > > > in the array instead of calling kdump_get_conf_val.
> > > >
> > > > Thanks
> > > > Philipp
> > > >
> > > > Philipp Rudo (15):
> > > >   kdump-capture.service: switch to journal for stdout
> > > >   kdumpctl: source dependencies before defining globals
> > > >   kdump-lib: fix typo in variable name
> > > >   kdumpctl: remove unnecessary uses of $?
> > > >   kdump-lib-initramfs: merge definitions for default ssh key
> > > >   kdumpctl: fix comment in check_and_wait_network_ready
> > > >   kdumpctl: forbid aliases from ssh config
> > > >   kdumpctl: simplify propagate_ssh_key
> > > >   kdumpctl: merge check_ssh_config into check_config
> > > >   kdumpctl: reduce file operations on kdump.conf
> > > >   kdumpctl: drop SAVE_PATH variable
> > > >   kdumpctl: drop SSH_KEY_LOCATION variable
> > > >   kdumpctl: drop DUMP_TARGET variable
> > > >   kdumpctl: remove kdump_get_conf_val in save_raw
> > > >   kdumpctl: simplify local_fs_dump_target
> > > >
> > > > dracut-kdump-capture.service |   4 +-
> > > >  dracut-kdump.sh              |   2 +-
> > > >  kdump-lib-initramfs.sh       |   1 +
> > > >  kdump-lib.sh                 |  14 +-
> > > >  kdumpctl                     | 258 ++++++++++++++++-------------------
> > > >  mkdumprd                     |   2 +-
> > > >  6 files changed, 128 insertions(+), 153 deletions(-)
> > > >
> > > > --
> > > > 2.34.1
> > > > _______________________________________________
> > > > kexec mailing list -- [e-mail]
> > > > To unsubscribe send an email to [e-mail]
> > > > 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/[e-mail]
> > > > Do not reply to spam on the list, report it: https://pagure.io/fedora-infrastructure
> > >
> > > kexec mailing list -- [e-mail]
> > > To unsubscribe send an email to [e-mail]
> > > 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/[e-mail]
> > > Do not reply to spam on the list, report it: https://pagure.io/fedora-infrastructure
> >
> > --
> > Best regards,
> > Coiby

[Tao Liu]

Hi Coiby,

Thanks for the info!

BTW, for this patch set, I have reviewed patch 1~8, which are LGTM.
You can add
Reviewed-by: Tao Liu 
to these patches. So I don't need to send the signature one by one patch.

I will look into the remaining patches 9~15 tomorrow.

Thanks,
Tao Liu

On Mon, Feb 21, 2022 at 5:41 PM Coiby Xu  wrote:
> On Mon, Feb 21, 2022 at 04:19:22PM +0800, Tao Liu wrote:
> > Hi Coiby,
> >
> > On Mon, Feb 21, 2022 at 3:41 PM Coiby Xu  wrote:
> > > Hi Tao,
> > >
> > > On Mon, Feb 21, 2022 at 03:09:24PM +0800, Tao Liu wrote:
> > > > Hi Philipp,
> > > >
> > > > I noticed there are ^M characters after the lines which you modified
> > > > from this patch set.
> > > >
> > > > For example in the first patch:
> > > >
> > > > -StandardOutput=syslog
> > > > -StandardError=syslog+console
> > > > +StandardOutput=journal^M
> > > > +StandardError=journal+console^M
> > > >
> > > > I think the ^M chars are unnecessary, and should be removed.
> > >
> > > This should be a know issue [1].
> >
> > hmm... Interesting, it looks like an email-mutt issue right?
>
> It's likely an email relay server adds these trailing ^Ms.
> > Just for curiosity, Will you merge the patches without any modification
> > on the patch set or will you use tools like dox2unix to strip these chars
> > before the merge?
>
> I use a tool [2] from Kairui to remove trailing ^Ms before applying them.
> But that tool can process one patch one time.
>
> [2] https://gitlab.cee.redhat.com/kasong/dot-home/-/blob/master/misc/format-patc...
> > I didn't use git am much, this will be the first time I encountered this, if
> > the patches are OK , then please ignore my comments...
> >
> > Thanks,
> > Tao Liu
> > > It would be good for Phillipp to push the changes to a remote repo
> > > instead.
> > >
> > > [1] https://pagure.io/fedora-infrastructure/issue/9831
> > > > Btw, the checkpatch.pl tool gives the similar results on my machine:
> > > >
> > > > ERROR: DOS line endings
> > > > #32: FILE: dracut-kdump-capture.service:23:
> > > > +StandardOutput=journal^M$
> > > >
> > > > ERROR: DOS line endings
> > > > #33: FILE: dracut-kdump-capture.service:24:
> > > > +StandardError=journal+console^M$
> > > >
> > > > I don't know if it is caused by your editor or my mutt program when downloading
> > > > the patches.
> > > >
> > > > As for the other patches, I'm still reviewing them, please wait for a while.
> > > >
> > > > Thanks,
> > > > Tao Liu
> > > >
> > > > On Sat, Feb 5, 2022 at 1:34 AM Philipp Rudo  wrote:
> > > > > Hi,
> > > > >
> > > > > while looking into transforming mkdumprd and mkfadumprd into library functions
> > > > > I noticed various nits in kdumpctl. This series addresses them.
> > > > >
> > > > > The series is made up of two parts:
> > > > >
> > > > > Patches 1-8 are small independent cleanups and fixes.
> > > > >
> > > > > Patches 9-15 tries to reduce the number of file accesses to /etc/kdump.conf. It
> > > > > achieves this by only parsing kdump.conf once in check_config and storing the
> > > > > parsed values in an array. Later accesses can then simply use the value stored
> > > > > in the array instead of calling kdump_get_conf_val.
> > > > >
> > > > > Thanks
> > > > > Philipp
> > > > >
> > > > > Philipp Rudo (15):
> > > > >   kdump-capture.service: switch to journal for stdout
> > > > >   kdumpctl: source dependencies before defining globals
> > > > >   kdump-lib: fix typo in variable name
> > > > >   kdumpctl: remove unnecessary uses of $?
> > > > >   kdump-lib-initramfs: merge definitions for default ssh key
> > > > >   kdumpctl: fix comment in check_and_wait_network_ready
> > > > >   kdumpctl: forbid aliases from ssh config
> > > > >   kdumpctl: simplify propagate_ssh_key
> > > > >   kdumpctl: merge check_ssh_config into check_config
> > > > >   kdumpctl: reduce file operations on kdump.conf
> > > > >   kdumpctl: drop SAVE_PATH variable
> > > > >   kdumpctl: drop SSH_KEY_LOCATION variable
> > > > >   kdumpctl: drop DUMP_TARGET variable
> > > > >   kdumpctl: remove kdump_get_conf_val in save_raw
> > > > >   kdumpctl: simplify local_fs_dump_target
> > > > >
> > > > > dracut-kdump-capture.service |   4 +-
> > > > >  dracut-kdump.sh              |   2 +-
> > > > >  kdump-lib-initramfs.sh       |   1 +
> > > > >  kdump-lib.sh                 |  14 +-
> > > > >  kdumpctl                     | 258 ++++++++++++++++-------------------
> > > > >  mkdumprd                     |   2 +-
> > > > >  6 files changed, 128 insertions(+), 153 deletions(-)
> > > > >
> > > > > --
> > > > > 2.34.1
> > > > > _______________________________________________
> > > > > kexec mailing list -- [e-mail]
> > > > > To unsubscribe send an email to [e-mail]
> > > > > 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/[e-mail]
> > > > > Do not reply to spam on the list, report it: https://pagure.io/fedora-infrastructure
> > > >
> > > > kexec mailing list -- [e-mail]
> > > > To unsubscribe send an email to [e-mail]
> > > > 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/[e-mail]
> > > > Do not reply to spam on the list, report it: https://pagure.io/fedora-infrastructure
> > >
> > > --
> > > Best regards,
> > > Coiby
>
> --
> Best regards,
> Coiby

[Philipp Rudo]

Hi Tao,

On Mon, 21 Feb 2022 18:03:05 +0800
Tao Liu  wrote:
> Hi Coiby,
>
> Thanks for the info!

it's a real pain, that the mail server changes the patches...

I'll try to push the series to src.fedoraproject.org. But currently it
doesn't allow me to push to my fork...
> BTW, for this patch set, I have reviewed patch 1~8, which are LGTM.
> You can add
> Reviewed-by: Tao Liu 
> to these patches. So I don't need to send the signature one by one patch.
>
> I will look into the remaining patches 9~15 tomorrow.

Thanks a lot.

Philipp
> Thanks,
> Tao Liu
>
> On Mon, Feb 21, 2022 at 5:41 PM Coiby Xu  wrote:
> > On Mon, Feb 21, 2022 at 04:19:22PM +0800, Tao Liu wrote:
> > > Hi Coiby,
> > >
> > > On Mon, Feb 21, 2022 at 3:41 PM Coiby Xu  wrote:
> > > > Hi Tao,
> > > >
> > > > On Mon, Feb 21, 2022 at 03:09:24PM +0800, Tao Liu wrote:
> > > > > Hi Philipp,
> > > > >
> > > > > I noticed there are ^M characters after the lines which you modified
> > > > > from this patch set.
> > > > >
> > > > > For example in the first patch:
> > > > >
> > > > > -StandardOutput=syslog
> > > > > -StandardError=syslog+console
> > > > > +StandardOutput=journal^M
> > > > > +StandardError=journal+console^M
> > > > >
> > > > > I think the ^M chars are unnecessary, and should be removed.
> > > >
> > > > This should be a know issue [1].
> > >
> > > hmm... Interesting, it looks like an email-mutt issue right?
> >
> > It's likely an email relay server adds these trailing ^Ms.
> > > Just for curiosity, Will you merge the patches without any modification
> > > on the patch set or will you use tools like dox2unix to strip these chars
> > > before the merge?
> >
> > I use a tool [2] from Kairui to remove trailing ^Ms before applying them.
> > But that tool can process one patch one time.
> >
> > [2] https://gitlab.cee.redhat.com/kasong/dot-home/-/blob/master/misc/format-patc...
> > > I didn't use git am much, this will be the first time I encountered this, if
> > > the patches are OK , then please ignore my comments...
> > >
> > > Thanks,
> > > Tao Liu
> > > > It would be good for Phillipp to push the changes to a remote repo
> > > > instead.
> > > >
> > > > [1] https://pagure.io/fedora-infrastructure/issue/9831
> > > > > Btw, the checkpatch.pl tool gives the similar results on my machine:
> > > > >
> > > > > ERROR: DOS line endings
> > > > > #32: FILE: dracut-kdump-capture.service:23:
> > > > > +StandardOutput=journal^M$
> > > > >
> > > > > ERROR: DOS line endings
> > > > > #33: FILE: dracut-kdump-capture.service:24:
> > > > > +StandardError=journal+console^M$
> > > > >
> > > > > I don't know if it is caused by your editor or my mutt program when downloading
> > > > > the patches.
> > > > >
> > > > > As for the other patches, I'm still reviewing them, please wait for a while.
> > > > >
> > > > > Thanks,
> > > > > Tao Liu
> > > > >
> > > > > On Sat, Feb 5, 2022 at 1:34 AM Philipp Rudo  wrote:
> > > > > > Hi,
> > > > > >
> > > > > > while looking into transforming mkdumprd and mkfadumprd into library functions
> > > > > > I noticed various nits in kdumpctl. This series addresses them.
> > > > > >
> > > > > > The series is made up of two parts:
> > > > > >
> > > > > > Patches 1-8 are small independent cleanups and fixes.
> > > > > >
> > > > > > Patches 9-15 tries to reduce the number of file accesses to /etc/kdump.conf. It
> > > > > > achieves this by only parsing kdump.conf once in check_config and storing the
> > > > > > parsed values in an array. Later accesses can then simply use the value stored
> > > > > > in the array instead of calling kdump_get_conf_val.
> > > > > >
> > > > > > Thanks
> > > > > > Philipp
> > > > > >
> > > > > > Philipp Rudo (15):
> > > > > >   kdump-capture.service: switch to journal for stdout
> > > > > >   kdumpctl: source dependencies before defining globals
> > > > > >   kdump-lib: fix typo in variable name
> > > > > >   kdumpctl: remove unnecessary uses of $?
> > > > > >   kdump-lib-initramfs: merge definitions for default ssh key
> > > > > >   kdumpctl: fix comment in check_and_wait_network_ready
> > > > > >   kdumpctl: forbid aliases from ssh config
> > > > > >   kdumpctl: simplify propagate_ssh_key
> > > > > >   kdumpctl: merge check_ssh_config into check_config
> > > > > >   kdumpctl: reduce file operations on kdump.conf
> > > > > >   kdumpctl: drop SAVE_PATH variable
> > > > > >   kdumpctl: drop SSH_KEY_LOCATION variable
> > > > > >   kdumpctl: drop DUMP_TARGET variable
> > > > > >   kdumpctl: remove kdump_get_conf_val in save_raw
> > > > > >   kdumpctl: simplify local_fs_dump_target
> > > > > >
> > > > > > dracut-kdump-capture.service |   4 +-
> > > > > >  dracut-kdump.sh              |   2 +-
> > > > > >  kdump-lib-initramfs.sh       |   1 +
> > > > > >  kdump-lib.sh                 |  14 +-
> > > > > >  kdumpctl                     | 258 ++++++++++++++++-------------------
> > > > > >  mkdumprd                     |   2 +-
> > > > > >  6 files changed, 128 insertions(+), 153 deletions(-)
> > > > > >
> > > > > > --
> > > > > > 2.34.1
> > > > > > _______________________________________________
> > > > > > kexec mailing list -- [e-mail]
> > > > > > To unsubscribe send an email to [e-mail]
> > > > > > 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/[e-mail]
> > > > > > Do not reply to spam on the list, report it: https://pagure.io/fedora-infrastructure
> > > > >
> > > > > kexec mailing list -- [e-mail]
> > > > > To unsubscribe send an email to [e-mail]
> > > > > 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/[e-mail]
> > > > > Do not reply to spam on the list, report it: https://pagure.io/fedora-infrastructure
> > > >
> > > > --
> > > > Best regards,
> > > > Coiby
> >
> > --
> > Best regards,
> > Coiby

[Coiby Xu]

Hi Philipp,

On Mon, Feb 21, 2022 at 04:38:54PM +0100, Philipp Rudo wrote:
> Hi Tao,
>
> On Mon, 21 Feb 2022 18:03:05 +0800
> Tao Liu  wrote:
> > Hi Coiby,
> >
> > Thanks for the info!
>
> it's a real pain, that the mail server changes the patches...
>
> I'll try to push the series to src.fedoraproject.org. But currently it
> doesn't allow me to push to my fork...

Have you added your SSH pub key to https://accounts.fedoraproject.org?

[Philipp Rudo]

Hi Coiby,

On Tue, 22 Feb 2022 14:28:55 +0800
Coiby Xu  wrote:
> Hi Philipp,
>
> On Mon, Feb 21, 2022 at 04:38:54PM +0100, Philipp Rudo wrote:
> > Hi Tao,
> >
> > On Mon, 21 Feb 2022 18:03:05 +0800
> > Tao Liu  wrote:
> > > Hi Coiby,
> > >
> > > Thanks for the info!
> >
> > it's a real pain, that the mail server changes the patches...
> >
> > I'll try to push the series to src.fedoraproject.org. But currently it
> > doesn't allow me to push to my fork...
>
> Have you added your SSH pub key to https://accounts.fedoraproject.org?

yes, I have. But I don't think this makes a difference. I'm no packager
and thus shouldn't have direct ssh access to dist-git.

I've followed these instructions [1]. But when I try to push it only
shows me a link to id.fedoraproject.org. When I click it and log in it
redirects me to localhost:12345 (not a joke) with some verification
code where I get stuck...

Thanks
Philipp

[1] https://docs.fedoraproject.org/en-US/ci/pull-requests/

[Coiby Xu]

On Tue, Feb 22, 2022 at 10:55:12AM +0100, Philipp Rudo wrote:
> Hi Coiby,
>
> On Tue, 22 Feb 2022 14:28:55 +0800
> Coiby Xu  wrote:
> > Hi Philipp,
> >
> > On Mon, Feb 21, 2022 at 04:38:54PM +0100, Philipp Rudo wrote:
> > > Hi Tao,
> > >
> > > On Mon, 21 Feb 2022 18:03:05 +0800
> > > Tao Liu  wrote:
> > > > Hi Coiby,
> > > >
> > > > Thanks for the info!
> > >
> > > it's a real pain, that the mail server changes the patches...
> > >
> > > I'll try to push the series to src.fedoraproject.org. But currently it
> > > doesn't allow me to push to my fork...
> >
> > Have you added your SSH pub key to https://accounts.fedoraproject.org?
>
> yes, I have. But I don't think this makes a difference. I'm no packager
> and thus shouldn't have direct ssh access to dist-git.

I thought you pushed the changes to a fork like [2]. You can fork the
repo then you won't have permission issue.

[2] https://src.fedoraproject.org/fork/coiby/rpms/kexec-tools
> I've followed these instructions [1]. But when I try to push it only
> shows me a link to id.fedoraproject.org. When I click it and log in it
> redirects me to localhost:12345 (not a joke) with some verification
> code where I get stuck...

This could be related to 2-factor authentication. But I haven't
encountered this issue when pushing change to my fork [2].
> Thanks
> Philipp
>
> [1] https://docs.fedoraproject.org/en-US/ci/pull-requests/

[Philipp Rudo]

Hi Coiby,

On Wed, 23 Feb 2022 08:50:32 +0800
Coiby Xu  wrote:
> On Tue, Feb 22, 2022 at 10:55:12AM +0100, Philipp Rudo wrote:
> > Hi Coiby,
> >
> > On Tue, 22 Feb 2022 14:28:55 +0800
> > Coiby Xu  wrote:
> > > Hi Philipp,
> > >
> > > On Mon, Feb 21, 2022 at 04:38:54PM +0100, Philipp Rudo wrote:
> > > > Hi Tao,
> > > >
> > > > On Mon, 21 Feb 2022 18:03:05 +0800
> > > > Tao Liu  wrote:
> > > > > Hi Coiby,
> > > > >
> > > > > Thanks for the info!
> > > >
> > > > it's a real pain, that the mail server changes the patches...
> > > >
> > > > I'll try to push the series to src.fedoraproject.org. But currently it
> > > > doesn't allow me to push to my fork...
> > >
> > > Have you added your SSH pub key to https://accounts.fedoraproject.org?
> >
> > yes, I have. But I don't think this makes a difference. I'm no packager
> > and thus shouldn't have direct ssh access to dist-git.
>
> I thought you pushed the changes to a fork like [2]. You can fork the
> repo then you won't have permission issue.
>
> [2] https://src.fedoraproject.org/fork/coiby/rpms/kexec-tools

yes, I tried to push to my fork [3]. The problem is that I'm not in the
packager group and thus cannot access dist-git via ssh. This is
explained in [1]

"Contributors that are not in the packager group cannot ssh into
dist-git. This is for security reasons and will not be changed.

However, pagure on dist-git supports now pushing via https."

When I try to push via https as described in [1]...

[3] https://src.fedoraproject.org/fork/prudo/rpms/kexec-tools
> > I've followed these instructions [1]. But when I try to push it only
> > shows me a link to id.fedoraproject.org. When I click it and log in it
> > redirects me to localhost:12345 (not a joke) with some verification
> > code where I get stuck...
>
> This could be related to 2-factor authentication. But I haven't
> encountered this issue when pushing change to my fork [2].

... I end up in the situation described above. BTW the login with 2FA
works. It's the redirect afterwards that fails/ends up in nirvana.

I believe difference between us is that you are the Fedora maintainer.
So you are in the packager group and can access everything via ssh
which will take care of authentication.

Thanks
Philipp
> > Thanks
> > Philipp
> >
> > [1] https://docs.fedoraproject.org/en-US/ci/pull-requests/

[Coiby Xu]

On Thu, Feb 24, 2022 at 06:11:03PM +0100, Philipp Rudo wrote:
> Hi Coiby,
>
> On Wed, 23 Feb 2022 08:50:32 +0800
> Coiby Xu  wrote:
> > On Tue, Feb 22, 2022 at 10:55:12AM +0100, Philipp Rudo wrote:
> > > Hi Coiby,
> > >
> > > On Tue, 22 Feb 2022 14:28:55 +0800
> > > Coiby Xu  wrote:
> > > > Hi Philipp,
> > > >
> > > > On Mon, Feb 21, 2022 at 04:38:54PM +0100, Philipp Rudo wrote:
> > > > > Hi Tao,
> > > > >
> > > > > On Mon, 21 Feb 2022 18:03:05 +0800
> > > > > Tao Liu  wrote:
> > > > > > Hi Coiby,
> > > > > >
> > > > > > Thanks for the info!
> > > > >
> > > > > it's a real pain, that the mail server changes the patches...
> > > > >
> > > > > I'll try to push the series to src.fedoraproject.org. But currently it
> > > > > doesn't allow me to push to my fork...
> > > >
> > > > Have you added your SSH pub key to https://accounts.fedoraproject.org?
> > >
> > > yes, I have. But I don't think this makes a difference. I'm no packager
> > > and thus shouldn't have direct ssh access to dist-git.
> >
> > I thought you pushed the changes to a fork like [2]. You can fork the
> > repo then you won't have permission issue.
> >
> > [2] https://src.fedoraproject.org/fork/coiby/rpms/kexec-tools
>
> yes, I tried to push to my fork [3]. The problem is that I'm not in the
> packager group and thus cannot access dist-git via ssh. This is
> explained in [1]
>
> "Contributors that are not in the packager group cannot ssh into
> dist-git. This is for security reasons and will not be changed.
>
> However, pagure on dist-git supports now pushing via https."

Thanks for the info!
> When I try to push via https as described in [1]...
>
> [3] https://src.fedoraproject.org/fork/prudo/rpms/kexec-tools
> > > I've followed these instructions [1]. But when I try to push it only
> > > shows me a link to id.fedoraproject.org. When I click it and log in it
> > > redirects me to localhost:12345 (not a joke) with some verification
> > > code where I get stuck...
> >
> > This could be related to 2-factor authentication. But I haven't
> > encountered this issue when pushing change to my fork [2].
>
> ... I end up in the situation described above. BTW the login with 2FA
> works. It's the redirect afterwards that fails/ends up in nirvana.
>
> I believe difference between us is that you are the Fedora maintainer.
> So you are in the packager group and can access everything via ssh
> which will take care of authentication.

I think I'm in the packager group because of having requested [4] sponsorship for
dmidecode. You can file a similar ticket as [4] and I can sponsor you. Or
your can push your change to gitlab or github if it is more convenient for
you.

[4] https://pagure.io/packager-sponsors/issue/469
> Thanks
> Philipp
> > > Thanks
> > > Philipp
> > >
> > > [1] https://docs.fedoraproject.org/en-US/ci/pull-requests/

[Coiby Xu]

On Fri, Feb 25, 2022 at 02:57:59PM +0800, Coiby Xu wrote:
> On Thu, Feb 24, 2022 at 06:11:03PM +0100, Philipp Rudo wrote:
> > Hi Coiby,

[...]
> I think I'm in the packager group because of having requested [4] sponsorship for
> dmidecode. You can file a similar ticket as [4] and I can sponsor you. Or
> your can push your change to gitlab or github if it is more convenient for
> you.

FYI, Tao has shared the tip of alt+shift+c i.e. decode_and_copy in mutt.
There is no need to push the changes to a remote repo now.

[Tao Liu]

Hi Philipp and Coiby,

On Mon, Feb 21, 2022 at 11:39 PM Philipp Rudo  wrote:
> Hi Tao,
>
> On Mon, 21 Feb 2022 18:03:05 +0800
> Tao Liu  wrote:
> > Hi Coiby,
> >
> > Thanks for the info!
>
> it's a real pain, that the mail server changes the patches...

I just talked to Dave, he told me the way he used to download the patches,
which worked fine to me.

Previously I downloaded the patches through mutt, tagging the patches
within the patch set, and shift+c to copy the mail to my local drive, then
git am to apply the mail in my source tree.

Now I use alt+shift+c, which will decode the base64 patch content within
mutt first then copy the mail to local drive, then git am as before, guess what,
the ^M chars disappear after git am the mail.

So it seems not a mail server issue, rather than mutt or git am issue.

Hope it can help.

Thanks,
Tao Liu
> I'll try to push the series to src.fedoraproject.org. But currently it
> doesn't allow me to push to my fork...
> > BTW, for this patch set, I have reviewed patch 1~8, which are LGTM.
> > You can add
> > Reviewed-by: Tao Liu 
> > to these patches. So I don't need to send the signature one by one patch.
> >
> > I will look into the remaining patches 9~15 tomorrow.
>
> Thanks a lot.
>
> Philipp
> > Thanks,
> > Tao Liu
> >
> > On Mon, Feb 21, 2022 at 5:41 PM Coiby Xu  wrote:
> > > On Mon, Feb 21, 2022 at 04:19:22PM +0800, Tao Liu wrote:
> > > > Hi Coiby,
> > > >
> > > > On Mon, Feb 21, 2022 at 3:41 PM Coiby Xu  wrote:
> > > > > Hi Tao,
> > > > >
> > > > > On Mon, Feb 21, 2022 at 03:09:24PM +0800, Tao Liu wrote:
> > > > > > Hi Philipp,
> > > > > >
> > > > > > I noticed there are ^M characters after the lines which you modified
> > > > > > from this patch set.
> > > > > >
> > > > > > For example in the first patch:
> > > > > >
> > > > > > -StandardOutput=syslog
> > > > > > -StandardError=syslog+console
> > > > > > +StandardOutput=journal^M
> > > > > > +StandardError=journal+console^M
> > > > > >
> > > > > > I think the ^M chars are unnecessary, and should be removed.
> > > > >
> > > > > This should be a know issue [1].
> > > >
> > > > hmm... Interesting, it looks like an email-mutt issue right?
> > >
> > > It's likely an email relay server adds these trailing ^Ms.
> > > > Just for curiosity, Will you merge the patches without any modification
> > > > on the patch set or will you use tools like dox2unix to strip these chars
> > > > before the merge?
> > >
> > > I use a tool [2] from Kairui to remove trailing ^Ms before applying them.
> > > But that tool can process one patch one time.
> > >
> > > [2] https://gitlab.cee.redhat.com/kasong/dot-home/-/blob/master/misc/format-patc...
> > > > I didn't use git am much, this will be the first time I encountered this, if
> > > > the patches are OK , then please ignore my comments...
> > > >
> > > > Thanks,
> > > > Tao Liu
> > > > > It would be good for Phillipp to push the changes to a remote repo
> > > > > instead.
> > > > >
> > > > > [1] https://pagure.io/fedora-infrastructure/issue/9831
> > > > > > Btw, the checkpatch.pl tool gives the similar results on my machine:
> > > > > >
> > > > > > ERROR: DOS line endings
> > > > > > #32: FILE: dracut-kdump-capture.service:23:
> > > > > > +StandardOutput=journal^M$
> > > > > >
> > > > > > ERROR: DOS line endings
> > > > > > #33: FILE: dracut-kdump-capture.service:24:
> > > > > > +StandardError=journal+console^M$
> > > > > >
> > > > > > I don't know if it is caused by your editor or my mutt program when downloading
> > > > > > the patches.
> > > > > >
> > > > > > As for the other patches, I'm still reviewing them, please wait for a while.
> > > > > >
> > > > > > Thanks,
> > > > > > Tao Liu
> > > > > >
> > > > > > On Sat, Feb 5, 2022 at 1:34 AM Philipp Rudo  wrote:
> > > > > > >
> > > > > > > Hi,
> > > > > > >
> > > > > > > while looking into transforming mkdumprd and mkfadumprd into library functions
> > > > > > > I noticed various nits in kdumpctl. This series addresses them.
> > > > > > >
> > > > > > > The series is made up of two parts:
> > > > > > >
> > > > > > > Patches 1-8 are small independent cleanups and fixes.
> > > > > > >
> > > > > > > Patches 9-15 tries to reduce the number of file accesses to /etc/kdump.conf. It
> > > > > > > achieves this by only parsing kdump.conf once in check_config and storing the
> > > > > > > parsed values in an array. Later accesses can then simply use the value stored
> > > > > > > in the array instead of calling kdump_get_conf_val.
> > > > > > >
> > > > > > > Thanks
> > > > > > > Philipp
> > > > > > >
> > > > > > > Philipp Rudo (15):
> > > > > > >   kdump-capture.service: switch to journal for stdout
> > > > > > >   kdumpctl: source dependencies before defining globals
> > > > > > >   kdump-lib: fix typo in variable name
> > > > > > >   kdumpctl: remove unnecessary uses of $?
> > > > > > >   kdump-lib-initramfs: merge definitions for default ssh key
> > > > > > >   kdumpctl: fix comment in check_and_wait_network_ready
> > > > > > >   kdumpctl: forbid aliases from ssh config
> > > > > > >   kdumpctl: simplify propagate_ssh_key
> > > > > > >   kdumpctl: merge check_ssh_config into check_config
> > > > > > >   kdumpctl: reduce file operations on kdump.conf
> > > > > > >   kdumpctl: drop SAVE_PATH variable
> > > > > > >   kdumpctl: drop SSH_KEY_LOCATION variable
> > > > > > >   kdumpctl: drop DUMP_TARGET variable
> > > > > > >   kdumpctl: remove kdump_get_conf_val in save_raw
> > > > > > >   kdumpctl: simplify local_fs_dump_target
> > > > > > >
> > > > > > >  dracut-kdump-capture.service |   4 +-
> > > > > > >  dracut-kdump.sh              |   2 +-
> > > > > > >  kdump-lib-initramfs.sh       |   1 +
> > > > > > >  kdump-lib.sh                 |  14 +-
> > > > > > >  kdumpctl                     | 258 ++++++++++++++++-------------------
> > > > > > >  mkdumprd                     |   2 +-
> > > > > > >  6 files changed, 128 insertions(+), 153 deletions(-)
> > > > > > >
> > > > > > > --
> > > > > > > 2.34.1
> > > > > > > _______________________________________________
> > > > > > > kexec mailing list -- [e-mail]
> > > > > > > To unsubscribe send an email to [e-mail]
> > > > > > > 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/[e-mail]
> > > > > > > Do not reply to spam on the list, report it: https://pagure.io/fedora-infrastructure
> > > > > > _______________________________________________
> > > > > > kexec mailing list -- [e-mail]
> > > > > > To unsubscribe send an email to [e-mail]
> > > > > > 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/[e-mail]
> > > > > > Do not reply to spam on the list, report it: https://pagure.io/fedora-infrastructure
> > > > >
> > > > > --
> > > > > Best regards,
> > > > > Coiby
> > >
> > > --
> > > Best regards,
> > > Coiby

[Dave Young]

On Thu, 24 Feb 2022 at 14:30, Tao Liu  wrote:
> Hi Philipp and Coiby,
>
> On Mon, Feb 21, 2022 at 11:39 PM Philipp Rudo  wrote:
> > Hi Tao,
> >
> > On Mon, 21 Feb 2022 18:03:05 +0800
> > Tao Liu  wrote:
> > > Hi Coiby,
> > >
> > > Thanks for the info!
> >
> > it's a real pain, that the mail server changes the patches...
>
> I just talked to Dave, he told me the way he used to download the patches,
> which worked fine to me.
>
> Previously I downloaded the patches through mutt, tagging the patches
> within the patch set, and shift+c to copy the mail to my local drive, then
> git am to apply the mail in my source tree.
>
> Now I use alt+shift+c, which will decode the base64 patch content within
> mutt first then copy the mail to local drive, then git am as before, guess what,
> the ^M chars disappear after git am the mail.
>
> So it seems not a mail server issue, rather than mutt or git am issue.

Hi Tao,

Actually mutt correctly decode the base64 content, and then it removed
the ^M before saving the mail to files :)

So our assumption is wrong, ^M is still added on server part

Please see below kde bz discussion:
https://bugs.kde.org/show_bug.cgi?id=55624

Someone mentioned below:

6.8.  Base64 Content-Transfer-Encoding
...
   Care must be taken to use the proper octets for line breaks if base64
   encoding is applied directly to text material that has not been
   converted to canonical form.  In particular, text line breaks must be
   converted into CRLF sequences prior to base64 encoding.  The
   important thing to note is that this may be done directly by the
   encoder rather than in a prior canonicalization step in some
   implementations.
...

So this is required by base64 encoding :(

Anyway the mutt decode-copy just works for me.
> Hope it can help.
>
> Thanks,
> Tao Liu
> > I'll try to push the series to src.fedoraproject.org. But currently it
> > doesn't allow me to push to my fork...
> > > BTW, for this patch set, I have reviewed patch 1~8, which are LGTM.
> > > You can add
> > > Reviewed-by: Tao Liu 
> > > to these patches. So I don't need to send the signature one by one patch.
> > >
> > > I will look into the remaining patches 9~15 tomorrow.
> >
> > Thanks a lot.
> >
> > Philipp
> > > Thanks,
> > > Tao Liu
> > >
> > > On Mon, Feb 21, 2022 at 5:41 PM Coiby Xu  wrote:
> > > > On Mon, Feb 21, 2022 at 04:19:22PM +0800, Tao Liu wrote:
> > > > > Hi Coiby,
> > > > >
> > > > > On Mon, Feb 21, 2022 at 3:41 PM Coiby Xu  wrote:
> > > > > > Hi Tao,
> > > > > >
> > > > > > On Mon, Feb 21, 2022 at 03:09:24PM +0800, Tao Liu wrote:
> > > > > > >Hi Philipp,
> > > > > > >
> > > > > > >I noticed there are ^M characters after the lines which you modified
> > > > > > >from this patch set.
> > > > > > >
> > > > > > >For example in the first patch:
> > > > > > >
> > > > > > >-StandardOutput=syslog
> > > > > > >-StandardError=syslog+console
> > > > > > >+StandardOutput=journal^M
> > > > > > >+StandardError=journal+console^M
> > > > > > >
> > > > > > >I think the ^M chars are unnecessary, and should be removed.
> > > > > >
> > > > > > This should be a know issue [1].
> > > > >
> > > > > hmm... Interesting, it looks like an email-mutt issue right?
> > > >
> > > > It's likely an email relay server adds these trailing ^Ms.
> > > > > Just for curiosity, Will you merge the patches without any modification
> > > > > on the patch set or will you use tools like dox2unix to strip these chars
> > > > > before the merge?
> > > >
> > > > I use a tool [2] from Kairui to remove trailing ^Ms before applying them.
> > > > But that tool can process one patch one time.
> > > >
> > > > [2] https://gitlab.cee.redhat.com/kasong/dot-home/-/blob/master/misc/format-patc...
> > > > > I didn't use git am much, this will be the first time I encountered this, if
> > > > > the patches are OK , then please ignore my comments...
> > > > >
> > > > > Thanks,
> > > > > Tao Liu
> > > > > > It would be good for Phillipp to push the changes to a remote repo
> > > > > > instead.
> > > > > >
> > > > > > [1] https://pagure.io/fedora-infrastructure/issue/9831
> > > > > > >
> > > > > > >Btw, the checkpatch.pl tool gives the similar results on my machine:
> > > > > > >
> > > > > > >ERROR: DOS line endings
> > > > > > >#32: FILE: dracut-kdump-capture.service:23:
> > > > > > >+StandardOutput=journal^M$
> > > > > > >
> > > > > > >ERROR: DOS line endings
> > > > > > >#33: FILE: dracut-kdump-capture.service:24:
> > > > > > >+StandardError=journal+console^M$
> > > > > > >
> > > > > > >I don't know if it is caused by your editor or my mutt program when downloading
> > > > > > >the patches.
> > > > > > >
> > > > > > >As for the other patches, I'm still reviewing them, please wait for a while.
> > > > > > >
> > > > > > >Thanks,
> > > > > > >Tao Liu
> > > > > > >
> > > > > > >
> > > > > > >On Sat, Feb 5, 2022 at 1:34 AM Philipp Rudo  wrote:
> > > > > > >>
> > > > > > >> Hi,
> > > > > > >>
> > > > > > >> while looking into transforming mkdumprd and mkfadumprd into library functions
> > > > > > >> I noticed various nits in kdumpctl. This series addresses them.
> > > > > > >>
> > > > > > >> The series is made up of two parts:
> > > > > > >>
> > > > > > >> Patches 1-8 are small independent cleanups and fixes.
> > > > > > >>
> > > > > > >> Patches 9-15 tries to reduce the number of file accesses to /etc/kdump.conf. It
> > > > > > >> achieves this by only parsing kdump.conf once in check_config and storing the
> > > > > > >> parsed values in an array. Later accesses can then simply use the value stored
> > > > > > >> in the array instead of calling kdump_get_conf_val.
> > > > > > >>
> > > > > > >> Thanks
> > > > > > >> Philipp
> > > > > > >>
> > > > > > >> Philipp Rudo (15):
> > > > > > >>   kdump-capture.service: switch to journal for stdout
> > > > > > >>   kdumpctl: source dependencies before defining globals
> > > > > > >>   kdump-lib: fix typo in variable name
> > > > > > >>   kdumpctl: remove unnecessary uses of $?
> > > > > > >>   kdump-lib-initramfs: merge definitions for default ssh key
> > > > > > >>   kdumpctl: fix comment in check_and_wait_network_ready
> > > > > > >>   kdumpctl: forbid aliases from ssh config
> > > > > > >>   kdumpctl: simplify propagate_ssh_key
> > > > > > >>   kdumpctl: merge check_ssh_config into check_config
> > > > > > >>   kdumpctl: reduce file operations on kdump.conf
> > > > > > >>   kdumpctl: drop SAVE_PATH variable
> > > > > > >>   kdumpctl: drop SSH_KEY_LOCATION variable
> > > > > > >>   kdumpctl: drop DUMP_TARGET variable
> > > > > > >>   kdumpctl: remove kdump_get_conf_val in save_raw
> > > > > > >>   kdumpctl: simplify local_fs_dump_target
> > > > > > >>
> > > > > > >>  dracut-kdump-capture.service |   4 +-
> > > > > > >>  dracut-kdump.sh              |   2 +-
> > > > > > >>  kdump-lib-initramfs.sh       |   1 +
> > > > > > >>  kdump-lib.sh                 |  14 +-
> > > > > > >>  kdumpctl                     | 258 ++++++++++++++++-------------------
> > > > > > >>  mkdumprd                     |   2 +-
> > > > > > >>  6 files changed, 128 insertions(+), 153 deletions(-)
> > > > > > >>
> > > > > > >> --
> > > > > > >> 2.34.1
> > > > > > >> _______________________________________________
> > > > > > >> kexec mailing list -- [e-mail]
> > > > > > >> To unsubscribe send an email to [e-mail]
> > > > > > >> 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/[e-mail]
> > > > > > >> Do not reply to spam on the list, report it: https://pagure.io/fedora-infrastructure
> > > > > > >_______________________________________________
> > > > > > >kexec mailing list -- [e-mail]
> > > > > > >To unsubscribe send an email to [e-mail]
> > > > > > >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/[e-mail]
> > > > > > >Do not reply to spam on the list, report it: https://pagure.io/fedora-infrastructure
> > > > > >
> > > > > > --
> > > > > > Best regards,
> > > > > > Coiby
> > > >
> > > > --
> > > > Best regards,
> > > > Coiby
>
> kexec mailing list -- [e-mail]
> To unsubscribe send an email to [e-mail]
> 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/[e-mail]
> Do not reply to spam on the list, report it: https://pagure.io/fedora-infrastructure

[Tao Liu]

Hi Dave,

On Thu, Feb 24, 2022 at 2:43 PM Dave Young  wrote:
> On Thu, 24 Feb 2022 at 14:30, Tao Liu  wrote:
> > Hi Philipp and Coiby,
> >
> > On Mon, Feb 21, 2022 at 11:39 PM Philipp Rudo  wrote:
> > > Hi Tao,
> > >
> > > On Mon, 21 Feb 2022 18:03:05 +0800
> > > Tao Liu  wrote:
> > > > Hi Coiby,
> > > >
> > > > Thanks for the info!
> > >
> > > it's a real pain, that the mail server changes the patches...
> >
> > I just talked to Dave, he told me the way he used to download the patches,
> > which worked fine to me.
> >
> > Previously I downloaded the patches through mutt, tagging the patches
> > within the patch set, and shift+c to copy the mail to my local drive, then
> > git am to apply the mail in my source tree.
> >
> > Now I use alt+shift+c, which will decode the base64 patch content within
> > mutt first then copy the mail to local drive, then git am as before, guess what,
> > the ^M chars disappear after git am the mail.
> >
> > So it seems not a mail server issue, rather than mutt or git am issue.
>
> Hi Tao,
>
> Actually mutt correctly decode the base64 content, and then it removed
> the ^M before saving the mail to files :)
>
> So our assumption is wrong, ^M is still added on server part
>
> Please see below kde bz discussion:
> https://bugs.kde.org/show_bug.cgi?id=55624
>
> Someone mentioned below:
>
> 6.8.  Base64 Content-Transfer-Encoding
> ...
>    Care must be taken to use the proper octets for line breaks if base64
>    encoding is applied directly to text material that has not been
>    converted to canonical form.  In particular, text line breaks must be
>    converted into CRLF sequences prior to base64 encoding.  The
>    important thing to note is that this may be done directly by the
>    encoder rather than in a prior canonicalization step in some
>    implementations.
> ...
>
> So this is required by base64 encoding :(

OK, I see, thanks for the explanation!

Thanks,
Tao Liu
> Anyway the mutt decode-copy just works for me.
> > Hope it can help.
> >
> > Thanks,
> > Tao Liu
> > > I'll try to push the series to src.fedoraproject.org. But currently it
> > > doesn't allow me to push to my fork...
> > > > BTW, for this patch set, I have reviewed patch 1~8, which are LGTM.
> > > > You can add
> > > > Reviewed-by: Tao Liu 
> > > > to these patches. So I don't need to send the signature one by one patch.
> > > >
> > > > I will look into the remaining patches 9~15 tomorrow.
> > >
> > > Thanks a lot.
> > >
> > > Philipp
> > > > Thanks,
> > > > Tao Liu
> > > >
> > > > On Mon, Feb 21, 2022 at 5:41 PM Coiby Xu  wrote:
> > > > > On Mon, Feb 21, 2022 at 04:19:22PM +0800, Tao Liu wrote:
> > > > > > Hi Coiby,
> > > > > >
> > > > > > On Mon, Feb 21, 2022 at 3:41 PM Coiby Xu  wrote:
> > > > > > >
> > > > > > > Hi Tao,
> > > > > > >
> > > > > > > On Mon, Feb 21, 2022 at 03:09:24PM +0800, Tao Liu wrote:
> > > > > > > >Hi Philipp,
> > > > > > > >
> > > > > > > >I noticed there are ^M characters after the lines which you modified
> > > > > > > >from this patch set.
> > > > > > > >
> > > > > > > >For example in the first patch:
> > > > > > > >
> > > > > > > >-StandardOutput=syslog
> > > > > > > >-StandardError=syslog+console
> > > > > > > >+StandardOutput=journal^M
> > > > > > > >+StandardError=journal+console^M
> > > > > > > >
> > > > > > > >I think the ^M chars are unnecessary, and should be removed.
> > > > > > >
> > > > > > > This should be a know issue [1].
> > > > > > >
> > > > > > hmm... Interesting, it looks like an email-mutt issue right?
> > > > >
> > > > > It's likely an email relay server adds these trailing ^Ms.
> > > > > > Just for curiosity, Will you merge the patches without any modification
> > > > > > on the patch set or will you use tools like dox2unix to strip these chars
> > > > > > before the merge?
> > > > >
> > > > > I use a tool [2] from Kairui to remove trailing ^Ms before applying them.
> > > > > But that tool can process one patch one time.
> > > > >
> > > > > [2] https://gitlab.cee.redhat.com/kasong/dot-home/-/blob/master/misc/format-patc...
> > > > > > I didn't use git am much, this will be the first time I encountered this, if
> > > > > > the patches are OK , then please ignore my comments...
> > > > > >
> > > > > > Thanks,
> > > > > > Tao Liu
> > > > > >
> > > > > > > It would be good for Phillipp to push the changes to a remote repo
> > > > > > > instead.
> > > > > > >
> > > > > > > [1] https://pagure.io/fedora-infrastructure/issue/9831
> > > > > > > >
> > > > > > > >Btw, the checkpatch.pl tool gives the similar results on my machine:
> > > > > > > >
> > > > > > > >ERROR: DOS line endings
> > > > > > > >#32: FILE: dracut-kdump-capture.service:23:
> > > > > > > >+StandardOutput=journal^M$
> > > > > > > >
> > > > > > > >ERROR: DOS line endings
> > > > > > > >#33: FILE: dracut-kdump-capture.service:24:
> > > > > > > >+StandardError=journal+console^M$
> > > > > > > >
> > > > > > > >I don't know if it is caused by your editor or my mutt program when downloading
> > > > > > > >the patches.
> > > > > > > >
> > > > > > > >As for the other patches, I'm still reviewing them, please wait for a while.
> > > > > > > >
> > > > > > > >Thanks,
> > > > > > > >Tao Liu
> > > > > > > >
> > > > > > > >
> > > > > > > >On Sat, Feb 5, 2022 at 1:34 AM Philipp Rudo  wrote:
> > > > > > > >>
> > > > > > > >> Hi,
> > > > > > > >>
> > > > > > > >> while looking into transforming mkdumprd and mkfadumprd into library functions
> > > > > > > >> I noticed various nits in kdumpctl. This series addresses them.
> > > > > > > >>
> > > > > > > >> The series is made up of two parts:
> > > > > > > >>
> > > > > > > >> Patches 1-8 are small independent cleanups and fixes.
> > > > > > > >>
> > > > > > > >> Patches 9-15 tries to reduce the number of file accesses to /etc/kdump.conf. It
> > > > > > > >> achieves this by only parsing kdump.conf once in check_config and storing the
> > > > > > > >> parsed values in an array. Later accesses can then simply use the value stored
> > > > > > > >> in the array instead of calling kdump_get_conf_val.
> > > > > > > >>
> > > > > > > >> Thanks
> > > > > > > >> Philipp
> > > > > > > >>
> > > > > > > >> Philipp Rudo (15):
> > > > > > > >>   kdump-capture.service: switch to journal for stdout
> > > > > > > >>   kdumpctl: source dependencies before defining globals
> > > > > > > >>   kdump-lib: fix typo in variable name
> > > > > > > >>   kdumpctl: remove unnecessary uses of $?
> > > > > > > >>   kdump-lib-initramfs: merge definitions for default ssh key
> > > > > > > >>   kdumpctl: fix comment in check_and_wait_network_ready
> > > > > > > >>   kdumpctl: forbid aliases from ssh config
> > > > > > > >>   kdumpctl: simplify propagate_ssh_key
> > > > > > > >>   kdumpctl: merge check_ssh_config into check_config
> > > > > > > >>   kdumpctl: reduce file operations on kdump.conf
> > > > > > > >>   kdumpctl: drop SAVE_PATH variable
> > > > > > > >>   kdumpctl: drop SSH_KEY_LOCATION variable
> > > > > > > >>   kdumpctl: drop DUMP_TARGET variable
> > > > > > > >>   kdumpctl: remove kdump_get_conf_val in save_raw
> > > > > > > >>   kdumpctl: simplify local_fs_dump_target
> > > > > > > >>
> > > > > > > >>  dracut-kdump-capture.service |   4 +-
> > > > > > > >>  dracut-kdump.sh              |   2 +-
> > > > > > > >>  kdump-lib-initramfs.sh       |   1 +
> > > > > > > >>  kdump-lib.sh                 |  14 +-
> > > > > > > >>  kdumpctl                     | 258 ++++++++++++++++-------------------
> > > > > > > >>  mkdumprd                     |   2 +-
> > > > > > > >>  6 files changed, 128 insertions(+), 153 deletions(-)
> > > > > > > >>
> > > > > > > >> --
> > > > > > > >> 2.34.1
> > > > > > > >> _______________________________________________
> > > > > > > >> kexec mailing list -- [e-mail]
> > > > > > > >> To unsubscribe send an email to [e-mail]
> > > > > > > >> 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/[e-mail]
> > > > > > > >> Do not reply to spam on the list, report it: https://pagure.io/fedora-infrastructure
> > > > > > > >_______________________________________________
> > > > > > > >kexec mailing list -- [e-mail]
> > > > > > > >To unsubscribe send an email to [e-mail]
> > > > > > > >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/[e-mail]
> > > > > > > >Do not reply to spam on the list, report it: https://pagure.io/fedora-infrastructure
> > > > > > >
> > > > > > > --
> > > > > > > Best regards,
> > > > > > > Coiby
> > > > > > >
> > > > >
> > > > > --
> > > > > Best regards,
> > > > > Coiby
> >
> > kexec mailing list -- [e-mail]
> > To unsubscribe send an email to [e-mail]
> > 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/[e-mail]
> > Do not reply to spam on the list, report it: https://pagure.io/fedora-infrastructure

[Coiby Xu]

On Thu, Feb 24, 2022 at 02:42:48PM +0800, Dave Young wrote:
> On Thu, 24 Feb 2022 at 14:30, Tao Liu  wrote:
> > Hi Philipp and Coiby,
> >
> > On Mon, Feb 21, 2022 at 11:39 PM Philipp Rudo  wrote:
> > > Hi Tao,
> > >
> > > On Mon, 21 Feb 2022 18:03:05 +0800
> > > Tao Liu  wrote:
> > > > Hi Coiby,
> > > >
> > > > Thanks for the info!
> > >
> > > it's a real pain, that the mail server changes the patches...
> >
> > I just talked to Dave, he told me the way he used to download the patches,
> > which worked fine to me.
> >
> > Previously I downloaded the patches through mutt, tagging the patches
> > within the patch set, and shift+c to copy the mail to my local drive, then
> > git am to apply the mail in my source tree.
> >
> > Now I use alt+shift+c, which will decode the base64 patch content within
> > mutt first then copy the mail to local drive, then git am as before, guess what,
> > the ^M chars disappear after git am the mail.
> >
> > So it seems not a mail server issue, rather than mutt or git am issue.
>
> Hi Tao,
>
> Actually mutt correctly decode the base64 content, and then it removed
> the ^M before saving the mail to files :)
>
> So our assumption is wrong, ^M is still added on server part
>
> Please see below kde bz discussion:
> https://bugs.kde.org/show_bug.cgi?id=55624
>
> Someone mentioned below:
>
> 6.8.  Base64 Content-Transfer-Encoding
> ...
>   Care must be taken to use the proper octets for line breaks if base64
>   encoding is applied directly to text material that has not been
>   converted to canonical form.  In particular, text line breaks must be
>   converted into CRLF sequences prior to base64 encoding.  The
>   important thing to note is that this may be done directly by the
>   encoder rather than in a prior canonicalization step in some
>   implementations.
> ...
>
> So this is required by base64 encoding :(
>
> Anyway the mutt decode-copy just works for me.

Thanks for providing this clue! After doing more investigation, I think
the problem is "git am" as an email client doesn't convert these CRLF
sequences after doing base64 decoding. This issue seems to have been
there for a few years,
1. "git-am doesn't strip CRLF line endings when the mbox is base64-encoded - George Dunlap" 
     https://lore.kernel.org/git/[e-mail]/)
2. "1469098 – git am doesn't apply for base64 encoded mail"
     https://bugzilla.redhat.com/show_bug.cgi?id=1469098)

It seems git developer somehow doesn't want to fix this easy problem.

[Coiby Xu]

On Thu, Feb 24, 2022 at 02:28:55PM +0800, Tao Liu wrote:
> Hi Philipp and Coiby,

[..]
> Now I use alt+shift+c, which will decode the base64 patch content within
> mutt first then copy the mail to local drive, then git am as before, guess what,
> the ^M chars disappear after git am the mail.

Thanks for sharing the tip! It works for me.
> So it seems not a mail server issue, rather than mutt or git am issue.
>
> Hope it can help.
>
> Thanks,
> Tao Liu
> > I'll try to push the series to src.fedoraproject.org. But currently it
> > doesn't allow me to push to my fork...
> > > BTW, for this patch set, I have reviewed patch 1~8, which are LGTM.
> > > You can add
> > > Reviewed-by: Tao Liu 
> > > to these patches. So I don't need to send the signature one by one patch.
> > >
> > > I will look into the remaining patches 9~15 tomorrow.
> >
> > Thanks a lot.
> >
> > Philipp
> > > Thanks,
> > > Tao Liu
> > >
> > > On Mon, Feb 21, 2022 at 5:41 PM Coiby Xu  wrote:
> > > > On Mon, Feb 21, 2022 at 04:19:22PM +0800, Tao Liu wrote:
> > > > > Hi Coiby,
> > > > >
> > > > > On Mon, Feb 21, 2022 at 3:41 PM Coiby Xu  wrote:
> > > > > > Hi Tao,
> > > > > >
> > > > > > On Mon, Feb 21, 2022 at 03:09:24PM +0800, Tao Liu wrote:
> > > > > > >Hi Philipp,
> > > > > > >
> > > > > > >I noticed there are ^M characters after the lines which you modified
> > > > > > >from this patch set.
> > > > > > >
> > > > > > >For example in the first patch:
> > > > > > >
> > > > > > >-StandardOutput=syslog
> > > > > > >-StandardError=syslog+console
> > > > > > >+StandardOutput=journal^M
> > > > > > >+StandardError=journal+console^M
> > > > > > >
> > > > > > >I think the ^M chars are unnecessary, and should be removed.
> > > > > >
> > > > > > This should be a know issue [1].
> > > > >
> > > > > hmm... Interesting, it looks like an email-mutt issue right?
> > > >
> > > > It's likely an email relay server adds these trailing ^Ms.
> > > > > Just for curiosity, Will you merge the patches without any modification
> > > > > on the patch set or will you use tools like dox2unix to strip these chars
> > > > > before the merge?
> > > >
> > > > I use a tool [2] from Kairui to remove trailing ^Ms before applying them.
> > > > But that tool can process one patch one time.
> > > >
> > > > [2] https://gitlab.cee.redhat.com/kasong/dot-home/-/blob/master/misc/format-patc...
> > > > > I didn't use git am much, this will be the first time I encountered this, if
> > > > > the patches are OK , then please ignore my comments...
> > > > >
> > > > > Thanks,
> > > > > Tao Liu
> > > > > > It would be good for Phillipp to push the changes to a remote repo
> > > > > > instead.
> > > > > >
> > > > > > [1] https://pagure.io/fedora-infrastructure/issue/9831
> > > > > > >
> > > > > > >Btw, the checkpatch.pl tool gives the similar results on my machine:
> > > > > > >
> > > > > > >ERROR: DOS line endings
> > > > > > >#32: FILE: dracut-kdump-capture.service:23:
> > > > > > >+StandardOutput=journal^M$
> > > > > > >
> > > > > > >ERROR: DOS line endings
> > > > > > >#33: FILE: dracut-kdump-capture.service:24:
> > > > > > >+StandardError=journal+console^M$
> > > > > > >
> > > > > > >I don't know if it is caused by your editor or my mutt program when downloading
> > > > > > >the patches.
> > > > > > >
> > > > > > >As for the other patches, I'm still reviewing them, please wait for a while.
> > > > > > >
> > > > > > >Thanks,
> > > > > > >Tao Liu
> > > > > > >
> > > > > > >
> > > > > > >On Sat, Feb 5, 2022 at 1:34 AM Philipp Rudo  wrote:
> > > > > > >>
> > > > > > >> Hi,
> > > > > > >>
> > > > > > >> while looking into transforming mkdumprd and mkfadumprd into library functions
> > > > > > >> I noticed various nits in kdumpctl. This series addresses them.
> > > > > > >>
> > > > > > >> The series is made up of two parts:
> > > > > > >>
> > > > > > >> Patches 1-8 are small independent cleanups and fixes.
> > > > > > >>
> > > > > > >> Patches 9-15 tries to reduce the number of file accesses to /etc/kdump.conf. It
> > > > > > >> achieves this by only parsing kdump.conf once in check_config and storing the
> > > > > > >> parsed values in an array. Later accesses can then simply use the value stored
> > > > > > >> in the array instead of calling kdump_get_conf_val.
> > > > > > >>
> > > > > > >> Thanks
> > > > > > >> Philipp
> > > > > > >>
> > > > > > >> Philipp Rudo (15):
> > > > > > >>   kdump-capture.service: switch to journal for stdout
> > > > > > >>   kdumpctl: source dependencies before defining globals
> > > > > > >>   kdump-lib: fix typo in variable name
> > > > > > >>   kdumpctl: remove unnecessary uses of $?
> > > > > > >>   kdump-lib-initramfs: merge definitions for default ssh key
> > > > > > >>   kdumpctl: fix comment in check_and_wait_network_ready
> > > > > > >>   kdumpctl: forbid aliases from ssh config
> > > > > > >>   kdumpctl: simplify propagate_ssh_key
> > > > > > >>   kdumpctl: merge check_ssh_config into check_config
> > > > > > >>   kdumpctl: reduce file operations on kdump.conf
> > > > > > >>   kdumpctl: drop SAVE_PATH variable
> > > > > > >>   kdumpctl: drop SSH_KEY_LOCATION variable
> > > > > > >>   kdumpctl: drop DUMP_TARGET variable
> > > > > > >>   kdumpctl: remove kdump_get_conf_val in save_raw
> > > > > > >>   kdumpctl: simplify local_fs_dump_target
> > > > > > >>
> > > > > > >>  dracut-kdump-capture.service |   4 +-
> > > > > > >>  dracut-kdump.sh              |   2 +-
> > > > > > >>  kdump-lib-initramfs.sh       |   1 +
> > > > > > >>  kdump-lib.sh                 |  14 +-
> > > > > > >>  kdumpctl                     | 258 ++++++++++++++++-------------------
> > > > > > >>  mkdumprd                     |   2 +-
> > > > > > >>  6 files changed, 128 insertions(+), 153 deletions(-)
> > > > > > >>
> > > > > > >> --
> > > > > > >> 2.34.1
> > > > > > >> _______________________________________________
> > > > > > >> kexec mailing list -- [e-mail]
> > > > > > >> To unsubscribe send an email to [e-mail]
> > > > > > >> 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/[e-mail]
> > > > > > >> Do not reply to spam on the list, report it: https://pagure.io/fedora-infrastructure
> > > > > > >_______________________________________________
> > > > > > >kexec mailing list -- [e-mail]
> > > > > > >To unsubscribe send an email to [e-mail]
> > > > > > >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/[e-mail]
> > > > > > >Do not reply to spam on the list, report it: https://pagure.io/fedora-infrastructure
> > > > > >
> > > > > > --
> > > > > > Best regards,
> > > > > > Coiby
> > > >
> > > > --
> > > > Best regards,
> > > > Coiby

[Coiby Xu]

Hi Philipp,

Thanks for improving kdumpctl!

Except for the following two patches,
- [PATCH 02/15] kdumpctl: source dependencies before defining globals
- [PATCH 13/15] kdumpctl: drop DUMP_TARGET variable

this patch set looks good to me.  Do you want me to merge the 13 good
patches first or send all 15 patches in next version?

On Fri, Feb 04, 2022 at 06:34:14PM +0100, Philipp Rudo wrote:
> Hi,
>
> while looking into transforming mkdumprd and mkfadumprd into library functions
> I noticed various nits in kdumpctl. This series addresses them.
>
> The series is made up of two parts:
>
> Patches 1-8 are small independent cleanups and fixes.
>
> Patches 9-15 tries to reduce the number of file accesses to /etc/kdump.conf. It
> achieves this by only parsing kdump.conf once in check_config and storing the
> parsed values in an array. Later accesses can then simply use the value stored
> in the array instead of calling kdump_get_conf_val.
>
> Thanks
> Philipp
>
> Philipp Rudo (15):
>  kdump-capture.service: switch to journal for stdout
>  kdumpctl: source dependencies before defining globals
>  kdump-lib: fix typo in variable name
>  kdumpctl: remove unnecessary uses of $?
>  kdump-lib-initramfs: merge definitions for default ssh key
>  kdumpctl: fix comment in check_and_wait_network_ready
>  kdumpctl: forbid aliases from ssh config
>  kdumpctl: simplify propagate_ssh_key
>  kdumpctl: merge check_ssh_config into check_config
>  kdumpctl: reduce file operations on kdump.conf
>  kdumpctl: drop SAVE_PATH variable
>  kdumpctl: drop SSH_KEY_LOCATION variable
>  kdumpctl: drop DUMP_TARGET variable
>  kdumpctl: remove kdump_get_conf_val in save_raw
>  kdumpctl: simplify local_fs_dump_target
>
> dracut-kdump-capture.service |   4 +-
> dracut-kdump.sh              |   2 +-
> kdump-lib-initramfs.sh       |   1 +
> kdump-lib.sh                 |  14 +-
> kdumpctl                     | 258 ++++++++++++++++-------------------
> mkdumprd                     |   2 +-
> 6 files changed, 128 insertions(+), 153 deletions(-)
>
> -- 
> 2.34.1
> _______________________________________________
> kexec mailing list -- [e-mail]
> To unsubscribe send an email to [e-mail]
> 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/[e-mail]
> Do not reply to spam on the list, report it: https://pagure.io/fedora-infrastructure

[Philipp Rudo]

Hi Coiby,

On Tue, 1 Mar 2022 12:39:42 +0800
Coiby Xu  wrote:
> Hi Philipp,
>
> Thanks for improving kdumpctl!
>
> Except for the following two patches,
> [PATCH 02/15] kdumpctl: source dependencies before defining globals
> [PATCH 13/15] kdumpctl: drop DUMP_TARGET variable
>
> this patch set looks good to me.  Do you want me to merge the 13 good
> patches first or send all 15 patches in next version?

I'll send a v2 with path 2 dropped and patch 13 fixed.

Thanks
Philipp
> On Fri, Feb 04, 2022 at 06:34:14PM +0100, Philipp Rudo wrote:
> > Hi,
> >
> > while looking into transforming mkdumprd and mkfadumprd into library functions
> > I noticed various nits in kdumpctl. This series addresses them.
> >
> > The series is made up of two parts:
> >
> > Patches 1-8 are small independent cleanups and fixes.
> >
> > Patches 9-15 tries to reduce the number of file accesses to /etc/kdump.conf. It
> > achieves this by only parsing kdump.conf once in check_config and storing the
> > parsed values in an array. Later accesses can then simply use the value stored
> > in the array instead of calling kdump_get_conf_val.
> >
> > Thanks
> > Philipp
> >
> > Philipp Rudo (15):
> >  kdump-capture.service: switch to journal for stdout
> >  kdumpctl: source dependencies before defining globals
> >  kdump-lib: fix typo in variable name
> >  kdumpctl: remove unnecessary uses of $?
> >  kdump-lib-initramfs: merge definitions for default ssh key
> >  kdumpctl: fix comment in check_and_wait_network_ready
> >  kdumpctl: forbid aliases from ssh config
> >  kdumpctl: simplify propagate_ssh_key
> >  kdumpctl: merge check_ssh_config into check_config
> >  kdumpctl: reduce file operations on kdump.conf
> >  kdumpctl: drop SAVE_PATH variable
> >  kdumpctl: drop SSH_KEY_LOCATION variable
> >  kdumpctl: drop DUMP_TARGET variable
> >  kdumpctl: remove kdump_get_conf_val in save_raw
> >  kdumpctl: simplify local_fs_dump_target
> >
> > dracut-kdump-capture.service |   4 +-
> > dracut-kdump.sh              |   2 +-
> > kdump-lib-initramfs.sh       |   1 +
> > kdump-lib.sh                 |  14 +-
> > kdumpctl                     | 258 ++++++++++++++++-------------------
> > mkdumprd                     |   2 +-
> > 6 files changed, 128 insertions(+), 153 deletions(-)
> >
> > -- 
> > 2.34.1
> > _______________________________________________
> > kexec mailing list -- [e-mail]
> > To unsubscribe send an email to [e-mail]
> > 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/[e-mail]
> > Do not reply to spam on the list, report it: https://pagure.io/fedora-infrastructure