1. Patchew
  2. Libvirt
  3. View series

[PATCH v1] remove sysconfig files

Olaf Hering posted 1 patch 2 years, 9 months ago
Test syntax-check failed
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/libvirt tags/patchew/20210720170020.3048-1-olaf@aepfle.de
There is a newer version of this series
NEWS.rst                                |  10 ++
docs/remote.html.in                     |   2 +-
libvirt.spec.in                         | 134 +++++++++++++++---------
src/ch/meson.build                      |   5 -
src/ch/virtchd.service.in               |   1 +
src/ch/virtchd.sysconf                  |   3 -
src/interface/meson.build               |   5 -
src/interface/virtinterfaced.service.in |   1 +
src/interface/virtinterfaced.sysconf    |   3 -
src/libxl/meson.build                   |   5 -
src/libxl/virtxend.service.in           |   1 +
src/libxl/virtxend.sysconf              |   3 -
src/locking/meson.build                 |   5 -
src/locking/virtlockd.sysconf           |   3 -
src/logging/meson.build                 |   5 -
src/logging/virtlogd.sysconf            |   3 -
src/lxc/meson.build                     |   5 -
src/lxc/virtlxcd.service.in             |   1 +
src/lxc/virtlxcd.sysconf                |   3 -
src/meson.build                         |  16 ---
src/network/meson.build                 |   5 -
src/network/virtnetworkd.service.in     |   1 +
src/network/virtnetworkd.sysconf        |   3 -
src/node_device/meson.build             |   5 -
src/node_device/virtnodedevd.service.in |   1 +
src/node_device/virtnodedevd.sysconf    |   3 -
src/nwfilter/meson.build                |   5 -
src/nwfilter/virtnwfilterd.service.in   |   1 +
src/nwfilter/virtnwfilterd.sysconf      |   3 -
src/qemu/meson.build                    |   5 -
src/qemu/virtqemud.service.in           |   7 ++
src/qemu/virtqemud.sysconf              |  12 ---
src/remote/libvirtd.service.in          |   7 ++
src/remote/libvirtd.sysconf             |  21 ----
src/remote/meson.build                  |  10 --
src/remote/virtproxyd.service.in        |   1 +
src/remote/virtproxyd.sysconf           |   3 -
src/secret/meson.build                  |   5 -
src/secret/virtsecretd.service.in       |   1 +
src/secret/virtsecretd.sysconf          |   3 -
src/storage/meson.build                 |   5 -
src/storage/virtstoraged.service.in     |   1 +
src/storage/virtstoraged.sysconf        |   3 -
src/vbox/meson.build                    |   5 -
src/vbox/virtvboxd.service.in           |   1 +
src/vbox/virtvboxd.sysconf              |   3 -
src/vz/meson.build                      |   5 -
src/vz/virtvzd.service.in               |   1 +
src/vz/virtvzd.sysconf                  |   3 -
tools/libvirt-guests.sh.in              |  40 +++++++
tools/libvirt-guests.sysconf            |  50 ---------
tools/meson.build                       |   6 --
52 files changed, 163 insertions(+), 276 deletions(-)
delete mode 100644 src/ch/virtchd.sysconf
delete mode 100644 src/interface/virtinterfaced.sysconf
delete mode 100644 src/libxl/virtxend.sysconf
delete mode 100644 src/locking/virtlockd.sysconf
delete mode 100644 src/logging/virtlogd.sysconf
delete mode 100644 src/lxc/virtlxcd.sysconf
delete mode 100644 src/network/virtnetworkd.sysconf
delete mode 100644 src/node_device/virtnodedevd.sysconf
delete mode 100644 src/nwfilter/virtnwfilterd.sysconf
delete mode 100644 src/qemu/virtqemud.sysconf
delete mode 100644 src/remote/libvirtd.sysconf
delete mode 100644 src/remote/virtproxyd.sysconf
delete mode 100644 src/secret/virtsecretd.sysconf
delete mode 100644 src/storage/virtstoraged.sysconf
delete mode 100644 src/vbox/virtvboxd.sysconf
delete mode 100644 src/vz/virtvzd.sysconf
delete mode 100644 tools/libvirt-guests.sysconf
[PATCH v1] remove sysconfig files
Posted by Olaf Hering 2 years, 9 months ago 
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.

Remove the sysconfig file and place the current desired default into
the service file.

Local customizations can now go either into /etc/sysconfig/name
or /etc/systemd/system/name.service.d/my-knobs.conf

Attempt to handle upgrades in libvirt.spec.
Dirty files which are marked as %config will be renamed to file.rpmsave.
To restore them automatically, move stale .rpmsave files away, and
catch any new rpmsave files in %posttrans.

Signed-off-by: Olaf Hering <olaf@aepfle.de>
---
 NEWS.rst                                |  10 ++
 docs/remote.html.in                     |   2 +-
 libvirt.spec.in                         | 134 +++++++++++++++---------
 src/ch/meson.build                      |   5 -
 src/ch/virtchd.service.in               |   1 +
 src/ch/virtchd.sysconf                  |   3 -
 src/interface/meson.build               |   5 -
 src/interface/virtinterfaced.service.in |   1 +
 src/interface/virtinterfaced.sysconf    |   3 -
 src/libxl/meson.build                   |   5 -
 src/libxl/virtxend.service.in           |   1 +
 src/libxl/virtxend.sysconf              |   3 -
 src/locking/meson.build                 |   5 -
 src/locking/virtlockd.sysconf           |   3 -
 src/logging/meson.build                 |   5 -
 src/logging/virtlogd.sysconf            |   3 -
 src/lxc/meson.build                     |   5 -
 src/lxc/virtlxcd.service.in             |   1 +
 src/lxc/virtlxcd.sysconf                |   3 -
 src/meson.build                         |  16 ---
 src/network/meson.build                 |   5 -
 src/network/virtnetworkd.service.in     |   1 +
 src/network/virtnetworkd.sysconf        |   3 -
 src/node_device/meson.build             |   5 -
 src/node_device/virtnodedevd.service.in |   1 +
 src/node_device/virtnodedevd.sysconf    |   3 -
 src/nwfilter/meson.build                |   5 -
 src/nwfilter/virtnwfilterd.service.in   |   1 +
 src/nwfilter/virtnwfilterd.sysconf      |   3 -
 src/qemu/meson.build                    |   5 -
 src/qemu/virtqemud.service.in           |   7 ++
 src/qemu/virtqemud.sysconf              |  12 ---
 src/remote/libvirtd.service.in          |   7 ++
 src/remote/libvirtd.sysconf             |  21 ----
 src/remote/meson.build                  |  10 --
 src/remote/virtproxyd.service.in        |   1 +
 src/remote/virtproxyd.sysconf           |   3 -
 src/secret/meson.build                  |   5 -
 src/secret/virtsecretd.service.in       |   1 +
 src/secret/virtsecretd.sysconf          |   3 -
 src/storage/meson.build                 |   5 -
 src/storage/virtstoraged.service.in     |   1 +
 src/storage/virtstoraged.sysconf        |   3 -
 src/vbox/meson.build                    |   5 -
 src/vbox/virtvboxd.service.in           |   1 +
 src/vbox/virtvboxd.sysconf              |   3 -
 src/vz/meson.build                      |   5 -
 src/vz/virtvzd.service.in               |   1 +
 src/vz/virtvzd.sysconf                  |   3 -
 tools/libvirt-guests.sh.in              |  40 +++++++
 tools/libvirt-guests.sysconf            |  50 ---------
 tools/meson.build                       |   6 --
 52 files changed, 163 insertions(+), 276 deletions(-)
 delete mode 100644 src/ch/virtchd.sysconf
 delete mode 100644 src/interface/virtinterfaced.sysconf
 delete mode 100644 src/libxl/virtxend.sysconf
 delete mode 100644 src/locking/virtlockd.sysconf
 delete mode 100644 src/logging/virtlogd.sysconf
 delete mode 100644 src/lxc/virtlxcd.sysconf
 delete mode 100644 src/network/virtnetworkd.sysconf
 delete mode 100644 src/node_device/virtnodedevd.sysconf
 delete mode 100644 src/nwfilter/virtnwfilterd.sysconf
 delete mode 100644 src/qemu/virtqemud.sysconf
 delete mode 100644 src/remote/libvirtd.sysconf
 delete mode 100644 src/remote/virtproxyd.sysconf
 delete mode 100644 src/secret/virtsecretd.sysconf
 delete mode 100644 src/storage/virtstoraged.sysconf
 delete mode 100644 src/vbox/virtvboxd.sysconf
 delete mode 100644 src/vz/virtvzd.sysconf
 delete mode 100644 tools/libvirt-guests.sysconf

