Hi Andrea,
Thanks for the review.
On Fri, Apr 03, 2026 at 02:37:13AM -0700, Andrea Bolognani wrote:
> On Mon, Mar 30, 2026 at 09:44:51PM +0530, Arun Menon via Devel wrote:
> > The monolithic libvirtd.service currently has a dependency on
> > virt-secret-init-encryption.service. This causes libvirtd to fail
> > to start on systems where the secret driver is not installed or
> > enabled, as systemd cannot satisfy the Requires= / After= units or the
> > LoadCredentialEncrypted= path. See below,
> >
> > Requires=virt-secret-init-encryption.service
> > After=virt-secret-init-encryption.service
> > LoadCredentialEncrypted=secrets-encryption-key:@localstatedir@/lib/libvirt/secrets/secrets-encryption-key
>
> Slight correction, After= is not really a problem, systemd will
> happily ignore that.
>
> > +++ b/libvirt.spec.in
> > @@ -2259,6 +2259,8 @@ exit 0
> > %{_unitdir}/virtsecretd.socket
> > %{_unitdir}/virtsecretd-ro.socket
> > %{_unitdir}/virtsecretd-admin.socket
> > +%dir %attr(0700, root, root) %{_unitdir}/libvirtd.service.d/
> > +%{_unitdir}/libvirtd.service.d/50-libvirtd-secret.conf
>
> 0700 is unnecessarily restrictive, make it 0755 to match the unit
> itself.
>
> I looked at the %{_unitdir}/*.d/*.conf files that exist in my local
> installation and it seems that most use 10 as the sorting number. I
> don't know if there's any specific guideline for that, but unless you
> have a good reason to pick 50 I'd say stick with what others are
> doing. Including word "libvirtd" in the name of the drop-in seems
> unnecessary too. So I suggest using 10-secret.conf here.
Yes, I see now. Package drop-ins should not accidentally override the
ones defined by the user. Thanks.
https://www.freedesktop.org/software/systemd/man/latest/systemd-system.conf.html#Configuration%20Directories%20and%20Precedence
>
> > +++ b/src/remote/libvirtd-secret.conf.in
> > @@ -0,0 +1,7 @@
> > +[Unit]
> > +Requires=@service@
> > +After=@service@
>
> Using templating for this part feels a bit excessive, we know the
> name of the unit ahead of time and it's not going to change.
>
> > +++ b/src/remote/meson.build
> > +# The monolithic libvirt daemon only attempts to load the
> > +# secrets encryption credentials if the secret driver is enabled
> > +if conf.has('WITH_SECRETS')
> > + secret_dropin_conf = configuration_data()
> > + secret_dropin_conf.set('service', 'virt-secret-init-encryption.service')
> > + secret_dropin_conf.set('localstatedir', localstatedir)
>
> So here you can use the slightly nicer
>
> secret_dropin_conf = configuration_data({
> 'localstatedir': localstatedir,
> })
>
Yes.
> matching most of our uses of configuration_data. Feel free to update
> how virt_secret_init_encryption_conf is defined too, in a separate
> patch ;)
I have incorporated all the feedback in v2 of this series.
>
> --
> Andrea Bolognani / Red Hat / Virtualization
>
Regards,
Arun Menon
