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

[Arnon Gilboa]

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.
>
> <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.
> <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.
>
> Regards,
>
> Hans
> _______________________________________________
> Spice-devel mailing list
> Spice-devel at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/spice-devel