On Fri, Sep 3, 2021 at 7:58 PM Philipp Rudo <prudo(a)redhat.com> wrote:
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.
Great idea, I'll do it.
* 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)
Good idea, it's a simple fix indeed.
* 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.
Squashing 47 and 48 should be OK, changes in the logger are not too
much, one commit should be OK.
For patch 45 I did a bit more self-review and found some trivial
issues, I can do some adjusting.
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:
Thanks for all the help!
--
Best Regards,
Kairui Song