#1480 PR merged: Avoid apply_layout_mappings Error if input file is empty

Labels: enhancement, fixed / solved / done

schabrolles opened issue at 2017-09-11 10:51:

  • @schlomo, The change made in d89623404c08cf8b4b485e35295fb4caf2a35232 can produce error in some Linux Distro (like Sles11).
    It is not directly link with the Linux version, but how scripts are dealing with apply_layout_mappings() function. In 250_migrate_disk_devices_layout.sh a list of file to migrate is provided to the function. Depending to the configuration, some of them can e empty (or a link which point to an empty file.)

I think we just need to be sure that the function is called with 1 argument...
But the fact to provide an empty file to the function should not produce an ERROR which kill the recovery process. After all, if the file is empty, no modification will be done.

  • I also made some changes in 250_migrate_disk_devices_layout.sh in order to modify link target when absolute path is used. @schlomo @jsmeix your feedback/review is welcome here. (other scripts in finalize workflow should be modified in the same way, but lets validate this one before changing the others.)

jsmeix commented at 2017-09-12 08:41:

@schabrolles
I fail to understand how $1 is used in your
apply_layout_mappings function because I find '$1'
therein only in

    [ "$1" ] || BugError "..."
...
    if [ -s "$1" ] ; then

but afterwards it seems $1 is nowhere used?

FYI only a cosmetical proposal:
In your apply_layout_mappings function you could eliminate the outer

    # Only apply layout mapping on non-empty file.
    if [ -s "$1" ] ; then
        ...
    fi

clause by "Return early, return often" coding style via

    # Only apply layout mapping on non-empty file:
    test -s "$1" || return 0

Is the apply_layout_mappings function perhaps meant like this:

function apply_layout_mappings() {

    # Nothing to do if not in migration mode:
    test "$MIGRATION_MODE" || return 0

    local file_to_migrate="$1"

    # apply_layout_mappings needs one argument:
    [ "$file_to_migrate" ] || BugError "apply_layout_mappings function called without argument (file_to_migrate)."

    # Only apply layout mapping on non-empty file:
    test -s "$file_to_migrate" || return 0

    ...

schabrolles commented at 2017-09-12 09:01:

@jsmeix, I've seen the $1 issue during my test this morning. it is corrected now.
I also really like your test proposal.... It is very clean. I will certainly update the code with it.

schabrolles commented at 2017-09-12 10:06:

@jsmeix @schlomo Updated with @jsmeix test proposal. Feedback welcome.

jsmeix commented at 2017-09-12 12:25:

From plain looking at the code it looks very good now
and I even understand what the apply_layout_mappings
function is meant to do :-)

schabrolles commented at 2017-09-13 09:25:

I'm gonna merge this one soon (this afternoon)

[Export of Github issue for rear/rear.]