Nwfilter can be edited by the user and we don't want to overwrite the editings.
Also the filters in %{datadir} does not have UUIDs and these are generated on
libvirtd start. Thus this patch also fixes regeneration of UUIDs on libvirtd
update.

Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy@virtuozzo.com>
---

It is a successor to original version of the patch [1]. The discussion can
be found in [2].

Diff to v1:
- just keep existing nwfilters untouched instead of bringing new
  version of nwfilter while keeping UUID 

[1] https://www.redhat.com/archives/libvir-list/2020-October/msg01357.html
[2] https://www.redhat.com/archives/libvir-list/2020-December/msg00260.html

 libvirt.spec.in | 19 ++++++++++++++-----
 1 file changed, 14 insertions(+), 5 deletions(-)

diff --git a/libvirt.spec.in b/libvirt.spec.in
index 880051b..98914ce 100644
--- a/libvirt.spec.in
+++ b/libvirt.spec.in
@@ -1439,12 +1439,21 @@ fi
 rm -rf %{_localstatedir}/lib/rpm-state/libvirt || :
 
 %post daemon-config-nwfilter
-cp %{_datadir}/libvirt/nwfilter/*.xml %{_sysconfdir}/libvirt/nwfilter/
-# libvirt saves these files with mode 600
-chmod 600 %{_sysconfdir}/libvirt/nwfilter/*.xml
+restart_daemon=0
+for dfile in %{_datadir}/libvirt/nwfilter/*.xml; do
+  sfile=%{_sysconfdir}/libvirt/nwfilter/`basename $dfile`
+  if [ ! -f "$sfile" ]; then
+    cp "$dfile" "$sfile"
+    # libvirt saves these files with mode 600
+    chmod 600 "$sfile"
+    restart_daemon=1
+  fi
+done
 # Make sure libvirt picks up the new nwfilter defininitons
-mkdir -p %{_localstatedir}/lib/rpm-state/libvirt || :
-touch %{_localstatedir}/lib/rpm-state/libvirt/restart || :
+if [ $restart_daemon -eq 1 ]; then
+  mkdir -p %{_localstatedir}/lib/rpm-state/libvirt || :
+  touch %{_localstatedir}/lib/rpm-state/libvirt/restart || :
+fi
 
 %posttrans daemon-config-nwfilter
 if [ -f %{_localstatedir}/lib/rpm-state/libvirt/restart ]; then
-- 
1.8.3.1

On 12/7/20 9:38 AM, Nikolay Shirokovskiy wrote:
> Nwfilter can be edited by the user and we don't want to overwrite the editings.
> Also the filters in %{datadir} does not have UUIDs and these are generated on
> libvirtd start. Thus this patch also fixes regeneration of UUIDs on libvirtd
> update.
> 
> Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy@virtuozzo.com>
> ---
> 
> It is a successor to original version of the patch [1]. The discussion can
> be found in [2].
> 
> Diff to v1:
> - just keep existing nwfilters untouched instead of bringing new
>    version of nwfilter while keeping UUID
> 
> [1] https://www.redhat.com/archives/libvir-list/2020-October/msg01357.html
> [2] https://www.redhat.com/archives/libvir-list/2020-December/msg00260.html
> 
>   libvirt.spec.in | 19 ++++++++++++++-----
>   1 file changed, 14 insertions(+), 5 deletions(-)

Reviewed-by: Michal Privoznik <mprivozn@redhat.com>

But you may want to wait for Andrea's approval, since he's suggested it.

Michal

On Mon, 2020-12-07 at 11:38 +0300, Nikolay Shirokovskiy wrote:
> +++ b/libvirt.spec.in
> +restart_daemon=0
> +for dfile in %{_datadir}/libvirt/nwfilter/*.xml; do
> +  sfile=%{_sysconfdir}/libvirt/nwfilter/`basename $dfile`

Please use $() instead of ``. Some additional quoting would also be
nice.

> +  if [ ! -f "$sfile" ]; then
> +    cp "$dfile" "$sfile"
> +    # libvirt saves these files with mode 600
> +    chmod 600 "$sfile"

Instead of copying the file and changing its permissions afterwards,
just do

  install -m 0600 "$dfile" "$sfile"

May I also suggest using more explicit names than "dfile" and
"sfile"? I have no idea what you were going for with them, but one
possible interpretation that comes to mind is "destination file" and
"source file" respectively - except they are used exactly the
opposite way in practice ;)

>  # Make sure libvirt picks up the new nwfilter defininitons
> -mkdir -p %{_localstatedir}/lib/rpm-state/libvirt || :
> -touch %{_localstatedir}/lib/rpm-state/libvirt/restart || :
> +if [ $restart_daemon -eq 1 ]; then
> +  mkdir -p %{_localstatedir}/lib/rpm-state/libvirt || :
> +  touch %{_localstatedir}/lib/rpm-state/libvirt/restart || :
> +fi

Do we even need to do this conditionally? For the default network we
don't, so I wonder if we should be more careful there? At the same
time, you're supposed to be able to restart the daemon pretty much
anytime without suffering any consequences, so perhaps we don't need
to be this careful here, especially since chances are that you'll be
updating the daemon at the same time - and that *definitely* requires
a restart at the end of the transaction anyway.

-- 
Andrea Bolognani / Red Hat / Virtualization