noon
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(a)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
12:54 p.m.
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(a)redhat.com>
Reviewed-by: Laine Stump <laine(a)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
1:13 p.m.
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
2:48 p.m.
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.
3:48 a.m.
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 :|
4:02 a.m.
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
4:55 a.m.
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 :|
8:13 a.m.
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?)
12:57 p.m.
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(a)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
3:49 a.m.
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 :|
2438
days inactive
2439
days old
9 comments
3 participants
participants (3)
-
Andrea Bolognani -
Daniel P. Berrangé -
Laine Stump