[Spice-devel] [PATCH 1/3] vdagent: remove vdi_port, use vio_serial directly

Wed Nov 28 08:04:45 PST 2012

seems ok, feel free to review ;)

Arnon Gilboa wrote:
> NACK, seems it needs some more testing
>
> Arnon Gilboa wrote:
>> ---
>> vdagent/vdagent.cpp | 239 
>> +++++++++++++++++++++++----------------------------
>> 1 files changed, 109 insertions(+), 130 deletions(-)
>>
>> diff --git a/vdagent/vdagent.cpp b/vdagent/vdagent.cpp
>> index 2bb466d..85244c0 100644
>> --- a/vdagent/vdagent.cpp
>> +++ b/vdagent/vdagent.cpp
>> @@ -16,8 +16,6 @@
>> */
>>
>> #include "vdcommon.h"
>> -#include "virtio_vdi_port.h"
>> -#include "pci_vdi_port.h"
>> #include "desktop_layout.h"
>> #include "display_setting.h"
>> #include "ximage.h"
>> @@ -65,11 +63,7 @@ typedef struct ALIGN_VC VDIChunk {
>> } ALIGN_GCC VDIChunk;
>>
>> #define VD_MESSAGE_HEADER_SIZE (sizeof(VDIChunk) + 
>> sizeof(VDAgentMessage))
>> -
>> -enum {
>> - VD_EVENT_CONTROL = 0,
>> - VD_STATIC_EVENTS_COUNT // Must be last
>> -};
>> +#define VD_READ_BUF_SIZE (sizeof(VDIChunk) + VD_AGENT_MAX_DATA_SIZE)
>>
>> class VDAgent {
>> public:
>> @@ -90,8 +84,6 @@ private:
>> bool handle_clipboard_request(VDAgentClipboardRequest* 
>> clipboard_request);
>> void handle_clipboard_release();
>> bool handle_display_config(VDAgentDisplayConfig* display_config, 
>> uint32_t port);
>> - void handle_port_in();
>> - void handle_port_out();
>> void handle_chunk(VDIChunk* chunk);
>> void on_clipboard_grab();
>> void on_clipboard_request(UINT format);
>> @@ -101,7 +93,9 @@ private:
>> static HGLOBAL utf8_alloc(LPCSTR data, int size);
>> static LRESULT CALLBACK wnd_proc(HWND hwnd, UINT message, WPARAM 
>> wparam, LPARAM lparam);
>> static DWORD WINAPI event_thread_proc(LPVOID param);
>> - static void dispatch_message(VDAgentMessage* msg, uint32_t port);
>> + static VOID CALLBACK read_completion(DWORD err, DWORD bytes, 
>> LPOVERLAPPED overlapped);
>> + static VOID CALLBACK write_completion(DWORD err, DWORD bytes, 
>> LPOVERLAPPED overlapped);
>> + void dispatch_message(VDAgentMessage* msg, uint32_t port);
>> uint32_t get_clipboard_format(uint32_t type);
>> uint32_t get_clipboard_type(uint32_t format);
>> DWORD get_cximage_format(uint32_t type);
>> @@ -114,7 +108,7 @@ private:
>> void enqueue_chunk(VDIChunk* msg);
>> bool write_message(uint32_t type, uint32_t size, void* data);
>> bool write_clipboard(VDAgentMessage* msg, uint32_t size);
>> - bool init_vdi_port();
>> + bool init_vio_serial();
>> bool send_input();
>> void set_display_depth(uint32_t depth);
>> void load_display_setting();
>> @@ -134,16 +128,19 @@ private:
>> INPUT _input;
>> DWORD _input_time;
>> HANDLE _control_event;
>> - HANDLE* _events;
>> VDAgentMessage* _in_msg;
>> uint32_t _in_msg_pos;
>> - uint32_t _events_count;
>> bool _pending_input;
>> bool _running;
>> bool _desktop_switch;
>> DesktopLayout* _desktop_layout;
>> DisplaySetting _display_setting;
>> - VDIPort* _vdi_port;
>> + HANDLE _vio_serial;
>> + OVERLAPPED _read_overlapped;
>> + OVERLAPPED _write_overlapped;
>> + CHAR _read_buf[VD_READ_BUF_SIZE];
>> + DWORD _read_pos;
>> + DWORD _write_pos;
>> mutex_t _control_mutex;
>> mutex_t _message_mutex;
>> std::queue<int> _control_queue;
>> @@ -163,6 +160,8 @@ private:
>>
>> VDAgent* VDAgent::_singleton = NULL;
>>
>> +#define VIOSERIAL_PORT_PATH L"\\\\.\\Global\\com.redhat.spice.0"
>> +
>> VDAgent* VDAgent::get()
>> {
>> if (!_singleton) {
>> @@ -181,16 +180,16 @@ VDAgent::VDAgent()
>> , _mouse_y (0)
>> , _input_time (0)
>> , _control_event (NULL)
>> - , _events (NULL)
>> , _in_msg (NULL)
>> , _in_msg_pos (0)
>> - , _events_count (0)
>> , _pending_input (false)
>> , _running (false)
>> , _desktop_switch (false)
>> , _desktop_layout (NULL)
>> , _display_setting (VD_AGENT_REGISTRY_KEY)
>> - , _vdi_port (NULL)
>> + , _vio_serial (NULL)
>> + , _read_pos (0)
>> + , _write_pos (0)
>> , _logon_desktop (false)
>> , _display_setting_initialized (false)
>> , _client_caps (NULL)
>> @@ -204,7 +203,10 @@ VDAgent::VDAgent()
>> swprintf_s(log_path, MAX_PATH, VD_AGENT_LOG_PATH, temp_path);
>> _log = VDLog::get(log_path);
>> }
>> - ZeroMemory(&_input, sizeof(INPUT));
>> + ZeroMemory(&_input, sizeof(_input));
>> + ZeroMemory(&_read_overlapped, sizeof(_read_overlapped));
>> + ZeroMemory(&_write_overlapped, sizeof(_write_overlapped));
>> + ZeroMemory(_read_buf, sizeof(_read_buf));
>> MUTEX_INIT(_control_mutex);
>> MUTEX_INIT(_message_mutex);
>>
>> @@ -213,7 +215,6 @@ VDAgent::VDAgent()
>>
>> VDAgent::~VDAgent()
>> {
>> - delete _events;
>> delete _log;
>> delete[] _client_caps;
>> }
>> @@ -277,8 +278,13 @@ bool VDAgent::run()
>> if (_desktop_layout->get_display_count() == 0) {
>> vd_printf("No QXL devices!");
>> }
>> - if (!init_vdi_port()) {
>> - vd_printf("Failed to create VDIPort instance");
>> + if (!init_vio_serial()) {
>> + cleanup();
>> + return false;
>> + }
>> + if (!ReadFileEx(_vio_serial, _read_buf, sizeof(VDIChunk), 
>> &_read_overlapped, read_completion) &&
>> + GetLastError() != ERROR_IO_PENDING) {
>> + vd_printf("vio_serial read error %lu", GetLastError());
>> cleanup();
>> return false;
>> }
>> @@ -290,12 +296,6 @@ bool VDAgent::run()
>> return false;
>> }
>> send_announce_capabilities(true);
>> -
>> - _events_count = VD_STATIC_EVENTS_COUNT + _vdi_port->get_num_events();
>> - _events = new HANDLE[_events_count];
>> - ZeroMemory(_events, _events_count);
>> - _events[VD_EVENT_CONTROL] = _control_event;
>> - _vdi_port->fill_events(&_events[VD_STATIC_EVENTS_COUNT]);
>> vd_printf("Connected to server");
>>
>> while (_running) {
>> @@ -313,7 +313,7 @@ bool VDAgent::run()
>> void VDAgent::cleanup()
>> {
>> CloseHandle(_control_event);
>> - delete _vdi_port;
>> + CloseHandle(_vio_serial);
>> delete _desktop_layout;
>> }
>>
>> @@ -431,52 +431,23 @@ void VDAgent::event_dispatcher(DWORD timeout, 
>> DWORD wake_mask)
>> DWORD wait_ret;
>> MSG msg;
>>
>> - int cont_read = _vdi_port->read();
>> - int cont_write = _vdi_port->write();
>> - bool cont = false;
>> -
>> - if (cont_read >= 0 && cont_write >= 0) {
>> - cont = cont_read || cont_write;
>> - } else if (cont_read == VDI_PORT_ERROR || cont_write == 
>> VDI_PORT_ERROR) {
>> - vd_printf("VDI Port error, read %d write %d", cont_read, cont_write);
>> - _running = false;
>> - return;
>> - } else if (cont_read == VDI_PORT_RESET || cont_write == 
>> VDI_PORT_RESET) {
>> - vd_printf("VDI Port reset, read %d write %d", cont_read, cont_write);
>> - _running = false;
>> - return;
>> - }
>> - if (cont_read) {
>> - handle_port_in();
>> - }
>> - if (cont_write) {
>> - handle_port_out();
>> - }
>> -
>> - wait_ret = MsgWaitForMultipleObjects(_events_count, _events, FALSE, 
>> cont ? 0 : timeout,
>> - wake_mask);
>> -
>> - if (wake_mask && wait_ret == WAIT_OBJECT_0 + _events_count) {
>> + wait_ret = MsgWaitForMultipleObjectsEx(1, &_control_event, timeout, 
>> wake_mask, MWMO_ALERTABLE);
>> + switch (wait_ret) {
>> + case WAIT_OBJECT_0:
>> + handle_control_event();
>> + break;
>> + case WAIT_OBJECT_0 + 1:
>> while (PeekMessage(&msg, NULL, 0, 0, PM_REMOVE)) {
>> TranslateMessage(&msg);
>> DispatchMessage(&msg);
>> }
>> - return;
>> - }
>> - switch (wait_ret) {
>> - case WAIT_OBJECT_0 + VD_EVENT_CONTROL:
>> - handle_control_event();
>> break;
>> + case WAIT_IO_COMPLETION:
>> case WAIT_TIMEOUT:
>> break;
>> default:
>> - DWORD vdi_event = wait_ret - VD_STATIC_EVENTS_COUNT - WAIT_OBJECT_0;
>> - if (vdi_event >= 0 && vdi_event < _vdi_port->get_num_events()) {
>> - _running = _vdi_port->handle_event(vdi_event);
>> - } else {
>> - vd_printf("MsgWaitForMultipleObjectsEx failed: %lu %lu", wait_ret, 
>> GetLastError());
>> - _running = false;
>> - }
>> + vd_printf("MsgWaitForMultipleObjectsEx failed: %lu %lu", wait_ret, 
>> GetLastError());
>> + _running = false;
>> }
>> }
>>
>> @@ -1145,29 +1116,15 @@ void VDAgent::set_clipboard_owner(int new_owner)
>> _clipboard_owner = new_owner;
>> }
>>
>> -VDIPort *create_virtio_vdi_port()
>> -{
>> - return new VirtioVDIPort();
>> -}
>> -
>> -VDIPort *create_pci_vdi_port()
>> +bool VDAgent::init_vio_serial()
>> {
>> - return new PCIVDIPort();
>> -}
>> -
>> -bool VDAgent::init_vdi_port()
>> -{
>> - VDIPort* (*creators[])(void) = { create_virtio_vdi_port, 
>> create_pci_vdi_port };
>> -
>> - for (unsigned int i = 0 ; i < sizeof(creators)/sizeof(creators[0]); 
>> ++i) {
>> - _vdi_port = creators[i]();
>> - if (_vdi_port->init()) {
>> - return true;
>> - }
>> - delete _vdi_port;
>> + _vio_serial = CreateFile(VIOSERIAL_PORT_PATH, GENERIC_READ | 
>> GENERIC_WRITE , 0, NULL,
>> + OPEN_EXISTING, FILE_FLAG_OVERLAPPED, NULL);
>> + if (_vio_serial == INVALID_HANDLE_VALUE) {
>> + vd_printf("Failed opening %ls, error %u", VIOSERIAL_PORT_PATH, 
>> GetLastError());
>> + return false;
>> }
>> - _vdi_port = NULL;
>> - return false;
>> + return true;
>> }
>>
>> void VDAgent::dispatch_message(VDAgentMessage* msg, uint32_t port)
>> @@ -1213,37 +1170,42 @@ void 
>> VDAgent::dispatch_message(VDAgentMessage* msg, uint32_t port)
>> }
>> }
>>
>> -void VDAgent::handle_port_in()
>> +VOID VDAgent::read_completion(DWORD err, DWORD bytes, LPOVERLAPPED 
>> overlapped)
>> {
>> - static char buf[sizeof(VDIChunk) + VD_AGENT_MAX_DATA_SIZE] = {0, };
>> - VDIChunk* chunk = (VDIChunk*)buf;
>> - uint32_t chunk_size;
>> -
>> - while (_running) {
>> - if (!chunk->hdr.size && _vdi_port->read_ring_size() >= 
>> sizeof(VDIChunk)) {
>> - if (_vdi_port->ring_read(chunk, sizeof(VDIChunk)) != 
>> sizeof(VDIChunk)) {
>> - vd_printf("ring_read of chunk header failed");
>> - _running = false;
>> - break;
>> - }
>> - if (sizeof(VDIChunk) + chunk->hdr.size > sizeof(buf)) {
>> - vd_printf("chunk is too large, size %u port %u", chunk->hdr.size, 
>> chunk->hdr.port);
>> - _running = false;
>> - break;
>> - }
>> - }
>> - chunk_size = chunk->hdr.size;
>> - if (!chunk_size || _vdi_port->read_ring_size() < chunk_size) {
>> - break;
>> - }
>> - if (_vdi_port->ring_read(chunk->data, chunk_size) != chunk_size) {
>> - vd_printf("ring_read of chunk data failed");
>> - _running = false;
>> - break;
>> + VDAgent* a = _singleton;
>> + VDIChunk* chunk = (VDIChunk*)a->_read_buf;
>> + DWORD count;
>> +
>> + if (err != 0 && err != ERROR_OPERATION_ABORTED && err != 
>> ERROR_NO_SYSTEM_RESOURCES) {
>> + vd_printf("vio_serial read completion error %lu", err);
>> + a->_running = false;
>> + return;
>> + }
>> +
>> + a->_read_pos += bytes;
>> + if (a->_read_pos < sizeof(VDIChunk)) {
>> + count = sizeof(VDIChunk) - a->_read_pos;
>> + } else if (a->_read_pos == sizeof(VDIChunk)) {
>> + count = chunk->hdr.size;
>> + if (a->_read_pos + count > sizeof(a->_read_buf)) {
>> + vd_printf("chunk is too large, size %u port %u", chunk->hdr.size, 
>> chunk->hdr.port);
>> + a->_running = false;
>> + return;
>> }
>> - handle_chunk(chunk);
>> - chunk->hdr.size = 0;
>> - }
>> + } else if (a->_read_pos == sizeof(VDIChunk) + chunk->hdr.size){
>> + a->handle_chunk(chunk);
>> + count = sizeof(VDIChunk);
>> + a->_read_pos = 0;
>> + } else {
>> + ASSERT(a->_read_pos < sizeof(VDIChunk) + chunk->hdr.size);
>> + count = sizeof(VDIChunk) + chunk->hdr.size - a->_read_pos;
>> + }
>> +
>> + if (!ReadFileEx(a->_vio_serial, a->_read_buf + a->_read_pos, count, 
>> overlapped,
>> + read_completion) && GetLastError() != ERROR_IO_PENDING) {
>> + vd_printf("vio_serial read error %lu", GetLastError());
>> + a->_running = false;
>> + }
>> }
>>
>> void VDAgent::handle_chunk(VDIChunk* chunk)
>> @@ -1289,24 +1251,39 @@ void VDAgent::cleanup_in_msg()
>> _in_msg = NULL;
>> }
>>
>> -void VDAgent::handle_port_out()
>> +void VDAgent::write_completion(DWORD err, DWORD bytes, LPOVERLAPPED 
>> overlapped)
>> {
>> - MUTEX_LOCK(_message_mutex);
>> - while (_running && !_message_queue.empty()) {
>> - VDIChunk* chunk = _message_queue.front();
>> - DWORD size = sizeof(VDIChunk) + chunk->hdr.size;
>> + VDAgent* a = _singleton;
>> + VDIChunk* chunk;
>> + DWORD count;
>>
>> - if (size > _vdi_port->write_ring_free_space()) {
>> - break;
>> + ASSERT(!a->_message_queue.empty());
>> + if (err != 0) {
>> + vd_printf("vio_serial write completion error %lu", err);
>> + a->_running = false;
>> + return;
>> + }
>> + MUTEX_LOCK(a->_message_mutex);
>> + a->_write_pos += bytes;
>> + chunk = a->_message_queue.front();
>> + count = sizeof(VDIChunk) + chunk->hdr.size - a->_write_pos;
>> + if (count == 0) {
>> + a->_message_queue.pop();
>> + a->_write_pos = 0;
>> + delete chunk;
>> + if (!a->_message_queue.empty()) {
>> + chunk = a->_message_queue.front();
>> + count = sizeof(VDIChunk) + chunk->hdr.size;
>> }
>> - _message_queue.pop();
>> - if (_vdi_port->ring_write(chunk, size) != size) {
>> - vd_printf("ring_write failed");
>> - _running = false;
>> + }
>> + if (count) {
>> + if (!WriteFileEx(a->_vio_serial, (char*)chunk + a->_write_pos, 
>> count, overlapped,
>> + write_completion) && GetLastError() != ERROR_IO_PENDING) {
>> + vd_printf("vio_serial write error %lu", GetLastError());
>> + a->_running = false;
>> }
>> - delete chunk;
>> }
>> - MUTEX_UNLOCK(_message_mutex);
>> + MUTEX_UNLOCK(a->_message_mutex);
>> }
>>
>> VDIChunk* VDAgent::new_chunk(DWORD bytes)
>> @@ -1318,8 +1295,10 @@ void VDAgent::enqueue_chunk(VDIChunk* chunk)
>> {
>> MUTEX_LOCK(_message_mutex);
>> _message_queue.push(chunk);
>> + if (_message_queue.size() == 1) {
>> + write_completion(0, 0, &_write_overlapped);
>> + }
>> MUTEX_UNLOCK(_message_mutex);
>> - handle_port_out();
>> }
>>
>> LRESULT CALLBACK VDAgent::wnd_proc(HWND hwnd, UINT message, WPARAM 
>> wparam, LPARAM lparam)
>
> _______________________________________________
> Spice-devel mailing list
> Spice-devel at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/spice-devel