[Spice-devel] [PATCH VDAgent-win] Initialize buffers before querying display config

[Frediano Ziglio]

> 
> The buffers and buffer sizes should be initialized and allocated. This
> patch fixies a possible case where vdagent can get stuck, as the

fixes

> function _pfnQueryDisplayConfig can return ERROR_INVALID_PARAMETER
> if the buffers aren't intialized.
> The call to free_config_buffers is superfluous becuase it is called

because

> inside get_config_buffers.
> 
> Signed-off-by: Sameeh Jubran <sameeh at daynix.com>
> ---
>  vdagent/display_configuration.cpp | 8 +++++---
>  1 file changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/vdagent/display_configuration.cpp
> b/vdagent/display_configuration.cpp
> index bab4c95..409725e 100644
> --- a/vdagent/display_configuration.cpp
> +++ b/vdagent/display_configuration.cpp
> @@ -700,7 +700,9 @@ CCD::~CCD()
>  bool CCD::query_display_config()
>  {
>      LONG query_error(ERROR_SUCCESS);
> -
> +    if (!get_config_buffers()) {
> +        return false;
> +    }

Why using brackets here? Looks like you are following a style where if/while/for/do
statements always require a block enclosed by brackets.
However some of the new code seems to not follow this style and this patch
is enforcing this.

Not your fault, there is no clear coding style document for vd agent.

Being new code both styles are fine.

>      //Until we get it or error != ERROR_INSUFFICIENT_BUFFER
>      do {
>          query_error = _pfnQueryDisplayConfig(QDC_ALL_PATHS,
>          &_numPathElements, _pPathInfo,
> @@ -711,9 +713,9 @@ bool CCD::query_display_config()
>          //(see
>          https://msdn.microsoft.com/en-us/library/windows/hardware/ff569215(v=vs.85).aspx
>          )
>          if (query_error) {
>               if (query_error == ERROR_INSUFFICIENT_BUFFER) {
> -                free_config_buffers();

Agreed

> -                if (!get_config_buffers())
> +                if (!get_config_buffers()) {
>                      return false;
> +                }

this just change style, avoid.

>              } else {
>                  vd_printf("%s failed QueryDisplayConfig with 0x%lx",
>                  __FUNCTION__, query_error);
>                  return false;

Other than this patch looks fine.

Frediano