[Spice-devel] [vdagent-linux v2] systemd: Remove unneded virtio-port dependencies

[Christophe Fergeau]

On Wed, Dec 19, 2018 at 04:11:09AM -0500, Frediano Ziglio wrote:
> > 
> > On Tue, Dec 18, 2018 at 12:15:09PM -0500, Frediano Ziglio wrote:
> > > > 
> > > > The udev rule already adds SYSTEMD_WANTS=spice-vdagentd.socket
> > > > to the relevant virtio devices, which automatically adds Wants=${device}
> > > > to spice-vdagentd.socket (see 'systemctl show spice-vdagentd.socket').
> > > > Adding a Requisite/After for these ports is at best redundant, and most
> > > > likely wrong as this is causing boot time warnings:
> > > > 
> > > 
> > > Lot of doubts, were you able to reproduce the issue?
> > 
> > Yes I was able to reproduce, and the message is gone with this patch.
> > 
> > > Looking at systemd documentation is not clear why the Requisite causes
> > > an INFO message. Maybe something is attempting to launch the service?
> > 
> > Did not dig too deeply as to why that's causing an error. I'm not sure
> > this is a valid unit name to be honest, which would explain a lot, but
> > if that's the reason, then it would be easy to reproduce..
> > 
> > > What happen if Requisite is removed and service is enabled? Are we going
> > > to have an error at every restart?
> > 
> > I don't understand your question. What kind of error do you think we
> > will get? Note that we will still have a Wants= associated with the
> > .socket file (automatically added by udev rule).
> > 
> > 
> > Christophe
> > 
> 
> Did some experiment, I think I got the full picture.
> 
> How to reproduce the bug: simply do a "systemctl start spice-vdagentd.socket" !
> How did I get here is a bit more complicated.
> The INFO/WARNING lines from the bug did not come from the activation from udev
> but from sockets.target. The lines:
> 
>    [Install]
>    WantedBy=sockets.target
> 
> tell systemd to activate the unit when sockets.target is activated (so when
> network is configured). In the rpm you have (rpm -q --scripts spice-vdagentd):
> 
>     postinstall scriptlet (using /bin/sh):
> 
>     if [ $1 -eq 1 ] ; then 
>           # Initial installation 
>           systemctl preset spice-vdagentd.service spice-vdagentd.socket >/dev/null 2>&1 || : 
>     fi
> 
> which will create a dependency from sockets.target to spice-vdagentd.socket
> (like doing a "systemctl enable spice-vdagentd.socket").
> To sum up the "[Install]" lines will enable the socket in all case. To test this
> remove/comment the "Requisite" and "After", remove the com.redhat.spice.0 device
> from the VM and start again the VM. Now with a "netstat -anop | grep spice" you
> can see that the /var/run/spice-vdagentd/spice-vdagent-sock socket is in listen
> mode (by systemd) even if there's no device. If you try to open the socket
> (for instance with a "nc -U /var/run/spice-vdagentd/spice-vdagent-sock") systemd
> will launch the SPICE service.
> 
> I think that a better solution, instead of removing the "Requisite", is to
> remove the "[Install]" section. I tried it and correctly udev is activating
> the socket if the device is detected. If the device is not detected the socket
> won't be activated (you need also to preset the socket unit like in the rpm
> script) which seems correct to me.

man systemd.socket says:
   Default Dependencies
       The following dependencies are added unless DefaultDependencies=no is set:

       ·   Socket units automatically gain a Before= dependency on sockets.target.

So we basically get
Before=sockets.target
WantedBy=sockets.target

We also get a
Wants=${device}
through the udev rule, which is what starts spice-vdagentd.service when
the device is there.

So with the additional information you provided, I agree that the
WantedBy does not seem needed. But I still think the Requisite/After
dependencies we currently have are not needed either.

Christophe
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <https://lists.freedesktop.org/archives/spice-devel/attachments/20181219/f1e31a2a/attachment-0001.sig>