#2325 PR merged: Make USE_DHCLIENT more fail-safe in ReaR recovery system.

Labels: cleanup, fixed / solved / done, minor bug

gozora opened issue at 2020-01-31 14:39:

  • Type: Bug Fix

  • Impact: Normal

  • Reference to related issue (URL): #2323

  • How was this pull request tested?
    With boot and check of ReaR rescue system on SLES12 SP4.

  • Brief description of the changes in this pull request:
    Make USE_DHCLIENT more fail-safe in ReaR recovery system.
    (during execution of /etc/scripts/system-setup)
    Instead just evaluating whether USE_DHCLIENT is empty, evaluate
    whether USE_DHCLIENT is set to 'true' or 'false'.

jsmeix commented at 2020-02-04 08:50:

@gozora
only as a side note:

While you are at it - but only if you like - you may also improve
things regarding USE_STATIC_NETWORKING in the same way:

# find usr/sbin/rear usr/share/rear/ -type f | xargs grep 'USE_STATIC_NETWORKING' | grep -v ': *#'

usr/share/rear/rescue/GNU/Linux/310_network_devices.sh:
[[ ! -z "\$USE_DHCLIENT" && -z "\$USE_STATIC_NETWORKING" ]] && return

usr/share/rear/rescue/GNU/Linux/350_routing.sh:
[[ ! -z "\$USE_DHCLIENT" && -z "\$USE_STATIC_NETWORKING" ]] && return

usr/share/rear/skel/default/etc/scripts/system-setup.d/58-start-dhclient.sh:
test "$USE_STATIC_NETWORKING" && return

gozora commented at 2020-02-04 08:54:

@jsmeix USE_STATIC_NETWORKING looks somehow OK to me.
Because default.conf says:

# Say "y", "Yes" (or any not empty string) to enable static networking (overrules USE_DHCLIENT):
USE_STATIC_NETWORKING=

So checking USE_STATIC_NETWORKING with -z look sufficient.
What kind of improvement is you your mind?

V.

gozora commented at 2020-02-04 08:56:

What I mean is that USE_DHCLIENT can have 3 values:

  • yes
  • no
  • empty (autodetect)

Where USE_STATIC_NETWORKING can be:

  • empty (off)
  • non-enpty (on)

V.

jsmeix commented at 2020-02-04 09:26:

Yes, USE_STATIC_NETWORKING is currently "somehow OK"
where "somehow OK" means "technically fully correct"
because its behaviour is documented so there is no need
to fix something here because there is no (technical) bug.

My side note was only meant as an improvement
suggestion to make it behave more fail-safe for
"users who do not read the documentation" ;-)
like:

/etc/rear/local.conf excerpts:
...
# Ensure to not USE_STATIC_NETWORKING
USE_STATIC_NETWORKING="No"
...

I noticed things like that in user config files several times in the past.

gozora commented at 2020-02-04 09:31:

Ah, so you just want to do something like:

... -z "$USE_STATIC_NETWORKING" || is_false $USE_STATIC_NETWORKING && return ?

jsmeix commented at 2020-02-04 11:51:

Yes, I was thinking about using
is_true versus ! is_true versus is_false versus ! is_false, cf.
https://github.com/rear/rear/blob/master/usr/share/rear/lib/global-functions.sh#L101
also for testing the USE_STATIC_NETWORKING value.

gozora commented at 2020-02-04 12:22:

ok, I'll update it (hopefully) soon.

V.

gozora commented at 2020-02-04 14:25:

@jsmeix I've done the discussed modifications, can you please a quick look (when time permits)?

Thx

V.

jsmeix commented at 2020-02-04 15:21:

@gozora
I am sorry for my USE_STATIC_NETWORKING cleanup proposal
https://github.com/rear/rear/pull/2325#issuecomment-581803457

It seems what looks like an easy "by the way" cleanup
becomes some kind of nightmare...

Feel free to skip that part - unless you really like to clean it up
(which I would of course appreciate so much - but I cannot expect it).

gozora commented at 2020-02-05 19:01:

Hello @jsmeix,
I've updated this PR according your proposal, hope it looks better now ;-)

V.

gozora commented at 2020-02-06 09:27:

@jsmeix anytime!

I'll merge this PR later this week.

V.

[Export of Github issue for rear/rear.]