Hi! On Tue, 2021-06-29 at 08:01:49 +0200, Helmut Grohne wrote: > Package: debianutils > Version: 4.11.2 > Severity: wishlist > Tags: patch > User: helm...@debian.org > Usertags: rebootstrap
> We've had a discussion on the expectations on /etc/shells on > d-devel@l.d.o recently. You can find the entry point at: > https://lists.debian.org/ymjtirkqbjdjy...@alf.mars Thanks for handling and implementing this! :) > As a result of the discussion, we propose that client packages can > replace their add-shell invocations with adding a file > /usr/share/debianutils/shells.d/$PKG containing the shells to be added. I'm wondering whether tying the pathname to a Debian specific package name might make a possible "standardization" or adoption by say other distributions or upstream harder? Perhaps a more generic name would leave the door open to such adoption, so that upstreams could ship those files directly f.ex.? > diff --minimal -Nru debianutils-4.11.2/update-shells > debianutils-4.11.2+nmu1/update-shells > --- debianutils-4.11.2/update-shells 1970-01-01 01:00:00.000000000 +0100 > +++ debianutils-4.11.2+nmu1/update-shells 2021-06-28 20:14:17.000000000 > +0200 > @@ -0,0 +1,144 @@ > +# Check whether hashset $1 contains element $2. > +hashset_contains() { > + case "$1" in > + *"#$2#"*) return 0 ;; > + *) return 1 ;; > + esac > +} > + > +log() { > + if [ "$VERBOSE" = 1 ]; then > + echo "$*" > + fi > +} > + > +ROOT= > +VERBOSE=0 > +NOACT=0 > + > +while [ $# -gt 0 ]; do > + case "$1" in > + --help) > + cat <<EOF > +usage: $0 [options] > + > + --no-act Do not move the actual update into place > + --verbose Be more verbose > + --root DIR Operate on the given chroot, defaults to / > +EOF > + exit 0 > + ;; > + --no-act) > + NOACT=1 > + ;; > + --root) > + shift > + if [ "$#" -lt 1 ]; then > + echo "missing argument to --root" 1>&2 > + exit 1 > + fi > + ROOT=$1 > + ;; > + --verbose) > + VERBOSE=1 > + ;; > + *) > + echo "unrecognized option $1" 1>&2 > + exit 1 > + ;; > + esac > + shift > +done > + > +PKG_DIR="$ROOT/usr/share/debianutils/shells.d" > +STATE_FILE="$ROOT/var/lib/shells.state" > +ETC_FILE="$ROOT/etc/shells" > +NEW_ETC_FILE="$ETC_FILE.tmp" > +NEW_STATE_FILE="$STATE_FILE.tmp" > + > +PKG_SHELLS='#' > +LC_COLLATE=C.UTF-8 # glob in reproducible order > +for f in "$PKG_DIR/"*; do > + [ "$f" = "$PKG_DIR/*" ] && break > + while IFS='#' read -r line _; do > + [ -n "$line" ] || continue > + PKG_SHELLS="$PKG_SHELLS$line#" > + realshell=$(dpkg-realpath --root "$ROOT" "$line") Not sure how desirable this would be, but if DPKG_ROOT is set in the caller environment, but the caller did not specify --root for this program, then DPKG_ROOT will not be taken into account by dpkg-realpath, as this unconditionally overrides it. Perhaps this program should be defaulting ROOT to DPKG_ROOT? > + if [ "$line" != "$realshell" ]; then > + PKG_SHELLS="$PKG_SHELLS$realshell#" > + fi > + done < "$f" > +done Doesn't seem to handle an interrupted invocation by removing staged files for in-between steps for the shells and state files. > +STATE_SHELLS='#' > +if [ -e "$STATE_FILE" ] ; then > + while IFS='#' read -r line _; do > + [ -n "$line" ] && STATE_SHELLS="$STATE_SHELLS$line#" > + done < "$STATE_FILE" > +fi > + > +cleanup() { > + rm -f "$NEW_ETC_FILE" "$NEW_STATE_FILE" > +} > +trap cleanup EXIT > + > +: > "$NEW_ETC_FILE" > +ETC_SHELLS='#' > +while IFS= read -r line; do > + shell=${line%%#*} > + # copy all comment lines, packaged shells and local additions > + if [ -z "$shell" ] || > + hashset_contains "$PKG_SHELLS" "$shell" || > + ! hashset_contains "$STATE_SHELLS" "$shell"; then > + echo "$line" >> "$NEW_ETC_FILE" > + ETC_SHELLS="$ETC_SHELLS$shell#" Isn't this missing the filesystem existence check for the shell to handle local admin additions and the default fragment default cases, as described in the algorithm? > + else > + log "removing shell $shell" > + fi > +done < "$ETC_FILE" > + > +: > "$NEW_STATE_FILE" > +saved_IFS=$IFS > +IFS='#' > +set -f > +# shellcheck disable=SC2086 # word splitting intended, globbing disabled > +set -- ${PKG_SHELLS###} > +set +f > +IFS=$saved_IFS > +for shell; do > + echo "$shell" >> "$NEW_STATE_FILE" > + # add shells that are neither already present nor locally removed > + if ! hashset_contains "$ETC_SHELLS" "$shell" && > + ! hashset_contains "$STATE_SHELLS" "$shell"; then > + echo "$shell" >> "$NEW_ETC_FILE" > + log "adding shell $shell" > + fi > +done I've just skimmed over this algo (and run out of time now), but it does not match directly to the one proposed on the list so I'll have to think about this a bit more. :) > +if [ "$NOACT" = 0 ]; then > + if [ -e "$STATE_FILE" ]; then > + chmod --reference="$STATE_FILE" "$NEW_STATE_FILE" > + chown --reference="$STATE_FILE" "$NEW_STATE_FILE" > + else > + chmod 0644 "$NEW_STATE_FILE" > + fi > + chmod --reference="$ETC_FILE" "$NEW_ETC_FILE" > + chown --reference="$ETC_FILE" "$NEW_ETC_FILE" > + sync --data "$NEW_ETC_FILE" "$NEW_STATE_FILE" > + mv "$NEW_ETC_FILE" "$ETC_FILE" > + sync "$ETC_FILE" > + mv "$NEW_STATE_FILE" "$STATE_FILE" > + sync "$STATE_FILE" I think this is missing sync'ing the containing directories too. > + trap "" EXIT > +fi > diff --minimal -Nru debianutils-4.11.2/update-shells.8 > debianutils-4.11.2+nmu1/update-shells.8 > --- debianutils-4.11.2/update-shells.8 1970-01-01 01:00:00.000000000 > +0100 > +++ debianutils-4.11.2+nmu1/update-shells.8 2021-06-28 18:17:57.000000000 > +0200 > @@ -0,0 +1,31 @@ > +.TH UPDATE-SHELLS 8 "28 Jun 2021" > +.SH NAME > +update-shells \- update the list of valid login shells > +.SH SYNOPSIS > +.B update-shells > +.RI [ options ] > +.SH DESCRIPTION > +.B update-shells > +locates the shells provided by packages from > +.I /usr/share/debianutils/shells.d > +and updates > +.I /etc/shells > +with newly added or removed shells. > +To track changes made by the administrator, it consults a state file in > +.I /var/lib/shells.state . Perhaps move or add the file descriptions to a FILES section? Thanks, Guillem
