[libvirt] [PATCH] rpm: fix incorrect expansion of %systemd_preun macro

[Laine Stump]

On 03/20/2018 01:00 PM, Daniel P. Berrangé wrote:
> Macros in RPMs are expanded before line continuations, so when we write
>
>    %systemd_preun foo \
>                   bar
>
> What happens is that it expands to
>
>    if [ $1 -eq 0 ] ; then
>         # Package removal, not upgrade
>         systemctl --no-reload disable --now foo \ > /dev/null 2>&1 || :
>    fi
>                  bar
>
> which is obviously complete garbage and not what we expected. It is
> simply not safe to ever use line continuations in combination with
> macros.

Introduced in commit bffdd6c3034164127b1543ffd2e9ed599baf4838, present
in released libvirt-4.1.0.


This is going to be problematic for any rpm-based distro that has a
4.1.0 rpm, e.g. Fedora rawhide and F28 - if someone has updated to the
broken rpm, they won't be able to get rid of it with a plain update, and
dnf has no command that passes through the necessary --nopreun command
to rpm. Instead, they'll need to run rpm manually - "rpm --nopreun blah
blah".

If there is already a 4.1.0-maint branch, we should pull this patch back
to there, and think about how to notify the poor F28/rawhide users of
their predicament (hopefully there aren't too many, as F28 isn't yet
released)

>
> Signed-off-by: Daniel P. Berrangé <berrange at redhat.com>

Reviewed-by: Laine Stump <laine at laine.org>

> ---
>  libvirt.spec.in | 12 ++++++------
>  1 file changed, 6 insertions(+), 6 deletions(-)
>
> diff --git a/libvirt.spec.in b/libvirt.spec.in
> index bc8257f34b..6bf8368476 100644
> --- a/libvirt.spec.in
> +++ b/libvirt.spec.in
> @@ -1512,9 +1512,9 @@ exit 0
>  
>  %if %{with_systemd}
>      %if %{with_systemd_macros}
> -        %systemd_post virtlockd.socket virtlockd-admin.socket \
> -            virtlogd.socket virtlogd-admin.socket \
> -            libvirtd.service
> +        %systemd_post virtlockd.socket virtlockd-admin.socket
> +        %systemd_post virtlogd.socket virtlogd-admin.socket
> +        %systemd_post libvirtd.service

It ends up being a bit inefficient in the shell script that's created,
since it puts each line in its own conditional, but that's not visible
to the user anywhere, and the performance impact is effectively 0 :-)

>      %else
>  if [ $1 -eq 1 ] ; then
>      # Initial installation
> @@ -1549,9 +1549,9 @@ touch %{_localstatedir}/lib/rpm-state/libvirt/restart || :
>  %preun daemon
>  %if %{with_systemd}
>      %if %{with_systemd_macros}
> -        %systemd_preun libvirtd.service \
> -            virtlogd.socket virtlogd-admin.socket virtlogd.service \
> -            virtlockd.socket virtlockd-admin.socket virtlockd.service
> +        %systemd_preun libvirtd.service
> +        %systemd_preun virtlogd.socket virtlogd-admin.socket virtlogd.service
> +        %systemd_preun virtlockd.socket virtlockd-admin.socket virtlockd.service
>      %else
>  if [ $1 -eq 0 ] ; then
>      # Package removal, not upgrade