[Spice-devel] [PATCH spice 9/9] spicec: add controller

Mon Oct 18 00:25:48 PDT 2010

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
>>> +
>>> +#endif
>>
>> another 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