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