#2376 PR `merged`: Auto-detect DHCP client with systemd-networkd (closes #2375)¶

Labels: enhancement, cleanup, fixed / solved / done

OliverO2 opened issue at 2020-04-17 12:25:¶

Pull Request Details:¶

Type: Enhancement
Impact: Normal
Reference to related issue (URL): #2375
How was this pull request tested? On Ubuntu 18.04.4 LTS
Brief description of the changes in this pull request: See issue.

jsmeix commented at 2020-04-17 12:36:¶

I would appreciate it if another ReaR maintainer
could additionally have a look and also approve it
or request changes if needed.

OliverO2 commented at 2020-04-17 13:36:¶

After doing some more testing with overrides, I found a bug: The present code will accept an enabled DHCP client on any network connection. However, with Docker there may be additional network connections configured with DHCP enabled. These should be ignored. Fix upcoming.

jsmeix commented at 2020-04-17 14:14:¶

@OliverO2
while you are working on that file could you
by the way also improve

COPY_AS_IS=( "${COPY_AS_IS[@]}" "/etc/localtime" "/usr/lib/dhcpcd/*" )
PROGS=( "${PROGS[@]}" arping ipcalc usleep "${dhclients[@]}" )

to

COPY_AS_IS+=( "/etc/localtime" "/usr/lib/dhcpcd/*" )
PROGS+=( arping ipcalc usleep "${dhclients[@]}" )

cf. https://github.com/rear/rear/issues/2364

OliverO2 commented at 2020-04-17 14:23:¶

@jsmeix
Sure, also dhclients+=("$x") in line 19, I guess...

jsmeix commented at 2020-04-17 14:39:¶

@OliverO2
yes of course!
You know my current regexp doesn't find all ;-)
By the way: I am finished with what my current regexp found :-)

OliverO2 commented at 2020-04-17 15:08:¶

@jsmeix

Done. I wanted to clean up the entire dhclients variable setting but didn't dare to: I'm still not sure what dhclients changes in array/string type across the file are all about.

For me it looks like that the define_dhclients_variable function could be replaced by something like

define_dhclients_variable() {
    dhclients=$(printf "%s\n" "${DHCLIENT_BIN##*/}" "${DHCLIENT6_BIN##*/}" dhcpcd dhclient dhcp6c dhclient6 | grep . | sort -u)
}

jsmeix commented at 2020-04-17 15:59:¶

I think @gdha is our DHCP expert here ;-)

OliverO2 commented at 2020-04-20 10:54:¶

@jsmeix
Yes, these type changes between string and array are what I was wondering about. I'll come up with a commit trying to clean up that dhclient stuff.

OliverO2 commented at 2020-04-20 14:06:¶

@jsmeix The old code was a bit too messy for my taste that I re-wrote it to what I think was really intended. Also added a few checks which were not there originally. See commit remarks and code.

What still remains: The distinction between DHCLIENT_BIN and DHCLIENT6_BIN is not really necessary as even the code picking up these variables in 58-start-dhclient.sh does not really need it. However, these variables (although still undocumented in default.conf) might be present in some user's local configuration files.

jsmeix commented at 2020-04-20 15:24:¶

According to

localhost:~/rear.github.master # find . -type f | xargs grep -l 'DHCLIENT_BIN'

./usr/share/rear/skel/default/etc/scripts/system-setup.d/58-start-dhclient.sh
./usr/share/rear/prep/GNU/Linux/210_include_dhclient.sh

DHCLIENT_BIN and DHCLIENT6_BIN are internal variables.

In general what is not documented in default.conf
is not meant as a user config variable.

We have tons of internal variables with uppercase names
that are not meant as user config variables,
cf. the section "Variables" in
https://github.com/rear/rear/wiki/Coding-Style

All variables that are used in more than a single script must be all-caps:
$FOO instead of $foo or $Foo.

Of course we also have globally used variables
that do not have uppercase names e.g. backuparchive:

# find usr/sbin/rear usr/share/rear -type f | xargs egrep -l 'backuparchive=|\$backuparchive' | wc -l
18

:-(

OliverO2 commented at 2020-04-20 15:30:¶

@jsmeix The problem is that configuring DHCLIENT[6]_BIN in local.conf was suggested in comments of 210_include_dhclient.sh before. Comments and actual code were also not consistent as the comments suggested one could define dhclient binaries beyond what was included in the known clients list. However, 58-start-dhclient.sh could not deal with such an unknown client.

jsmeix commented at 2020-04-20 15:36:¶

@OliverO2
personally I would say feel free to clean it up mercilessly
to make things straightforward and consistent.

Personally I was too often completely confused
with the DHCP setup code so I would so much appreciate
a generic cleanup in this area.

With a more straightforward code we could much more
reliably maintain that code in the future.

@gdha
what do you think about a generic cleanup of the DHCP setup code?

jsmeix commented at 2020-04-20 15:49:¶

@OliverO2
in particular you can safely remove all what is not supported by
skel/default/etc/scripts/system-setup.d/58-start-dhclient.sh
because no user reported that something is missing in this area
which indicates that no user uses more than what is supported by
skel/default/etc/scripts/system-setup.d/58-start-dhclient.sh

gdha commented at 2020-04-20 15:54:¶

@OliverO2 @jsmeix the dhcp code was an ugly copy/paste from ?? (old RH code I guess) and does indeed require a serious facelift.

OliverO2 commented at 2020-04-20 15:55:¶

@gdha @jsmeix OK, so I'll clean up the last bits and then ask you to have a final look.

OliverO2 commented at 2020-04-20 18:38:¶

@gdha @jsmeix

[x] Did the final cleanup (as far as I am concerned).
[x] Tested on Ubuntu 18.04.4 LTS Desktiop and Server edition.

Over to you!

OliverO2 commented at 2020-04-21 11:15:¶

@jsmeix
Thanks for your thorough review. Always appreciated.

[Export of Github issue for rear/rear.]

#2376 PR merged: Auto-detect DHCP client with systemd-networkd (closes #2375)¶