[Spice-devel] [PATCH win-vdagent] Don't exit vdagent when a client disconnects

[Pavel Grunt]

On Tue, 2017-04-04 at 11:31 -0500, Jonathon Jongsma wrote:
> Anybody want to ACK or NACK this patch?
oh, sorry, I forgot /o\

yes, it looks good to me

Ack

> 
> 
> On Fri, 2017-03-24 at 14:28 -0500, Jonathon Jongsma wrote:
> > On Fri, 2017-03-24 at 11:49 +0100, Pavel Grunt wrote:
> > > Hi,
> > > 
> > > looks good to me. How does it work on log out - it seems that in
> > > that
> > > case it kills the agent.
> > > 
> > 
> > Yeah, for all other situations it works as it did before. So on
> > logout
> > the agent will exit. This patch only changes the behavior when a
> > client
> > disconnects. 
> > 
> > Jonathon
> > 
> > 
> > > Pavel
> > > 
> > > On Thu, 2017-03-16 at 10:08 -0500, Jonathon Jongsma wrote:
> > > > Previously, when a client disconnects, the vdagent executable
> > > > exited.
> > > > The agent was restarted immediately in order to be ready for
> > > > the
> > > > next
> > > > client to reconnect.
> > > > 
> > > > This is generally fine, but there are certain situations
> > > > where it causes issues. For example, if client A is connected
> > > > to
> > > > a
> > > > guest
> > > > and client B connects to the same guest, client A will be
> > > > forcibly
> > > > disconnected and causes the the vdagent to restart. This
> > > > scenario
> > > > is
> > > > racy because the agent can take some time to exit and restart.
> > > > Client B
> > > > may think that the agent is connected at startup and may send
> > > > agent
> > > > messages to the guest.  At some point the server will recieve
> > > > notification that the agent has exited and send an
> > > > AGENT_DISCONNECTED
> > > > message to client B. After the agent has been fully restarted,
> > > > an
> > > > AGENT_CONNECTED message will be sent to the client, but any
> > > > messages
> > > > sent between client connection and the AGENT_DISCONNECTED
> > > > message
> > > > will
> > > > be lost. This causes problems especially with fullscreen mode
> > > > in
> > > > virt-viewer.
> > > > 
> > > > The solution is to not exit and restart the agent when a
> > > > client
> > > > disconnects. This is how the linux vdagent behaves. Instead,
> > > > we
> > > > simply
> > > > cancel all ongoing file transfers and reset the clipboard when
> > > > a
> > > > client
> > > > is disconnected.
> > > > 
> > > > Fixes: rhbz#1064495
> > > > ---
> > > >  vdagent/file_xfer.cpp | 20 +++++++++++++++-----
> > > >  vdagent/file_xfer.h   |  3 +++
> > > >  vdagent/vdagent.cpp   | 11 ++++++++---
> > > >  3 files changed, 26 insertions(+), 8 deletions(-)
> > > > 
> > > > diff --git a/vdagent/file_xfer.cpp b/vdagent/file_xfer.cpp
> > > > index 0e90ebe..f763ed3 100644
> > > > --- a/vdagent/file_xfer.cpp
> > > > +++ b/vdagent/file_xfer.cpp
> > > > @@ -31,17 +31,22 @@
> > > >  #include "file_xfer.h"
> > > >  #include "as_user.h"
> > > >  
> > > > -FileXfer::~FileXfer()
> > > > +void FileXfer::reset()
> > > >  {
> > > >      FileXferTasks::iterator iter;
> > > >      FileXferTask* task;
> > > >  
> > > >      for (iter = _tasks.begin(); iter != _tasks.end(); iter++)
> > > > {
> > > >          task = iter->second;
> > > > -        CloseHandle(task->handle);
> > > > -        DeleteFile(task->name);
> > > > +        task->cancel();
> > > >          delete task;
> > > >      }
> > > > +    _tasks.clear();
> > > > +}
> > > > +
> > > > +FileXfer::~FileXfer()
> > > > +{
> > > > +    reset();
> > > >  }
> > > >  
> > > >  void FileXfer::handle_start(VDAgentFileXferStartMessage*
> > > > start,
> > > > @@ -152,6 +157,12 @@ fin:
> > > >      return true;
> > > >  }
> > > >  
> > > > +void FileXferTask::cancel()
> > > > +{
> > > > +    CloseHandle(handle);
> > > > +    DeleteFile(name);
> > > > +}
> > > > +
> > > >  void FileXfer::handle_status(VDAgentFileXferStatusMessage*
> > > > status)
> > > >  {
> > > >      FileXferTasks::iterator iter;
> > > > @@ -168,8 +179,7 @@ void
> > > > FileXfer::handle_status(VDAgentFileXferStatusMessage* status)
> > > >          return;
> > > >      }
> > > >      task = iter->second;
> > > > -    CloseHandle(task->handle);
> > > > -    DeleteFile(task->name);
> > > > +    task->cancel();
> > > >      _tasks.erase(iter);
> > > >      delete task;
> > > >  }
> > > > diff --git a/vdagent/file_xfer.h b/vdagent/file_xfer.h
> > > > index d9c65f3..25cd5c2 100644
> > > > --- a/vdagent/file_xfer.h
> > > > +++ b/vdagent/file_xfer.h
> > > > @@ -34,6 +34,8 @@ typedef struct ALIGN_VC FileXferTask {
> > > >      uint64_t size;
> > > >      uint64_t pos;
> > > >      TCHAR name[MAX_PATH];
> > > > +
> > > > +    void cancel();
> > > >  } ALIGN_GCC FileXferTask;
> > > >  
> > > >  typedef std::map<uint32_t, FileXferTask*> FileXferTasks;
> > > > @@ -42,6 +44,7 @@ class FileXfer {
> > > >  public:
> > > >      ~FileXfer();
> > > >      bool dispatch(VDAgentMessage* msg,
> > > > VDAgentFileXferStatusMessage* status);
> > > > +    void reset();
> > > >  
> > > >  private:
> > > >      void handle_start(VDAgentFileXferStartMessage* start,
> > > > VDAgentFileXferStatusMessage* status);
> > > > diff --git a/vdagent/vdagent.cpp b/vdagent/vdagent.cpp
> > > > index e3ba14b..c25a077 100644
> > > > --- a/vdagent/vdagent.cpp
> > > > +++ b/vdagent/vdagent.cpp
> > > > @@ -112,7 +112,7 @@ private:
> > > >      DWORD get_cximage_format(uint32_t type) const;
> > > >      enum { owner_none, owner_guest, owner_client };
> > > >      void set_clipboard_owner(int new_owner);
> > > > -    enum { CONTROL_STOP, CONTROL_DESKTOP_SWITCH,
> > > > CONTROL_LOGON,
> > > > CONTROL_CLIPBOARD };
> > > > +    enum { CONTROL_STOP, CONTROL_RESET,
> > > > CONTROL_DESKTOP_SWITCH,
> > > > CONTROL_LOGON, CONTROL_CLIPBOARD };
> > > >      void set_control_event(int control_command);
> > > >      void handle_control_event();
> > > >      VDIChunk* new_chunk(DWORD bytes = 0);
> > > > @@ -379,6 +379,11 @@ void VDAgent::handle_control_event()
> > > >          _control_queue.pop();
> > > >          vd_printf("Control command %d", control_command);
> > > >          switch (control_command) {
> > > > +        case CONTROL_RESET:
> > > > +            _file_xfer.reset();
> > > > +            set_control_event(CONTROL_CLIPBOARD);
> > > > +            set_clipboard_owner(owner_none);
> > > > +            break;
> > > >          case CONTROL_STOP:
> > > >              _running = false;
> > > >              break;
> > > > @@ -1321,8 +1326,8 @@ void
> > > > VDAgent::dispatch_message(VDAgentMessage*
> > > > msg, uint32_t port)
> > > >          break;
> > > >      }
> > > >      case VD_AGENT_CLIENT_DISCONNECTED:
> > > > -        vd_printf("Client disconnected, agent to be
> > > > restarted");
> > > > -        set_control_event(CONTROL_STOP);
> > > > +        vd_printf("Client disconnected, resetting agent
> > > > state");
> > > > +        set_control_event(CONTROL_RESET);
> > > >          break;
> > > >      case VD_AGENT_MAX_CLIPBOARD:
> > > >          res = handle_max_clipboard((VDAgentMaxClipboard*)msg-
> > > > > data, 
> > > > 
> > > > msg->size);
> > 
> > _______________________________________________
> > Spice-devel mailing list
> > Spice-devel at lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/spice-devel