diff --git a/NEWS.rst b/NEWS.rst
index d95750e776..3e5b790e03 100644
--- a/NEWS.rst
+++ 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.
+
 * **Bug fixes**
 
   * qemu: Fix migration with VIR_MIGRATE_NON_SHARED_INC
diff --git a/docs/remote.html.in b/docs/remote.html.in
index cc8db80c95..1e31af0e60 100644
--- a/docs/remote.html.in
+++ 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
   to cause the server to come up in listening mode whenever it is started.
 </td>
       </tr>
diff --git a/libvirt.spec.in b/libvirt.spec.in
index cb48dd0be0..0d44629d08 100644
--- a/libvirt.spec.in
+++ b/libvirt.spec.in
@@ -197,6 +197,18 @@
 
 %define tls_priority "@LIBVIRT,SYSTEM"
 
+%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}
 
 Summary: Library providing a simple virtualization API
 Name: libvirt
@@ -1249,6 +1261,7 @@ mv $RPM_BUILD_ROOT%{_datadir}/systemtap/tapset/libvirt_qemu_probes.stp \
 VIR_TEST_DEBUG=1 %meson_test --no-suite syntax-check --timeout-multiplier 10
 
 %pre daemon
+%libvirt_sc_pre libvirtd virtproxyd virtlogd virtlockd libvirt-guests
 # 'libvirt' group is just to allow password-less polkit access to
 # libvirtd. The uid number is irrelevant, so we use dynamic allocation
 # described at the above link.
@@ -1256,21 +1269,6 @@ getent group libvirt >/dev/null || groupadd -r libvirt
 
 exit 0
 
-%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 || :
-
 %preun daemon
 %global preun_units \\\
         libvirtd.service \\\
@@ -1302,10 +1300,28 @@ if [ $1 -ge 1 ] ; then
 fi
 
 %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 || :
+
 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
     if test $? = 0
     then
         # Then lets keep honouring --listen and *not* use
@@ -1417,23 +1433,6 @@ fi
 rm -rf %{_localstatedir}/lib/rpm-state/libvirt || :
 
 
-%if %{with_qemu}
-%pre daemon-driver-qemu
-# 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
-%endif
-
 %if %{with_lxc}
 %pre login-shell
 getent group virtlogin >/dev/null || groupadd -r virtlogin
@@ -1470,16 +1469,11 @@ exit 0
 %{_unitdir}/virtlockd.socket
 %{_unitdir}/virtlockd-admin.socket
 %{_unitdir}/libvirt-guests.service
-%config(noreplace) %{_sysconfdir}/sysconfig/libvirtd
-%config(noreplace) %{_sysconfdir}/sysconfig/virtproxyd
-%config(noreplace) %{_sysconfdir}/sysconfig/virtlogd
-%config(noreplace) %{_sysconfdir}/sysconfig/virtlockd
 %config(noreplace) %{_sysconfdir}/libvirt/libvirtd.conf
 %config(noreplace) %{_sysconfdir}/libvirt/virtproxyd.conf
 %config(noreplace) %{_sysconfdir}/libvirt/virtlogd.conf
 %config(noreplace) %{_sysconfdir}/libvirt/virtlockd.conf
 %config(noreplace) %{_sysconfdir}/sasl2/libvirt.conf
-%config(noreplace) %{_sysconfdir}/sysconfig/libvirt-guests
 %config(noreplace) %{_prefix}/lib/sysctl.d/60-libvirtd.conf
 
 %config(noreplace) %{_sysconfdir}/logrotate.d/libvirtd
