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
