This patch fix the following issues with _crashkernel_add, 1. it couldn't address a terabyte memory range e.g. _crashkernel_add "128G-1T:4G" "0" actually returns empty result 2. it allows a memory delta without a unit specified and returns the incorrect result e.g. _crashkernel_add 1G-4G:256M,4G-64G:320M,64G-:576M 100 actually returns 1G-4G:268435556,4G-64G:335544420,64G-:603979876
Note a memory delta without unit like 0M is allowed.
Fixes: 64f2827a ("kdump-lib: Harden _crashkernel_add") Signed-off-by: Coiby Xu coxu@redhat.com --- kdump-lib.sh | 9 ++++++++- spec/kdump-lib_spec.sh | 7 +++++-- 2 files changed, 13 insertions(+), 3 deletions(-)
diff --git a/kdump-lib.sh b/kdump-lib.sh index ae39c236..d182bab8 100755 --- a/kdump-lib.sh +++ b/kdump-lib.sh @@ -862,7 +862,9 @@ has_aarch64_smmu() ls /sys/devices/platform/arm-smmu-* 1> /dev/null 2>&1 }
-is_memsize() { [[ "$1" =~ ^[+-]?[0-9]+[KkMmGg]?$ ]]; } +is_memdelta() { [[ "$1" == 0 || "$1" =~ ^[+-]?[0-9]+[BbKkMmGgTt]$ ]]; } + +is_memsize() { [[ "$1" =~ ^[+-]?[0-9]+[BbKkMmGgTt]?$ ]]; }
# range defined for crashkernel parameter # i.e. <start>-[<end>] @@ -881,6 +883,9 @@ to_bytes() is_memsize "$_s" || return 1
case "${_s: -1}" in + B|b) + _s=${_s::-1} + ;; K|k) _s=${_s::-1} _s="$((_s * 1024))" @@ -964,6 +969,8 @@ _crashkernel_add() delta="$2" ret=""
+ is_memdelta "$delta" || return 1 + while IFS=';' read -r size range offset; do if [[ -n "$offset" ]]; then ret="${ret%,}$offset" diff --git a/spec/kdump-lib_spec.sh b/spec/kdump-lib_spec.sh index 814f9618..ee6c9b27 100644 --- a/spec/kdump-lib_spec.sh +++ b/spec/kdump-lib_spec.sh @@ -55,8 +55,10 @@ Describe 'kdump-lib' "1G-4G:256M,4G-64G:320M,64G-:576M@4G" "100M" "1G-4G:356M,4G-64G:420M,64G-:676M@4G" "1G-4G:1G,4G-64G:2G,64G-:3G@4G" "100M" "1G-4G:1124M,4G-64G:2148M,64G-:3172M@4G" "1G-4G:10000K,4G-64G:20000K,64G-:40000K@4G" "100M" "1G-4G:112400K,4G-64G:122400K,64G-:142400K@4G" - "1,high" "1" "2,high" - "1K,low" "1" "1025,low" + "128G-1T:4G" "0" "128G-1T:4G" + "128G-1T:4G" "0M" "128G-1T:4G" + "1,high" "1b" "2,high" + "1K,low" "1b" "1025,low" "1M@1G" "1k" "1025K@1G" "500M@1G" "-100m" "400M@1G" "1099511627776" "0" "1024G" @@ -68,6 +70,7 @@ Describe 'kdump-lib' End Context "For invalid input values" Parameters + "1G-4G:256M" "100" "1G-4G:256M.4G-64G:320M" "100M" "foo" "1" "1" "bar"
Hi Coiby,
On Tue, 10 Oct 2023 14:50:13 +0800 Coiby Xu coxu@redhat.com wrote:
This patch fix the following issues with _crashkernel_add,
- it couldn't address a terabyte memory range e.g. _crashkernel_add "128G-1T:4G" "0" actually returns empty result
True, that is a problem. But I would appreciate if you could also adjust to_bytes and memsize_add to handle terra bytes as well. Otherwise I expect calls like _crashkernel_add "10T-100T:1T" "0" to fail as well. That shouldn't be a problem in practice but for completeness I'd prefer to have it.
- it allows a memory delta without a unit specified and returns the incorrect result e.g. _crashkernel_add 1G-4G:256M,4G-64G:320M,64G-:576M 100 actually returns 1G-4G:268435556,4G-64G:335544420,64G-:603979876
This is actually expected behavior although I must admit it's not obvious...
Thing is that the code behaves like the kernels crashkernel parameter. In that all sizes are in Bytes unless an unit is provided. In addition it avoids any rounding when it reduces the sizes back to its human readable form, i.e. it only uses a unit when $size % 1024 == 0. Combining both you get the output you have above as, e.g. 268435556 = 268435456 + 100 = 256M + 100
Thanks Philipp
Note a memory delta without unit like 0M is allowed.
Fixes: 64f2827a ("kdump-lib: Harden _crashkernel_add") Signed-off-by: Coiby Xu coxu@redhat.com
kdump-lib.sh | 9 ++++++++- spec/kdump-lib_spec.sh | 7 +++++-- 2 files changed, 13 insertions(+), 3 deletions(-)
diff --git a/kdump-lib.sh b/kdump-lib.sh index ae39c236..d182bab8 100755 --- a/kdump-lib.sh +++ b/kdump-lib.sh @@ -862,7 +862,9 @@ has_aarch64_smmu() ls /sys/devices/platform/arm-smmu-* 1> /dev/null 2>&1 }
-is_memsize() { [[ "$1" =~ ^[+-]?[0-9]+[KkMmGg]?$ ]]; } +is_memdelta() { [[ "$1" == 0 || "$1" =~ ^[+-]?[0-9]+[BbKkMmGgTt]$ ]]; }
+is_memsize() { [[ "$1" =~ ^[+-]?[0-9]+[BbKkMmGgTt]?$ ]]; }
# range defined for crashkernel parameter # i.e. <start>-[<end>] @@ -881,6 +883,9 @@ to_bytes() is_memsize "$_s" || return 1
case "${_s: -1}" in
B|b)_s=${_s::-1};;@@ -964,6 +969,8 @@ _crashkernel_add() delta="$2" ret=""
- is_memdelta "$delta" || return 1
- while IFS=';' read -r size range offset; do if [[ -n "$offset" ]]; then ret="${ret%,}$offset"
diff --git a/spec/kdump-lib_spec.sh b/spec/kdump-lib_spec.sh index 814f9618..ee6c9b27 100644 --- a/spec/kdump-lib_spec.sh +++ b/spec/kdump-lib_spec.sh @@ -55,8 +55,10 @@ Describe 'kdump-lib' "1G-4G:256M,4G-64G:320M,64G-:576M@4G" "100M" "1G-4G:356M,4G-64G:420M,64G-:676M@4G" "1G-4G:1G,4G-64G:2G,64G-:3G@4G" "100M" "1G-4G:1124M,4G-64G:2148M,64G-:3172M@4G" "1G-4G:10000K,4G-64G:20000K,64G-:40000K@4G" "100M" "1G-4G:112400K,4G-64G:122400K,64G-:142400K@4G"
"1,high" "1" "2,high""1K,low" "1" "1025,low"
"128G-1T:4G" "0" "128G-1T:4G""128G-1T:4G" "0M" "128G-1T:4G""1,high" "1b" "2,high""1K,low" "1b" "1025,low" "1M@1G" "1k" "1025K@1G" "500M@1G" "-100m" "400M@1G" "1099511627776" "0" "1024G"@@ -68,6 +70,7 @@ Describe 'kdump-lib' End Context "For invalid input values" Parameters
"1G-4G:256M" "100" "1G-4G:256M.4G-64G:320M" "100M" "foo" "1" "1" "bar"
Hi Philipp,
On Tue, Oct 10, 2023 at 12:04:22PM +0200, Philipp Rudo wrote:
Hi Coiby,
On Tue, 10 Oct 2023 14:50:13 +0800 Coiby Xu coxu@redhat.com wrote:
This patch fix the following issues with _crashkernel_add,
- it couldn't address a terabyte memory range e.g. _crashkernel_add "128G-1T:4G" "0" actually returns empty result
True, that is a problem. But I would appreciate if you could also adjust to_bytes and memsize_add to handle terra bytes as well. Otherwise I expect calls like _crashkernel_add "10T-100T:1T" "0" to fail as well. That shouldn't be a problem in practice but for completeness I'd prefer to have it.
Applied to v2, thanks for the suggestion!
- it allows a memory delta without a unit specified and returns the incorrect result e.g. _crashkernel_add 1G-4G:256M,4G-64G:320M,64G-:576M 100 actually returns 1G-4G:268435556,4G-64G:335544420,64G-:603979876
This is actually expected behavior although I must admit it's not obvious...
Thing is that the code behaves like the kernels crashkernel parameter. In that all sizes are in Bytes unless an unit is provided. In addition it avoids any rounding when it reduces the sizes back to its human readable form, i.e. it only uses a unit when $size % 1024 == 0. Combining both you get the output you have above as, e.g. 268435556 = 268435456 + 100 = 256M + 100
Thanks for the detailed explanation! My judgement is only half correct because I assumed the default unit is megabyte. I've rephrased it as "now a memory delta is required to to have a explicit unit but the check isn't enforced" in v2.
This patch fix the following issues with _crashkernel_add, 1. it couldn't address a terabyte memory range e.g. _crashkernel_add "128G-1T:4G" "0" actually returns empty result 2. now a memory delta is required to to have a explicit unit but the check isn't enforced
Note a memory delta without unit like 0 is allowed.
Fixes: 64f2827a ("kdump-lib: Harden _crashkernel_add") Signed-off-by: Coiby Xu coxu@redhat.com --- kdump-lib.sh | 15 +++++++++++++-- spec/kdump-lib_spec.sh | 11 ++++++++--- 2 files changed, 21 insertions(+), 5 deletions(-)
diff --git a/kdump-lib.sh b/kdump-lib.sh index ae39c236..d2c00c73 100755 --- a/kdump-lib.sh +++ b/kdump-lib.sh @@ -862,7 +862,9 @@ has_aarch64_smmu() ls /sys/devices/platform/arm-smmu-* 1> /dev/null 2>&1 }
-is_memsize() { [[ "$1" =~ ^[+-]?[0-9]+[KkMmGg]?$ ]]; } +is_memdelta() { [[ "$1" == 0 || "$1" =~ ^[+-]?[0-9]+[BbKkMmGgTt]$ ]]; } + +is_memsize() { [[ "$1" =~ ^[+-]?[0-9]+[BbKkMmGgTt]?$ ]]; }
# range defined for crashkernel parameter # i.e. <start>-[<end>] @@ -881,6 +883,9 @@ to_bytes() is_memsize "$_s" || return 1
case "${_s: -1}" in + B|b) + _s=${_s::-1} + ;; K|k) _s=${_s::-1} _s="$((_s * 1024))" @@ -893,6 +898,10 @@ to_bytes() _s=${_s::-1} _s="$((_s * 1024 * 1024 * 1024))" ;; + T|t) + _s=${_s::-1} + _s="$((_s * 1024 * 1024 * 1024 * 1024))" + ;; *) ;; esac @@ -901,7 +910,7 @@ to_bytes()
memsize_add() { - local -a units=("" "K" "M" "G") + local -a units=("" "K" "M" "G" "T") local i a b
a=$(to_bytes "$1") || return 1 @@ -964,6 +973,8 @@ _crashkernel_add() delta="$2" ret=""
+ is_memdelta "$delta" || return 1 + while IFS=';' read -r size range offset; do if [[ -n "$offset" ]]; then ret="${ret%,}$offset" diff --git a/spec/kdump-lib_spec.sh b/spec/kdump-lib_spec.sh index 814f9618..41995114 100644 --- a/spec/kdump-lib_spec.sh +++ b/spec/kdump-lib_spec.sh @@ -52,14 +52,18 @@ Describe 'kdump-lib' Context "For valid input values" Parameters "1G-4G:256M,4G-64G:320M,64G-:576M" "100M" "1G-4G:356M,4G-64G:420M,64G-:676M" + "1G-4G:256M" "100B" "1G-4G:268435556" # avoids any rounding when size % 1024 != 0 "1G-4G:256M,4G-64G:320M,64G-:576M@4G" "100M" "1G-4G:356M,4G-64G:420M,64G-:676M@4G" "1G-4G:1G,4G-64G:2G,64G-:3G@4G" "100M" "1G-4G:1124M,4G-64G:2148M,64G-:3172M@4G" "1G-4G:10000K,4G-64G:20000K,64G-:40000K@4G" "100M" "1G-4G:112400K,4G-64G:122400K,64G-:142400K@4G" - "1,high" "1" "2,high" - "1K,low" "1" "1025,low" + "128G-1T:4G" "0" "128G-1T:4G" + "10T-100T:1T" "0" "10T-100T:1T" + "128G-1T:4G" "0M" "128G-1T:4G" + "1,high" "1b" "2,high" + "1K,low" "1b" "1025,low" "1M@1G" "1k" "1025K@1G" "500M@1G" "-100m" "400M@1G" - "1099511627776" "0" "1024G" + "1099511627776" "0" "1T" End It "should add delta to every value after ':'" When call _crashkernel_add "$1" "$2" @@ -68,6 +72,7 @@ Describe 'kdump-lib' End Context "For invalid input values" Parameters + "1G-4G:256M" "100" # the delta unit needs to be specified explicitly "1G-4G:256M.4G-64G:320M" "100M" "foo" "1" "1" "bar"
Hi Coiby,
On Mon, 16 Oct 2023 11:32:51 +0800 Coiby Xu coxu@redhat.com wrote:
This patch fix the following issues with _crashkernel_add,
- it couldn't address a terabyte memory range e.g. _crashkernel_add "128G-1T:4G" "0" actually returns empty result
- now a memory delta is required to to have a explicit unit but the check isn't enforced
I still don't see what the benefit of 2. is. The way I see it having it only makes things more complicated as we now have two almost identical functions is_memsize and is_memrange. That's why I would appreciate it if we could stick with what the kernel does, i.e. follow lib/cmdline.c:memparse where a unit-less number is interpreted as bytes.
In addition I don't see how your fix would have prevented the bug that Baoquan introduced. In fact I would argue it would have made it even worse. In case a unit-less delta is passed to _crashkernel_add, aka. !is_memdelta, it would simply have returned 1 without throwing an error. That means kdump_get_arch_recommend_crashkernel would have returned an empty string, which for example would have caused reset_crashkernel to remove the crashkernel parameter all together. IMHO that would have been much more confusing compared to having an unexpected size.
Personally I believe that rather than enforcing a unit in _crashkernel_add we should make sure that kdump_get_arch_recommend_crashkernel always adds the unit. For example we could convert the code to something like this
kdump_get_arch_recommend_crashkernel() { local _delta=0 [...] is_sme_or_sev_active && _delta="$(memsize_add "$_delta" 64M)" [...] echo -n "$(_crashkernel_add "$_ck_cmdline" "$_delta")" }
Thanks Philipp
Note a memory delta without unit like 0 is allowed.
Fixes: 64f2827a ("kdump-lib: Harden _crashkernel_add") Signed-off-by: Coiby Xu coxu@redhat.com
kdump-lib.sh | 15 +++++++++++++-- spec/kdump-lib_spec.sh | 11 ++++++++--- 2 files changed, 21 insertions(+), 5 deletions(-)
diff --git a/kdump-lib.sh b/kdump-lib.sh index ae39c236..d2c00c73 100755 --- a/kdump-lib.sh +++ b/kdump-lib.sh @@ -862,7 +862,9 @@ has_aarch64_smmu() ls /sys/devices/platform/arm-smmu-* 1> /dev/null 2>&1 }
-is_memsize() { [[ "$1" =~ ^[+-]?[0-9]+[KkMmGg]?$ ]]; } +is_memdelta() { [[ "$1" == 0 || "$1" =~ ^[+-]?[0-9]+[BbKkMmGgTt]$ ]]; }
+is_memsize() { [[ "$1" =~ ^[+-]?[0-9]+[BbKkMmGgTt]?$ ]]; }
# range defined for crashkernel parameter # i.e. <start>-[<end>] @@ -881,6 +883,9 @@ to_bytes() is_memsize "$_s" || return 1
case "${_s: -1}" in
B|b)_s=${_s::-1};;@@ -893,6 +898,10 @@ to_bytes() _s=${_s::-1} _s="$((_s * 1024 * 1024 * 1024))" ;;
T|t)_s=${_s::-1}_s="$((_s * 1024 * 1024 * 1024 * 1024))";;@@ -901,7 +910,7 @@ to_bytes()
memsize_add() {
- local -a units=("" "K" "M" "G")
local -a units=("" "K" "M" "G" "T") local i a b
a=$(to_bytes "$1") || return 1
@@ -964,6 +973,8 @@ _crashkernel_add() delta="$2" ret=""
- is_memdelta "$delta" || return 1
- while IFS=';' read -r size range offset; do if [[ -n "$offset" ]]; then ret="${ret%,}$offset"
diff --git a/spec/kdump-lib_spec.sh b/spec/kdump-lib_spec.sh index 814f9618..41995114 100644 --- a/spec/kdump-lib_spec.sh +++ b/spec/kdump-lib_spec.sh @@ -52,14 +52,18 @@ Describe 'kdump-lib' Context "For valid input values" Parameters "1G-4G:256M,4G-64G:320M,64G-:576M" "100M" "1G-4G:356M,4G-64G:420M,64G-:676M"
"1G-4G:256M" "100B" "1G-4G:268435556" # avoids any rounding when size % 1024 != 0 "1G-4G:256M,4G-64G:320M,64G-:576M@4G" "100M" "1G-4G:356M,4G-64G:420M,64G-:676M@4G" "1G-4G:1G,4G-64G:2G,64G-:3G@4G" "100M" "1G-4G:1124M,4G-64G:2148M,64G-:3172M@4G" "1G-4G:10000K,4G-64G:20000K,64G-:40000K@4G" "100M" "1G-4G:112400K,4G-64G:122400K,64G-:142400K@4G"
"1,high" "1" "2,high""1K,low" "1" "1025,low"
"128G-1T:4G" "0" "128G-1T:4G""10T-100T:1T" "0" "10T-100T:1T""128G-1T:4G" "0M" "128G-1T:4G""1,high" "1b" "2,high""1K,low" "1b" "1025,low" "1M@1G" "1k" "1025K@1G" "500M@1G" "-100m" "400M@1G"
"1099511627776" "0" "1024G"
"1099511627776" "0" "1T" End It "should add delta to every value after ':'" When call _crashkernel_add "$1" "$2"@@ -68,6 +72,7 @@ Describe 'kdump-lib' End Context "For invalid input values" Parameters
"1G-4G:256M" "100" # the delta unit needs to be specified explicitly "1G-4G:256M.4G-64G:320M" "100M" "foo" "1" "1" "bar"
Hi Philipp,
On Tue, Oct 24, 2023 at 04:23:23PM +0200, Philipp Rudo wrote:
Hi Coiby,
On Mon, 16 Oct 2023 11:32:51 +0800 Coiby Xu coxu@redhat.com wrote:
This patch fix the following issues with _crashkernel_add,
- it couldn't address a terabyte memory range e.g. _crashkernel_add "128G-1T:4G" "0" actually returns empty result
- now a memory delta is required to to have a explicit unit but the check isn't enforced
I still don't see what the benefit of 2. is. The way I see it having it only makes things more complicated as we now have two almost identical functions is_memsize and is_memrange. That's why I would appreciate it if we could stick with what the kernel does, i.e. follow lib/cmdline.c:memparse where a unit-less number is interpreted as bytes.
It seems I misunderstood your previous commit,
commit 64f2827a4bfa3f26974972f2f5a4477eec58f221 Author: Philipp Rudo prudo@redhat.com Date: Wed Sep 6 10:49:47 2023 +0200
kdump-lib: Harden _crashkernel_add ... Furthermore require the delta to have a explicit unit, i.e. no longer assume that is in megabytes, i.e. 100 -> 100M.
I don't know how I should interpret the last sentence If I don't interpret it as a explicit unit is required thus enforcing the check is desirable.
In addition I don't see how your fix would have prevented the bug that Baoquan introduced.
I would say Baoquan's patch revealed a bug in _crashkernel_add i.e. it couldn't address terabyte. And this is what I try to address mainly.
In fact I would argue it would have made it even worse. In case a unit-less delta is passed to _crashkernel_add, aka. !is_memdelta, it would simply have returned 1 without throwing an error. That means kdump_get_arch_recommend_crashkernel would have returned an empty string, which for example would have caused reset_crashkernel to remove the crashkernel parameter all together.
Thanks for coming up with this scenario! I think we need to reach a consensus on the interface design of _crashkernel_add first. Please check my next comment.
IMHO that would have been much more confusing compared to having an unexpected size.
Personally I believe that rather than enforcing a unit in _crashkernel_add we should make sure that kdump_get_arch_recommend_crashkernel always adds the unit. For example we could convert the code to something like this
kdump_get_arch_recommend_crashkernel() { local _delta=0 [...] is_sme_or_sev_active && _delta="$(memsize_add "$_delta" 64M)" [...] echo -n "$(_crashkernel_add "$_ck_cmdline" "$_delta")" }
If your previous design is _crashkernel_add should only accept a memory delta with a explicit unit (which I think is a good idea because it makes it easier for developers to remember the intended usage of this function), then a check is desirable. And we should check the return code of _crashkernel_add to address your scenario. In fact, in the tests of spec/kdump-lib_spec.sh you already check the return code of _crashkernel_add. And we can also add a comment on top of _crashkernel_add that the caller should add a unit to the memory delta.
Thanks Philipp
Hi Coiby,
On Thu, 26 Oct 2023 18:18:40 +0800 Coiby Xu coxu@redhat.com wrote:
Hi Philipp,
On Tue, Oct 24, 2023 at 04:23:23PM +0200, Philipp Rudo wrote:
Hi Coiby,
On Mon, 16 Oct 2023 11:32:51 +0800 Coiby Xu coxu@redhat.com wrote:
This patch fix the following issues with _crashkernel_add,
- it couldn't address a terabyte memory range e.g. _crashkernel_add "128G-1T:4G" "0" actually returns empty result
- now a memory delta is required to to have a explicit unit but the check isn't enforced
I still don't see what the benefit of 2. is. The way I see it having it only makes things more complicated as we now have two almost identical functions is_memsize and is_memrange. That's why I would appreciate it if we could stick with what the kernel does, i.e. follow lib/cmdline.c:memparse where a unit-less number is interpreted as bytes.
It seems I misunderstood your previous commit,
commit 64f2827a4bfa3f26974972f2f5a4477eec58f221 Author: Philipp Rudo prudo@redhat.com Date: Wed Sep 6 10:49:47 2023 +0200
kdump-lib: Harden _crashkernel_add ... Furthermore require the delta to have a explicit unit, i.e. no longer assume that is in megabytes, i.e. 100 -> 100M.I don't know how I should interpret the last sentence If I don't interpret it as a explicit unit is required thus enforcing the check is desirable.
Fair, this sentence is easy to mis-interpret. What I wanted to say is that before that commit the delta that is passed to _crashkernel_add was just a number which had an implicit unit of MB. With this commit the M has to be explicitly given. But yeah, that only means that the Mega has to be given explicitly, the Bytes is still assumed...
In addition I don't see how your fix would have prevented the bug that Baoquan introduced.
I would say Baoquan's patch revealed a bug in _crashkernel_add i.e. it couldn't address terabyte. And this is what I try to address mainly.
Yes, the missing Terra Byte support is a bug. But that is not what I meant here.
What I've meant was the missing 'M' for the delta when calling _crashkernel_add in kdump_get_arch_recommend_crashkernel. That was a bug that Baoquan's patch would have introduced. And which in my opinion would have been harder to find IMHO with this patch as described below. But that is completely independent from the missing Terra Byte support.
In fact I would argue it would have made it even worse. In case a unit-less delta is passed to _crashkernel_add, aka. !is_memdelta, it would simply have returned 1 without throwing an error. That means kdump_get_arch_recommend_crashkernel would have returned an empty string, which for example would have caused reset_crashkernel to remove the crashkernel parameter all together.
Thanks for coming up with this scenario! I think we need to reach a consensus on the interface design of _crashkernel_add first. Please check my next comment.
IMHO that would have been much more confusing compared to having an unexpected size.
Personally I believe that rather than enforcing a unit in _crashkernel_add we should make sure that kdump_get_arch_recommend_crashkernel always adds the unit. For example we could convert the code to something like this
kdump_get_arch_recommend_crashkernel() { local _delta=0 [...] is_sme_or_sev_active && _delta="$(memsize_add "$_delta" 64M)" [...] echo -n "$(_crashkernel_add "$_ck_cmdline" "$_delta")" }
If your previous design is _crashkernel_add should only accept a memory delta with a explicit unit (which I think is a good idea because it makes it easier for developers to remember the intended usage of this function), then a check is desirable. And we should check the return code of _crashkernel_add to address your scenario. In fact, in the tests of spec/kdump-lib_spec.sh you already check the return code of _crashkernel_add. And we can also add a comment on top of _crashkernel_add that the caller should add a unit to the memory delta.
The idea in the example above was more to make sure that $_delta has a unit. That is something a simple add does not guarantee but memsize_add does. Plus it makes it easier to see a missing unit when you require it all the time. With that I hoped we can skip to test the return value of _crashkernel_add. But sticking with a simple add and checking the return value is also a valid option. So yeah, I believe in the end it's more a question of personal preference. Although checking the return value will be much lighter for sure...
Thanks Philipp
-
Coiby Xu -
Philipp Rudo