[Spice-devel,vdagent-win,1/6] vdservice: add control events RHBZ #719140 #722980

Submitted by Arnon Gilboa on July 24, 2011, 5:48 p.m.

Details

Message ID 1311504498-31132-2-git-send-email-agilboa@redhat.com
State New, archived
Headers show

Not browsing as part of any series.

Commit Message

Arnon Gilboa July 24, 2011, 5:48 p.m.
prevent race between service control manager (SCM) & the service main thread.
use events for stop, logon, agent restart.
---
 vdservice/vdservice.cpp |   70 +++++++++++++++++++++++++++++-----------------
 1 files changed, 44 insertions(+), 26 deletions(-)

Patch hide | download patch | download mbox

diff --git a/vdservice/vdservice.cpp b/vdservice/vdservice.cpp
index 32c0ca5..4d4937f 100644
--- a/vdservice/vdservice.cpp
+++ b/vdservice/vdservice.cpp
@@ -52,10 +52,16 @@  enum {
     VD_EVENT_PIPE_READ = 0,
     VD_EVENT_PIPE_WRITE,
     VD_EVENT_CONTROL,
-    VD_EVENT_LOGON,
     VD_STATIC_EVENTS_COUNT // Must be last
 };
 
+enum {
+    VD_CONTROL_IDLE = 0,
+    VD_CONTROL_STOP,
+    VD_CONTROL_LOGON,
+    VD_CONTROL_RESTART_AGENT,
+};
+
 class VDService {
 public:
     static VDService* get();
@@ -71,6 +77,8 @@  private:
     static DWORD WINAPI control_handler(DWORD control, DWORD event_type,
                                         LPVOID event_data, LPVOID context);
     static VOID WINAPI main(DWORD argc, TCHAR * argv[]);
+    void set_control_event(int control_command);
+    void handle_control_event();
     void pipe_write_completion();
     void write_agent_control(uint32_t type, uint32_t opaque);
     void read_pipe();
@@ -79,7 +87,6 @@  private:
     bool handle_agent_control(VDPipeMessage* msg);
     bool restart_agent(bool normal_restart);
     bool launch_agent();
-    void send_logon();
     bool kill_agent();
     unsigned fill_agent_event() {
         ASSERT(_events);
@@ -96,7 +103,6 @@  private:
     SERVICE_STATUS_HANDLE _status_handle;
     PROCESS_INFORMATION _agent_proc_info;
     HANDLE _control_event;
-    HANDLE _logon_event;
     HANDLE* _events;
     TCHAR _agent_path[MAX_PATH];
     VDIPort* _vdi_port;
@@ -109,6 +115,7 @@  private:
     DWORD _last_agent_restart_time;
     int _agent_restarts;
     int _system_version;
+    int _control_command;
     bool _pipe_connected;
     bool _pending_reset;
     bool _pending_write;
@@ -164,6 +171,7 @@  VDService::VDService()
     , _chunk_size (0)
     , _last_agent_restart_time (0)
     , _agent_restarts (0)
+    , _control_command (VD_CONTROL_IDLE)
     , _pipe_connected (false)
     , _pending_reset (false)
     , _pending_write (false)
@@ -180,7 +188,6 @@  VDService::VDService()
     _control_event = CreateEvent(NULL, FALSE, FALSE, NULL);
     _pipe_state.write.overlap.hEvent = CreateEvent(NULL, FALSE, FALSE, NULL);
     _pipe_state.read.overlap.hEvent = CreateEvent(NULL, FALSE, FALSE, NULL);
-    _logon_event = CreateEvent(NULL, FALSE, FALSE, NULL);
     _agent_path[0] = wchar_t('\0');
     MUTEX_INIT(_agent_mutex);
     _singleton = this;
@@ -188,7 +195,6 @@  VDService::VDService()
 
 VDService::~VDService()
 {
-    CloseHandle(_logon_event);
     CloseHandle(_pipe_state.read.overlap.hEvent);
     CloseHandle(_pipe_state.write.overlap.hEvent);
     CloseHandle(_control_event);
@@ -293,6 +299,35 @@  const char* session_events[] = {
     "LOCK", "UNLOCK", "REMOTE_CONTROL"
 };
 
+void VDService::set_control_event(int control_command)
+{
+    _control_command = control_command;  
+    if (_control_event && !SetEvent(_control_event)) {
+        vd_printf("SetEvent() failed: %u", GetLastError());
+    }
+}
+
+void VDService::handle_control_event()
+{
+    switch (_control_command) {
+    case VD_CONTROL_IDLE:
+        break;
+    case VD_CONTROL_STOP:
+        _running = false;
+        break;
+    case VD_CONTROL_LOGON:
+        vd_printf("logon event");
+        write_agent_control(VD_AGENT_SESSION_LOGON, 0);
+        break;
+    case VD_CONTROL_RESTART_AGENT:
+        _running = restart_agent(true);
+        break;
+    default:
+        vd_printf("Unsupported control command %u", _control_command);
+    }
+    _control_command = VD_CONTROL_IDLE;
+}
+
 DWORD WINAPI VDService::control_handler(DWORD control, DWORD event_type, LPVOID event_data,
                                         LPVOID context)
 {
@@ -319,11 +354,9 @@  DWORD WINAPI VDService::control_handler(DWORD control, DWORD event_type, LPVOID
         if (s->_system_version != SYS_VER_UNSUPPORTED) {
             if (event_type == WTS_CONSOLE_CONNECT) {
                 s->_session_id = session_id;
-                if (!s->restart_agent(true)) {
-                    s->stop();
-               }
+                s->set_control_event(VD_CONTROL_RESTART_AGENT);
             } else if (event_type == WTS_SESSION_LOGON) {
-                s->send_logon();
+                s->set_control_event(VD_CONTROL_LOGON);
             }
         }
         break;
@@ -471,7 +504,6 @@  bool VDService::execute()
     _events[VD_EVENT_PIPE_READ] = _pipe_state.read.overlap.hEvent;
     _events[VD_EVENT_PIPE_WRITE] = _pipe_state.write.overlap.hEvent;
     _events[VD_EVENT_CONTROL] = _control_event;
-    _events[VD_EVENT_LOGON] = _logon_event;
     _agent_proc_info.hProcess;
     _vdi_port->fill_events(&_events[_events_vdi_port_base]);
     _chunk_size = _chunk_port = 0;
@@ -524,10 +556,7 @@  bool VDService::execute()
                 break;
             case WAIT_OBJECT_0 + VD_EVENT_CONTROL:
                 vd_printf("Control event");
-                break;
-            case WAIT_OBJECT_0 + VD_EVENT_LOGON:
-                vd_printf("logon event");
-                write_agent_control(VD_AGENT_SESSION_LOGON, 0);
+                handle_control_event();
                 break;
             case WAIT_TIMEOUT:
                 break;
@@ -918,17 +947,7 @@  bool VDService::restart_agent(bool normal_restart)
 void VDService::stop()
 {
     vd_printf("Service stopped");
-    _running = false;
-    if (_control_event && !SetEvent(_control_event)) {
-        vd_printf("SetEvent() failed: %u", GetLastError());
-    }
-}
-
-void VDService::send_logon()
-{
-    if (_logon_event && !SetEvent(_logon_event)) {
-        vd_printf("SetEvent() failed: %u", GetLastError());
-    }
+    set_control_event(VD_CONTROL_STOP);
 }
 
 void VDService::pipe_write_completion()
@@ -1154,4 +1173,3 @@  int _tmain(int argc, TCHAR* argv[], TCHAR* envp[])
     delete vdservice;
     return (success ? 0 : -1);
 }
-

Comments

On Sun, Jul 24, 2011 at 01:48:13PM +0300, Arnon Gilboa wrote:
> prevent race between service control manager (SCM) & the service main thread.
> use events for stop, logon, agent restart.
> ---
>  vdservice/vdservice.cpp |   70 +++++++++++++++++++++++++++++-----------------
>  1 files changed, 44 insertions(+), 26 deletions(-)
> 
> diff --git a/vdservice/vdservice.cpp b/vdservice/vdservice.cpp
> index 32c0ca5..4d4937f 100644
> --- a/vdservice/vdservice.cpp
> +++ b/vdservice/vdservice.cpp
> @@ -52,10 +52,16 @@ enum {
>      VD_EVENT_PIPE_READ = 0,
>      VD_EVENT_PIPE_WRITE,
>      VD_EVENT_CONTROL,
> -    VD_EVENT_LOGON,
>      VD_STATIC_EVENTS_COUNT // Must be last
>  };
>  
> +enum {
> +    VD_CONTROL_IDLE = 0,
> +    VD_CONTROL_STOP,
> +    VD_CONTROL_LOGON,
> +    VD_CONTROL_RESTART_AGENT,
> +};
> +
>  class VDService {
>  public:
>      static VDService* get();
> @@ -71,6 +77,8 @@ private:
>      static DWORD WINAPI control_handler(DWORD control, DWORD event_type,
>                                          LPVOID event_data, LPVOID context);
>      static VOID WINAPI main(DWORD argc, TCHAR * argv[]);
> +    void set_control_event(int control_command);
> +    void handle_control_event();
>      void pipe_write_completion();
>      void write_agent_control(uint32_t type, uint32_t opaque);
>      void read_pipe();
> @@ -79,7 +87,6 @@ private:
>      bool handle_agent_control(VDPipeMessage* msg);
>      bool restart_agent(bool normal_restart);
>      bool launch_agent();
> -    void send_logon();
>      bool kill_agent();
>      unsigned fill_agent_event() {
>          ASSERT(_events);
> @@ -96,7 +103,6 @@ private:
>      SERVICE_STATUS_HANDLE _status_handle;
>      PROCESS_INFORMATION _agent_proc_info;
>      HANDLE _control_event;
> -    HANDLE _logon_event;
>      HANDLE* _events;
>      TCHAR _agent_path[MAX_PATH];
>      VDIPort* _vdi_port;
> @@ -109,6 +115,7 @@ private:
>      DWORD _last_agent_restart_time;
>      int _agent_restarts;
>      int _system_version;
> +    int _control_command;
>      bool _pipe_connected;
>      bool _pending_reset;
>      bool _pending_write;
> @@ -164,6 +171,7 @@ VDService::VDService()
>      , _chunk_size (0)
>      , _last_agent_restart_time (0)
>      , _agent_restarts (0)
> +    , _control_command (VD_CONTROL_IDLE)
>      , _pipe_connected (false)
>      , _pending_reset (false)
>      , _pending_write (false)
> @@ -180,7 +188,6 @@ VDService::VDService()
>      _control_event = CreateEvent(NULL, FALSE, FALSE, NULL);
>      _pipe_state.write.overlap.hEvent = CreateEvent(NULL, FALSE, FALSE, NULL);
>      _pipe_state.read.overlap.hEvent = CreateEvent(NULL, FALSE, FALSE, NULL);
> -    _logon_event = CreateEvent(NULL, FALSE, FALSE, NULL);
>      _agent_path[0] = wchar_t('\0');
>      MUTEX_INIT(_agent_mutex);
>      _singleton = this;
> @@ -188,7 +195,6 @@ VDService::VDService()
>  
>  VDService::~VDService()
>  {
> -    CloseHandle(_logon_event);
>      CloseHandle(_pipe_state.read.overlap.hEvent);
>      CloseHandle(_pipe_state.write.overlap.hEvent);
>      CloseHandle(_control_event);
> @@ -293,6 +299,35 @@ const char* session_events[] = {
>      "LOCK", "UNLOCK", "REMOTE_CONTROL"
>  };
>  
> +void VDService::set_control_event(int control_command)
> +{
> +    _control_command = control_command;  

There may be an unhandled _control_command which will be overwritten
by this, no?

> +    if (_control_event && !SetEvent(_control_event)) {
> +        vd_printf("SetEvent() failed: %u", GetLastError());
> +    }
> +}
> +
> +void VDService::handle_control_event()
> +{
> +    switch (_control_command) {
> +    case VD_CONTROL_IDLE:
> +        break;
> +    case VD_CONTROL_STOP:
> +        _running = false;
> +        break;
> +    case VD_CONTROL_LOGON:
> +        vd_printf("logon event");
> +        write_agent_control(VD_AGENT_SESSION_LOGON, 0);
> +        break;
> +    case VD_CONTROL_RESTART_AGENT:
> +        _running = restart_agent(true);
> +        break;
> +    default:
> +        vd_printf("Unsupported control command %u", _control_command);
> +    }
> +    _control_command = VD_CONTROL_IDLE;
> +}
> +
>  DWORD WINAPI VDService::control_handler(DWORD control, DWORD event_type, LPVOID event_data,
>                                          LPVOID context)
>  {
> @@ -319,11 +354,9 @@ DWORD WINAPI VDService::control_handler(DWORD control, DWORD event_type, LPVOID
>          if (s->_system_version != SYS_VER_UNSUPPORTED) {
>              if (event_type == WTS_CONSOLE_CONNECT) {
>                  s->_session_id = session_id;
> -                if (!s->restart_agent(true)) {
> -                    s->stop();
> -               }
> +                s->set_control_event(VD_CONTROL_RESTART_AGENT);
>              } else if (event_type == WTS_SESSION_LOGON) {
> -                s->send_logon();
> +                s->set_control_event(VD_CONTROL_LOGON);
>              }
>          }
>          break;
> @@ -471,7 +504,6 @@ bool VDService::execute()
>      _events[VD_EVENT_PIPE_READ] = _pipe_state.read.overlap.hEvent;
>      _events[VD_EVENT_PIPE_WRITE] = _pipe_state.write.overlap.hEvent;
>      _events[VD_EVENT_CONTROL] = _control_event;
> -    _events[VD_EVENT_LOGON] = _logon_event;
>      _agent_proc_info.hProcess;
>      _vdi_port->fill_events(&_events[_events_vdi_port_base]);
>      _chunk_size = _chunk_port = 0;
> @@ -524,10 +556,7 @@ bool VDService::execute()
>                  break;
>              case WAIT_OBJECT_0 + VD_EVENT_CONTROL:
>                  vd_printf("Control event");
> -                break;
> -            case WAIT_OBJECT_0 + VD_EVENT_LOGON:
> -                vd_printf("logon event");
> -                write_agent_control(VD_AGENT_SESSION_LOGON, 0);
> +                handle_control_event();
>                  break;
>              case WAIT_TIMEOUT:
>                  break;
> @@ -918,17 +947,7 @@ bool VDService::restart_agent(bool normal_restart)
>  void VDService::stop()
>  {
>      vd_printf("Service stopped");
> -    _running = false;
> -    if (_control_event && !SetEvent(_control_event)) {
> -        vd_printf("SetEvent() failed: %u", GetLastError());
> -    }
> -}
> -
> -void VDService::send_logon()
> -{
> -    if (_logon_event && !SetEvent(_logon_event)) {
> -        vd_printf("SetEvent() failed: %u", GetLastError());
> -    }
> +    set_control_event(VD_CONTROL_STOP);
>  }
>  
>  void VDService::pipe_write_completion()
> @@ -1154,4 +1173,3 @@ int _tmain(int argc, TCHAR* argv[], TCHAR* envp[])
>      delete vdservice;
>      return (success ? 0 : -1);
>  }
> -
> -- 
> 1.7.4.1
> 
> _______________________________________________
> Spice-devel mailing list
> Spice-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/spice-devel
Alon Levy wrote:
> On Sun, Jul 24, 2011 at 01:48:13PM +0300, Arnon Gilboa wrote:
>   
>> prevent race between service control manager (SCM) & the service main thread.
>> use events for stop, logon, agent restart.
>> ---
>>  vdservice/vdservice.cpp |   70 +++++++++++++++++++++++++++++-----------------
>>  1 files changed, 44 insertions(+), 26 deletions(-)
>>
>>
>>     
<snoopy>
>> +void VDService::set_control_event(int control_command)
>> +{
>> +    _control_command = control_command;  
>>     
>
> There may be an unhandled _control_command which will be overwritten
> by this, no?
>
>   
Thanks, you are right, although practically the events are discreetly 
distanced so it's unlikely to happen.
However, I'll replace it with a thread-safe queue.