#561 Issue closed: Probably misleadingly written code in 13_include_mount_filesystem_code.sh¶

Labels: enhancement, cleanup, fixed / solved / done

jsmeix opened issue at 2015-03-06 13:36:¶

This is an enhancement/cleanup request for after rear 1.17:

In https://github.com/rear/rear/issues/555#issuecomment-76932069 I was mislead by the following code in 13_include_mount_filesystem_code.sh

    local fs device mp fstype uuid label attributes
    ## mp: mount point
    read fs device mp fstype uuid label options < <(grep "^fs.* ${1#fs:} " "$LAYOUT_FILE")

    label=${label#label=}
    uuid=${uuid#uuid=}

    # Extract mount options.
    local option mountopts
    for option in $options ; do
        name=${option%%=*}     # options can contain more '=' signs
        value=${option#*=}

        case $name in
            (options)
                ### Do not mount nodev, as chrooting later on would fail.
                mountopts=${value//nodev/dev}
                ;;
        esac
    done

It reads the "fs" lines and therein field 7 and all further fields are read into the variable options.

In the disklayout.conf file a "fs" line is of the form

# Filesystems (only ext2,ext3,ext4,vfat,xfs,reiserfs,btrfs are supported).
# Format: fs <device> <mountpoint> <fstype> [uuid=<uuid>] [label=<label>] [<attributes>]
fs /dev/cciss/c0d0p1 /boot ext4 uuid=964edbd0-3eb2-4caa-9704-3fca7540ba71 label= blocksize=1024 fragmentsize=1024 reserved_blocks=5% max_mounts=-1 check_interval=0d bytes_per_inode=4047 default_mount_options=user_xattr,acl options=rw,relatime,barrier=1,data=ordered

This means the variable options does not contain only the "options=rw,relatime,barrier=1,data=ordered" but instead the variable options contains all attributes "fragmentsize=1024 reserved_blocks=5% max_mounts=-1 check_interval=0d bytes_per_inode=4047 default_mount_options=user_xattr,acl options=rw,relatime,barrier=1,data=ordered".

Therefore I suggest to rename the variables options and option to attributes and attribute so that the wording in the code matches the comment in disklayout.conf and avoids confusion by multiple meanings of the word "option(s)" as follows:

    local fs device mp fstype uuid label attributes
    ## mp: mount point
    read fs device mp fstype uuid label attributes < <(grep "^fs.* ${1#fs:} " "$LAYOUT_FILE")

    label=${label#label=}
    uuid=${uuid#uuid=}

    # Extract mount attributes.
    # For example the attributes variable can contain a value like:
    #   reserved_blocks=5% max_mounts=-1 default_mount_options=user_xattr,acl options=rw,relatime,barrier=1,data=ordered
    local attribute mountopts
    for attribute in $attributes ; do
        # an attribute can contain more '=' signs for example "options=rw,relatime,barrier=1,data=ordered"
        name=${attribute%%=*}
        value=${attribute#*=}
        case $name in
            (options)
                # The attribute with name "options" contains the mount options.
                # Do not mount nodev, as chrooting later on would fail.
                mountopts=${value//nodev/dev}
                ;;
        esac
    done

If you agree I make a pull request after rear 1.17 was released.

gdha commented at 2015-11-04 20:41:¶

@jsmeix do you still want to change the variable names? If not, then just close this request.
Thank you.

jsmeix commented at 2015-11-05 13:25:¶

@gdha
acually I had set the "waiting on feedback" label
to get feedback from rear upstream authors "If you agree"
and if yes "I make a pull request"
;-)

jsmeix commented at 2015-12-02 11:30:¶

As usual I have tested https://github.com/rear/rear/pull/727 on my SLES12-SP1 test system with btrfs (where this code is actually used) and it still works for me.

jsmeix commented at 2015-12-03 09:48:¶

Fixed via https://github.com/rear/rear/pull/727

---

[Export of Github issue for rear/rear.]