From e66f3671002ee041cd4ca9ac3ee69be7278aa1d9 Mon Sep 17 00:00:00 2001 From: Arnon Gilboa Date: Tue, 23 Mar 2010 16:20:18 +0200 Subject: spice: client: fix controller & foreign menu review comments #558247 --- client/application.cpp | 85 +++++++++++++++++++++++++++--------------------- client/application.h | 23 ++++++++++--- client/controller.cpp | 38 +++++++++++++++------- client/controller.h | 10 ++++-- client/controller_prot.h | 4 +++ client/foreign_menu.cpp | 50 ++++++++++++++++++++++------ client/foreign_menu.h | 11 +++++-- 7 files changed, 152 insertions(+), 69 deletions(-) (limited to 'client') diff --git a/client/application.cpp b/client/application.cpp index 52b0be63..0c0c0f4d 100644 --- a/client/application.cpp +++ b/client/application.cpp @@ -298,14 +298,10 @@ enum AppCommands { APP_CMD_CONNECT, APP_CMD_DISCONNECT, #endif - APP_CMD_FOREIGN_MENU_MASK = 0x01000000, - APP_CMD_CONTROLLER_MENU_MASK = 0x02000000, + APP_CMD_EXTERNAL_BEGIN = 0x400, + APP_CMD_EXTERNAL_END = 0x800, }; -#define MENU_ID_MASK 0x000000ff -#define MENU_CONN_MASK 0x0000ffff -#define MENU_CONN_SHIFT 8 - Application::Application() : ProcessLoop (this) , _client (*this) @@ -821,35 +817,56 @@ void Application::do_command(int command) break; #endif default: - int32_t conn_ref = _pipe_connections[(command >> MENU_CONN_SHIFT) & MENU_CONN_MASK]; - if ((command & APP_CMD_FOREIGN_MENU_MASK) == APP_CMD_FOREIGN_MENU_MASK) { + AppMenuItemMap::iterator iter = _app_menu_items.find(command); + ASSERT(iter != _app_menu_items.end()); + AppMenuItem* item = &(*iter).second; + if (item->type == APP_MENU_ITEM_TYPE_FOREIGN) { ASSERT(*_foreign_menu); - (*_foreign_menu)->on_command(conn_ref, command & MENU_ID_MASK); - } else if ((command & APP_CMD_CONTROLLER_MENU_MASK) == APP_CMD_CONTROLLER_MENU_MASK) { + (*_foreign_menu)->on_command(item->conn_ref, item->ext_id); + } else if (item->type == APP_MENU_ITEM_TYPE_CONTROLLER) { ASSERT(*_controller); - (*_controller)->on_command(conn_ref, command & MENU_ID_MASK); + (*_controller)->on_command(item->conn_ref, item->ext_id); } } } -void Application::add_foreign_menu(int32_t opaque_conn_ref, Menu* sub_menu) +int Application::get_menu_item_id(AppMenuItemType type, int32_t conn_ref, uint32_t ext_id) { - _pipe_connections[opaque_conn_ref & MENU_CONN_MASK] = opaque_conn_ref; - (*_app_menu)->add_sub(sub_menu); - update_menu(); + int free_id = APP_CMD_EXTERNAL_BEGIN; + AppMenuItem item = {type, conn_ref, ext_id}; + AppMenuItemMap::iterator iter = _app_menu_items.begin(); + for (; iter != _app_menu_items.end(); iter++) { + if (!memcmp(&(*iter).second, &item, sizeof(item))) { + return (*iter).first; + } else if (free_id == (*iter).first && ++free_id > APP_CMD_EXTERNAL_END) { + return APP_CMD_INVALID; + } + } + _app_menu_items[free_id] = item; + return free_id; } -void Application::delete_foreign_menu(int32_t opaque_conn_ref, Menu* sub_menu) +void Application::clear_menu_items(int32_t opaque_conn_ref) { - _pipe_connections.erase(opaque_conn_ref & MENU_CONN_MASK); - (*_app_menu)->remove_sub(sub_menu); - update_menu(); + AppMenuItemMap::iterator iter = _app_menu_items.begin(); + AppMenuItemMap::iterator curr; + + while (iter != _app_menu_items.end()) { + curr = iter++; + if (((*curr).second).conn_ref == opaque_conn_ref) { + _app_menu_items.erase(curr); + } + } +} + +void Application::remove_menu_item(int item_id) +{ + _app_menu_items.erase(item_id); } int Application::get_foreign_menu_item_id(int32_t opaque_conn_ref, uint32_t msg_id) { - return APP_CMD_FOREIGN_MENU_MASK | ((opaque_conn_ref & MENU_CONN_MASK) << MENU_CONN_SHIFT) | - (msg_id & MENU_ID_MASK); + return get_menu_item_id(APP_MENU_ITEM_TYPE_FOREIGN, opaque_conn_ref, msg_id); } void Application::update_menu() @@ -861,17 +878,6 @@ void Application::update_menu() } } -void Application::add_controller(int32_t opaque_conn_ref) -{ - _pipe_connections[opaque_conn_ref & MENU_CONN_MASK] = opaque_conn_ref; -} - -void Application::delete_controller(int32_t opaque_conn_ref) -{ - _pipe_connections.erase(opaque_conn_ref & MENU_CONN_MASK); - delete_menu(); -} - bool Application::connect(const std::string& host, int port, int sport, const std::string& password) { _client.set_target(host, port, sport); @@ -914,20 +920,25 @@ void Application::set_hotkeys(const std::string& hotkeys) int Application::get_controller_menu_item_id(int32_t opaque_conn_ref, uint32_t msg_id) { - return APP_CMD_CONTROLLER_MENU_MASK | ((opaque_conn_ref & MENU_CONN_MASK) << MENU_CONN_SHIFT) | - (msg_id & MENU_ID_MASK); + return get_menu_item_id(APP_MENU_ITEM_TYPE_CONTROLLER, opaque_conn_ref, msg_id); } void Application::set_menu(Menu* menu) { - _app_menu.reset(menu->ref()); + if (menu) { + _app_menu.reset(menu->ref()); + } else { + init_menu(); + } + if (*_foreign_menu) { + (*_foreign_menu)->add_sub_menus(); + } update_menu(); } void Application::delete_menu() { - init_menu(); - update_menu(); + set_menu(NULL); } #ifdef REDKEY_DEBUG diff --git a/client/application.h b/client/application.h index 62a9d129..97b2f9b7 100644 --- a/client/application.h +++ b/client/application.h @@ -128,6 +128,20 @@ public: virtual void response(AbstractProcessLoop& events_loop); }; +enum AppMenuItemType { + APP_MENU_ITEM_TYPE_INVALID, + APP_MENU_ITEM_TYPE_FOREIGN, + APP_MENU_ITEM_TYPE_CONTROLLER, +}; + +typedef struct AppMenuItem { + AppMenuItemType type; + int32_t conn_ref; + uint32_t ext_id; +} AppMenuItem; + +typedef std::map AppMenuItemMap; + class Application : public ProcessLoop, public Platform::EventListener, public Platform::DisplayModeListner, @@ -186,13 +200,11 @@ public: Menu* get_app_menu(); virtual void do_command(int command); - void add_foreign_menu(int32_t opaque_conn_ref, Menu* sub_menu); - void delete_foreign_menu(int32_t opaque_conn_ref, Menu* sub_menu); int get_foreign_menu_item_id(int32_t opaque_conn_ref, uint32_t msg_id); + void clear_menu_items(int32_t opaque_conn_ref); + void remove_menu_item(int item_id); void update_menu(); - void add_controller(int32_t opaque_conn_ref); - void delete_controller(int32_t opaque_conn_ref); bool connect(const std::string& host, int port, int sport, const std::string& password); void show_me(bool full_screen, bool auto_display_res); void hide_me(); @@ -244,6 +256,7 @@ private: void send_command_hotkey(int command); void send_hotkey_key_set(const HotkeySet& key_set); void menu_item_callback(unsigned int item_id); + int get_menu_item_id(AppMenuItemType type, int32_t conn_ref, uint32_t ext_id); int get_hotkeys_commnad(); bool is_key_set_pressed(const HotkeySet& key_set); bool is_cad_pressed(); @@ -298,7 +311,7 @@ private: AutoRef _foreign_menu; bool _enable_controller; AutoRef _controller; - std::map _pipe_connections; + AppMenuItemMap _app_menu_items; }; #endif diff --git a/client/controller.cpp b/client/controller.cpp index 248a373f..d9774cfd 100644 --- a/client/controller.cpp +++ b/client/controller.cpp @@ -41,7 +41,7 @@ Controller::Controller(ControllerInterface *handler) { char pipe_name[PIPE_NAME_MAX_LEN]; - ASSERT(_handler != NULL); + ASSERT(_handler); snprintf(pipe_name, PIPE_NAME_MAX_LEN, PIPE_NAME, Platform::get_process_id()); LOG_INFO("Creating a controller connection %s", pipe_name); _pipe = NamedPipe::create(pipe_name, *this); @@ -52,9 +52,9 @@ Controller::Controller(ControllerInterface *handler) Controller::~Controller() { - _handler = NULL; std::map::const_iterator conn; for (conn = _connections.begin(); conn != _connections.end(); ++conn) { + conn->second->reset_handler(); delete conn->second; } if (_pipe) { @@ -131,8 +131,9 @@ ControllerConnection::~ControllerConnection() if (_opaque != NamedPipe::INVALID_CONNECTION) { NamedPipe::destroy_connection(_opaque); } - if (_parent.handler_attached()) { - _handler->delete_controller(_opaque); + if (_handler) { + _handler->clear_menu_items(_opaque); + _handler->delete_menu(); } } @@ -159,7 +160,7 @@ bool ControllerConnection::read_msgs() size_t nread = _read_pos - _read_buf; int32_t size; - ASSERT(_handler != NULL); + ASSERT(_handler); ASSERT(_opaque != NamedPipe::INVALID_CONNECTION); size = NamedPipe::read(_opaque, (uint8_t*)_read_pos, sizeof(_read_buf) - nread); if (size == 0) { @@ -215,18 +216,24 @@ bool ControllerConnection::read_msgs() bool ControllerConnection::write_msg(const void *buf, int len) { + RecurciveLock lock(_write_lock); uint8_t *pos; int32_t written = 0; ASSERT(_opaque != NamedPipe::INVALID_CONNECTION); if (_write_pending && buf != _write_pos) { - if (_write_pending + len > sizeof(_write_buf)) { - DBG(0, "Dropping msg due to pending write %d", _write_pending); + if ((_write_pos + _write_pending + len > _write_buf + sizeof(_write_buf)) && + !write_msg(_write_pos, _write_pending)) { return false; } - memcpy(_write_buf + _write_pending, buf, len); - _write_pending += len; - return true; + if (_write_pending) { + if (_write_pos + _write_pending + len > _write_buf + sizeof(_write_buf)) { + DBG(0, "Dropping message, due to insufficient space in write buffer"); + return true; + } + memcpy(_write_pos + _write_pending, buf, len); + _write_pending += len; + } } pos = (uint8_t*)buf; while (len && (written = NamedPipe::write(_opaque, pos, len)) > 0) { @@ -253,6 +260,7 @@ bool ControllerConnection::write_msg(const void *buf, int len) bool ControllerConnection::handle_init(ControllerInit *init) { + ASSERT(_handler); if (init->credentials != 0) { LOG_ERROR("Controller menu has wrong credentials 0x%x", init->credentials); return false; @@ -260,7 +268,6 @@ bool ControllerConnection::handle_init(ControllerInit *init) if (!_parent.set_exclusive(init->flags & CONTROLLER_FLAG_EXCLUSIVE)) { return false; } - _handler->add_controller(_opaque); return true; } @@ -269,6 +276,7 @@ bool ControllerConnection::handle_message(ControllerMsg *hdr) uint32_t value = ((ControllerValue*)hdr)->value; char *data = (char*)((ControllerData*)hdr)->data; + ASSERT(_handler); switch (hdr->id) { case CONTROLLER_HOST: _host.assign(data); @@ -285,6 +293,12 @@ bool ControllerConnection::handle_message(ControllerMsg *hdr) case CONTROLLER_SECURE_CHANNELS: case CONTROLLER_DISABLE_CHANNELS: return set_multi_val(hdr->id, data); + case CONTROLLER_TLS_CIPHERS: + return _handler->set_connection_ciphers(data, "Controller"); + case CONTROLLER_CA_FILE: + return _handler->set_ca_file(data, "Controller"); + case CONTROLLER_HOST_SUBJECT: + return _handler->set_host_cert_subject(data, "Controller"); case CONTROLLER_FULL_SCREEN: _full_screen = !!(value & CONTROLLER_SET_FULL_SCREEN); _auto_display_res = !!(value & CONTROLLER_AUTO_DISPLAY_RES); @@ -342,6 +356,7 @@ bool ControllerConnection::create_menu(wchar_t* resource) int state; int id; + ASSERT(_handler); AutoRef app_menu(_handler->get_app_menu()); AutoRef menu(new Menu((*app_menu)->get_target(), "")); wchar_t* item = next_tok(resource, CONTROLLER_MENU_ITEM_DELIMITER, &item_state); @@ -406,6 +421,7 @@ bool ControllerConnection::set_multi_val(uint32_t op, char* multi_val) char* argv[] = {NULL, (char*)"--set", multi_val}; char* val; + ASSERT(_handler); parser.add(id, "set", "none", "none", true); parser.set_multi(id, ','); parser.begin(3, argv); diff --git a/client/controller.h b/client/controller.h index cd1ead93..f59b5c18 100644 --- a/client/controller.h +++ b/client/controller.h @@ -19,6 +19,7 @@ #define _H_CONTROLLER_MENU #include "named_pipe.h" +#include "threads.h" class ControllerConnection; struct ControllerInit; @@ -30,8 +31,6 @@ class ControllerInterface { public: virtual ~ControllerInterface() {} - virtual void add_controller(int32_t opaque_conn_ref) = 0; - virtual void delete_controller(int32_t opaque_conn_ref) = 0; virtual bool connect(const std::string& host, int port, int sport, const std::string& password) = 0; virtual void set_title(const std::wstring& title) = 0; @@ -41,8 +40,12 @@ public: const char* arg0) = 0; virtual bool set_enable_channels(CmdLineParser& parser, bool enable, char *val, const char* arg0) = 0; + virtual bool set_connection_ciphers(const char* ciphers, const char* arg0) = 0; + virtual bool set_ca_file(const char* ca_file, const char* arg0) = 0; + virtual bool set_host_cert_subject(const char* subject, const char* arg0) = 0; virtual void set_hotkeys(const std::string& hotkeys) = 0; virtual int get_controller_menu_item_id(int32_t opaque_conn_ref, uint32_t id) = 0; + virtual void clear_menu_items(int32_t opaque_conn_ref) = 0; virtual Menu* get_app_menu() = 0; virtual void set_menu(Menu* menu) = 0; virtual void delete_menu() = 0; @@ -53,7 +56,6 @@ public: Controller(ControllerInterface *handler); virtual ~Controller(); - bool handler_attached() { return !!_handler;} Controller* ref() { _refs++; return this;} void unref() { if (!--_refs) delete this;} @@ -81,6 +83,7 @@ public: virtual void bind(NamedPipe::ConnectionRef conn_ref); virtual void on_data(); bool write_msg(const void *buf, int len); + void reset_handler() { _handler = NULL;} private: bool read_msgs(); @@ -99,6 +102,7 @@ private: uint8_t *_read_pos; uint8_t _write_buf[CONTROLLER_BUF_SIZE]; uint8_t _read_buf[CONTROLLER_BUF_SIZE]; + RecurciveMutex _write_lock; std::string _host; std::string _password; diff --git a/client/controller_prot.h b/client/controller_prot.h index bd62577e..2091ecb5 100644 --- a/client/controller_prot.h +++ b/client/controller_prot.h @@ -58,6 +58,10 @@ enum { CONTROLLER_SECURE_CHANNELS, CONTROLLER_DISABLE_CHANNELS, + CONTROLLER_TLS_CIPHERS, + CONTROLLER_CA_FILE, + CONTROLLER_HOST_SUBJECT, + CONTROLLER_FULL_SCREEN, CONTROLLER_SET_TITLE, diff --git a/client/foreign_menu.cpp b/client/foreign_menu.cpp index db0badec..3298d638 100644 --- a/client/foreign_menu.cpp +++ b/client/foreign_menu.cpp @@ -51,9 +51,9 @@ ForeignMenu::ForeignMenu(ForeignMenuInterface *handler) ForeignMenu::~ForeignMenu() { - _handler = NULL; std::map::const_iterator conn; for (conn = _connections.begin(); conn != _connections.end(); ++conn) { + conn->second->reset_handler(); delete conn->second; } if (_foreign_menu) { @@ -87,6 +87,14 @@ void ForeignMenu::remove_connection(NamedPipe::ConnectionRef conn_ref) delete conn; } +void ForeignMenu::add_sub_menus() +{ + std::map::const_iterator conn; + for (conn = _connections.begin(); conn != _connections.end(); ++conn) { + conn->second->add_sub_menu(); + } +} + void ForeignMenu::on_command(NamedPipe::ConnectionRef conn_ref, int32_t id) { ForeignMenuConnection *conn = _connections[conn_ref]; @@ -144,8 +152,11 @@ ForeignMenuConnection::~ForeignMenuConnection() if (_opaque != NamedPipe::INVALID_CONNECTION) { NamedPipe::destroy_connection(_opaque); } - if (_parent.handler_attached()) { - _handler->delete_foreign_menu(_opaque, _sub_menu); + if (_handler) { + AutoRef app_menu(_handler->get_app_menu()); + (*app_menu)->remove_sub(_sub_menu); + _handler->update_menu(); + _handler->clear_menu_items(_opaque); } if (_sub_menu) { _sub_menu->unref(); @@ -175,7 +186,7 @@ bool ForeignMenuConnection::read_msgs() size_t nread = _read_pos - _read_buf; int32_t size; - ASSERT(_handler != NULL); + ASSERT(_handler); ASSERT(_opaque != NamedPipe::INVALID_CONNECTION); size = NamedPipe::read(_opaque, (uint8_t*)_read_pos, sizeof(_read_buf) - nread); if (size == 0) { @@ -231,18 +242,24 @@ bool ForeignMenuConnection::read_msgs() bool ForeignMenuConnection::write_msg(const void *buf, int len) { + RecurciveLock lock(_write_lock); uint8_t *pos; int32_t written = 0; ASSERT(_opaque != NamedPipe::INVALID_CONNECTION); if (_write_pending && buf != _write_pos) { - if (_write_pending + len > sizeof(_write_buf)) { - DBG(0, "Dropping msg due to pending write %d", _write_pending); + if ((_write_pos + _write_pending + len > _write_buf + sizeof(_write_buf)) && + !write_msg(_write_pos, _write_pending)) { return false; } - memcpy(_write_buf + _write_pending, buf, len); - _write_pending += len; - return true; + if (_write_pending) { + if (_write_pos + _write_pending + len > _write_buf + sizeof(_write_buf)) { + DBG(0, "Dropping message, due to insufficient space in write buffer"); + return true; + } + memcpy(_write_pos + _write_pending, buf, len); + _write_pending += len; + } } pos = (uint8_t*)buf; while (len && (written = NamedPipe::write(_opaque, pos, len)) > 0) { @@ -271,6 +288,7 @@ bool ForeignMenuConnection::handle_init(FrgMenuInit *init) { std::string title = "Untitled"; + ASSERT(_handler); if (_sub_menu) { LOG_ERROR("Foreign menu already initialized"); return false; @@ -284,13 +302,23 @@ bool ForeignMenuConnection::handle_init(FrgMenuInit *init) title = (char*)init->title; } _sub_menu = new Menu((CommandTarget&)*_handler, title); - _handler->add_foreign_menu(_opaque, _sub_menu); + add_sub_menu(); + _handler->update_menu(); return true; } +void ForeignMenuConnection::add_sub_menu() +{ + if (_sub_menu) { + AutoRef app_menu(_handler->get_app_menu()); + (*app_menu)->add_sub(_sub_menu); + } +} + bool ForeignMenuConnection::handle_message(FrgMenuMsg *hdr) { ASSERT(_sub_menu); + ASSERT(_handler); switch (hdr->id) { case FOREIGN_MENU_SET_TITLE: ((char*)hdr)[hdr->size - 1] = '\0'; @@ -306,10 +334,12 @@ bool ForeignMenuConnection::handle_message(FrgMenuMsg *hdr) case FOREIGN_MENU_REMOVE_ITEM: { int id = _handler->get_foreign_menu_item_id(_opaque, ((FrgMenuRmItem*)hdr)->id); _sub_menu->remove_command(id); + _handler->remove_menu_item(id); break; } case FOREIGN_MENU_CLEAR: _sub_menu->clear(); + _handler->clear_menu_items(_opaque); break; case FOREIGN_MENU_MODIFY_ITEM: default: diff --git a/client/foreign_menu.h b/client/foreign_menu.h index b0f1a8ca..01d5280e 100644 --- a/client/foreign_menu.h +++ b/client/foreign_menu.h @@ -29,9 +29,10 @@ class ForeignMenuInterface : public CommandTarget { public: virtual ~ForeignMenuInterface() {} - virtual void add_foreign_menu(int32_t opaque_conn_ref, Menu* sub_menu) = 0; - virtual void delete_foreign_menu(int32_t opaque_conn_ref, Menu* sub_menu) = 0; virtual int get_foreign_menu_item_id(int32_t opaque_conn_ref, uint32_t msg_id) = 0; + virtual void clear_menu_items(int32_t opaque_conn_ref) = 0; + virtual void remove_menu_item(int item_id) = 0; + virtual Menu* get_app_menu() = 0; virtual void update_menu() = 0; }; @@ -40,13 +41,13 @@ public: ForeignMenu(ForeignMenuInterface *handler); virtual ~ForeignMenu(); - bool handler_attached() { return !!_handler;} ForeignMenu* ref() { _refs++; return this;} void unref() { if (!--_refs) delete this;} virtual NamedPipe::ConnectionInterface &create(); void add_connection(NamedPipe::ConnectionRef conn_ref, ForeignMenuConnection *conn); void remove_connection(NamedPipe::ConnectionRef conn_ref); + void add_sub_menus(); void on_command(NamedPipe::ConnectionRef conn_ref, int32_t id); void on_activate(); void on_deactivate(); @@ -68,9 +69,12 @@ class ForeignMenuConnection : public NamedPipe::ConnectionInterface { public: ForeignMenuConnection(ForeignMenuInterface *handler, ForeignMenu& parent); virtual ~ForeignMenuConnection(); + virtual void bind(NamedPipe::ConnectionRef conn_ref); virtual void on_data(); bool write_msg(const void *buf, int len); + void reset_handler() { _handler = NULL;} + void add_sub_menu(); private: bool read_msgs(); @@ -88,6 +92,7 @@ private: uint8_t *_read_pos; uint8_t _write_buf[FOREIGN_MENU_BUF_SIZE]; uint8_t _read_buf[FOREIGN_MENU_BUF_SIZE]; + RecurciveMutex _write_lock; }; #endif -- cgit