#678 Issue `closed`: Move DISKLAYOUT_FILE to default.conf?¶

Labels: cleanup, fixed / solved / done

schlomo opened issue at 2015-10-29 17:41:¶

When looking at #668 I realized that DISKLAYOUT_FILE is defined in many places:

$ grep -r DISKLAYOUT_FILE= .
./usr/share/rear/layout/save/GNU/Linux/10_create_layout_file.sh:DISKLAYOUT_FILE=${DISKLAYOUT_FILE:-$VAR_DIR/layout/disklayout.conf}
./usr/share/rear/lib/checklayout-workflow.sh:    DISKLAYOUT_FILE=$TEMP_LAYOUT
./usr/share/rear/lib/mkbackuponly-workflow.sh:  DISKLAYOUT_FILE=$TMP_DIR/backuplayout.conf
./usr/share/rear/lib/savelayout-workflow.sh:    DISKLAYOUT_FILE=$VAR_DIR/layout/disklayout.conf

But it is not defined in default.conf and the setting in savelayout-workflow.sh actually repeats the builtin default in 10_create_layout_file.sh.

I suspect that it would be better (more ReaR-like and less error prone) to have the default setting in default.conf.

jsmeix commented at 2015-11-23 11:38:¶

@gdha
You added the DISKLAYOUT_FILE to usr/share/rear/conf/default.conf
in the following section:

# DRLM (Disaster Recovery Linux Manager) Variables

I assume the DISKLAYOUT_FILE variable
does not belog only to DRLM but is a
generic global variable for any disk-layout code.

If I am right I woul like to place the DISKLAYOUT_FILE variable
in a section for generic global variables in default.conf

gdha commented at 2015-11-23 11:59:¶

@jsmeix no problem - you may move it

jsmeix commented at 2015-11-23 12:06:¶

I relation to https://github.com/rear/rear/issues/708 I think that
neither the DISKLAYOUT_FILE variable
nor a RECOVERY_FS_ROOT variable
belongs into usr/share/rear/conf/default.conf
because those variables are not meant to be
configured by the user, see default.conf

# PLEASE NOTE:
#
# * Here we define and describe ALL configuration variables and set them to a sane
#   default. Please do NOT change them here, but rather copy them to site.conf or
#   local.conf
# * Most variables can be set to an empty value (VAR=) which means that this
#   setting is off or set to some automatic mode.

I think generic global variables like DISKLAYOUT_FILE
and RECOVERY_FS_ROOT belong into usr/sbin/rear
like SHARE_DIR CONFIG_DIR VAR_DIR LOG_DIR ...

gdha commented at 2015-11-23 14:04:¶

@jhoekx what is your opinion on this topic?

schlomo commented at 2015-11-23 14:12:¶

What is actually the purpose of making DISKLAYOUT_FILE and RECOVERY_FS_ROOT
a variable?

Whom do you expect in which situation to change it?

Answering those questions will also tell you where to put them.

On 23 November 2015 at 15:04, gdha notifications@github.com wrote:

@jhoekx https://github.com/jhoekx what is your opinion on this topic?

—
Reply to this email directly or view it on GitHub
https://github.com/rear/rear/issues/678#issuecomment-158940186.

jsmeix commented at 2015-11-24 08:40:¶

My (I cannot speak for the others) actual purpose of using
things like DISKLAYOUT_FILE and RECOVERY_FS_ROOT
is not as a real variable (i.e. where the value is meant to change)
but as a constant that has a meaningful name to avoid
hardcoded meaningless literal values in the scripts.

For me things like DISKLAYOUT_FILE and RECOVERY_FS_ROOT
represent values that rear needs internally to work.

I think we should declare those kind of variables
as bash constants via

readonly VAR_DIR="$REAR_DIR_PREFIX/var/lib/rear"
...
readonly DISKLAYOUT_FILE="$VAR_DIR/layout/disklayout.conf"
readonly RECOVERY_FS_ROOT="/mnt/local"

I think such rear-internal values can be defined in usr/sbin/rear or
in a separated file (not default.conf) that is sourced by usr/sbin/rear.

schlomo commented at 2015-11-24 10:01:¶

+1 for readonly constants. I would put them in the main rear script, unless
you plan to read them also from outside the rear framework, e.g. from the
system-setup scripts in the rescue system.

On 24 November 2015 at 09:40, Johannes Meixner notifications@github.com
wrote:

My (I cannot speak for the others) actual purpose of using
things like DISKLAYOUT_FILE and RECOVERY_FS_ROOT
is not as a real variable (i.e. where the value is meant to change)
but as a constant that has a meaningful name to avoid
hardcoded meaningless literal values in the scripts.

For me things like DISKLAYOUT_FILE and RECOVERY_FS_ROOT
represent values that rear needs internally to work.

I think we should declare those kind of variables
as bash constants via

readonly VAR_DIR="$REAR_DIR_PREFIX/var/lib/rear"
...
readonly DISKLAYOUT_FILE="$VAR_DIR/layout/disklayout.conf"
readonly RECOVERY_FS_ROOT="/mnt/local"

I think such rear-internal values can be defined in usr/sbin/rear or
in a separated file (not default.conf) that is sourced by usr/sbin/rear.

—
Reply to this email directly or view it on GitHub
https://github.com/rear/rear/issues/678#issuecomment-159196316.

jsmeix commented at 2015-11-24 12:03:¶

I will have readonly constants in the main rear script because this is
simplest and only if really needed have them in a separated file.

Depending on how variables are meant to be used they can be
easily moved from usr/sbin/rear to default.conf or vice versa
and they can be easily declared as readonly or not.

jsmeix commented at 2015-11-24 16:39:¶

Of course https://github.com/rear/rear/pull/710 is not yet
the final solution - but it should be a first working step
(at least "rear mkbackup" and "rear recover" still works for me).

jsmeix commented at 2016-09-21 13:25:¶

This particular issue about DISKLAYOUT_FILE is fixed.
It is now in usr/sbin/rear

# Generic global variables that are not meant to be configured by the user
# (i.e. that do not belong into usr/share/rear/conf/default.conf),
# see https://github.com/rear/rear/issues/678
# and https://github.com/rear/rear/issues/708
# Location of the disklayout.conf file created by savelayout
# (no readonly DISKLAYOUT_FILE because it is also set in checklayout-workflow.sh and mkbackuponly-workflow.sh):
DISKLAYOUT_FILE="$VAR_DIR/layout/disklayout.conf"

I close this particular issue.

I will continue to work on this kind of issues
as needed in the future.

[Export of Github issue for rear/rear.]

#678 Issue closed: Move DISKLAYOUT_FILE to default.conf?¶