[Spice-devel] [RFC PATCH vdagent 14/16] Adding ioctl operation to update Vdagent state

Wed Jul 20 17:57:53 UTC 2016

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 

----- Original Message -----

> From: "Sameeh Jubran" <sameeh at daynix.com>
> To: "Frediano Ziglio" <fziglio at redhat.com>
> Cc: "Dmitry Fleytman" <dmitry at daynix.com>, spice-devel at lists.freedesktop.org
> Sent: Wednesday, July 20, 2016 6:11:27 PM
> Subject: Re: [Spice-devel] [RFC PATCH vdagent 14/16] Adding ioctl operation
> to update Vdagent state

> This style was used in order to match the current style of VDAgent.

> On Mon, Jul 18, 2016 at 10:54 AM, Frediano Ziglio < fziglio at redhat.com >
> wrote:

> > >
> 
> > > From: Sameeh Jubran < sameeh at daynix.com >
> 
> > >
> 
> > > 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 at daynix.com >
> 
> > > Signed-off-by: Dmitry Fleytman < dmitry at daynix.com >
> 
> > > ---
> 
> > > 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;
> 

> > This style is really confusing, it's a mix of C and C++,
> 
> > 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)
> 

> > > #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
> 

> > Frediano
> 

> --
> Respectfully,
> Sameeh Jubran
> Mobile: +972 054-2509642
> Linkedin
> Junior Software Engineer @ Daynix .
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/spice-devel/attachments/20160720/7f4b7495/attachment.html>