[libvirt] [PATCH] rpm: fix incorrect expansion of %systemd_preun macro
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. Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- 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 %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 -- 2.14.3
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@redhat.com>
Reviewed-by: Laine Stump <laine@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
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. Assuming the above is correct, I'd argue the fix is possibly not even worth backporting to the maintenance branch. 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. -- Andrea Bolognani / Red Hat / Virtualization
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.
On Tue, Mar 20, 2018 at 07:13:24PM +0100, 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.
No that is not the case. On every release I synchronize the libvirt upstream spec into the Fedora spec. The only thing different about Fedora spec is that it has changelog entries. That's why we go to the trouble of making sure conditionals cope with multiple distros in our upstream spec. Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
On Wed, 2018-03-21 at 08:48 +0000, Daniel P. Berrangé wrote:
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.
No that is not the case. On every release I synchronize the libvirt upstream spec into the Fedora spec. The only thing different about Fedora spec is that it has changelog entries. That's why we go to the trouble of making sure conditionals cope with multiple distros in our upstream spec.
Oh, cool: I thought it couldn't possibly be that simple, but I'm glad to hear that's not the case :) -- Andrea Bolognani / Red Hat / Virtualization
On Tue, Mar 20, 2018 at 01:54:49PM -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".
It is not quite as bad as I feared. When I upgraded using dnf i get this Running scriptlet: libvirt-daemon-4.1.0-1.fc27.x86_64 144/146 /var/tmp/rpm-tmp.wB7JPz: line 6: virtlogd.socket: command not found error: %preun(libvirt-daemon-4.1.0-1.fc27.x86_64) scriptlet failed, exit status 127 Error in PREUN scriptlet in rpm package libvirt-daemon Error in PREUN scriptlet in rpm package libvirt-daemon And as a result have this: # rpm -q libvirt-daemon libvirt-daemon-4.1.0-1.fc27.x86_64 libvirt-daemon-4.1.0-2.fc27.x86_64 This is merely a cosmetic problem though - all the files from the outdated version are gone. Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
On 03/21/2018 05:55 AM, Daniel P. Berrangé wrote:
On Tue, Mar 20, 2018 at 01:54:49PM -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". It is not quite as bad as I feared. When I upgraded using dnf i get this
Running scriptlet: libvirt-daemon-4.1.0-1.fc27.x86_64 144/146 /var/tmp/rpm-tmp.wB7JPz: line 6: virtlogd.socket: command not found error: %preun(libvirt-daemon-4.1.0-1.fc27.x86_64) scriptlet failed, exit status 127 Error in PREUN scriptlet in rpm package libvirt-daemon Error in PREUN scriptlet in rpm package libvirt-daemon
And as a result have this:
# rpm -q libvirt-daemon libvirt-daemon-4.1.0-1.fc27.x86_64 libvirt-daemon-4.1.0-2.fc27.x86_64
This is merely a cosmetic problem though - all the files from the outdated version are gone.
And then can the cosmetic problem be safely solved with "rpm -e --nodeps --noscripts --justdb libvirt-daemon-4.1.0-1"? (or maybe not all of those are required?)
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.
I forgot to ask this in the previous message - would it be reasonable to have a syntax-check rule that forbid any line in libvirt.spec.in that started with "<whitespace>%" and ended with \ ? (Personally I had no idea of this rule, and I'm sure most others didn't either).
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- 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 %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
On Tue, Mar 20, 2018 at 01:57:16PM -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.
I forgot to ask this in the previous message - would it be reasonable to have a syntax-check rule that forbid any line in libvirt.spec.in that started with "<whitespace>%" and ended with \ ? (Personally I had no idea of this rule, and I'm sure most others didn't either).
Unfortunately it is hard to distinguish from a %macro that is accepting arguments, vs a %macro that is just expanding to a plain variable, so something like grep '%.*\\' has too many false positives. Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
participants (3)
-
Andrea Bolognani -
Daniel P. Berrangé -
Laine Stump