[Spice-devel] [PATCH 1/4] start refactoring vdi_port
Alon Levy
alevy at redhat.com
Sun Jan 2 04:49:19 PST 2011
On Sun, Jan 02, 2011 at 01:33:55PM +0200, Arnon Gilboa wrote:
> Alon Levy wrote:
> >introduce VDIPort::get_num_events and VDIPort::fill_events,
> >change VDService::_events to be dynamically allocated
> >document _events contents: STATIC events, then vdi_port, then agent.
> >---
> > vdservice/vdi_port.cpp | 2 +-
> > vdservice/vdi_port.h | 17 +++++++--
> > vdservice/vdservice.cpp | 83 +++++++++++++++++++++++++++++-----------------
> > 3 files changed, 66 insertions(+), 36 deletions(-)
> >
> >diff --git a/vdservice/vdi_port.cpp b/vdservice/vdi_port.cpp
> >index e36ff86..f4dfaeb 100644
> >--- a/vdservice/vdi_port.cpp
> >+++ b/vdservice/vdi_port.cpp
> >@@ -56,7 +56,7 @@ bool VDIPort::init()
> > _handle = CreateFile(VIOSERIAL_PORT_PATH, GENERIC_READ | GENERIC_WRITE , 0, NULL,
> > OPEN_EXISTING, FILE_FLAG_OVERLAPPED, NULL);
> > if (_handle == INVALID_HANDLE_VALUE) {
> >- vd_printf("CreateFile() failed: %u", GetLastError());
> >+ vd_printf("CreateFile() %s failed: %u", VIOSERIAL_PORT_PATH, GetLastError());
> > return false;
> > }
> > _write.overlap.hEvent = CreateEvent(NULL, FALSE, FALSE, NULL);
> >diff --git a/vdservice/vdi_port.h b/vdservice/vdi_port.h
> >index a10b346..8e057bb 100644
> >--- a/vdservice/vdi_port.h
> >+++ b/vdservice/vdi_port.h
> >@@ -46,14 +46,23 @@ public:
> > size_t ring_read(void* buf, size_t size);
> > size_t read_ring_size();
> > size_t read_ring_continuous_remaining_size();
> >- HANDLE get_write_event() { return _write.overlap.hEvent; }
> >- HANDLE get_read_event() { return _read.overlap.hEvent; }
> >+ unsigned get_num_events() { return 2; }
> >+ void fill_events(HANDLE *handle) {
> >+ handle[0] = _write.overlap.hEvent;
> >+ handle[1] = _read.overlap.hEvent;
> >+ }
> >+ void handle_event(int event) {
> >+ switch (event) {
> >+ case 0: write_completion(); break;
> >+ case 1: read_completion(); break;
> >+ }
> >+ }
> not 1-liners, so will be nicer in the cpp
ok.
> consider using #defines
ok.
> HANDLE *handle -> HANDLE* handles
ok.
> > int write();
> > int read();
> >- void write_completion();
> >- void read_completion();
> > private:
> >+ void write_completion();
> >+ void read_completion();
> > int handle_error();
> > private:
> >diff --git a/vdservice/vdservice.cpp b/vdservice/vdservice.cpp
> >index ae5ad87..72882ea 100644
> >--- a/vdservice/vdservice.cpp
> >+++ b/vdservice/vdservice.cpp
> >@@ -40,15 +40,19 @@
> > #define CREATE_PROC_MAX_RETRIES 10
> > #define CREATE_PROC_INTERVAL_MS 500
> >+// This enum simplifies WaitForMultipleEvents for static
> >+// events, that is handles that are guranteed non NULL.
> >+// It doesn't include:
> >+// VDIPort Handles - these are filled by an interface because
> >+// of variable handle number.
> >+// VDAgent handle - this can be 1 or 0 (NULL or not), so it is also added at
> >+// the end of VDService::_events
> > enum {
> > VD_EVENT_PIPE_READ = 0,
> > VD_EVENT_PIPE_WRITE,
> > VD_EVENT_CONTROL,
> >- VD_EVENT_READ,
> >- VD_EVENT_WRITE,
> > VD_EVENT_LOGON,
> >- VD_EVENT_AGENT, // Must be before last
> >- VD_EVENTS_COUNT // Must be last
> >+ VD_STATIC_EVENTS_COUNT // Must be last
> > };
> nice
> > class VDService {
> >@@ -76,7 +80,15 @@ private:
> > bool launch_agent();
> > void send_logon();
> > bool kill_agent();
> >-
> >+ unsigned fill_agent_event() {
> >+ ASSERT(_events);
> >+ if (_agent_proc_info.hProcess) {
> >+ _events[_events_count - 1] = _agent_proc_info.hProcess;
> >+ return _events_count;
> >+ } else {
> >+ return _events_count - 1;
> >+ }
> >+ }
> like above, will be nicer in the cpp
> > private:
> > static VDService* _singleton;
> > SERVICE_STATUS _status;
> >@@ -84,7 +96,7 @@ private:
> > PROCESS_INFORMATION _agent_proc_info;
> > HANDLE _control_event;
> > HANDLE _logon_event;
> >_pending_write
> >- HANDLE _events[VD_EVENTS_COUNT];
> >+ HANDLE* _events;
> > TCHAR _agent_path[MAX_PATH];
> > VDIPort* _vdi_port;
> > VDPipeState _pipe_state;
> >@@ -103,6 +115,8 @@ private:
> > bool _agent_alive;
> > bool _running;
> > VDLog* _log;
> >+ unsigned _events_count;
> >+ unsigned _events_vdi_port_base;
> > };
> > VDService* VDService::_singleton = NULL;
> >@@ -142,6 +156,7 @@ int supported_system_version()
> > VDService::VDService()
> > : _status_handle (0)
> >+ , _events (NULL)
> > , _vdi_port (NULL)
> > , _connection_id (0)
> > , _session_id (0)
> >@@ -156,10 +171,11 @@ VDService::VDService()
> > , _agent_alive (false)
> > , _running (false)
> > , _log (NULL)
> >+ , _events_count(0)
> >+ , _events_vdi_port_base(0)
> > {
> > ZeroMemory(&_agent_proc_info, sizeof(_agent_proc_info));
> > ZeroMemory(&_pipe_state, sizeof(_pipe_state));
> >- ZeroMemory(_events, sizeof(_events));
> > _system_version = supported_system_version();
> > _control_event = CreateEvent(NULL, FALSE, FALSE, NULL);
> > _pipe_state.write.overlap.hEvent = CreateEvent(NULL, FALSE, FALSE, NULL);
> >@@ -176,6 +192,7 @@ VDService::~VDService()
> > CloseHandle(_pipe_state.read.overlap.hEvent);
> > CloseHandle(_pipe_state.write.overlap.hEvent);
> > CloseHandle(_control_event);
> >+ delete _events;
> > delete _log;
> > }
> >@@ -423,14 +440,19 @@ bool VDService::execute()
> > CloseHandle(pipe);
> > return false;
> > }
> >+ _events_count = VD_STATIC_EVENTS_COUNT + _vdi_port->get_num_events() + 1 /*for agent*/;
> >+ _events = new HANDLE[_events_count];
> >+ _events_vdi_port_base = VD_STATIC_EVENTS_COUNT;
> >+ ZeroMemory(_events, _events_count);
> > vd_printf("Connected to server");
> > _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_READ] = _vdi_port->get_read_event();
> >- _events[VD_EVENT_WRITE] = _vdi_port->get_write_event();
> > _events[VD_EVENT_LOGON] = _logon_event;
> >- _events[VD_EVENT_AGENT] = _agent_proc_info.hProcess;
> >+ _agent_proc_info.hProcess;
> >+ vd_printf("Calling fill_events");
> >+ vd_printf("&_events[VD_ENVETS_COUNT] == %p", &_events[VD_STATIC_EVENTS_COUNT]);
> i guess these logs are later removed ;)
> >+ _vdi_port->fill_events(&_events[_events_vdi_port_base]);
> > _chunk_size = _chunk_port = 0;
> > read_pipe();
> > while (_running) {
> >@@ -456,8 +478,8 @@ bool VDService::execute()
> > handle_pipe_data(0);
> > }
> > if (_running && (!cont || _pending_read || _pending_write)) {
> >- DWORD events_count = _events[VD_EVENT_AGENT] ? VD_EVENTS_COUNT : VD_EVENTS_COUNT - 1;
> >- DWORD wait_ret = WaitForMultipleObjects(events_count, _events, FALSE,
> >+ unsigned actual_events = fill_agent_event();
> why is fill_agent_event() called in each iteration? is it for cases
> agent is already down?
Yes. Or up - the agent state can change between each iteration, so we refill the _events
array, that's what fill_agent_event does, and it returns the number of total events.
> >+ DWORD wait_ret = WaitForMultipleObjects(actual_events, _events, FALSE,
> > cont ? 0 : INFINITE);
> > switch (wait_ret) {
> > case WAIT_OBJECT_0 + VD_EVENT_PIPE_READ: {
> >@@ -482,28 +504,29 @@ bool VDService::execute()
> > case WAIT_OBJECT_0 + VD_EVENT_CONTROL:
> > vd_printf("Control event");
> > break;
> >- case WAIT_OBJECT_0 + VD_EVENT_READ:
> >- _vdi_port->read_completion();
> >- break;
> >- case WAIT_OBJECT_0 + VD_EVENT_WRITE:
> >- _vdi_port->write_completion();
> >- break;
> > case WAIT_OBJECT_0 + VD_EVENT_LOGON:
> > vd_printf("logon event");
> > write_agent_control(VD_AGENT_SESSION_LOGON, 0);
> > break;
> >- case WAIT_OBJECT_0 + VD_EVENT_AGENT:
> >- vd_printf("Agent killed");
> >- if (_system_version == SYS_VER_WIN_XP) {
> >- restart_agent(false);
> >- } else if (_system_version == SYS_VER_WIN_7) {
> >- kill_agent();
> >- }
> >- break;
> > case WAIT_TIMEOUT:
> > break;
> > default:
> >- vd_printf("WaitForMultipleObjects failed %u", GetLastError());
> >+ if (wait_ret == WAIT_OBJECT_0 + _events_count - 1) {
> >+ vd_printf("Agent killed");
> >+ if (_system_version == SYS_VER_WIN_XP) {
> >+ restart_agent(false);
> >+ } else if (_system_version == SYS_VER_WIN_7) {
> >+ kill_agent();
> >+ }
> >+ } else {
> following if-else should be part of the above else
> >+ if (wait_ret >= WAIT_OBJECT_0 + _events_vdi_port_base &&
> >+ wait_ret < WAIT_OBJECT_0 +
> >+ _events_vdi_port_base + _vdi_port->get_num_events()) {
> >+ _vdi_port->handle_event(wait_ret - VD_STATIC_EVENTS_COUNT - WAIT_OBJECT_0);
> >+ } else {
> >+ vd_printf("WaitForMultipleObjects failed %u", GetLastError());
> >+ }
> >+ }
> > }
> > }
> > }
> >@@ -802,7 +825,6 @@ bool VDService::launch_agent()
> > vd_printf("ConnectNamedPipe() failed: %u", GetLastError());
> > return false;
> > }
> >- _events[VD_EVENT_AGENT] = _agent_proc_info.hProcess;
> > return true;
> > }
> >@@ -815,7 +837,6 @@ bool VDService::kill_agent()
> > if (!_agent_alive) {
> > return true;
> > }
> >- _events[VD_EVENT_AGENT] = 0;
> > _agent_alive = false;
> > if (_pipe_connected) {
> > _pipe_connected = false;
> >@@ -909,8 +930,8 @@ void VDService::pipe_write_completion()
> > } else {
> > vd_printf("GetOverlappedResult() failed : %d", GetLastError());
> > }
> >- _pending_write = false;
> >- }
> >+ _pending_write = false;
> >+ }
> > if (ps->write.start < ps->write.end) {
> > _pending_write = true;
>
More information about the Spice-devel
mailing list