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

[Laine Stump]

On 03/20/2018 02:13 PM, Andrea Bolognani wrote:
> On Tue, 2018-03-20 at 13:54 -0400, Laine Stump wrote:
>> 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)
> IIUC Fedora and other distributions each have their own spec file
> which, while probably derived from and for the most part identical
> to the upstream one, is actually maintained separately.

True, for some varying definition of "maintained separately". Usually
when the package is rebased, a new specfile will be taken from upstream
and then modified with any downstream-specific bits, and if an
individual patch is backported that modifies the specfile, the
downstream copy of the specfile will be updated accordingly.


(In Fedora, occasionally a "proven packager" will come through and make
changes to the specfiles in packages with no explicit notification to
upstream. This is *very* annoying because if there isn't someone
watching closely, those changes will be eliminated the next time the
specfile is re-imported from upstream on a rebase. And then there are
the people who actually create downstream-only patches not with a patch
file that's applied by the specfile, but by adding a series of "sed -e
s/blah/blah/...." commands to the specfile!!! Don't even get me started
about that...)

>
> Assuming the above is correct, I'd argue the fix is possibly not
> even worth backporting to the maintenance branch.

Whether/how the downstream maintainer uses the libvirt.spec.in from the
-maint branch in upstream git all depends on how they deal with updating
the specfile in their repo. If it's semi-automated, then it would
certainly be important to backport the patch to the -maint branch. Even
if not, it's good information. In any case, it's simple to do (just a
cherry-pick), and it certainly doesn't *hurt* to have the proper file on
the branch in git, for reference if nothing else.


(Periodically Cole updates the libvirt in Fedora by "rebasing" to a new
tarball from the -maint branch - it may not look like a rebase since
it's not moving to a new major release of libvirt, but it *is
operationally the same - a minor release is tagged in upstream git, a
new source tarball is taken from that tag, the libvirt.spec re-imported
so that the backport patchlist is zero'ed out, and any "permanent"
downstream patches re-added)

Of course that's all a bit moot right now, since I think we don't even
yet *have* a v4.1-maint branch :-P

>  The downstream
> maintainers, on the other hand, should definitely be notified of
> the issue so that they can make sure their own spec files are not
> affected by it.

The F28 and Rawhide specfiles *are* affected, most probably because Dan
did something like what I outlined above when he rebased it to 4.1.0. So
of course just pulling it into a v4.1-maint branch upstream isn't going
to automatically get it fixed in Fedora (or whatever other distro might
use rpms), and the maintainers need to know about it. Depending on how
they update, it may happen semi-automatically, but certainly it should
be done as soon as possible, to lower the number of people with "stuck"
libvirt-daemon packages.