@@ -1550,8 +1544,11 @@ exit 0
 %{_datadir}/libvirt/nwfilter/*.xml
 %ghost %{_sysconfdir}/libvirt/nwfilter/*.xml
 
+%pre daemon-driver-interface
+%libvirt_sc_posttrans virtinterfaced
+%posttrans daemon-driver-interface
+%libvirt_sc_posttrans virtinterfaced
 %files daemon-driver-interface
-%config(noreplace) %{_sysconfdir}/sysconfig/virtinterfaced
 %config(noreplace) %{_sysconfdir}/libvirt/virtinterfaced.conf
 %{_datadir}/augeas/lenses/virtinterfaced.aug
 %{_datadir}/augeas/lenses/tests/test_virtinterfaced.aug
@@ -1563,8 +1560,11 @@ exit 0
 %{_libdir}/%{name}/connection-driver/libvirt_driver_interface.so
 %{_mandir}/man8/virtinterfaced.8*
 
+%pre daemon-driver-network
+%libvirt_sc_posttrans virtnetworkd
+%posttrans daemon-driver-network
+%libvirt_sc_posttrans virtnetworkd
 %files daemon-driver-network
-%config(noreplace) %{_sysconfdir}/sysconfig/virtnetworkd
 %config(noreplace) %{_sysconfdir}/libvirt/virtnetworkd.conf
 %{_datadir}/augeas/lenses/virtnetworkd.aug
 %{_datadir}/augeas/lenses/tests/test_virtnetworkd.aug
@@ -1587,8 +1587,11 @@ exit 0
 %{_prefix}/lib/firewalld/zones/libvirt.xml
 %endif
 
+%pre daemon-driver-nodedev
+%libvirt_sc_posttrans virtnodedevd
+%posttrans daemon-driver-nodedev
+%libvirt_sc_posttrans virtnodedevd
 %files daemon-driver-nodedev
-%config(noreplace) %{_sysconfdir}/sysconfig/virtnodedevd
 %config(noreplace) %{_sysconfdir}/libvirt/virtnodedevd.conf
 %{_datadir}/augeas/lenses/virtnodedevd.aug
 %{_datadir}/augeas/lenses/tests/test_virtnodedevd.aug
@@ -1600,8 +1603,11 @@ exit 0
 %{_libdir}/%{name}/connection-driver/libvirt_driver_nodedev.so
 %{_mandir}/man8/virtnodedevd.8*
 
+%pre daemon-driver-nwfilter
+%libvirt_sc_posttrans virtnwfilterd
+%posttrans daemon-driver-nwfilter
+%libvirt_sc_posttrans virtnwfilterd
 %files daemon-driver-nwfilter
-%config(noreplace) %{_sysconfdir}/sysconfig/virtnwfilterd
 %config(noreplace) %{_sysconfdir}/libvirt/virtnwfilterd.conf
 %{_datadir}/augeas/lenses/virtnwfilterd.aug
 %{_datadir}/augeas/lenses/tests/test_virtnwfilterd.aug
@@ -1615,8 +1621,11 @@ exit 0
 %{_libdir}/%{name}/connection-driver/libvirt_driver_nwfilter.so
 %{_mandir}/man8/virtnwfilterd.8*
 
+%pre daemon-driver-secret
+%libvirt_sc_posttrans virtsecretd
+%posttrans daemon-driver-secret
+%libvirt_sc_posttrans virtsecretd
 %files daemon-driver-secret
-%config(noreplace) %{_sysconfdir}/sysconfig/virtsecretd
 %config(noreplace) %{_sysconfdir}/libvirt/virtsecretd.conf
 %{_datadir}/augeas/lenses/virtsecretd.aug
 %{_datadir}/augeas/lenses/tests/test_virtsecretd.aug
@@ -1630,8 +1639,11 @@ exit 0
 
 %files daemon-driver-storage
 
+%pre daemon-driver-storage-core
+%libvirt_sc_posttrans virtstoraged
+%posttrans daemon-driver-storage-core
+%libvirt_sc_posttrans virtstoraged
 %files daemon-driver-storage-core
-%config(noreplace) %{_sysconfdir}/sysconfig/virtstoraged
 %config(noreplace) %{_sysconfdir}/libvirt/virtstoraged.conf
 %{_datadir}/augeas/lenses/virtstoraged.aug
 %{_datadir}/augeas/lenses/tests/test_virtstoraged.aug
@@ -1688,8 +1700,25 @@ exit 0
 %endif
 
 %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
+
+%posttrans daemon-driver-qemu
+%libvirt_sc_posttrans virtqemud
 %files daemon-driver-qemu
-%config(noreplace) %{_sysconfdir}/sysconfig/virtqemud
 %config(noreplace) %{_sysconfdir}/libvirt/virtqemud.conf
 %{_datadir}/augeas/lenses/virtqemud.aug
 %{_datadir}/augeas/lenses/tests/test_virtqemud.aug
@@ -1717,8 +1746,11 @@ exit 0
 %endif
 
 %if %{with_lxc}
+%pre daemon-driver-lxc
+%libvirt_sc_pre virtlxcd
+%posttrans daemon-driver-lxc
+%libvirt_sc_posttrans virtlxcd
 %files daemon-driver-lxc
-%config(noreplace) %{_sysconfdir}/sysconfig/virtlxcd
 %config(noreplace) %{_sysconfdir}/libvirt/virtlxcd.conf
 %{_datadir}/augeas/lenses/virtlxcd.aug
 %{_datadir}/augeas/lenses/tests/test_virtlxcd.aug
@@ -1740,8 +1772,11 @@ exit 0
 %endif
 
 %if %{with_libxl}
+%pre daemon-driver-libxl
+%libvirt_sc_pre virtxend
+%posttrans daemon-driver-libxl
+%libvirt_sc_posttrans virtxend
 %files daemon-driver-libxl
-%config(noreplace) %{_sysconfdir}/sysconfig/virtxend
 %config(noreplace) %{_sysconfdir}/libvirt/virtxend.conf
 %{_datadir}/augeas/lenses/virtxend.aug
 %{_datadir}/augeas/lenses/tests/test_virtxend.aug
@@ -1763,8 +1798,11 @@ exit 0
 %endif
 
 %if %{with_vbox}
+%pre daemon-driver-vbox
+%libvirt_sc_pre virtvboxd
+%posttrans daemon-driver-vbox
+%libvirt_sc_posttrans virtvboxd
 %files daemon-driver-vbox
-%config(noreplace) %{_sysconfdir}/sysconfig/virtvboxd
 %config(noreplace) %{_sysconfdir}/libvirt/virtvboxd.conf
 %{_datadir}/augeas/lenses/virtvboxd.aug
 %{_datadir}/augeas/lenses/tests/test_virtvboxd.aug
diff --git a/src/ch/meson.build b/src/ch/meson.build
index e34974d56c..22d4366a21 100644
--- a/src/ch/meson.build
+++ b/src/ch/meson.build
@@ -62,11 +62,6 @@ if conf.has('WITH_CH')
     'sockets': [ 'main', 'ro', 'admin' ],
   }
 
-  sysconf_files += {
-    'name': 'virtchd',
-    'file': files('virtchd.sysconf'),
-  }
-
   virt_install_dirs += [
     localstatedir / 'lib' / 'libvirt' / 'ch',
     runstatedir / 'libvirt' / 'ch',
diff --git a/src/ch/virtchd.service.in b/src/ch/virtchd.service.in
index cc1e85d1df..f08339f211 100644
--- a/src/ch/virtchd.service.in
+++ b/src/ch/virtchd.service.in
@@ -18,6 +18,7 @@ Documentation=https://libvirt.org
 
 [Service]
 Type=notify
+Environment=VIRTCHD_ARGS="--timeout 120"
 EnvironmentFile=-@sysconfdir@/sysconfig/virtchd
 ExecStart=@sbindir@/virtchd $VIRTCHD_ARGS
 ExecReload=/bin/kill -HUP $MAINPID
diff --git a/src/ch/virtchd.sysconf b/src/ch/virtchd.sysconf
deleted file mode 100644
index 5ee44be5cf..0000000000
--- a/src/ch/virtchd.sysconf
+++ /dev/null
@@ -1,3 +0,0 @@
-# Customizations for the virtchd.service systemd unit
-
-VIRTCHD_ARGS="--timeout 120"
diff --git a/src/interface/meson.build b/src/interface/meson.build
index 46076921ba..360f2ed8a3 100644
--- a/src/interface/meson.build
+++ b/src/interface/meson.build
@@ -55,9 +55,4 @@ if conf.has('WITH_INTERFACE')
     'name': 'virtinterfaced',
     'in_file': files('virtinterfaced.init.in')
   }
-
-  sysconf_files += {
-    'name': 'virtinterfaced',
-    'file': files('virtinterfaced.sysconf'),
-  }
 endif
diff --git a/src/interface/virtinterfaced.service.in b/src/interface/virtinterfaced.service.in
index 73d409b81b..3d944e17a9 100644
--- a/src/interface/virtinterfaced.service.in
+++ b/src/interface/virtinterfaced.service.in
@@ -13,6 +13,7 @@ Documentation=https://libvirt.org
 
 [Service]
 Type=notify
+Environment=VIRTINTERFACED_ARGS="--timeout 120"
 EnvironmentFile=-@sysconfdir@/sysconfig/virtinterfaced
 ExecStart=@sbindir@/virtinterfaced $VIRTINTERFACED_ARGS
 ExecReload=/bin/kill -HUP $MAINPID
diff --git a/src/interface/virtinterfaced.sysconf b/src/interface/virtinterfaced.sysconf
deleted file mode 100644
index 0685da31b8..0000000000
--- a/src/interface/virtinterfaced.sysconf
+++ /dev/null
@@ -1,3 +0,0 @@
-# Customizations for the virtinterfaced.service systemd unit
-
-VIRTINTERFACED_ARGS="--timeout 120"
diff --git a/src/libxl/meson.build b/src/libxl/meson.build
index 9793899106..8347a3c966 100644
--- a/src/libxl/meson.build
+++ b/src/libxl/meson.build
@@ -78,11 +78,6 @@ if conf.has('WITH_LIBXL')
     'in_file': files('virtxend.init.in'),
   }
 
-  sysconf_files += {
-    'name': 'virtxend',
-    'file': files('virtxend.sysconf'),
-  }
-
   virt_install_dirs += [
     localstatedir / 'lib' / 'libvirt' / 'libxl',
     runstatedir / 'libvirt' / 'libxl',
diff --git a/src/libxl/virtxend.service.in b/src/libxl/virtxend.service.in
index a863917467..7175feff1c 100644
--- a/src/libxl/virtxend.service.in
+++ b/src/libxl/virtxend.service.in
@@ -17,6 +17,7 @@ ConditionPathExists=/proc/xen/capabilities
 
 [Service]
 Type=notify
+Environment=VIRTXEND_ARGS="--timeout 120"
 EnvironmentFile=-@sysconfdir@/sysconfig/virtxend
 ExecStart=@sbindir@/virtxend $VIRTXEND_ARGS
 ExecReload=/bin/kill -HUP $MAINPID
diff --git a/src/libxl/virtxend.sysconf b/src/libxl/virtxend.sysconf
deleted file mode 100644
index 301da47e8d..0000000000
--- a/src/libxl/virtxend.sysconf
+++ /dev/null
@@ -1,3 +0,0 @@
-# Customizations for the virtxend.service systemd unit
-
-VIRTXEND_ARGS="--timeout 120"
diff --git a/src/locking/meson.build b/src/locking/meson.build
index 184d3c3f56..72f7780438 100644
--- a/src/locking/meson.build
+++ b/src/locking/meson.build
@@ -156,11 +156,6 @@ if conf.has('WITH_LIBVIRTD')
     'in_file': files('virtlockd.init.in'),
   }
 
-  sysconf_files += {
-    'name': 'virtlockd',
-    'file': files('virtlockd.sysconf'),
-  }
-
   if conf.has('WITH_SANLOCK')
     virt_helpers += {
       'name': 'libvirt_sanlock_helper',
diff --git a/src/locking/virtlockd.sysconf b/src/locking/virtlockd.sysconf
deleted file mode 100644
index 03aea9e1bc..0000000000
--- a/src/locking/virtlockd.sysconf
+++ /dev/null
@@ -1,3 +0,0 @@
-# Customizations for the virtlockd.service systemd unit
-
-VIRTLOCKD_ARGS=""
diff --git a/src/logging/meson.build b/src/logging/meson.build
index 996d4265fc..f06be6050d 100644
--- a/src/logging/meson.build
+++ b/src/logging/meson.build
@@ -96,11 +96,6 @@ if conf.has('WITH_LIBVIRTD')
     'name': 'virtlogd',
     'in_file': files('virtlogd.init.in'),
   }
-
-  sysconf_files += {
-    'name': 'virtlogd',
-    'file': files('virtlogd.sysconf'),
-  }
 endif
 
 log_inc_dir = include_directories('.')
diff --git a/src/logging/virtlogd.sysconf b/src/logging/virtlogd.sysconf
deleted file mode 100644
index 67993e83ce..0000000000
--- a/src/logging/virtlogd.sysconf
+++ /dev/null
@@ -1,3 +0,0 @@
-# Customizations for the virtlogd.service systemd unit
-
-VIRTLOGD_ARGS=""
diff --git a/src/lxc/meson.build b/src/lxc/meson.build
index ad5c659dba..c1f71b43e1 100644
--- a/src/lxc/meson.build
+++ b/src/lxc/meson.build
@@ -175,11 +175,6 @@ if conf.has('WITH_LXC')
     'in_file': files('virtlxcd.init.in'),
   }
 
-  sysconf_files += {
-    'name': 'virtlxcd',
-    'file': files('virtlxcd.sysconf'),
-  }
-
   virt_install_dirs += [
     localstatedir / 'lib' / 'libvirt' / 'lxc',
     runstatedir / 'libvirt' / 'lxc',
diff --git a/src/lxc/virtlxcd.service.in b/src/lxc/virtlxcd.service.in
index 3af7c1a52d..d58bde9f5d 100644
--- a/src/lxc/virtlxcd.service.in
+++ b/src/lxc/virtlxcd.service.in
@@ -18,6 +18,7 @@ Documentation=https://libvirt.org
 
 [Service]
 Type=notify
+Environment=VIRTLXCD_ARGS="--timeout 120"
 EnvironmentFile=-@sysconfdir@/sysconfig/virtlxcd
 ExecStart=@sbindir@/virtlxcd $VIRTLXCD_ARGS
 ExecReload=/bin/kill -HUP $MAINPID
diff --git a/src/lxc/virtlxcd.sysconf b/src/lxc/virtlxcd.sysconf
deleted file mode 100644
index 119a4a23f3..0000000000
--- a/src/lxc/virtlxcd.sysconf
+++ /dev/null
@@ -1,3 +0,0 @@
-# Customizations for the virtlxcd.service systemd unit
-
-VIRTLXCD_ARGS="--timeout 120"
diff --git a/src/meson.build b/src/meson.build
index 2bd88e6699..9ee8b987ae 100644
--- a/src/meson.build
+++ b/src/meson.build
@@ -208,12 +208,6 @@ virt_daemon_units = []
 #   * in_file - source init file (required)
 openrc_init_files = []
 
-# sysconf_files
-#   install libvirt daemon sysconf files
-#   * name - daemon name (required)
-#   * file - source sysconf file (required)
-sysconf_files = []
-
 # virt_install_dirs:
 #   list of directories to create during installation
 virt_install_dirs = []
@@ -868,16 +862,6 @@ if conf.has('WITH_LIBVIRTD')
   endif
 endif
 
-if init_script != 'none'
-  foreach sysconf : sysconf_files
-    install_data(
-      sysconf['file'],
-      install_dir: sysconfdir / 'sysconfig',
-      rename: [ sysconf['name'] ],
-    )
-  endforeach
-endif
-
 if conf.has('WITH_DTRACE_PROBES')
   custom_target(
     'libvirt_functions.stp',
diff --git a/src/network/meson.build b/src/network/meson.build
index d6fb624bb7..e7c43bc4c4 100644
--- a/src/network/meson.build
+++ b/src/network/meson.build
@@ -72,11 +72,6 @@ if conf.has('WITH_NETWORK')
     'in_file': files('virtnetworkd.init.in'),
   }
 
-  sysconf_files += {
-    'name': 'virtnetworkd',
-    'file': files('virtnetworkd.sysconf'),
-  }
-
   virt_install_dirs += [
     localstatedir / 'lib' / 'libvirt' / 'network',
     localstatedir / 'lib' / 'libvirt' / 'dnsmasq',
diff --git a/src/network/virtnetworkd.service.in b/src/network/virtnetworkd.service.in
index 4c39d2a5d7..3decfbbf1d 100644
--- a/src/network/virtnetworkd.service.in
+++ b/src/network/virtnetworkd.service.in
@@ -16,6 +16,7 @@ Documentation=https://libvirt.org
 
 [Service]
 Type=notify
+Environment=VIRTNETWORKD_ARGS="--timeout 120"
 EnvironmentFile=-@sysconfdir@/sysconfig/virtnetworkd
 ExecStart=@sbindir@/virtnetworkd $VIRTNETWORKD_ARGS
 ExecReload=/bin/kill -HUP $MAINPID
diff --git a/src/network/virtnetworkd.sysconf b/src/network/virtnetworkd.sysconf
deleted file mode 100644
index 93f3a7a327..0000000000
--- a/src/network/virtnetworkd.sysconf
+++ /dev/null
@@ -1,3 +0,0 @@
-# Customizations for the virtnetworkd.service systemd unit
-
-VIRTNETWORKD_ARGS="--timeout 120"
diff --git a/src/node_device/meson.build b/src/node_device/meson.build
index 15f9c3ad29..5013d825b3 100644
--- a/src/node_device/meson.build
+++ b/src/node_device/meson.build
@@ -62,9 +62,4 @@ if conf.has('WITH_NODE_DEVICES')
     'name': 'virtnodedevd',
     'in_file': files('virtnodedevd.init.in'),
   }
-
-  sysconf_files += {
-    'name': 'virtnodedevd',
-    'file': files('virtnodedevd.sysconf'),
-  }
 endif
diff --git a/src/node_device/virtnodedevd.service.in b/src/node_device/virtnodedevd.service.in
index d2453dd620..688cf89822 100644
--- a/src/node_device/virtnodedevd.service.in
+++ b/src/node_device/virtnodedevd.service.in
@@ -13,6 +13,7 @@ Documentation=https://libvirt.org
 
 [Service]
 Type=notify
+Environment=VIRTNODEDEVD_ARGS="--timeout 120"
 EnvironmentFile=-@sysconfdir@/sysconfig/virtnodedevd
 ExecStart=@sbindir@/virtnodedevd $VIRTNODEDEVD_ARGS
 ExecReload=/bin/kill -HUP $MAINPID
diff --git a/src/node_device/virtnodedevd.sysconf b/src/node_device/virtnodedevd.sysconf
deleted file mode 100644
index fa7faa3a79..0000000000
--- a/src/node_device/virtnodedevd.sysconf
+++ /dev/null
@@ -1,3 +0,0 @@
-# Customizations for the virtnodedevd.service systemd unit
-
-VIRTNODEDEVD_ARGS="--timeout 120"
diff --git a/src/nwfilter/meson.build b/src/nwfilter/meson.build
index a21e575925..ebbe712906 100644
--- a/src/nwfilter/meson.build
+++ b/src/nwfilter/meson.build
@@ -61,10 +61,5 @@ if conf.has('WITH_NWFILTER')
     'in_file': files('virtnwfilterd.init.in'),
   }
 
-  sysconf_files += {
-    'name': 'virtnwfilterd',
-    'file': files('virtnwfilterd.sysconf'),
-  }
-
   subdir('xml')
 endif
diff --git a/src/nwfilter/virtnwfilterd.service.in b/src/nwfilter/virtnwfilterd.service.in
index dda7c01a3d..36d00b58f0 100644
--- a/src/nwfilter/virtnwfilterd.service.in
+++ b/src/nwfilter/virtnwfilterd.service.in
@@ -13,6 +13,7 @@ Documentation=https://libvirt.org
 
 [Service]
 Type=notify
+Environment=VIRTNWFILTERD_ARGS="--timeout 120"
 EnvironmentFile=-@sysconfdir@/sysconfig/virtnwfilterd
 ExecStart=@sbindir@/virtnwfilterd $VIRTNWFILTERD_ARGS
 ExecReload=/bin/kill -HUP $MAINPID
diff --git a/src/nwfilter/virtnwfilterd.sysconf b/src/nwfilter/virtnwfilterd.sysconf
deleted file mode 100644
index 80cc645ba5..0000000000
--- a/src/nwfilter/virtnwfilterd.sysconf
+++ /dev/null
@@ -1,3 +0,0 @@
-# Customizations for the virtnwfilterd.service systemd unit
-
-VIRTNWFILTERD_ARGS="--timeout 120"
diff --git a/src/qemu/meson.build b/src/qemu/meson.build
index 3898d23877..6b4dd58309 100644
--- a/src/qemu/meson.build
+++ b/src/qemu/meson.build
@@ -165,11 +165,6 @@ if conf.has('WITH_QEMU')
     'in_file': files('virtqemud.init.in'),
   }
 
-  sysconf_files += {
-    'name': 'virtqemud',
-    'file': files('virtqemud.sysconf'),
-  }
-
   virt_install_dirs += [
     localstatedir / 'lib' / 'libvirt' / 'qemu',
     runstatedir / 'libvirt' / 'qemu',
diff --git a/src/qemu/virtqemud.service.in b/src/qemu/virtqemud.service.in
index 8abc9d3a7f..de5e311b2f 100644
--- a/src/qemu/virtqemud.service.in
+++ b/src/qemu/virtqemud.service.in
@@ -18,6 +18,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=VIRTQEMUD_ARGS="--timeout 120"
 EnvironmentFile=-@sysconfdir@/sysconfig/virtqemud
 ExecStart=@sbindir@/virtqemud $VIRTQEMUD_ARGS
 ExecReload=/bin/kill -HUP $MAINPID
diff --git a/src/qemu/virtqemud.sysconf b/src/qemu/virtqemud.sysconf
deleted file mode 100644
index 87b626e3ed..0000000000
--- a/src/qemu/virtqemud.sysconf
+++ /dev/null
@@ -1,12 +0,0 @@
-# Customizations for the virtqemud.service systemd unit
-
-VIRTQEMUD_ARGS="--timeout 120"
-
-# 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
-#QEMU_AUDIO_DRV=sdl
-#
-#SDL_AUDIODRIVER=pulse
diff --git a/src/remote/libvirtd.service.in b/src/remote/libvirtd.service.in
index cc0d4e3693..4d6b0510ae 100644
--- a/src/remote/libvirtd.service.in
+++ 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"
 EnvironmentFile=-@sysconfdir@/sysconfig/libvirtd
 ExecStart=@sbindir@/libvirtd $LIBVIRTD_ARGS
 ExecReload=/bin/kill -HUP $MAINPID
diff --git a/src/remote/libvirtd.sysconf b/src/remote/libvirtd.sysconf
deleted file mode 100644
index 18aec1ba67..0000000000
--- a/src/remote/libvirtd.sysconf
+++ /dev/null
@@ -1,21 +0,0 @@
-# Customizations for the libvirtd.service systemd unit
-
-# Default behaviour is for libvirtd.service to start on boot
-# so that VM autostart can be performed. We then want it to
-# shutdown again if nothing was started and rely on systemd
-# socket activation to start it again when some client app
-# connects.
-LIBVIRTD_ARGS="--timeout 120"
-
-# If systemd socket activation is disabled, then the following
-# can be used to listen on TCP/TLS sockets
-#LIBVIRTD_ARGS="--listen"
-
-# 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
-#QEMU_AUDIO_DRV=sdl
-#
-#SDL_AUDIODRIVER=pulse
diff --git a/src/remote/meson.build b/src/remote/meson.build
index 0a188268b5..fc98d0e5be 100644
--- a/src/remote/meson.build
+++ b/src/remote/meson.build
@@ -204,11 +204,6 @@ if conf.has('WITH_REMOTE')
       'confd': files('libvirtd.confd'),
     }
 
-    sysconf_files += {
-      'name': 'libvirtd',
-      'file': files('libvirtd.sysconf'),
-    }
-
     virt_daemons += {
       'name': 'virtproxyd',
       'c_args': [
@@ -239,11 +234,6 @@ if conf.has('WITH_REMOTE')
       'confd': files('virtproxyd.confd'),
     }
 
-    sysconf_files += {
-      'name': 'virtproxyd',
-      'file': files('virtproxyd.sysconf'),
-    }
-
     virt_install_dirs += [
       localstatedir / 'log' / 'libvirt',
     ]
diff --git a/src/remote/virtproxyd.service.in b/src/remote/virtproxyd.service.in
index f43ce9ee6e..10e8cf7263 100644
--- a/src/remote/virtproxyd.service.in
+++ b/src/remote/virtproxyd.service.in
@@ -13,6 +13,7 @@ Documentation=https://libvirt.org
 
 [Service]
 Type=notify
+Environment=VIRTPROXYD_ARGS="--timeout 120"
 EnvironmentFile=-@sysconfdir@/sysconfig/virtproxyd
 ExecStart=@sbindir@/virtproxyd $VIRTPROXYD_ARGS
 ExecReload=/bin/kill -HUP $MAINPID
diff --git a/src/remote/virtproxyd.sysconf b/src/remote/virtproxyd.sysconf
deleted file mode 100644
index 0fc5c61096..0000000000
--- a/src/remote/virtproxyd.sysconf
+++ /dev/null
@@ -1,3 +0,0 @@
-# Customizations for the virtproxyd.service systemd unit
-
-VIRTPROXYD_ARGS="--timeout 120"
diff --git a/src/secret/meson.build b/src/secret/meson.build
index a487055cde..efc0ebb1e6 100644
--- a/src/secret/meson.build
+++ b/src/secret/meson.build
@@ -43,9 +43,4 @@ if conf.has('WITH_SECRETS')
     'name': 'virtsecretd',
     'in_file': files('virtsecretd.init.in'),
   }
-
-  sysconf_files += {
-    'name': 'virtsecretd',
-    'file': files('virtsecretd.sysconf'),
-  }
 endif
diff --git a/src/secret/virtsecretd.service.in b/src/secret/virtsecretd.service.in
index 8444142a3a..cbd63fe0b2 100644
--- a/src/secret/virtsecretd.service.in
+++ b/src/secret/virtsecretd.service.in
@@ -13,6 +13,7 @@ Documentation=https://libvirt.org
 
 [Service]
 Type=notify
+Environment=VIRTSECRETD_ARGS="--timeout 120"
 EnvironmentFile=-@sysconfdir@/sysconfig/virtsecretd
 ExecStart=@sbindir@/virtsecretd $VIRTSECRETD_ARGS
 ExecReload=/bin/kill -HUP $MAINPID
diff --git a/src/secret/virtsecretd.sysconf b/src/secret/virtsecretd.sysconf
deleted file mode 100644
index 2247d05964..0000000000
--- a/src/secret/virtsecretd.sysconf
+++ /dev/null
@@ -1,3 +0,0 @@
-# Customizations for the virtsecretd.service systemd unit
-
-VIRTSECRETD_ARGS="--timeout 120"
diff --git a/src/storage/meson.build b/src/storage/meson.build
index 915ae46f61..db687784ab 100644
--- a/src/storage/meson.build
+++ b/src/storage/meson.build
@@ -125,11 +125,6 @@ if conf.has('WITH_STORAGE')
     'name': 'virtstoraged',
     'in_file': files('virtstoraged.init.in'),
   }
-
-  sysconf_files += {
-    'name': 'virtstoraged',
-    'file': files('virtstoraged.sysconf'),
-  }
 endif
 
 if conf.has('WITH_STORAGE_DISK')
diff --git a/src/storage/virtstoraged.service.in b/src/storage/virtstoraged.service.in
index fc3e9a1b69..f72f8426fd 100644
--- a/src/storage/virtstoraged.service.in
+++ b/src/storage/virtstoraged.service.in
@@ -15,6 +15,7 @@ Documentation=https://libvirt.org
 
 [Service]
 Type=notify
+Environment=VIRTSTORAGED_ARGS="--timeout 120"
 EnvironmentFile=-@sysconfdir@/sysconfig/virtstoraged
 ExecStart=@sbindir@/virtstoraged $VIRTSTORAGED_ARGS
 ExecReload=/bin/kill -HUP $MAINPID
diff --git a/src/storage/virtstoraged.sysconf b/src/storage/virtstoraged.sysconf
deleted file mode 100644
index 122373eb7c..0000000000
--- a/src/storage/virtstoraged.sysconf
+++ /dev/null
@@ -1,3 +0,0 @@
-# Customizations for the virtstoraged.service systemd unit
-
-VIRTSTORAGED_ARGS="--timeout 120"
diff --git a/src/vbox/meson.build b/src/vbox/meson.build
index df0cfb40e8..240f2389a9 100644
--- a/src/vbox/meson.build
+++ b/src/vbox/meson.build
@@ -68,9 +68,4 @@ if conf.has('WITH_VBOX')
     'name': 'virtvboxd',
     'in_file': files('virtvboxd.init.in'),
   }
-
-  sysconf_files += {
-    'name': 'virtvboxd',
-    'file': files('virtvboxd.sysconf'),
-  }
 endif
diff --git a/src/vbox/virtvboxd.service.in b/src/vbox/virtvboxd.service.in
index ebb31dde07..cfdafc39d2 100644
--- a/src/vbox/virtvboxd.service.in
+++ b/src/vbox/virtvboxd.service.in
@@ -14,6 +14,7 @@ Documentation=https://libvirt.org
 
 [Service]
 Type=notify
+Environment=VIRTVBOXD_ARGS="--timeout 120"
 EnvironmentFile=-@sysconfdir@/sysconfig/virtvboxd
 ExecStart=@sbindir@/virtvboxd $VIRTVBOXD_ARGS
 ExecReload=/bin/kill -HUP $MAINPID
diff --git a/src/vbox/virtvboxd.sysconf b/src/vbox/virtvboxd.sysconf
deleted file mode 100644
index 37ad353d54..0000000000
--- a/src/vbox/virtvboxd.sysconf
+++ /dev/null
@@ -1,3 +0,0 @@
-# Customizations for the virtvboxd.service systemd unit
-
-VIRTVBOXD_ARGS="--timeout 120"
diff --git a/src/vz/meson.build b/src/vz/meson.build
index 14f7280f66..d102696943 100644
--- a/src/vz/meson.build
+++ b/src/vz/meson.build
@@ -58,9 +58,4 @@ if conf.has('WITH_VZ')
     'name': 'virtvzd',
     'in_file': files('virtvzd.init.in'),
   }
-
-  sysconf_files += {
-    'name': 'virtvzd',
-    'file': files('virtvzd.sysconf'),
-  }
 endif
diff --git a/src/vz/virtvzd.service.in b/src/vz/virtvzd.service.in
index f551cb8fbf..7636bf2b9e 100644
--- a/src/vz/virtvzd.service.in
+++ b/src/vz/virtvzd.service.in
@@ -14,6 +14,7 @@ Documentation=https://libvirt.org
 
 [Service]
 Type=notify
+Environment=VIRTVZD_ARGS="--timeout 120"
 EnvironmentFile=-@sysconfdir@/sysconfig/virtvzd
 ExecStart=@sbindir@/virtvzd $VIRTVZD_ARGS
 ExecReload=/bin/kill -HUP $MAINPID
diff --git a/src/vz/virtvzd.sysconf b/src/vz/virtvzd.sysconf
deleted file mode 100644
index a86b9dfb6c..0000000000
--- a/src/vz/virtvzd.sysconf
+++ /dev/null
@@ -1,3 +0,0 @@
-# Customizations for the virtvzd.service systemd unit
-
-VIRTVZD_ARGS="--timeout 120"
diff --git a/tools/libvirt-guests.sh.in b/tools/libvirt-guests.sh.in
index 87f96af14d..74ca969468 100644
--- a/tools/libvirt-guests.sh.in
+++ b/tools/libvirt-guests.sh.in
@@ -30,13 +30,53 @@ test ! -r "$sysconfdir"/rc.d/init.d/functions ||
 
 export TEXTDOMAIN="@PACKAGE@" TEXTDOMAINDIR="@localedir@"
 
+# URIs to check for running guests
+# example: URIS='default xen:///system vbox+tcp://host/system lxc:///system'
 URIS="default"
+
+# action taken on host boot
+# - start   all guests which were running on shutdown are started on boot
+#           regardless on their autostart settings
+# - ignore  libvirt-guests init script won't start any guest on boot, however,
+#           guests marked as autostart will still be automatically started by
+#           libvirtd
 ON_BOOT="start"
+
+# action taken on host shutdown
+# - suspend   all running guests are suspended using virsh managedsave
+# - shutdown  all running guests are asked to shutdown. Please be careful with
+#             this settings since there is no way to distinguish between a
+#             guest which is stuck or ignores shutdown requests and a guest
+#             which just needs a long time to shutdown. When setting
+#             ON_SHUTDOWN=shutdown, you must also set SHUTDOWN_TIMEOUT to a
+#             value suitable for your guests.
 ON_SHUTDOWN="suspend"
+
+# Number of seconds we're willing to wait for a guest to shut down. If parallel
+# shutdown is enabled, this timeout applies as a timeout for shutting down all
+# guests on a single URI defined in the variable URIS. If this is 0, then there
+# is no time out (use with caution, as guests might not respond to a shutdown
+# request). The default value is 300 seconds (5 minutes).
 SHUTDOWN_TIMEOUT=300
+
+# Number of guests will be shutdown concurrently, taking effect when
+# "ON_SHUTDOWN" is set to "shutdown". If Set to 0, guests will be shutdown one
+# after another. Number of guests on shutdown at any time will not exceed number
+# set in this variable.
 PARALLEL_SHUTDOWN=0
+
+# Number of seconds to wait between each guest start. Set to 0 to allow
+# parallel startup.
 START_DELAY=0
+
+# If non-zero, try to bypass the file system cache when saving and
+# restoring guests, even though this may give slower operation for
+# some file systems.
 BYPASS_CACHE=0
+
+# 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
 
 test -f "$sysconfdir"/sysconfig/libvirt-guests &&
diff --git a/tools/libvirt-guests.sysconf b/tools/libvirt-guests.sysconf
deleted file mode 100644
index 4f83edab90..0000000000
--- a/tools/libvirt-guests.sysconf
+++ /dev/null
@@ -1,50 +0,0 @@
-# Customizations for the libvirt-guests.service systemd unit
-
-# URIs to check for running guests
-# example: URIS='default xen:///system vbox+tcp://host/system lxc:///system'
-#URIS=default
-
-# action taken on host boot
-# - start   all guests which were running on shutdown are started on boot
-#           regardless on their autostart settings
-# - ignore  libvirt-guests init script won't start any guest on boot, however,
-#           guests marked as autostart will still be automatically started by
-#           libvirtd
-#ON_BOOT=start
-
-# Number of seconds to wait between each guest start. Set to 0 to allow
-# parallel startup.
-#START_DELAY=0
-
-# action taken on host shutdown
-# - suspend   all running guests are suspended using virsh managedsave
-# - shutdown  all running guests are asked to shutdown. Please be careful with
-#             this settings since there is no way to distinguish between a
-#             guest which is stuck or ignores shutdown requests and a guest
-#             which just needs a long time to shutdown. When setting
-#             ON_SHUTDOWN=shutdown, you must also set SHUTDOWN_TIMEOUT to a
-#             value suitable for your guests.
-#ON_SHUTDOWN=suspend
-
-# Number of guests will be shutdown concurrently, taking effect when
-# "ON_SHUTDOWN" is set to "shutdown". If Set to 0, guests will be shutdown one
-# after another. Number of guests on shutdown at any time will not exceed number
-# set in this variable.
-#PARALLEL_SHUTDOWN=0
-
-# Number of seconds we're willing to wait for a guest to shut down. If parallel
-# shutdown is enabled, this timeout applies as a timeout for shutting down all
-# guests on a single URI defined in the variable URIS. If this is 0, then there
-# is no time out (use with caution, as guests might not respond to a shutdown
-# request). The default value is 300 seconds (5 minutes).
-#SHUTDOWN_TIMEOUT=300
-
-# If non-zero, try to bypass the file system cache when saving and
-# restoring guests, even though this may give slower operation for
-# some file systems.
-#BYPASS_CACHE=0
-
-# 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=1
diff --git a/tools/meson.build b/tools/meson.build
index 2acf7b0aaf..ab1067017b 100644
--- a/tools/meson.build
+++ b/tools/meson.build
@@ -273,12 +273,6 @@ configure_file(
 )
 
 if init_script == 'systemd'
-  install_data(
-    'libvirt-guests.sysconf',
-    install_dir: sysconfdir / 'sysconfig',
-    rename: 'libvirt-guests',
-  )
-
   configure_file(
     input: 'libvirt-guests.service.in',
     output: '@BASENAME@',
Re: [PATCH v1] remove sysconfig files
Posted by Andrea Bolognani 2 years, 9 months ago 
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
Re: [PATCH v1] remove sysconfig files
Posted by Olaf Hering 2 years, 9 months ago 
Am Wed, 21 Jul 2021 03:16:39 -0700
schrieb Andrea Bolognani <abologna@redhat.com>:

> 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/

I assumed a single host admin.

> > +++ 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.

Why is that systemd part needed?
The sysconfig files are recognized, they just have to be created.

> 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.

Yes, I have seen supposedly educated people struggling with the fact that a file does not exist on-disk.

> > +++ 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.

Only files marked as %config and which have been modified will be preserved as .rpmsave.

> 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.

Stale .rpmsave files will be preserved, just in case they contain any valuable data.
During upgrade rpm may create a .rpmsave file, which is then renamed back.

> 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?

If a transaction is interrupted for whatever reason the system is in an inconstant state.
No automation can get it out of such state.

> > -%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.

There is no point in restarting libvirtd in the middle of the transaction.
The spec file gives no ordering hints to rpm.

But yeah, I need to double check this part.
Perhaps it needs to be in a separate patch.

> >  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.

Yeah, I considered this. Lets use the ENOENT as condition.

> 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.

I will double check this part.

> 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.

There is also a check for version 1.3 from 2015.
I think in practice it is very unlikely that one can upgrade from a pre-1.3 version to libvirt.git#master on a live system.

> > +%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).

Thanks for spotting this.

> >  %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.

I can do such movements in a separate patch, good idea.

> > --- 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.

I think this is not required. The command line is obvious, what variable is expected, and what sysconfig would be parsed to obtain it.

> > +++ 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.

I think documentation should go into documentation files, instead of code or configuration files.

In case the commits which added --timeout and/or --listen failed to document it properly, that mistake has to be fixed in a separate patch.

> > +++ 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.

I just moved documentation into the code. Perhaps libvirt-guests is already documented elsewhere, and such documentation should be extended.

There old sysconfig file did not provide any defaults, as a result the service file does not need to provide defaults either. The defaults remain in the code.

> >  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 :(

I think the sourcing can be removed from the service file.
All other usage of EnvironmentFile= is just to obtain potential command line knobs.

This can be done in a separate patch.

> 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.

Yeah. On the SUSE side the sysconfig files are not owned by a package.
We could just wipe the templates.

But we would have to maintain a patch which puts the desired built-in defaults into the individual service files.

> 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.

It is up to the Debian package maintainer to sort this out in his environment.

I think it is and was wrong to put anything into /etc and claim these knobs are the default behavior.
Every default has to go into the code, so that it can be changed over time, if required.
I admit, a few packages with complex configuration have to be handled differently.

Olaf
Re: [PATCH v1] remove sysconfig files
Posted by Andrea Bolognani 2 years, 9 months ago 
On Wed, Jul 21, 2021 at 01:23:56PM +0200, Olaf Hering wrote:
> Am Wed, 21 Jul 2021 03:16:39 -0700 schrieb Andrea Bolognani <abologna@redhat.com>:
> > 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/
>
> I assumed a single host admin.

In this case, "they" is intended as a gender-neutral pronoun rather
than an indication that there is more than one person acting as admin
for the machine :)

> > >    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.
>
> Why is that systemd part needed?
> The sysconfig files are recognized, they just have to be created.

Both options are equally valid for configuring the service, and so
both should be documented.

> > 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?
>
> If a transaction is interrupted for whatever reason the system is in an inconstant state.
> No automation can get it out of such state.

I'm not entirely sure whether there are more precise guidelines on
what to do in these scenarios in the context of Fedora or some other
distribution, so I'll just say that this sounds reasonable enough.

> > 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.
>
> There is no point in restarting libvirtd in the middle of the transaction.
> The spec file gives no ordering hints to rpm.

Note that the daemon restart always happens *after* the transaction,
in the %posttrans part.

> > 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.
>
> There is also a check for version 1.3 from 2015.
> I think in practice it is very unlikely that one can upgrade from a pre-1.3 version to libvirt.git#master on a live system.

Yeah, I think that could go too. We have very broad backward
compatibility guarantees when it comes to libvirt, but in the context
of a spec file that explicitly refuses to work on anything but fairly
recent platforms I think it's absolutely fair to tone down these
guarantees accordingly.

> > > -# 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.
>
> I think this is not required. The command line is obvious, what variable is expected, and what sysconfig would be parsed to obtain it.

Spelling it out explicitly would help making things more
discoverable, the same way it currently does for the sysconfig file,
so I'd much rather we kept it even if in a different form.

> > > +# 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.
>
> I think documentation should go into documentation files, instead of code or configuration files.
>
> In case the commits which added --timeout and/or --listen failed to document it properly, that mistake has to be fixed in a separate patch.

Both options are indeed documented in libvirtd(8). --listen is a
particularly tricky one, but with virtproxyd taking over its role in
the modular daemon scenario and the latter slated to become the
default configuration soon, perhaps that's good enough.

> > > +# 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.
>
> I just moved documentation into the code. Perhaps libvirt-guests is already documented elsewhere, and such documentation should be extended.

It looks like the only documentation for the various options that can
be used to tweak the service's behavior was the one found in the
sysconfig file. Perhaps a man page should be introduced?

> > 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 :(
>
> I think the sourcing can be removed from the service file.
> All other usage of EnvironmentFile= is just to obtain potential command line knobs.

But then libvirt-guests would end up being the only service that
cannot be configured via a systemd override... I think we should
strive for consistency here.

> I think it is and was wrong to put anything into /etc and claim these knobs are the default behavior.
> Every default has to go into the code, so that it can be changed over time, if required.
> I admit, a few packages with complex configuration have to be handled differently.

For most software, the default configuration files consist of mostly
comments and act more like documentation for what the local
configuration might look like. Claiming that the defaults are
actually defined in the source code is correct, but also not very
useful if it means that the local admin needs to go poking at said
sources to figure out how to configure their machine...

-- 
Andrea Bolognani / Red Hat / Virtualization
Re: [PATCH v1] remove sysconfig files
Posted by Olaf Hering 2 years, 9 months ago 
Am Wed, 21 Jul 2021 06:00:12 -0700
schrieb Andrea Bolognani <abologna@redhat.com>:

> On Wed, Jul 21, 2021 at 01:23:56PM +0200, Olaf Hering wrote:
> > There is no point in restarting libvirtd in the middle of the transaction.
> > The spec file gives no ordering hints to rpm.  
> Note that the daemon restart always happens *after* the transaction,
> in the %posttrans part.

I was wrong, %systemd_post may not do a 'try-restart', but that certainly depends on what this macro actually does.
At least the macros I have will just do 'daemon-reload', they do nothing with the listed units.

It is not clear if 'try-restart' reloads the EnvironmentFile=.
If it is indeed reloaded, a modified file will not exist at this point because it was just renamed to .rpmsave.

In other words: the current '%post daemon' needs no modification.

> It looks like the only documentation for the various options that can
> be used to tweak the service's behavior was the one found in the
> sysconfig file. Perhaps a man page should be introduced?

Yeah, someone familiar with libvirt-guests.sh should provide one.
This patch just moves the existing comments elsewhere.
To mean it does not look like a user needs to tweak that script.

> But then libvirt-guests would end up being the only service that
> cannot be configured via a systemd override... I think we should
> strive for consistency here.

It certainly can be configured.
The environment can be constructed either with a .service.d/ file or with the (optional) sysconfig file.

Olaf
Re: [PATCH v1] remove sysconfig files
Posted by Olaf Hering 2 years, 9 months ago 
Am Wed, 21 Jul 2021 06:00:12 -0700
schrieb Andrea Bolognani <abologna@redhat.com>:

> For most software, the default configuration files consist of mostly
> comments and act more like documentation for what the local
> configuration might look like. Claiming that the defaults are
> actually defined in the source code is correct, but also not very
> useful if it means that the local admin needs to go poking at said
> sources to figure out how to configure their machine...

This is what documentation is for, either man/info/html pages.
Too bad, configuration was misused for this in the past decades.

I will respond to the other items tomorrow.


Olaf
Re: [PATCH v1] remove sysconfig files
Posted by Olaf Hering 2 years, 9 months ago 
Am Tue, 20 Jul 2021 19:00:20 +0200
schrieb Olaf Hering <olaf@aepfle.de>:

> +#Environment=QEMU_AUDIO_DRV=sdl
> +#Environment=SDL_AUDIODRIVER=pulse

These two knobs are undocumented. Instead of dropping them I decided to put them into the service files.

I do not know if both are intended for a wider audience, or just for the test suite.

Olaf

Patchew 2.1-devel-0870f84

© 2016 - 2024 Red Hat, Inc.