[Spice-devel] [PATCH vd_agent_linux v2] vdagent: Stop trying to connect to the daemon after a while

[Frediano Ziglio]

> On Mon, 2018-11-19 at 11:42 -0500, Frediano Ziglio wrote:
> > > 
> > > On Mon, 2018-11-19 at 10:23 +0000, Frediano Ziglio wrote:
> > > > Do not try  indefinitely to connect to the daemon, should not
> > > > take long to activate.
> > > > 
> > > > Signed-off-by: Frediano Ziglio <fziglio at redhat.com>
> > > > ---
> > > >  src/vdagent/vdagent.c | 6 ++++++
> > > >  1 file changed, 6 insertions(+)
> > > > 
> > > > Changes since v1:
> > > > - log error when we reach the limit
> > > > 
> > > > diff --git a/src/vdagent/vdagent.c b/src/vdagent/vdagent.c
> > > > index f7c8b72..7a58150 100644
> > > > --- a/src/vdagent/vdagent.c
> > > > +++ b/src/vdagent/vdagent.c
> > > > @@ -53,6 +53,7 @@ typedef struct VDAgent {
> > > >      struct vdagent_file_xfers *xfers;
> > > >      struct udscs_connection *conn;
> > > >      GIOChannel *x11_channel;
> > > > +    guint connection_attempts;
> > > >  
> > > >      GMainLoop *loop;
> > > >  } VDAgent;
> > > > @@ -370,6 +371,11 @@ static gboolean vdagent_init_async_cb(gpointer
> > > > user_data)
> > > >                                  daemon_read_complete,
> > > >                                  daemon_disconnect_cb,
> > > >                                  debug);
> > > >      if (agent->conn == NULL) {
> > > > +        // limit connection attempts, this will try for 5 minutes
> > > > +        if (++agent->connection_attempts > 5 * 60) {
> > > > +            syslog(LOG_ERR, "Attempted to contact daemon for 5
> > > > minutes,
> > > > giving up");
> > > > +            goto err_init;
> > > > +        }
> > > 
> > > (in reference to trying to keep it simple from v1 thread:)
> > > I think using 60 instead of 1 in g_timeout_add_seconds() wouldn't be
> > > much of a complication... unless there's something else I'm missing.
> > > I'm also thinking this is being launched from a .desktop file so AFAIK
> > > there is no easy way for a user to restart the service, a relogin is
> > > needed?
> > > 
> > 
> > It reconnects to the new daemon after detecting the disconnection.
> > Not sure what will happen if the agent is updated... I think nothing
> > or possibly the agent will end if not compatible with the new daemon.
> > If you retry every minute is possible that the user has to wait 1
> > minute instead of 1 second.
> > Probably the optimal way would be to use inotify on the socket to
> > wait for the new daemon, so something like:
> > - try 10 times with 1 second delay
> > - setup inotify
> > - try once to avoid races
> > - wait inotify
> > - attempt connection again (repeat from first step)
> > This way if the daemon is restored after even hours you don't have to
> > keep polling.
> 
> Well, you wanted a simple solution and I agree with that. I suggested
> something that keeps the agent running and doesn't really make it more
> complicated. Now you're suggesting a much more complex solution... :)
> 

Well... it all started with Victor comment "Why?". The feature that if the
daemon is resurrected the agent will be active in a second could be
valuable. Not that I like processes keeping polling all the time.
I though systemd or whatever program doing socket activation would reuse
the same socket, just keeping looking for new connections if the daemon
is not active but it seems that (at least systemd) create a new socket
(so the i-node and the directory changes and can be monitored). In case
the daemon is launched stand-alone (not with socket activation) it has
to recreate the socket. I'm not aware or other possible socket activator
so I assume is safe to check the i-node change.

On the other hand people can ask "why keeping the agents polling/monitoring
forever?". Maybe is just my paranoia that I don't like processes polling
for nothing and daemon should just be running and people fix that condition.

> > > I also just realized you should reset connection_attempts to 0 after a
> > > successful connect?
> > > 
> > 
> > The object is created again so will be 0 again (not that would hurt
> > to reset on a successful connection).
> 
> Right, didn't notice that...
> 
> Cheers,
> Lukas
> 
> > > Cheers,
> > > Lukas
> > > 
> > > >          g_timeout_add_seconds(1, vdagent_init_async_cb, agent);
> > > >          return G_SOURCE_REMOVE;
> > > >      }
>