About typedef of struct is true, all structures, even declared in C++ have typedefs.
About all upper case is false, beside the definitions added by these patches but are C definitions, not C++.
About space there is a space after the structure name before bracket ("{").
About empty lines mostly all structure declaration have a comment or an empty line before.
Frediano
From: "Sameeh Jubran" <sameeh@xxxxxxxxxx>
To: "Frediano Ziglio" <fziglio@xxxxxxxxxx>
Cc: "Dmitry Fleytman" <dmitry@xxxxxxxxxx>, spice-devel@xxxxxxxxxxxxxxxxxxxxx
Sent: Wednesday, July 20, 2016 6:11:27 PM
Subject: Re: [RFC PATCH vdagent 14/16] Adding ioctl operation to update Vdagent stateThis style was used in order to match the current style of VDAgent.On Mon, Jul 18, 2016 at 10:54 AM, Frediano Ziglio <fziglio@xxxxxxxxxx> wrote:This style is really confusing, it's a mix of C and C++,>
> From: Sameeh Jubran <sameeh@xxxxxxxxxx>
>
> This patch adds new ioctl operation to Vdagent in order to update
> the driver on Vdagent state. This allows the driver to know
> when Vdagent is running and when it is off.
>
> Signed-off-by: Sameeh Jubran <sameeh@xxxxxxxxxx>
> Signed-off-by: Dmitry Fleytman <dmitry@xxxxxxxxxx>
> ---
> vdagent/desktop_layout.cpp | 21 +++++++++++++++++++++
> vdagent/desktop_layout.h | 1 +
> vdagent/display_configuration.cpp | 15 +++++++++++++++
> vdagent/display_configuration.h | 2 ++
> 4 files changed, 39 insertions(+)
>
> diff --git a/vdagent/desktop_layout.cpp b/vdagent/desktop_layout.cpp
> index 7aff7e7..a8d9e2d 100644
> --- a/vdagent/desktop_layout.cpp
> +++ b/vdagent/desktop_layout.cpp
> @@ -44,6 +44,7 @@ DesktopLayout::DesktopLayout()
>
> DesktopLayout::~DesktopLayout()
> {
> + set_vdagent_running_for_displays(false);
> clean_displays();
> if (_display_config){
> delete _display_config;
> @@ -122,6 +123,8 @@ void DesktopLayout::set_displays()
> DWORD display_id = 0;
> int dev_sets = 0;
>
> + set_vdagent_running_for_displays(true);
> +
> lock();
> if (!consistent_displays()) {
> unlock();
> @@ -276,6 +279,22 @@ bool DesktopLayout::get_qxl_device_id(WCHAR* device_key,
> DWORD* device_id)
> return key_found;
> }
>
> +void DesktopLayout::set_vdagent_running_for_displays(bool running)
> +{
> + DISPLAY_DEVICE dev_info;
> + DWORD dev_id = 0;
> + lock();
> + ZeroMemory(&dev_info, sizeof(dev_info));
> + dev_info.cb = sizeof(dev_info);
> + while (EnumDisplayDevices(NULL, dev_id, &dev_info, 0)) {
> + dev_id++;
> + if (!(dev_info.StateFlags & DISPLAY_DEVICE_MIRRORING_DRIVER) &&
> wcsstr(dev_info.DeviceString, L"QXL")) {
> + _display_config->set_vdagent_running(dev_info.DeviceName,
> running);
> + }
> + }
> + unlock();
> +}
> +
> bool DesktopLayout::init_dev_mode(LPCTSTR dev_name, DEVMODE* dev_mode,
> DisplayMode* mode)
> {
> ZeroMemory(dev_mode, sizeof(DEVMODE));
> @@ -299,6 +318,8 @@ bool DesktopLayout::init_dev_mode(LPCTSTR dev_name,
> DEVMODE* dev_mode, DisplayMo
> // update current DisplayMode (so mouse scaling works properly)
> mode->_width = dev_mode->dmPelsWidth;
> mode->_height = dev_mode->dmPelsHeight;
> +
> + set_vdagent_running_for_displays(true);
> return true;
>
> }
> diff --git a/vdagent/desktop_layout.h b/vdagent/desktop_layout.h
> index fd6af76..da5a40b 100644
> --- a/vdagent/desktop_layout.h
> +++ b/vdagent/desktop_layout.h
> @@ -83,6 +83,7 @@ private:
> static bool consistent_displays();
> static bool is_attached(LPCTSTR dev_name);
> static bool get_qxl_device_id(WCHAR* device_key, DWORD* device_id);
> + void set_vdagent_running_for_displays(bool enable_pointer);
> private:
> mutex_t _mutex;
> Displays _displays;
> diff --git a/vdagent/display_configuration.cpp
> b/vdagent/display_configuration.cpp
> index f20605a..95fd4c3 100644
> --- a/vdagent/display_configuration.cpp
> +++ b/vdagent/display_configuration.cpp
> @@ -67,6 +67,15 @@ typedef struct WDDM_MONITOR_CONFIG_ESCAPE {
> int _ioctl;
> QXLHead _head;
> } WDDM_MONITOR_CONFIG_ESCAPE;
> +typedef struct WDDM_VDAGENT_RUNNING_ESCAPE{
> + WDDM_VDAGENT_RUNNING_ESCAPE(bool running)
> + {
> + _ioctl = QXL_ESCAPE_VDAGENT_RUNNING;
> + _vdagent_state.running = running;
> + }
> + int _ioctl;
> + QXLEscapeVDAgentRunning _vdagent_state;
> +} WDDM_VDAGENT_RUNNING_ESCAPE;
there is no reason for the typedef.
I think also there is some missing empty line and space.
Also the choice of all capital is confusing (maybe just me)
Frediano
> #endif
> DisplayConfig* DisplayConfig::create_config()
> {
> @@ -529,6 +538,12 @@ bool WDDMInterface::escape(LPCTSTR device_name, void*
> data, UINT size_data)
> return NT_SUCCESS(status);
> }
>
> +bool WDDMInterface::set_vdagent_running(LPCTSTR device_name, bool running)
> +{
> + WDDM_VDAGENT_RUNNING_ESCAPE wddm_escape(running);
> + return escape(device_name, &wddm_escape, sizeof(wddm_escape));
> +}
> +
> CCD::CCD()
> :_NumPathElements(0)
> ,_NumModeElements(0)
> diff --git a/vdagent/display_configuration.h
> b/vdagent/display_configuration.h
> index eeba1c2..06f592a 100644
> --- a/vdagent/display_configuration.h
> +++ b/vdagent/display_configuration.h
> @@ -132,6 +132,7 @@ public:
> DRIVER_TYPE type() { return _driver_type; };
> void set_monitors_config(bool flag) { _send_monitors_config = flag; }
> virtual void update_config_path() {};
> + virtual bool set_vdagent_running(LPCTSTR device_name, bool running) {
> return false; };
>
> protected:
> DRIVER_TYPE _driver_type;
> @@ -176,6 +177,7 @@ private:
>
> void close_adapter(D3DKMT_HANDLE handle);
> bool escape(LPCTSTR device_name, void* data, UINT sizeData);
> + bool set_vdagent_running(LPCTSTR device_name, bool running);
>
> private:
> //GDI Function pointers
--
_______________________________________________ Spice-devel mailing list Spice-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/spice-devel