Skip to content

kdump memory estimator - V2

Guilherme G. Piccolirequested to merge
est_v2 into master

This is the 2nd iteration of the kdump memory estimator (after ~6 months, sorry for the delay). I'd like to thank both @bdrung and @dannf for the great review, which allowed me to improve a lot the code. Also, thanks to @cascardo for some interesting discussions we had!

Below the differences plus a section about unchanged things that were suggested to change (and the reasoning). Cheers!

P.S. I'm not sure why this MR included 5 patches from Dann, that should be merged already IIUC - anyway, the goal for the MR is the 5 top patches.

V2:

Adressed most Dann and Benjamin points - below a summary, for details/all comments, see the old MR (debian/makedumpfile!7 (closed)):

  • Added spaces near arithmetic operators in expressions;
  • Changed units to base-2 in comments (MB -> MiB);
  • Make use of "case" instead of "if" for multiple conditionals;
  • Remove useless comment about a future improvement for other architectures;
  • Improved comment around hugepages;
  • Added check for zero-length file (initrd decompressed size);
  • Changed commands result checks in conditionals to use the command directly, instead of "$?";
  • "N/A" -> "Unknown" in case of estimate error output;
  • s/check/is-active in the systemctl command, to prevent used deprecated command;
  • Removed useless file remove (rm -f) in kdump_mem_estimator;
  • Accounted for Debian difference in creation-time of /var/lib/kdump in kdump_mem_estimator;
  • Always "succeed" in kdump_mem_estimator to prevent systemd service init failure;
  • Delete the mem_ and size_ files on kernel removal;
  • Sync new file created (initrd decompressed size);
  • Added HTTPs in URLs;
  • Improved grammar/wording in comments/commit messages;
  • Overall shellcheck analysis[0] (fixed code to not introduce errors);

The following points were not addressed:

(0) Nit: Can we call this kdump-mem-estimator for consistency with the other command kdump-config? I dislike having to remember which commands use - vs _.

Reason: The idea of keeping it differet is to avoid name collision, also kdump_mem_estimator is not a command, it's just a helper called by systemd early in boot process.

(1) if [ ! -f {KDIR}/size_initrd.img-(uname -r) ] Any chance the initrd might be a symlink? If so, perhaps -e to be safe?

Reason: seems -e and -f works equally for regular files and (valid and invalid) symlinks - tested on both bash and dash.

(2) cat /proc/meminfo 1> {KDUMP_DIR}/mem_(uname -r) Maybe sync mem_$(uname -r) to protect against e.g. power failure?

Reason: if we have a power failure here, next boot will re-calculate this. Happens in all boots - sync may be a bad/unnecessary thing here.

[0] Side note / venting: shellcheck errors and the fact the it now prevents the build drains my soul and will to improve kdump-tools - it is really annoying.

Merge request reports