[Spice-devel] [win-vd_agent PATCH] Check that client is capable of handling monitors_config from Windows guests.

[Christophe Fergeau]

Hey,

Forgot to comment on the patch at the same time as the log comments /o\
Looks good to me save for a few nits,

On Wed, Jul 29, 2015 at 05:21:15PM -0400, Sandy Stutsman wrote:
> If the client hasn't been updated to handle non multi-hat monitors_config,
> vd_agent will won't issue the MONITORS_CONFIG driver escape.
> 
> This addresses: rhbz#1248196
> ---
>  vdagent/desktop_layout.cpp | 4 ++++
>  vdagent/desktop_layout.h   | 5 +++--
>  vdagent/vdagent.cpp        | 4 ++++
>  3 files changed, 11 insertions(+), 2 deletions(-)
> 
> diff --git a/vdagent/desktop_layout.cpp b/vdagent/desktop_layout.cpp
> index 7a34f7f..db9047f 100644
> --- a/vdagent/desktop_layout.cpp
> +++ b/vdagent/desktop_layout.cpp
> @@ -35,6 +35,7 @@ void DisplayMode::set_res(DWORD width, DWORD height, DWORD depth)
>  DesktopLayout::DesktopLayout()
>      : _total_width (0)
>      , _total_height (0)
> +    , _configurable(0)

I'd use something a bit more explicit, maybe
'send_monitors_position' ? Fine with me if you want to stick with
'configurable'. Using 'false' would be better than '0' I think.

>  {
>      MUTEX_INIT(_mutex);
>      get_displays();
> @@ -365,6 +366,9 @@ bool DesktopLayout::update_monitor_config(LPCTSTR dev_name, DisplayMode* mode)
>      if (!mode || !mode->get_attached())
>          return false;
>  
> +    //Don't configure monitors unless the client supports it
> +    if(!_configurable) return FALSE;
> +
>      HDC hdc = CreateDC(dev_name, NULL, NULL, NULL);
>  
>      memset(&monitor_config, 0, sizeof(monitor_config));
> diff --git a/vdagent/desktop_layout.h b/vdagent/desktop_layout.h
> index 0e310e3..a2d32d4 100644
> --- a/vdagent/desktop_layout.h
> +++ b/vdagent/desktop_layout.h
> @@ -73,22 +73,23 @@ public:
>      size_t get_display_count() { return _displays.size();}
>      DWORD get_total_width() { return _total_width;}
>      DWORD get_total_height() { return _total_height;}
> -
> +    void set_configurable(bool flag) { _configurable = flag; }
>  private:
>      void clean_displays();
>      void normalize_displays_pos();
>      DisplayMode * get_primary_display();
> +    bool update_monitor_config(LPCTSTR dev_name, DisplayMode* mode);
>      static bool consistent_displays();
>      static bool is_attached(LPCTSTR dev_name);
>      static bool get_qxl_device_id(WCHAR* device_key, DWORD* device_id);
>      static bool init_dev_mode(LPCTSTR dev_name, DEVMODE* dev_mode, DisplayMode* mode,
>                                LONG normal_x, LONG normal_y, bool set_pos);
> -    static bool update_monitor_config(LPCTSTR dev_name, DisplayMode* mode);
>  private:
>      mutex_t _mutex;
>      Displays _displays;
>      DWORD _total_width;
>      DWORD _total_height;
> +    bool _configurable;
>  };
>  
>  #endif
> diff --git a/vdagent/vdagent.cpp b/vdagent/vdagent.cpp
> index efce981..d8c6bd2 100644
> --- a/vdagent/vdagent.cpp
> +++ b/vdagent/vdagent.cpp
> @@ -835,6 +835,10 @@ bool VDAgent::handle_announce_capabilities(VDAgentAnnounceCapabilities* announce
>          _client_caps_size = caps_size;
>      }
>      memcpy(_client_caps, announce_capabilities->caps, sizeof(_client_caps[0]) * caps_size);
> +
> +    _desktop_layout->set_configurable(VD_AGENT_HAS_CAPABILITY(_client_caps,
> +                                     _client_caps_size,
> +                                     VD_AGENT_CAP_MONITORS_CONFIG_POSITION));

In my opinion, this would be more readable as

if (VD_AGENT_HAS_CAPABILITY(_client_caps, _client_caps_size, VD_AGENT_CAP_MONITORS_CONFIG_POSITION))
    _desktop_layout->set_configurable(true);

Looks good otherwise,

Christophe
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://lists.freedesktop.org/archives/spice-devel/attachments/20150804/fcfeea67/attachment.sig>