[Pkg-libvirt-maintainers] Bug#1064126: Bug#1064126: libvirt: install NSS modules into /usr

[Helmut Grohne]

Hi Andrea,

On Wed, Aug 28, 2024 at 12:17:05AM +0200, Andrea Bolognani wrote:
> I read up a bit on the various problems and mitigations documented as
> part of DEP17 yesterday and figured that our changes would probably
> hit P1. I hoped that M7 might be enough to take care of that, but I
> trust your expertise when you say that M7+M8 is a better fit for our
> specific case.

I confirm. M7 should work most of the time. In particular, when the tool
used to perform the upgrades is apt or based on apt (such as aptitude),
which is probably like 99% of the upgrades. However, there is a chance
of Conflicts not working as expected (DEP17 P12) when upgrading using
dpkg. There also is a thin chance of apt invoking dpkg in that bad way
in more complex upgrade scenarios. Hence, we recommend to combine M8 for
packages with important functions.

> The fact that the diversions will only exist for the duration of the
> upgrade, instead of sticking around afterwards, makes this approach
> seem particularly attractive to me.

In other cases where we want to avoid Conflicts (e.g. in the essential
set), the M7 protective diversions have to persist longer. Hence the
proposal of combining.

> So this part is as simple as replacing the existing
> 
>   Breaks: libvirt-daemon-system (<< 10.6.0-2~)
>   Replaces: libvirt-daemon-system (<< 10.6.0-2~)
> 
> with
> 
>   Conflicts: libvirt-daemon-system (<< 10.6.0-2~)
> 
> in the case of libvirt-daemon-log, right? Or are there additional
> considerations?

This looks good. Will you do the same change for libvirt-daemon,
libvirt-daemon-common, and libvirt-daemon-lock?

> I don't fully understand how that would help avoid the file loss
> scenario, but once again I trust your expertise here so thankfull I
> don't really need to O:-)

In case, you are interested let me add detail. Otherwise skip. When dpkg
is faced with unpacking a package that declares Conflicts with another
package that is scheduled for removal, it may proceed with the unpack
before the other package is actually removed. Thus there is a brief
moment when both are unpacked concurrently despite having declared
Conflicts. When the other package is removed, dpkg observes that e.g.
/lib/systemd/system/virtlogd.socket is not installed by any other
package and hence removes this file not realizing that it aliases
/usr/lib/systemd/system/virtlogd.socket, which should be kept. The
diversion is installed in preinst (hence before unpacking) and removed
in postinst (hence after removing the other package) and thus covers
this brief concurrency window. The diversion then redirects the removal
of /lib/systemd/system/virtlogd.socket to a location that usually does
not exist and thus saves /usr/lib/systemd/system/virtlogd.socket.

> > Hence, lintian may be annoyed.
> 
> That's fine, we can install overrides. That'd be more than warranted
> in this case.

I appreciate if you handle adding the relevant overrides.

> I believe you're better positioned to get the details of the
> implementation right, but I obviously have my own preferences when it
> comes to what the code that gets committed to libvirt looks like.
> 
> So for now I've sketched out an implementation, which you can find
> below. The calls to dpkg-divert are most likely completely wrong, and
> I'd appreciate it if you could fix them.

Actually, that looks like it mostly just works.

> Up to you whether you want to go all the way and prepare a MR with
> the changes, or leave that last bit to me. You'll obviously be
> credited in debian/changelog either way :)

Let me do the review part here given that you already did most of the
work and given that a number of remarks have multiple options and I
don't happen to know your preference.

> diff --git a/debian/libvirt-daemon-log.postinst.in b/debian/libvirt-daemon-log.postinst.in
> index 21723d4307..d75b30d930 100644
> --- a/debian/libvirt-daemon-log.postinst.in
> +++ b/debian/libvirt-daemon-log.postinst.in
> @@ -15,6 +15,7 @@ set -e
>  # the debian-policy package
>  
>  #FINISH_CONFFILE_TRANSFER#
> +#DELETE_PROTECTIVE_DIVERSION#

If you happen to enjoy merge requests originating from the Debian
Janitor, you may wrap all the added code like this:

# begin-remove-after: released:trixie
#DELETE_PROTECTIVE_DIVERSION#
# end-remove-after

This serves both as a human-readable reminder for removing the code
eventually, but it also is machine-readable and will make the Debian
Janitor propose a MR for removing this code at the desired time. I'll
leave it up to you whether you want to add support for this or not. If
you do, please wrap all relevant code to avoid a partial removal.

>  DAEMON_SYSTEM_TO_DAEMON_LOG="
>      /etc/default/virtlogd
> @@ -24,6 +25,12 @@ DAEMON_SYSTEM_SYSV_TO_DAEMON_LOG="
>      /etc/init.d/virtlogd
>  "
>  
> +VIRTLOGD_UNITS="
> +    virtlogd-admin.socket
> +    virtlogd.service
> +    virtlogd.socket
> +"
> +
>  case "$1" in
>      configure)
>          for conf in $DAEMON_SYSTEM_TO_DAEMON_LOG; do
> @@ -46,6 +53,13 @@ case "$1" in
>                  -- \
>                  "$@"
>          done
> +        for unit in $VIRTLOGD_UNITS; do
> +            delete_protective_diversion \
> +                "/lib/systemd/system/$unit" \
> +                "10.6.0-3~" \
> +                -- \
> +                "$@"
> +        done

