Re: [vdagent-win v3 2/2] vdagent-win: start vdagent with lock info from session

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi,

On Fri, Aug 12, 2016 at 09:52:55AM -0400, Frediano Ziglio wrote:
> > 
> > Commit 5907b6cbb5c724f9729da59a644271b4258d122e started to handle
> > Lock/Unlock events from Session at VDAgent. Although that works just
> > fine, it does not cover all the situations as pointed by Andrei at [0]
> > and I quote:
> > 
> > > It fails for next test-case:
> > >
> > > * Connect with RV to VM
> > > * Lock VM (ctrl-alt-del)
> > > * Close RV
> > > * Connect with RV to the VM again
> > > * Do not unlock session. DnD files from client to locked VM.
> > >
> > > Result : Files are copied to VM.
> > 
> > [0] https://bugzilla.redhat.com/show_bug.cgi?id=1323628#c6
> > 
> > To solve this situation, we need VDService to inform VDAgent if
> > Session is Locked or not upon its initialization.
> > 
> > As the VDService should always set the unlock event in case client
> > connects to a unlocked Session, VDAgent now consider the session
> > locked till Session or VDService says its not.
> > 
> > Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1323628
> > Signed-off-by: Victor Toso <victortoso@xxxxxxxxxx>
> > Reported-by: Andrei Stepanov <astepano@xxxxxxxxxx>
> > ---
> > 
> > Changed the approach to be in the safe side. VDAgent starts now with the
> > session_is_locked property set to true and that should only change by VDAgent
> > _session_unlocked_event event (on startup) or by Session WTS_SESSION_UNLOCK
> > event.
> > 
> > ---
> >  common/vdcommon.h       |  1 +
> >  vdagent/vdagent.cpp     | 18 ++++++++++++++++--
> >  vdservice/vdservice.cpp | 15 +++++++++++++++
> >  3 files changed, 32 insertions(+), 2 deletions(-)
> > 
> > diff --git a/common/vdcommon.h b/common/vdcommon.h
> > index f4859e2..67fb034 100644
> > --- a/common/vdcommon.h
> > +++ b/common/vdcommon.h
> > @@ -35,6 +35,7 @@ typedef CRITICAL_SECTION mutex_t;
> >  
> >  #define VD_AGENT_REGISTRY_KEY "SOFTWARE\\Red Hat\\Spice\\vdagent\\"
> >  #define VD_AGENT_STOP_EVENT   TEXT("Global\\vdagent_stop_event")
> > +#define VD_AGENT_SESSION_UNLOCKED_EVENT
> > TEXT("Global\\vdagent_session_unlocked_event")
> >  
> >  #if defined __GNUC__
> >  #define ALIGN_GCC __attribute__ ((packed))
> > diff --git a/vdagent/vdagent.cpp b/vdagent/vdagent.cpp
> > index 8e7ba2b..aa64824 100644
> > --- a/vdagent/vdagent.cpp
> > +++ b/vdagent/vdagent.cpp
> > @@ -143,6 +143,7 @@ private:
> >      DWORD _input_time;
> >      HANDLE _control_event;
> >      HANDLE _stop_event;
> > +    HANDLE _session_unlocked_event;
> >      VDAgentMessage* _in_msg;
> >      uint32_t _in_msg_pos;
> >      bool _pending_input;
> > @@ -203,11 +204,12 @@ VDAgent::VDAgent()
> >      , _input_time (0)
> >      , _control_event (NULL)
> >      , _stop_event (NULL)
> > +    , _session_unlocked_event (NULL)
> >      , _in_msg (NULL)
> >      , _in_msg_pos (0)
> >      , _pending_input (false)
> >      , _running (false)
> > -    , _session_is_locked (false)
> > +    , _session_is_locked (true)
> >      , _desktop_switch (false)
> >      , _desktop_layout (NULL)
> >      , _display_setting (VD_AGENT_REGISTRY_KEY)
> > @@ -309,6 +311,7 @@ bool VDAgent::run()
> >          return false;
> >      }
> >      _stop_event = OpenEvent(SYNCHRONIZE, FALSE, VD_AGENT_STOP_EVENT);
> > +    _session_unlocked_event = OpenEvent(SYNCHRONIZE, FALSE,
> > VD_AGENT_SESSION_UNLOCKED_EVENT);
> >      memset(&wcls, 0, sizeof(wcls));
> >      wcls.lpfnWndProc = &VDAgent::wnd_proc;
> >      wcls.lpszClassName = VD_AGENT_WINCLASS_NAME;
> > @@ -481,13 +484,14 @@ void VDAgent::input_desktop_message_loop()
> >  
> >  void VDAgent::event_dispatcher(DWORD timeout, DWORD wake_mask)
> >  {
> > -    HANDLE events[2];
> > +    HANDLE events[3];
> >      DWORD event_count = 1;
> >      DWORD wait_ret;
> >      MSG msg;
> >      enum {
> >          CONTROL_ACTION,
> >          STOP_ACTION,
> > +        SESSION_UNLOCKED_ACTION,
> >      } actions[SPICE_N_ELEMENTS(events)], action;
> >  
> >      events[0] = _control_event;
> > @@ -498,6 +502,12 @@ void VDAgent::event_dispatcher(DWORD timeout, DWORD
> > wake_mask)
> >          event_count++;
> >      }
> >  
> > +    if (_session_unlocked_event) {
> > +        events[event_count] = _session_unlocked_event;
> > +        actions[event_count] = SESSION_UNLOCKED_ACTION;
> > +        event_count++;
> > +    }
> > +
> >      wait_ret = MsgWaitForMultipleObjectsEx(event_count, events, timeout,
> >      wake_mask, MWMO_ALERTABLE);
> >      if (wait_ret == WAIT_OBJECT_0 + event_count) {
> >          while (PeekMessage(&msg, NULL, 0, 0, PM_REMOVE)) {
> > @@ -522,6 +532,10 @@ void VDAgent::event_dispatcher(DWORD timeout, DWORD
> > wake_mask)
> >          vd_printf("%s: received stop event", __func__);
> >          _running = false;
> >          break;
> > +    case SESSION_UNLOCKED_ACTION:
> > +        vd_printf("%s: received session unlocked event", __func__);
> > +        _session_is_locked = false;
> > +        break;
> >      default:
> >          vd_printf("%s: action not handled (%d)", __func__, action);
> >          _running = false;
> > diff --git a/vdservice/vdservice.cpp b/vdservice/vdservice.cpp
> > index 89e0dbb..0df0c52 100644
> > --- a/vdservice/vdservice.cpp
> > +++ b/vdservice/vdservice.cpp
> > @@ -93,6 +93,7 @@ private:
> >      PROCESS_INFORMATION _agent_proc_info;
> >      HANDLE _control_event;
> >      HANDLE _agent_stop_event;
> > +    HANDLE _session_unlocked_event;
> >      HANDLE* _events;
> >      TCHAR _agent_path[MAX_PATH];
> >      VDControlQueue _control_queue;
> > @@ -105,6 +106,7 @@ private:
> >      int _system_version;
> >      bool _agent_alive;
> >      bool _running;
> > +    bool _session_is_locked;
> >      VDLog* _log;
> >      unsigned _events_count;
> >  };
> > @@ -128,6 +130,7 @@ VDService::VDService()
> >      , _agent_restarts (0)
> >      , _agent_alive (false)
> >      , _running (false)
> > +    , _session_is_locked (false)
> >      , _log (NULL)
> >      , _events_count(0)
> >  {
> > @@ -135,6 +138,7 @@ VDService::VDService()
> >      _system_version = supported_system_version();
> >      _control_event = CreateEvent(NULL, FALSE, FALSE, NULL);
> >      _agent_stop_event = CreateEvent(NULL, FALSE, FALSE,
> >      VD_AGENT_STOP_EVENT);
> > +    _session_unlocked_event = CreateEvent(NULL, FALSE, FALSE,
> > VD_AGENT_SESSION_UNLOCKED_EVENT);
> >      _agent_path[0] = wchar_t('\0');
> >      MUTEX_INIT(_agent_mutex);
> >      MUTEX_INIT(_control_mutex);
> > @@ -143,6 +147,7 @@ VDService::VDService()
> >  
> >  VDService::~VDService()
> >  {
> > +    CloseHandle(_session_unlocked_event);
> >      CloseHandle(_agent_stop_event);
> >      CloseHandle(_control_event);
> >      delete _events;
> > @@ -307,6 +312,10 @@ DWORD WINAPI VDService::control_handler(DWORD control,
> > DWORD event_type, LPVOID
> >          if (event_type == WTS_CONSOLE_CONNECT) {
> >              s->_session_id = session_id;
> >              s->set_control_event(VD_CONTROL_RESTART_AGENT);
> > +        } else if (event_type == WTS_SESSION_LOCK) {
> > +            s->_session_is_locked = true;
> > +        } else if (event_type == WTS_SESSION_UNLOCK) {
> > +            s->_session_is_locked = false;
> >          }
> >          break;
> >      }
> > @@ -744,6 +753,12 @@ bool VDService::launch_agent()
> >          return false;
> >      }
> >      _agent_alive = true;
> > +
> > +    if (!_session_is_locked) {
> > +        /* If we just create a new VDAgent but the session is locked, we
> > must
> > +         * notify it so it can handle requests from client correctly */
> > +        SetEvent(_session_unlocked_event);
> > +    }
> >      return true;
> >  }
> >
>
> Mumble... got an idea about this. Why instead of using the event as
> an event you use it as a flag? So on the service you set if unlocked
> and reset if locked (the event must be on manual).
> On the agent you use WaitForSingleObject with a 0 timeout just to
> read the status, WAIT_OBJECT_0 if set or WAIT_TIMEOUT if unset.
>
> Obviously you remove the event from the loop or if set you'll get
> an high CPU usage.

So, you think that the fact we have an extra event on VDAgent might be a
waste thus we can do it in a different part of the code, only once with
WaitForSingleObject?

It is fine by me but I don't see that much of difference to be honest.
I will try later.

>
> I don't know if you can use WTSQuerySessionInformation to just
> retrieve the state (lock/unlock) of the session.

Well, this seems better for a one-time procedure.
We can use it (WTSINFOEX) and parse the SessionFlags [0]. Windows 7 and
above which seems okay.

[0] https://msdn.microsoft.com/en-us/library/ee621019(v=vs.85).aspx

>
> Frediano

  toso
_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/spice-devel




[Index of Archives]     [Linux ARM Kernel]     [Linux ARM]     [Linux Omap]     [Fedora ARM]     [IETF Annouce]     [Security]     [Bugtraq]     [Linux]     [Linux OMAP]     [Linux MIPS]     [ECOS]     [Asterisk Internet PBX]     [Linux API]     [Monitors]