6:58 a.m.
Hi Kairui,
again, great series!
There are still some nits but to be fair almost all already existed and you only
had the bad luck to touch the line. However, I don't think it makes
sense to fix them in this series. The series as is is already quite large and a
huge improvement to before. Furthermore shellcheck still has many findings and
fixing them all in this series would be way too much. So please take my comment
as suggestions on what you could do next.
Having that said there are a few nits that should be fixed before pushing
* To the files that have to be POSIX compliant, add a short comment saying
exactly that. The shebang already indicates it but I think it is better to
be a little bit more verbose and better human readable. Something like this
should be more than enough.
# The code in this file might be run in an environment without bash.
# Any code added must be POSIX compliant.
* Fix the last shellcheck complaints about non-POSIX compliant code [1][2][3]
(I'm not totally sure if dracut-early-kdump.sh needs to be POSIX compliant
as it is run by the 1st kernel but the fix is so simple I would simply do it)
* Fix the last typos.
* Improve the commit massage in patch 47 and 48.
You got a little bit sloppy in the end and made much more changes than you
explain in the commit message. The quick and dirty solution would be to
simply squash both patches, take the commit message from patch 45 and simply
add the "make POSIX compatible" bits.
All in all I think it would have been better if you had picked one change,
e.g. fix SC2162 or remove the 'local' keyword for POSIX files etc., and
apply it to all files rather than pick one file and apply all the changes.
That would have made writing commit messages and my life as reviewer a little
bit easier. However, I don't think it makes sent to fix it now as
reassembling the full series is way to much effort and would introduce too
many new bugs. But keep it in mind for the next time.
I don't think you need to post a new version of the series. Although if you like
me to have an other look I'll be happy to help.
For the series with the nit above fixed
Acked-by: Philipp Rudo <prudo(a)redhat.com>
[1]
In dracut-early-kdump.sh line 44:
In dracut-early-kdump.sh line 67:
if [ $? == 0 ]; then
^-- SC3014: In POSIX sh, == in place of = is undefined.
[2]
In kdump-logger.sh line 116:
if [ "$UID" -ne 0 ]; then
^--^ SC3028: In POSIX sh, UID is undefined.
[3]
In kdump-logger.sh line 129:
exec 15>"$_systemdcatfile"
^-------------------^ SC3023: In POSIX sh, FDs outside 0-9 are
undefined.
On Thu, 19 Aug 2021 19:38:59 +0800
Kairui Song <kasong(a)redhat.com> wrote:
From: Kairui Song <kasong(a)redhat.com>
To: kexec(a)lists.fedoraproject.org
CC: Kairui Song <kasong(a)redhat.com>
Subject: [PATCH v2 00/49] Batch update and refactor
Date: Thu, 19 Aug 2021 19:38:59 +0800
This mass update is done with the assistant of shellcheck and shfmt.
This patch series fixed most syntax issues found by shellcheck,
tons of corner cases and issues are fixed.
Also simplified a lot of code according to suggestions by shellcheck.
Related shellcheck warn/error/suggest info links are included in the commit
message.
Since shellcheck can help to detect non-POSIX syntax issue in POSIX
script (#!/bin/sh), and the coding style in kexec-tools package is
very messy, this series refactored the code structure in following way:
- kdumpctl, mkdumprd, mkfadumprd, *-module-setup.sh and kdump-lib.sh
is "bash only" script. We rely on bash only syntax heavily, and they
will only be used in the first kernel, so just let them leverage bash
syntax for better maintainability and better performance.
- kdump-lib-initramfs.sh, and dracut-kdump.sh is now POSIX compatible,
and will be used in the second kernel.
This refactoring also enables kdump to use dash or busybox, instead of
bash in the second kernel, which improves performance and memory
usage. Tested with latest dracut on Fedora 34, now "dash" in second
kernel works well, it's faster than bash and save a lot of memory.
We may test dash or busybox as default kdump shell in the future for
better performance.
For reviewers:
Patch 25, 26, 30, 41, 46 is automatic code update using script and
tools like shfmt, other patch may need careful review.
Patch 1 added a editorconfig file for shfmt to work properly, which
is Acked by Pingfan but still posting it to make it easier to review.
Patch 2 - 29 focus on the bash part:
Patch 2, 3, 4, 5, 8 introduced new config file read and format helper,
which unified the config file reading in kexec-tools scripts. So space
in config file should be no longer a problem (previously random problem
may occur since kexec-tools scripts is using many different ways to read
the config file). And this make it much easier to clean up the code.
Patch 6, 7, 9 - 24 fixed many issues found with shellcheck in bash
scripts manually.
Some issues are already breaking kdump just no one noticed yet.
Patch 25, 26, 30 is an automatic code update using sed and shfmt, there
should be no code behaviour change.
Patch 27, 28, 29 batch fixed some common bash script issues manually
with hints from shellcheck.
Patch 31 - 48 focus on the POSIX part:
Patch 31 - 33 restructures the code, prepare to make
kdump-lib-initramfs.sh a POSIX script and drop kdump-lib.sh in kdump
kernel.
Patch 34 - 38 clean up code in dracut-kdump.sh, and fix misc issues
found by shellcheck manually, prepare to make it POSIX compatible.
Patch 39 contains a lot of code refactoring that makes dracut-kdump.sh POSIX
compatible.
Patch 40 contains a lot of code refactoring that makes kdump-lib-initramfs.sh
POSIX compatible.
Patch 41 is an automatic code update using sed, there should be no code
behaviour change.
Patch 42, 43 reworked how nmcli is being called by kexec-tools, fixed
word splitting issue and allow connection name to contain space.
Patch 44 optimized a sed call.
Patch 45 batch fixed some common bash script issues manually
with hints from shellcheck.
Patch 46 is an automatic code update using shfmt, there should be no code
behaviour change.
Patch 47, 48 contains a lot of change that makes kdump-logger.sh
POSIX compatible.
Patch 49 allows mkdumprd to use dash instead of bash in second kernel.
Update from V1:
- Fix many typos and code error, and fix a few more existing code issues
found while reviewing the patch, many thanks to Philipp Rudo.
- Add more info in commit message and cover letter.
Kairui Song (49):
Add a .editorconfig file
kdump-lib.sh: add a config format and read helper
kdump-lib.sh: add a config value retrive helper
kdump-lib.sh: use kdump_get_conf_val to read config values
kdumpctl: use kdump_get_conf_val to read config values
kdumpctl: get rid of a `wc` call
kdumpctl: fix fragile loops over find output
mkdumprd: use kdump_get_conf_val to read config values
mkdumprd: make dracut_args an array again
mkdumprd: remove some redundant echo
mkdumprd: fix multiple issues with get_ssh_size
mkdumprd: use array to store ssh arguments in mkdir_save_path_ssh
mkfadumprd: make _dracut_isolate_args an array
dracut-module-setup.sh: rework kdump_get_ip_route_field
dracut-module-setup.sh: remove an unused variable
dracut-module-setup.sh: fix _bondoptions wrong references
dracut-module-setup.sh: use "*" to expend array as string
dracut-module-setup.sh: fix a ambiguous variable reference
dracut-module-setup.sh: fix a loop over ls issue
dracut-module-setup.sh: make iscsi check fail early if cd failed
bash scripts: remove useless cat
bash scripts: get rid of expr and let
bash scripts: get rid of unnessary sed calls
bash scripts: always use "read -r"
bash scripts: use $(...) notation instead of legacy `...`
bash scripts: replace '[ ]' with '[[ ]]' for bash scripts
bash scripts: fix variable quoting issue
bash scripts: fix redundant exit code check
bash scripts: declare and assign separately
bash scripts: reformat with shfmt
Prepare to make kdump-lib-initramfs.sh a POSIX compatible lib
Merge kdump-error-handler.sh into kdump.sh
kdump-lib-initramfs.sh: move dump raleted functions to kdump.sh
dracut-kdump.sh: don't put KDUMP_SCRIPT_DIR in PATH
dracut-kdump.sh: remove add_dump_code
dracut-kdump.sh: simplify dump_ssh
dracut-kdump.sh: Use stat instead of ls to get vmcore size
dracut-kdump.sh: POSIX doesn't support pipefail
dracut-kdump.sh: make it POSIX compatible
kdump-lib-initramfs.sh: make it POSIX compatible
kdump-lib.sh: replace '[ ]' with '[[ ]]' and get rid of legacy ``
kdump-lib.sh: fix and simplify get_nmcli_value_by_field
kdump-lib.sh: fix word splitting of nmcli params
kdump-lib.sh: avoid calling sed multiple times in get_system_size
kdump-lib.sh: batch fix issues found with shellcheck
kdump-lib.sh: reformat with shfmt
kdump-logger.sh: refactor to remove some bash only syntax
kdump-logger.sh: make it POSIX compatible
mkdumprd: allow using dash
.editorconfig | 32 +
dracut-early-kdump-module-setup.sh | 11 +-
dracut-kdump-emergency.service | 2 +-
dracut-kdump-error-handler.sh | 10 -
dracut-kdump.sh | 511 ++++++++---
dracut-module-setup.sh | 668 +++++++-------
kdump-lib-initramfs.sh | 346 ++-----
kdump-lib.sh | 1356 +++++++++++++---------------
kdump-logger.sh | 117 +--
kdumpctl | 604 ++++++-------
kexec-tools.spec | 2 -
mkdumprd | 600 ++++++------
mkfadumprd | 21 +-
13 files changed, 2104 insertions(+), 2176 deletions(-)
create mode 100644 .editorconfig
delete mode 100755 dracut-kdump-error-handler.sh