#2871 Issue `open`: Customizing RSYNC_PORT¶

Labels: enhancement, discuss / RFC

fadamo opened issue at 2022-09-28 13:08:¶

I found that simply adding in local.conf

readonly RSYNC_PORT=nnnnn
export RSYNC_PORT

I can customize rsyncd port.
So I think it is possible to write

RSYNC_PORT=${RSYNC_PORT:-873}

instead of

RSYNC_PORT=873                  # default port (of rsync server)

in /usr/share/rear/verify/RSYNC/default/100_check_rsync.sh
to give a proper way to customize that port.

jsmeix commented at 2022-10-05 11:44:¶

In current ReaR 2.7 code there is
no longer a global variable RSYNC_PORT.
Instead there is a function rsync_port()
in usr/share/rear/lib/rsync-functions.sh
see
https://github.com/rear/rear/commit/e0d3c6e8da6d72cfaff580683839397ceab45cf4
which reads (excerpt):

... introducing generic functions for rsync URL parsing and
use them for both BACKUP_URL and OUTPUT_URL, as appropriate.
Replace all uses of global RSYNC_* variables derived
from BACKUP_URL by those functions

jsmeix commented at 2022-10-05 13:20:¶

@fadamo
by the way:

Your

readonly VARIABLE=value
export VARIABLE

method in etc/rear/local.conf is a good real world example
why make rear working with "set -ue -o pipefail", cf.
https://github.com/rear/rear/issues/700
is not such a good idea in practice, see the section
"Try hard to care about possible errors" in
https://github.com/rear/rear/wiki/Coding-Style
that reads (excerpt)

Using 'set -eu -o pipefail' also during runtime
is currently not recommended because
it is a double-edged sword
which can cause more problems in practice
(i.e. problems for ReaR users)
than it intends to solve in theory.

In this case here set -eu -o pipefail
(therein in particular the set -e part)
would hinder the user to do some "dirty hacks"
to make ReaR behave as he needs it in his specific case,
cf. the section "Dirty hacks welcome" in
https://github.com/rear/rear/wiki/Coding-Style
and my continuous mantra: "Final power to the user!" ;-)

fadamo commented at 2022-10-05 14:51:¶

In current ReaR 2.7 code there is no longer a global variable RSYNC_PORT. Instead there is a function rsync_port() in usr/share/rear/lib/rsync-functions.sh see e0d3c6e which reads (excerpt):
... introducing generic functions for rsync URL parsing and
use them for both BACKUP_URL and OUTPUT_URL, as appropriate.
Replace all uses of global RSYNC_* variables derived
from BACKUP_URL by those functions
See also #2831 (comment) and #2831 (comment)

In current ReaR 2.7 code there is no longer a global variable RSYNC_PORT. Instead there is a function rsync_port() in usr/share/rear/lib/rsync-functions.sh see e0d3c6e which reads (excerpt):
... introducing generic functions for rsync URL parsing and
use them for both BACKUP_URL and OUTPUT_URL, as appropriate.
Replace all uses of global RSYNC_* variables derived
from BACKUP_URL by those functions
See also #2831 (comment) and #2831 (comment)

This is fine: https://github.com/rear/rear/pull/2831#discussion_r909526458 !
Are you going to implement it ?

fadamo commented at 2022-10-05 15:03:¶

Using 'set -eu -o pipefail' also during runtime
is currently not recommended because
it is a double-edged sword
which can cause more problems in practice

In depends on how you start writing brand new scripts.
I always use

#!/bin/bash -eEu
set -o pipefail

Instead, modifying old scripts is more complex.
BTW, this is your coding style, not mine, so I respect your ideas. And I really appreciate ReaR.

pcahyna commented at 2022-10-05 16:28:¶

In current ReaR 2.7 code there is
no longer a global variable RSYNC_PORT.
Instead there is a function rsync_port()

to me it seemed that RSYNC_* variables have effectively been internal variables, not something that was intended to be customizable by users (the values have been supplied in BACKUP_URL). So I decided to restructure the code and get rid of them without feeling the need for backward compatibility. Setting it to readonly is a nice hack, but the proper fix would be to extract the port from the URL.

pcahyna commented at 2022-10-05 16:31:¶

Concerning

In this case here set -eu -o pipefail
(therein in particular the set -e part)
would hinder the user to do some "dirty hacks"

I am more positive towards stuff like set -e. I would not make it the default in production, but set it during test runs. This would allow to discover many problems (depending on test coverage), but not prevent from using "dirty hacks".

jsmeix commented at 2022-10-06 07:04:¶

Regarding set -eu -o pipefail see the whole text in
the section "Try hard to care about possible errors" in
https://github.com/rear/rear/wiki/Coding-Style
that also reads (excerpt)

Preferably during development of new scripts
or when scripts are much overhauled
and while testing new code use 'set -ue'
to die from unset variables and unhandled errors
and use 'set -o pipefail' to better notice
failures in a pipeline.
...
Using 'set -eu -o pipefail' also during runtime
is currently not recommended ...

where "during runtime" actually means
"during runtime on the user's system".
I made that text more clear so that it is now

Using 'set -eu -o pipefail' also in general
during runtime on the user's system
is currently not recommended ...

jsmeix commented at 2022-10-06 07:06:¶

@pcahyna
I agree with all your reasoning in your
https://github.com/rear/rear/issues/2871#issuecomment-1268662689
in particular with your conclusion that
the proper fix would be to extract the port from the URL.

jsmeix commented at 2022-10-06 07:36:¶

I did
https://github.com/rear/rear/issues/2877
to implement a sufficiently easy way in practice
so that certain scripts can be run with
specific bash options like set -eu -o pipefail
during development (e.g. while testing things).
Of course also users can use that as needed
(e.g. for debugging or whatever they like).