rhkdump / kdump-utils

Kernel crash dump collection utilities
GNU General Public License v2.0
3 stars 12 forks source link

Add suport for erofs in the kdump initrd #33

Closed prudo1 closed 3 months ago

prudo1 commented 3 months ago

This series adds support for erofs images in the kdump initrd. The feature was introduced in dracut with this PR and will be included with dracut 104.

While working on the feature I got a little bit side tracked so this series also includes a few cleanups.


This change is Reviewable

coiby commented 3 months ago

Hi @prudo1,

This PR looks good to me except for some issues about shellcheck.

prudo1 commented 3 months ago

Hi @coiby,

unfortunately github doesn't allow to reply in-line so ...

Reviewed 1 of 1 files at r1, 15 of 15 files at r2, 5 of 5 files at r3, 1 of 1 files at r4, 3 of 3 files at r5, 2 of 2 files at r6, 2 of 2 files at r7, 1 of 1 files at r8, 1 of 1 files at r9, 1 of 1 files at r10, 1 of 1 files at r11, 1 of 1 files at r12, 1 of 1 files at r13, 2 of 2 files at r14, 3 of 3 files at r15, 2 of 2 files at r16, all commit messages. Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @prudo1)

mkdumprd line 16 at r9 (raw file):

# shellcheck source=/dev/null
. "$dracutbasedir"/dracut-functions.sh
# shellcheck source-path=SCRIPTDIR

shellcheck still complains,

In mkdumprd line 10:
        . /etc/sysconfig/kdump
          ^------------------^ SC1091 (info): Not following: /etc/sysconfig/kdump was not specified as input (see shellcheck -x).

In mkdumprd line 17:
. /lib/kdump/kdump-lib.sh
  ^---------------------^ SC1091 (info): Not following: /lib/kdump/kdump-lib.sh was not specified as input (see shellcheck -x).

In mkdumprd line 18:
. /lib/kdump/kdump-logger.sh
  ^------------------------^ SC1091 (info): Not following: /lib/kdump/kdump-logger.sh was not specified as input (see shellcheck -x).

Those are all only hints that you need to call shellcheck with option -x for it to follow those files. When you do that the warnings goes away. Same for most of the other warnings you mentioned.

mkfadumprd line 13 at r10 (raw file):

. "$dracutbasedir"/dracut-functions.sh
# shellcheck source-path=SCRIPTDIR
. /lib/kdump/kdump-lib.sh

Unforunately, shellcheck still makes the following complaints,

[...]

In mkfadumprd line 24: trap ' ^-- SC2154 (warning): ret is referenced but not assigned.

Ouch, this one was masked by using -x as ret is defined in the kdump-logger.sh... I've disabled the warning here and in mkdumprd. I'm not entirely sure why it doesn't show in kdumpctl. Shall I disable it there, too?

[...]

In case the version matters, I use shellcheck-0.9.0 on F40 .

I'm using the same version. Although, when I created the series I was still on F39. But IIRC the shellcheck version was the same.

dracut/99kdumpbase/module-setup.sh line 21 at r5 (raw file):

    mkdir -p "$_DRACUT_KDUMP_NM_TMP_DIR"

    # shellcheck source-path=SCRIPTDIR/../../

Unfortunately, this doesn't work since we use absolute path.

Right, I misunderstood the shellcheck documentation. I thought that it would always only use the filename and attach it to SCRIPTDIR. Do you know if there is a way to tell shellcheck to print out the absolute path it uses as source? That would be handy when adding those statements so you can verify that shellcheck uses the right file.

Anyway, I've dropped the use of source-path and explicitly set source every time. This should also solve the problem with source=/dev/null hiding problems.

dracut/99kdumpbase/module-setup.sh line 1110 at r5 (raw file):

    inst "/lib/kdump/kdump-lib-initramfs.sh" "/lib/kdump-lib-initramfs.sh"
    inst "/lib/kdump/kdump-logger.sh" "/lib/kdump-logger.sh"
    # shellcheck disable=SC2154

# shellcheck disable=SC2154 is not needed here since the early rule also applies here.

Thanks! Dropped it.

prudo1 commented 3 months ago

Added changes requested by @coiby

prudo1 commented 3 months ago

Reviewable status: 8 of 19 files reviewed, 6 unresolved discussions (waiting on @coiby and @prudo1)

mkdumprd line 70 at r29 (raw file):

    done

    dracut --list-modules | grep -x -q "${_args[@]}"

I guess it will have a problem same as commit ("98087d78eda Use "grep -q <<< $(cmd)" instead of "cmd | grep -q""), could we use "<<<" instead?

@liutgnu Thanks for the hint! Fixed it.

Other than that, the patchset LGTM.

Thanks

prudo1 commented 3 months ago

fixed the problem pointed out by @liutgnu