I suggest running this snippet also in the abort-upgrade case.

Also add these calls to postrm matching the arguments failed-upgrade,
abort-install, and abort-upgrade.

Matching all of these should ensure that the protective diversions are
always removed at the end of a dpkg transaction and thus you do not have
to clean them up in a later upload.

>      ;;
>  
>      abort-upgrade|abort-remove|abort-deconfigure)
> diff --git a/debian/libvirt-daemon-log.preinst.in b/debian/libvirt-daemon-log.preinst.in
> new file mode 100644
> index 0000000000..f5dbf73de9
> --- /dev/null
> +++ b/debian/libvirt-daemon-log.preinst.in
> @@ -0,0 +1,45 @@
> +#!/bin/sh
> +
> +set -e
> +
> +# summary of how this script can be called:
> +#
> +#   * <new-preinst> `install'
> +#   * <new-preinst> `install' <old-version> <new-version>
> +#   * <new-preinst> `upgrade' <old-version> <new-version>
> +#   * <old-preinst> `abort-upgrade' <new-version>
> +#
> +# for details, see https://www.debian.org/doc/debian-policy/ or
> +# the debian-policy package
> +
> +#CREATE_PROTECTIVE_DIVERSION#
> +
> +VIRTLOGD_UNITS="
> +    virtlogd-admin.socket
> +    virtlogd.service
> +    virtlogd.socket
> +"
> +
> +case "$1" in
> +    install|upgrade)
> +        for unit in $VIRTLOGD_UNITS; do
> +            create_protective_diversion \
> +                "/lib/systemd/system/$unit" \
> +                "10.6.0-3~" \
> +                -- \
> +                "$@"
> +        done
> +    ;;
> +
> +    abort-upgrade)
> +    ;;
> +
> +    *)
> +        echo "preinst called with unknown argument \`$1'" >&2
> +        exit 1
> +    ;;
> +esac
> +
> +#DEBHELPER#
> +
> +exit 0

You have handled the libvirt-daemon-log units. The dumat report
mentioned three other packages libvirt-daemon, libvirt-daemon-common,
and libvirt-daemon-lock. Will you repeat the maintainer scripts for them
and their mentioned units in the dumat report? I hope it is fairly
obvious how to apply this.

> diff --git a/debian/snippets.sh b/debian/snippets.sh
> index 6b03f242d6..ad86432332 100644
> --- a/debian/snippets.sh
> +++ b/debian/snippets.sh
> @@ -194,6 +194,54 @@ remove_config_from_template() {
>  }
>  #END REMOVE_CONFIG_FROM_TEMPLATE
>  
> +#BEGIN CREATE_PROTECTIVE_DIVERSION
> +create_protective_diversion() {
> +    local usrfile="$1"
> +    local firstver="$2"
> +
> +    if [ "$2" != "--" ]; then

Do you mean $3 here? I think $2 is firstver.

> +        echo "create_protective_diversion called with the wrong number of arguments" >&2
> +        return 1
> +    fi
> +    for _ in $(seq 1 2); do
> +        shift
> +    done
> +
> +    if [ -z "$2" ] || dpkg --compare-versions -- "$2" gt "$lastver"; then
> +        return 0
> +    fi

Please remove this check. For instance, libvirt-daemon-common does not
exist in bookworm. Hence $2 will be empty and you'd skip the addition of
these diversions when installing libvirt-daemon-common, but they're
really needed on such a first installation. What you could check here is
the version of libvirt-daemon-system (the package we are moving the
units from), but the added value in doing so does not warrant the added
complexity in my opinion.

> +
> +    dpkg-divert \
> +        --no-rename \
> +        --divert "$usrfile.usr-is-merged" \
> +        --add "$usrfile"
> +}
> +#END CREATE_PROTECTIVE_DIVERSION
> +
> +#BEGIN DELETE_PROTECTIVE_DIVERSION
> +delete_protective_diversion() {
> +    local usrfile="$1"
> +    local firstver="$2"
> +
> +    if [ "$2" != "--" ]; then

Do you mean $3 here? I think $2 is firstver.

> +        echo "delete_protective_diversion called with the wrong number of arguments" >&2
> +        return 1
> +    fi
> +    for _ in $(seq 1 2); do
> +        shift
> +    done
> +
> +    if [ ! -e "$usrfile.usr-is-merged" ]; then
> +        return 0
> +    fi

Please remove this check. The diverted file is expected to never exist.
The sole point of the diversion is to redirect the removal of the file
to a harmless location. This check would result in the diversions never
getting removed. If you feel like you need to add a condition, consider
using this:

if [ -z "$(dpkg-divert --list "$usrfile")" ]; then
    return 0
fi

> +    dpkg-divert \
> +        --no-rename \
> +        --divert "$usrfile.usr-is-merged" \
> +        --remove "$usrfile"
> +}
> +#END DELETE_PROTECTIVE_DIVERSION
> +
>  #BEGIN SYSTEMD_DAEMON_RELOAD
>  systemd_daemon_reload() {
>      if [ -z "${DPKG_ROOT:-}" ] && [ -d /run/systemd/system ]; then

Does your salsa-ci pipeline run piuparts? If not, consider running it
locally before uploading to experimental. It should catch the worst of
issues.

Helmut