On Tue, Jul 20, 2021 at 07:00:20PM +0200, Olaf Hering wrote: > sysconfig files are owned by the admin of the host. He has the liberty > to put anything he wants into these files. This makes it difficult to > provide different built-in defaults. s/He has/They have/ s/he wants/they want/ > +++ b/NEWS.rst > @@ -15,6 +15,16 @@ v7.6.0 (unreleased) > > * **Improvements** > > + * packaging: sysconfig files no longer installed > + > + libvirt used to provide defaults in various /etc/sysconfig/ files, such > + as /etc/sysconfig/libvirt. Since these files are owned by the admin, this > + made it difficult to change built-in defaults in case such file was > + modified by the admin. The built-in defaults are now part of the provided > + systemd unit files, such as libvirtd.service. These unit files continue > + to parse sysconfig files, in case they are created by the admin and filled > + with the desired key=value pairs. The releae notes should be updated in a separate commit, to make it possible to backport the functional changes only without running into unnecessary conflicts. > +++ b/docs/remote.html.in > @@ -139,7 +139,7 @@ Blank lines and comments beginning with <code>#</code> are ignored. > <td> > Listen for secure TLS connections on the public TCP/IP port. > Note: it is also necessary to start the server in listening mode by > - running it with --listen or editing /etc/sysconfig/libvirtd by uncommenting the LIBVIRTD_ARGS="--listen" line > + running it with --listen or editing /etc/sysconfig/libvirtd by adding a LIBVIRTD_ARGS="--listen" line This should mention the alternative method of configuring the service, which is adding a systemd unit override for the Environment=LIBVIRTD_ARGS=... variable. I wonder if users will get this right? The interactions between the various ways of configuring the arguments for each daemon have just gotten more complex, and I can definitely foresee subtle configuration mistakes happening because of it. > +++ b/libvirt.spec.in > @@ -197,6 +197,18 @@ > +%define libvirt_sc_pre() \ > + for sc in %{?*} ; do \ > + test -f "%{_sysconfdir}/sysconfig/${sc}.rpmsave" || continue ;\ > + mv -v "%{_sysconfdir}/sysconfig/${sc}.rpmsave" "%{_sysconfdir}/sysconfig/${sc}.rpmsave.old" ;\ > + done \ > + %{nil} > +%define libvirt_sc_posttrans() \ > + for sc in %{?*} ; do \ > + test -f "%{_sysconfdir}/sysconfig/${sc}.rpmsave" || continue ;\ > + mv -v "%{_sysconfdir}/sysconfig/${sc}.rpmsave" "%{_sysconfdir}/sysconfig/${sc}" ;\ > + done \ > + %{nil} Please confirm whether I understand these correctly. The idea is that we want existing sysconfig files to be preserved when the package is updated, but rpm by default will rename them to .rpmsave once it realizes the corresponding files are gone from the package. So in %pre you make sure existing .rpmsave files are moved out of the way, and then in %posttrans you move the current sysconfig files, which had been renamed by rpm, them back to their original location. This will all turn into a no-op when you're upgrading from a package where the sysconf files have already been gone to a newer version, right? Any scenario in which the rpm run is interrupted, for example between %pre and %posttrans, and the state becomes inconsistent? > -%post daemon > -%global post_units \\\ > - virtlockd.socket virtlockd-admin.socket \\\ > - virtlogd.socket virtlogd-admin.socket \\\ > - libvirtd.socket libvirtd-ro.socket libvirtd-admin.socket \\\ > - libvirtd-tcp.socket libvirtd-tls.socket \\\ > - libvirtd.service \\\ > - libvirt-guests.service > - > -%systemd_post %post_units > - > -# request daemon restart in posttrans > -mkdir -p %{_localstatedir}/lib/rpm-state/libvirt || : > -touch %{_localstatedir}/lib/rpm-state/libvirt/restart || : > - > > %posttrans daemon > +%libvirt_sc_posttrans libvirtd virtproxyd virtlogd virtlockd libvirt-guests > +%global post_units \\\ > + virtlockd.socket virtlockd-admin.socket \\\ > + virtlogd.socket virtlogd-admin.socket \\\ > + libvirtd.socket libvirtd-ro.socket libvirtd-admin.socket \\\ > + libvirtd-tcp.socket libvirtd-tls.socket \\\ > + libvirtd.service \\\ > + libvirt-guests.service > + > +%systemd_post %post_units > + > +# request daemon restart in posttrans > +mkdir -p %{_localstatedir}/lib/rpm-state/libvirt || : > +touch %{_localstatedir}/lib/rpm-state/libvirt/restart || : Moving this stuff around changes its semantics. I'm not familiar enough with rpm packaging to understand exactly the consequences, but I think you should be extremely careful with this kind of change and definitely not perform it at the same time as you're adding new functionality. > if [ -f %{_localstatedir}/lib/rpm-state/libvirt/restart ]; then > # See if user has previously modified their install to > # tell libvirtd to use --listen > - grep -E '^LIBVIRTD_ARGS=.*--listen' /etc/sysconfig/libvirtd 1>/dev/null 2>&1 > + if test -f /etc/sysconfig/libvirtd > + then > + grep -E '^LIBVIRTD_ARGS=.*--listen' /etc/sysconfig/libvirtd 1>/dev/null 2>&1 > + fi I don't think you need to make this conditional: if the file doesn't exist, grep will exit with a non-zero code, same as if the file existed but no match was found in it. Pre-existing: am I missing something, or is the daemon actually *not* being restarted when --listen is found? We mask a bunch of units and that's pretty much it. Also pre-existing: do we even care about handling upgrades from versions of the daemon that didn't have support for systemd socket passing at this point? The .spec file explicitly limits support to RHEL 8 and Fedora 33, which should be plenty recent enough to make the entire dance unnecessary. > +%pre daemon-driver-interface > +%libvirt_sc_posttrans virtinterfaced Wrong function called in %pre (there are a few more instances of this mistake in the patch). > %if %{with_qemu} > +%pre daemon-driver-qemu > +%libvirt_sc_pre virtqemud > +# We want soft static allocation of well-known ids, as disk images > +# are commonly shared across NFS mounts by id rather than name; see > +# https://fedoraproject.org/wiki/Packaging:UsersAndGroups > +getent group kvm >/dev/null || groupadd -f -g 36 -r kvm > +getent group qemu >/dev/null || groupadd -f -g 107 -r qemu > +if ! getent passwd qemu >/dev/null; then > + if ! getent passwd 107 >/dev/null; then > + useradd -r -u 107 -g qemu -G kvm -d / -s /sbin/nologin -c "qemu user" qemu > + else > + useradd -r -g qemu -G kvm -d / -s /sbin/nologin -c "qemu user" qemu > + fi > +fi > +exit 0 Moving this section is probably a good idea, but it shouldn't happen at the same time as you're changing things. Make all pure code movements a separate preparatory commit, please. > --- a/src/locking/virtlockd.sysconf > +++ /dev/null > @@ -1,3 +0,0 @@ > -# Customizations for the virtlockd.service systemd unit > - > -VIRTLOCKD_ARGS="" You're dropping the sysconfig file without adding the corresponding Environment= directive in the .service file. Even though the default is to pass no arguments to this daemon (and virtlogd), we should still include that line for discoverability and consistency purposes. > +++ b/src/remote/libvirtd.service.in > @@ -28,6 +28,13 @@ Documentation=https://libvirt.org > > [Service] > Type=notify > +# Override the QEMU/SDL default audio driver probing when > +# starting virtual machines using SDL graphics > +# NB these have no effect for VMs using VNC, unless vnc_allow_host_audio > +# is enabled in /etc/libvirt/qemu.conf > +#Environment=QEMU_AUDIO_DRV=sdl > +#Environment=SDL_AUDIODRIVER=pulse > +Environment=LIBVIRTD_ARGS="--timeout 120" You're losing the documentation for the --timeout option, as well as both the documentation and the example usage for the --listen option. > +++ b/tools/libvirt-guests.sh.in > @@ -30,13 +30,53 @@ test ! -r "$sysconfdir"/rc.d/init.d/functions || > +# URIs to check for running guests > +# example: URIS='default xen:///system vbox+tcp://host/system lxc:///system' > URIS="default" [...] > +# If non-zero, try to sync guest time on domain resume. Be aware, that > +# this requires guest agent with support for time synchronization > +# running in the guest. By default, this functionality is turned off. > SYNC_TIME=0 Why did you move these here instead of adding them to the .service file? We certainly don't want users to edit the script directly in order to configure its behavior, or having to look at the source code to understand what the various settings do. > test -f "$sysconfdir"/sysconfig/libvirt-guests && Pre-existing: we source the sysconfig file in the .service file through the EnvironmentFile= directive, then set a bunch of defaults at the top of the script, then source the sysconfig file again. That seems sketchy, but honestly pretty much all of libvirt-guests feels fragile and poorly thought out :( All the comments I've made up until here are about the purely technical side of the changes. Overall, I'm still not entirely sold on the idea of this actually being an improvement over the status quo. In particular, I worry about changes in defaults being more difficult for users to detect: in Debian at least, changes to the default sysconfig files result in the admin being given the possibility to review and tweak their local customizations at package upgrade time, and by moving the defaults off to the .service files we're losing that convenience. I understand other distros don't have the same tooling around configuration files, but still it feels like a step backwards in this regard. -- Andrea Bolognani / Red Hat / Virtualization