#2384 PR `merged`: New OUTPUT_LFTP_OPTIONS config variable for lftp custom parameters¶

Labels: enhancement, fixed / solved / done

vigri opened issue at 2020-05-01 23:12:¶

Pull Request Details:¶

Type: Enhancement
Impact: Low
Reference to related issue (URL): /
How was this pull request tested? Manual test
Brief description of the changes in this pull request:

I've had the case where I needed to pass custom parameters to lftp.
This PR introduces a new variable OUTPUT_LFTP_OPTIONS
which can be set in local.conf or site.conf.

Example usage:

OUTPUT_LFTP_OPTIONS="set ftp:ssl-force true; set ftp:ssl-protect-data true;"

jsmeix commented at 2020-05-04 09:10:¶

@vigri
thank you for your improvement to ReaR!

I am not a lftp user so I cannot test/verify things on my own
but your code changes look good to me from plain looking at the code.

jsmeix commented at 2020-05-04 09:12:¶

@rear/contributors
I would like to have another review by another ReaR maintainer
to be more on the safe side.

vigri commented at 2020-05-04 13:38:¶

Hello @jsmeix,
I've made the changes you suggested.

Please let me know, if you think there is a better position inside default.conf.

jsmeix commented at 2020-05-05 07:32:¶

@gozora
thank you for your review!

I would like to merge it today afternoon
if no objections appear until then.

gozora commented at 2020-05-05 08:27:¶

