[PATCH v6 3/4] remove sysconfig files

[Andrea Bolognani]

On Tue, Dec 21, 2021 at 12:22:44PM +0100, Olaf Hering wrote:
> +++ b/docs/daemons.rst
> @@ -686,3 +686,23 @@ socket unit names into the service. When using these old versions, the
> +Changing command line options for daemons
> +=========================================
> +
> +Two ways exist to override the defaults in the provided service files:
> +Either a systemd "drop-in" configuration file, or a ``/etc/sysconfig/$daemon``

s/Either/either/

> +file must be created.  For example, changing the command line option

s/  / /
s/changing/to change/

> +for a debug session of ``libvirtd``, create a file
> +``/etc/systemd/system/libvirtd.service.d/my.conf`` with the following content:

Maybe call this debug.conf?

> +   ::
> +
> +      [Unit]
> +      Description=Virtualization daemon, with override from my.conf
> +      [Service]
> +      Environment=G_DEBUG=fatal-warnings
> +      Environment=LIBVIRTD_ARGS="--listen --verbose"

I'd skip the [Unit] part, it's not really that relevant. At the very
least, leave an empty line between the [Unit] part and the [Service]
part.

> +++ b/libvirt.spec.in
> @@ -206,6 +206,24 @@
> +# libvirt 8.0.0 stops distributing any sysconfig files.
> +# If the user has customized their sysconfig file,
> +# the RPM upgrade path will rename it to .rpmsave
> +# because the file is no longer managed by RPM.
> +# To prevent a regression we rename it back after the
> +# transaction to preserve the user's modifications
> +%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}

Can you please rename the macros to libvirt_sysconfig_*? "sc" is not
really a common or well-known shorthand for sysconfig, at least as
far as I'm aware.

Also please indent using spaces, not tabs, and have meaningful
indentation instead of just having the entire body at the same
level.

> --- a/src/remote/libvirtd.sysconf
> +++ /dev/null
> @@ -1,21 +0,0 @@
> -# If systemd socket activation is disabled, then the following
> -# can be used to listen on TCP/TLS sockets
> -#LIBVIRTD_ARGS="--listen"

This bit about passing --listen to libvirtd got lost during the move
to the unit file. Please make sure you preserve it.

> +++ 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"
[...]

This is the bit I still not entirely convinced of.

After this change, libvirt-guests' configuration becomes entirely
opaque: the only way the admin can learn how to configure the service
is by somehow realizing that it's a shell script as opposed to a
binary and looking inside it.

Can we do better? I don't have any brilliant ideas myself,
unfortunately. The best I could come up with is to have a comment in
libvirt-guests.service along the lines of

  [Service]
  # To learn what configuration knobs are available for this
  # service, check out the top of the libvirt-guest.sh script
  EnvironmentFile=- at sysconfdir@/sysconfig/libvirt-guests

We could also move the defaults and their documentation even further
up, right after the license blurb, and add some sort of heading.
That would give them more visibility and mitigate my concerns to some
extent.

-- 
Andrea Bolognani / Red Hat / Virtualization