[Spice-devel] [vdagent-win v3 2/2] vdagent-win: start vdagent with lock info from session
Frediano Ziglio
fziglio at redhat.com
Fri Aug 12 15:01:36 UTC 2016
Would be something like this (not tested)
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..0c24a92 100644
--- a/vdagent/vdagent.cpp
+++ b/vdagent/vdagent.cpp
@@ -236,6 +236,18 @@ VDAgent::VDAgent()
MUTEX_INIT(_control_mutex);
MUTEX_INIT(_message_mutex);
+ HANDLE _session_unlocked_event = OpenEvent(SYNCHRONIZE, FALSE, VD_AGENT_SESSION_UNLOCKED_EVENT);
+ switch (WaitForSingleObject(_session_unlocked_event, 0)) {
+ case WAIT_OBJECT_0:
+ _session_is_locked = false;
+ break;
+ default:
+ case WAIT_TIMEOUT:
+ _session_is_locked = true;
+ break;
+ }
+ CloseHandle(_session_unlocked_event);
+
_singleton = this;
}
diff --git a/vdservice/vdservice.cpp b/vdservice/vdservice.cpp
index 12f7644..e1476a0 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;
@@ -135,6 +136,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, TRUE, TRUE, VD_AGENT_SESSION_UNLOCKED_EVENT);
_agent_path[0] = wchar_t('\0');
MUTEX_INIT(_agent_mutex);
MUTEX_INIT(_control_mutex);
@@ -143,6 +145,7 @@ VDService::VDService()
VDService::~VDService()
{
+ CloseHandle(_session_unlocked_event);
CloseHandle(_agent_stop_event);
CloseHandle(_control_event);
delete[] _events;
@@ -307,6 +310,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) {
+ ResetEvent(s->_session_unlocked_event);
+ } else if (event_type == WTS_SESSION_UNLOCK) {
+ SetEvent(s->_session_unlocked_event);
}
break;
}
>
> 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's just another way to pass a flag... if it's set/tested just at startup
there is no reason to put in the main loop.
> 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
>
I found somebody suggesting the function for earlier versions (like
Windows 2000) but I didn't really find the lock/unlock flag.
> >
> > Frediano
>
> toso
>
--
Frediano
More information about the Spice-devel
mailing list