Hi, On 10/18/2010 09:08 AM, Arnon Gilboa wrote:
Hans de Goede wrote:Hi,See 1 comment inline. On 10/17/2010 04:13 PM, Arnon Gilboa wrote: <snip>@@ -2026,7 +2121,7 @@ void Application::register_channels() bool Application::process_cmd_line(int argc, char** argv) { - std::string host; + std::string host = ""; int sport = -1; int port = -1; bool auto_display_res = false; @@ -2050,6 +2145,7 @@ bool Application::process_cmd_line(int argc, char** argv) SPICE_OPT_CANVAS_TYPE, SPICE_OPT_DISPLAY_COLOR_DEPTH, SPICE_OPT_DISABLE_DISPLAY_EFFECTS, + SPICE_OPT_CONTROLLER, }; #ifdef USE_GUI @@ -2068,7 +2164,6 @@ bool Application::process_cmd_line(int argc, char** argv) CmdLineParser parser("Spice client", false); parser.add(SPICE_OPT_HOST, "host", "spice server address", "host", true, 'h'); - parser.set_reqired(SPICE_OPT_HOST); parser.add(SPICE_OPT_PORT, "port", "spice server port", "port", true, 'p'); parser.add(SPICE_OPT_SPORT, "secure-port", "spice server secure port", "port", true, 's'); parser.add(SPICE_OPT_SECURE_CHANNELS, "secure-channels", @@ -2107,6 +2202,8 @@ bool Application::process_cmd_line(int argc, char** argv) "disable guest display effects", "wallpaper/font-smooth/animation/all", true); parser.set_multi(SPICE_OPT_DISABLE_DISPLAY_EFFECTS, ','); + parser.add(SPICE_OPT_CONTROLLER, "controller", "enable external controller"); + for (int i = SPICE_CHANNEL_MAIN; i< SPICE_END_CHANNEL; i++) { _peer_con_opt[i] = RedPeer::ConnectionOptions::CON_OP_INVALID; } @@ -2203,6 +2300,15 @@ bool Application::process_cmd_line(int argc, char** argv) return false; } break; + case SPICE_OPT_CONTROLLER: + if (argc> 2) { + Platform::term_printf("%s: controller cannot be combined with other options\n", + argv[0]); + _exit_code = SPICEC_ERROR_CODE_INVALID_ARG; + return false; + } + _enable_controller = true; + return true; case CmdLineParser::OPTION_HELP: parser.show_help(); return false; @@ -2214,42 +2320,21 @@ bool Application::process_cmd_line(int argc, char** argv) } } + if (host.empty()) { + Platform::term_printf("%s: missing --host\n", argv[0]); + return false; + } +This seems weird, first you only allow SPICE_OPT_CONTROLLER by itself, but now you expect there to be a hostname too ?No. if we see a --controller (SPICE_OPT_CONTROLLER) we allow NO other parameters and return immediately from process_cmd_line. We reach the hostname check only if we had no controller, and in this case a hostname is a must-have.
Oops, I missed the return in that case.
<snip>@@ -155,7 +157,8 @@ typedef std::map<int, AppMenuItem> AppMenuItemMap; class Application : public ProcessLoop, public Platform::EventListener, public Platform::DisplayModeListener, - public ForeignMenuInterface { + public ForeignMenuInterface, + public ControllerInterface { public: enum State {and ControllerInterface re-appears. I do believe it would be better to simply leave it in in the previous patch :)You mixed ControllerInterface with the old CommandTarget. See patch 8 comments reply.
Ok.
<snip>diff --git a/client/controller_prot.h b/client/controller_prot.h new file mode 100644 index 0000000..6cf4ca0 --- /dev/null +++ b/client/controller_prot.h @@ -0,0 +1,115 @@ +/* + Copyright (C) 2009 Red Hat, Inc. + + This library is free software; you can redistribute it and/or + modify it under the terms of the GNU Lesser General Public + License as published by the Free Software Foundation; either + version 2.1 of the License, or (at your option) any later version. + + This library is distributed in the hope that it will be useful, + but WITHOUT ANY WARRANTY; without even the implied warranty of + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + Lesser General Public License for more details. + + You should have received a copy of the GNU Lesser General Public + License along with this library; if not, see<http://www.gnu.org/licenses/>. +*/ + +#ifndef _H_CONTROLLER_PROT +#define _H_CONTROLLER_PROT + +#define CONTROLLER_MAGIC (*(uint32_t*)"CTRL") +#define CONTROLLER_VERSION 1 + +#ifdef __GNUC__ +#define ATTR_PACKED __attribute__ ((__packed__)) +#else +#define ATTR_PACKED __declspec(align(1)) +#endif + +typedef struct ATTR_PACKED ControllerInitHeader { + uint32_t magic; + uint32_t version; + uint32_t size; +} ControllerInitHeader; + +typedef struct ATTR_PACKED ControllerInit { + ControllerInitHeader base; + uint64_t credentials; + uint32_t flags; +} ControllerInit; + +enum { + CONTROLLER_FLAG_EXCLUSIVE = 1<< 0, +}; + +typedef struct ATTR_PACKED ControllerMsg { + uint32_t id; + uint32_t size; +} ControllerMsg; + +enum { + //extrenal app -> spice client + CONTROLLER_HOST = 1, + CONTROLLER_PORT, + CONTROLLER_SPORT, + CONTROLLER_PASSWORD, + + CONTROLLER_SECURE_CHANNELS, + CONTROLLER_DISABLE_CHANNELS, + + CONTROLLER_TLS_CIPHERS, + CONTROLLER_CA_FILE, + CONTROLLER_HOST_SUBJECT, + + CONTROLLER_FULL_SCREEN, + CONTROLLER_SET_TITLE, + + CONTROLLER_CREATE_MENU, + CONTROLLER_DELETE_MENU, + + CONTROLLER_HOTKEYS, + CONTROLLER_SEND_CAD, + + CONTROLLER_CONNECT, + CONTROLLER_SHOW, + CONTROLLER_HIDE, + + //spice client -> extrenal app + CONTROLLER_MENU_ITEM_CLICK = 1001, +}; + +#define CONTROLLER_TRUE (1<< 0) + +enum { + CONTROLLER_SET_FULL_SCREEN = CONTROLLER_TRUE, + CONTROLLER_AUTO_DISPLAY_RES = 1<< 1, +}; + +typedef struct ATTR_PACKED ControllerValue { + ControllerMsg base; + uint32_t value; +} ControllerValue; + +typedef struct ATTR_PACKED ControllerData { + ControllerMsg base; + uint8_t data[0]; +} ControllerData; + +#define CONTROLLER_MENU_ITEM_DELIMITER L"\n" +#define CONTROLLER_MENU_PARAM_DELIMITER L"\r" + +enum { + CONTROLLER_MENU_FLAGS_SEPARATOR = 1<< 0, + CONTROLLER_MENU_FLAGS_DISABLED = 1<< 1, + CONTROLLER_MENU_FLAGS_POPUP = 1<< 2, + CONTROLLER_MENU_FLAGS_CHECKED = 1<< 3, + CONTROLLER_MENU_FLAGS_GRAYED = 1<< 4, +}; + +#define SPICE_MENU_INTERNAL_ID_BASE 0x1300 +#define SPICE_MENU_INTERNAL_ID_SHIFT 8 + +#undef ATTR_PACKED + +#endifanother candidate for spice-protocol.As the previous reply ("client-side only" protocol), let's discuss this one with the guys, but basically I tend to agree with you.
Yes, lets discuss this on irc. Regards, Hans _______________________________________________ Spice-devel mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/spice-devel
