[Spice-devel] [vdagent-win v3 2/2] vdagent-win: start vdagent with lock info from session
Victor Toso
victortoso at redhat.com
Fri Aug 12 14:13:16 UTC 2016
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 at redhat.com>
> > Reported-by: Andrei Stepanov <astepano at redhat.com>
> > ---
> >
> > 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
More information about the Spice-devel
mailing list