#2260 PR merged: Use is_true() for AUTOEXCLUDE_MULTIPATH and cleanup of get_partition_number()

Labels: cleanup, fixed / solved / done

jsmeix opened issue at 2019-10-23 13:25:

  • Type: Cleanup

  • Impact: Low

  • Reference to related issue (URL):
    https://github.com/rear/rear/issues/2245

  • How was this pull request tested?
    The AUTOEXCLUDE_MULTIPATH was not tested
    because I cannot test multipath.
    The cleanup of the get_partition_number function
    was tested by rear -D mkresue on my test system
    and it results the same disklayout.conf as before.

  • Brief description of the changes in this pull request:

Using is_true "$AUTOEXCLUDE_MULTIPATH"
instead of [[ "$AUTOEXCLUDE_MULTIPATH" =~ ^[yY1] ]]
according to "Relax-and-Recover functions" in
https://github.com/rear/rear/wiki/Coding-Style

Additionally a cleanup of the get_partition_number function:
When the comments talk about bug then BugError should be called.
Plus hopefully more comprehensible comments,
cf. "Code must be easy to understand" in
https://github.com/rear/rear/wiki/Coding-Style

jsmeix commented at 2019-10-23 13:31:

@schabrolles
perhaps you find time to have a look at it?
But don't worry if not.

jsmeix commented at 2019-10-23 14:13:

@gdha
could you have a closer look at the get_partition_number function here?

Could perhaps the current

test $number -le 128 || BugError "Partition $partition is numbered '$number'. More than 128 partitions are not supported."

be changed into a user notification

test $number -le 128 || LogPrintError "Partition $partition is numbered '$number'. More than 128 partitions can cause issues (GPT must be sufficiently large)."

On the other hand I know that relatively often ReaR errors out at that place
when weird other errors before had been ignored and it blindly proceeded
until it finally errors out here - so we may better keep that error exit here
as some kind of generic safeguard against bugs in ReaR elsewhere?

gdha commented at 2019-10-23 14:23:

@gdha
could you have a closer look at the get_partition_number function here?

Could perhaps the current

test $number -le 128 || BugError "Partition $partition is numbered '$number'. More than 128 partitions are not supported."

be changed into a user notification

test $number -le 128 || LogPrintError "Partition $partition is numbered '$number'. More than 128 partitions can cause issues (GPT must be sufficiently large)."

I would keep the BugError for the time being until we fully understand what is going on in that piece of code in combination with weird setups people have...

jsmeix commented at 2019-10-24 08:02:

Only a side note:
How comprehesible messages in rear -D mkrescue debug log
become once explanatory variable names are used

+++ get_partition_number sdb7
+++ local partition_block_device=sdb7
++++ echo sdb7
++++ grep -o -E '[0-9]+$'
+++ local partition_number=7
+++ test 7 -gt 0
+++ test 7 -le 128
+++ echo 7

;-)

[Export of Github issue for rear/rear.]