Draft: Refactor for improvements and shellcheck
Merge request reports
Activity
I started re-reading the changes here commit by commit, and a tip for the future is to not do refactoring and style changes in the same commit.
Is already fairly large, but if you had not done re-ordering and style changes it would have been easier to get an overview of the refactoring work being done.
I do understand parts of this is shellcheck complaining, but that should preferably be separate commits.
53 local dest=$(resolve_link "$chrootdir/etc/resolv.conf" "$chrootdir") 54 55 # If we don't have a source resolv.conf file, there's nothing useful we can do. 56 [[ -e $src ]] || return 0 57 58 if [[ ! -e $dest ]]; then 59 # There are two reasons the destination might not exist: 60 # 61 # 1. There may be no resolv.conf in the chroot. In this case, $dest won't exist, 62 # and it will be equal to $1/etc/resolv.conf. In this case, we'll just exit. 63 # The chroot environment must not be concerned with DNS resolution. 64 # 65 # 2. $1/etc/resolv.conf is (or resolves to) a broken link. The environment 66 # clearly intends to handle DNS resolution, but something's wrong. Maybe it 67 # normally creates the target at boot time. We'll (try to) take care of it by 68 # creating a dummy file at the target, so that we have something to bind to. Hmm, but I changed the logic to always create
resolv.confin chroot. An empty file doesn't hurt I guess?And if it's a symlink, after my change the symlink would be moved away and we bind mount to a newly-created regular file. The symlink will be restored during cleanup.
Shall I just add a description of the new logic?
Created by: evelikov
Greetings, recently I had a go at fixing some pet peeves in this project. I'm glad to see that you've addressed some of those, although it seems like this PR is far too long and doing multiple unrelated things at the same time.
Will drop some individual notes in a moment, hope you find them useful
2 2 3 3 shopt -s extglob 4 4 5 # m4_include() is recognized as a function definition 6 # shellcheck source=common disable=SC1073,SC1065,SC1064,SC1072 5 7 m4_include(common) 6 8 7 9 setup=chroot_setup 8 10 unshare=0 11 userspec='' 12 chroot_args=() -
Created by: evelikov
The commit mixes functional and cosmetic changes. It also introduces promotes local variables to global scope eg
chroot_args- that should either be voided to documented.I would suggest fixing the functional/shellcheck issues - one commit per script. Then doing the same on the cosmetic side.
38 [[ -n $root && $root != */ ]] && root=$root/ 39 40 while [[ -L $target ]]; do 41 target=$(readlink -m "$target") 42 # If a root was given, make sure the target is under it. 43 # Make sure to strip any leading slash from target first. 44 [[ -n $root && $target != $root* ]] && target=$root${target#/} 45 done 46 47 printf %s "$target" 48 } 49 50 chroot_add_resolv_conf() { 51 local chrootdir=$1 52 local src=$(resolve_link /etc/resolv.conf) 53 local dest=$(resolve_link "$chrootdir/etc/resolv.conf" "$chrootdir") -
Created by: evelikov
The commit moves some code around while also changes the functionality. These two should be separate commits.
The commit message mentions "chroot_add_resolve_link is cleaned up to be simpler" where chroot_add_resolve_link does not exist in either new or old code. Ideally the message will mention what or why the new approach is simpler.
To-do for myself: add a comment block to the end of the generated fstab, for quirks like !44 (merged)
130 159 160 chroot_add_resolv_conf() { 161 local src="$(resolve_link /etc/resolv.conf)" dest="$1/etc/resolv.conf" 162 163 # If we don't have a source resolv.conf file, there's nothing useful we can do. 164 if [[ ! -f "$src" ]]; then 165 warning 'Cannot find a usable resolv.conf. DNS requests might fail in chroot' 166 return 0 167 fi 168 169 # If resolv.conf in the chroot is a symlink, we make a backup of it 170 # and create a plain file so we can bind mount to it. 171 # The backup is restore during chroot_teardown(). 172 173 if [[ -L "$dest" ]]; then 174 mv "$dest" "$dest.orig" mentioned in merge request !65