Btw, before this patch one can configure lftp using ~/.lftprc and ~/.lftp/rc (or ~/.config/lftp/rc if ~/.lftp as described in man .

V.

jsmeix commented at 2020-05-05 09:31:¶

In general the advantage of ReaR config variables over config files is
that ReaR config variable settings apply specifically only to usr/sbin/rear
both on the original system and also in the ReaR recovery system
while config files apply to a particular user account or system-wide.

This leads to an interesting question:

Assume the user root has a /root/.someprog.conf file.
Then during "rear mkrescue/mkbackup" someprog is run
with the specific settings in /root/.someprog.conf
while during "rear recover" someprog is run with its default settings
because the ReaR recovery system contains only

/tmp/rear.3sLuDsk2QvoZSRT/rootfs/root
/tmp/rear.3sLuDsk2QvoZSRT/rootfs/root/.ssh
/tmp/rear.3sLuDsk2QvoZSRT/rootfs/root/.ssh/known_hosts
/tmp/rear.3sLuDsk2QvoZSRT/rootfs/root/.bash_history

where .bash_history is a ReaR specific one, see
https://github.com/rear/rear/blob/master/usr/share/rear/build/GNU/Linux/130_create_dotfiles.sh

schlomo commented at 2020-05-05 10:00:¶

Some thoughts from my perspective:

As LFTP_PARAMS is used only for OUTPUT it should be named OUTPUT_LFTP_PARAMS so that rear dump will also show it. At the moment it will be very difficult for us to debug problems related to wrong params set here.
The LFTP_PARAMS must end with a semicolon ; which I find dangerous. I strongly suggest to protect the integrity of the code by adding that if it is missing (what happens if lftp finds ; ; in the commands?)

While looking at the lftp related code I realized some further aspects, which are not related to this PR:

Concatenating the lftp commands is in general dangerous as it can lead to quoting related user pain. Maybe instead we should use the -f option to execute a generated script file? That script file could be composed of initializing stuff that the user can extend via an OUTPUT_LFTP_SETUP variable and contain also the generated lines that are now given via -c.

I can imagine that this approach will increase the stability of our lftp-related features and also make it easier to debug those as the script file would be stored in our temp are and could be written to debug log before execution.

For this PR I would therefore suggest to at least rename the variable to be more consistent with the variable names and to support rear dump. Rewriting the lftp calls can then be also done later.

jsmeix commented at 2020-05-05 10:48:¶

I agree that a more specific variable name like OUTPUT_LFTP_PARAMS
is better because a generic variable name like LFTP_PARAMS
will cause problems if in the future lftp might be called in another context,
cf. the section "Code must be easy to read" in
https://github.com/rear/rear/wiki/Coding-Style
that reads in particular "Variables and functions must have names
that explain what they do, even if it makes them longer. "

In general our user config variable names are rather inconsistent.
For example we have ISO_MKISOFS_OPTS
that is also only OUTPUT related and
it has OPTS instead of PARAMS while others have OPTIONS
and so on...

vigri commented at 2020-05-05 11:27:¶

The LFTP_PARAMS must end with a semicolon ; which I find dangerous. I strongly suggest to protect the integrity of the code by adding that if it is missing (what happens if lftp finds ; ; in the commands?)

This is a good point. I'll think about that and refactor the code.

jsmeix commented at 2020-05-05 12:23:¶

@vigri @schlomo
in general regarding user config variable value syntax checks:

In general we don't do syntax checks in ReaR.
Usually the user is free to set arbitrary stuff
and then things may just fail arbitrarily.
For one single example see
https://github.com/rear/rear/issues/2206

Im very many cases we don't even error out in ReaR
in the first place when mandatory program calls fail
but let things blindly proceed until things fall apart later
at an arbitrary unexpected place with an inexplicable
and weird error message.
Here the best example is our
More than 128 partitions is not supported
error message that one can get when
"something went wrong in the layout code"
cf. the comment in the code
https://github.com/rear/rear/blob/master/usr/share/rear/lib/layout-functions.sh#L405

So what we should do first and foremost is to error out
in the first place when a mandatory program call fails.

In particular we can easily error out during "rear mkrescue/mkbackup"
because here it does not cause harm to error out because the user can
relatively easily correct things and re-run "rear mkrescue/mkbackup".

Cf. "Try hard to care about possible errors" in
https://github.com/rear/rear/wiki/Coding-Style

In contrast we usually do not error out during "rear recover"
after the backup was successfully restored because at that late state
of the recovery it is better for the user to only inform him when things
like recreating the initrd/initramfs or installing the bootloader failed
and finish "rear recover" nevertheless than to abort "rear recover".

When recreating the initrd/initramfs or installing the bootloader failed
the user can after "rear recover" finished still in the ReaR recovery system
manually recreate his initrd/initramfs and/or install his the bootloader
e.g. via chroot into the recreated system chroot /mnt/local bash --login
cf. https://github.com/rear/rear/blob/master/usr/share/rear/finalize/default/890_finish_checks.sh#L23

For example assume recreating the initrd/initramfs failed but
installing the bootloader worked then the recreated system
could be perfectly OK (the user must decide whether or not)
because often it is not needed to recreate the initrd/initramfs
because usually booting works with the initrd/initramfs from
the original system that was restored from the backup.
This is in particular when the hardware (it could also be virtual hardware)
of the replacement system did not change compared to the original system
so that the original system initrd/initramfs also works
on the replacement hardware.

schlomo commented at 2020-05-05 14:34:¶

@vigri in such cases (give error message for failed external commands) it helps to create a local temporary variable, e.g. llftp_call_args that is then used both for lftp and in the error message if lftp fails. Thus users have a chance to spot the problem immediately.

jsmeix commented at 2020-05-05 15:06:¶

And - by the way - in https://github.com/rear/rear/pull/2382
we are right now developing a method to show
a program's STDOUT and STDERR both in ReaR's log file
and on the user's terminal, see in particular
https://github.com/rear/rear/pull/2382#discussion_r417852393
and the subsequent comments therein.

So if lftp is normally quiet on STDERR we may show
the lftp STDERR output on the user's terminal
so that he can immediately see when there is
e.g. a syntax error in the lftp command
for example with something like

lftp ... 2>> >( tee -a $RUNTIME_LOGFILE 1>&8 )

@schlomo
could you have a look at my questions in
https://github.com/rear/rear/pull/2382#discussion_r419378558

Perhaps you could help me to better understand what actually
goes on behind the surface with such advanced nested redirections
together with (possibly also nested) process substitutions?

jsmeix commented at 2020-05-12 08:18:¶

@vigri
do you intend to further work on this pull request
or could I help you here and do the minimal
requested changes so that we can merge it?

vigri commented at 2020-05-12 20:11:¶