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

[Frediano Ziglio]

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

The "Requisite" still prevents the socket to be activated with a
"systemctl enable spice-vdagentd.socket" launched manually.
You can obviously complaint that users should not do that but
I don't think that if the system says "you don't have the device
needed" when effectively don't have the needed device is a bad
approach. So personally I would like to keep the "Requisite".
About the "After" I have no idea of the difference that it can
make.

Frediano