[Spice-devel] [PATCH v3 06/28] Enhance code flow and indentation fix

[Frediano Ziglio]

> 
> This patch fixes code flow in StartDriver function.
> 
> Based on a patch by Sandy Stutsman <sstutsma at redhat.com>
> 
> Signed-off-by: Sameeh Jubran <sameeh at daynix.com>

The extra indentation is not a great enhancement.
The do {} while is quite useless.
The style changes without a clear coding style document are not
justified.
But the big problem is that the initial rationale (setting DriverStarted to FALSE)
were supposed to be moved in a different patch which seems missing (maybe I'm wrong).

> ---
>  qxldod/QxlDod.cpp | 107
>  ++++++++++++++++++++++++++----------------------------
>  1 file changed, 52 insertions(+), 55 deletions(-)
> 
> diff --git a/qxldod/QxlDod.cpp b/qxldod/QxlDod.cpp
> index dacfca3..dc83f26 100755
> --- a/qxldod/QxlDod.cpp
> +++ b/qxldod/QxlDod.cpp
> @@ -113,72 +113,71 @@ NTSTATUS QxlDod::StartDevice(_In_  DXGK_START_INFO*
> pDxgkStartInfo,
>  {
>      PHYSICAL_ADDRESS PhysicAddress;
>      PAGED_CODE();
> +    NTSTATUS Status;
>      QXL_ASSERT(pDxgkStartInfo != NULL);
>      QXL_ASSERT(pDxgkInterface != NULL);
>      QXL_ASSERT(pNumberOfViews != NULL);
>      QXL_ASSERT(pNumberOfChildren != NULL);
> -//CHECK ME!!!!!!!!!!!!!
> +    //CHECK ME!!!!!!!!!!!!!
>      RtlCopyMemory(&m_DxgkInterface, pDxgkInterface,
>      sizeof(m_DxgkInterface));
>      RtlZeroMemory(m_CurrentModes, sizeof(m_CurrentModes));
> -//CHECK ME!!!!!!!!!!!!!
> +    //CHECK ME!!!!!!!!!!!!!
>      m_CurrentModes[0].DispInfo.TargetId = D3DDDI_ID_UNINITIALIZED;

these indentation fix are fine, I would go to a more standard FIXME or TODO.

> -    // Get device information from OS.
> -    NTSTATUS Status =
> m_DxgkInterface.DxgkCbGetDeviceInformation(m_DxgkInterface.DeviceHandle,
> &m_DeviceInfo);
> -    if (!NT_SUCCESS(Status))
> -    {
> -        QXL_LOG_ASSERTION1("DxgkCbGetDeviceInformation failed with status
> 0x%X\n",
> -                           Status);
> -        return Status;
> -    }
> +    do {
>  
> -    Status = CheckHardware();
> -    if (NT_SUCCESS(Status))
> -    {
> -        m_pHWDevice = new(NonPagedPoolNx) QxlDevice(this);
> -    }
> -    else
> -    {
> -        m_pHWDevice = new(NonPagedPoolNx) VgaDevice(this);
> -    }
> +        // Get device information from OS.
> +        Status =
> m_DxgkInterface.DxgkCbGetDeviceInformation(m_DxgkInterface.DeviceHandle,
> &m_DeviceInfo);
> +        if (!NT_SUCCESS(Status)) {
> +            QXL_LOG_ASSERTION1("DxgkCbGetDeviceInformation failed with
> status 0x%X\n",
> +                Status);
> +            break;
> +        }
>  
> -    if (!m_pHWDevice)
> -    {
> -        Status = STATUS_NO_MEMORY;
> -        DbgPrint(TRACE_LEVEL_ERROR, ("HWInit failed to allocate memory\n"));
> -        return Status;
> -    }
> +        Status = CheckHardware();
> +        if (NT_SUCCESS(Status)) {
> +            m_pHWDevice = new(NonPagedPoolNx) QxlDevice(this);
> +        }
> +        else {
> +            m_pHWDevice = new(NonPagedPoolNx) VgaDevice(this);
> +        }
>  
> -    Status = m_pHWDevice->HWInit(m_DeviceInfo.TranslatedResourceList,
> &m_CurrentModes[0].DispInfo);
> -    if (!NT_SUCCESS(Status))
> -    {
> -        DbgPrint(TRACE_LEVEL_ERROR, ("HWInit failed with status 0x%X\n",
> Status));
> -        return Status;
> -    }
> +        if (!m_pHWDevice) {
> +            Status = STATUS_NO_MEMORY;
> +            DbgPrint(TRACE_LEVEL_ERROR, ("HWInit failed to allocate
> memory\n"));
> +            break;
> +        }
>  
> -    Status = RegisterHWInfo(m_pHWDevice->GetId());
> -    if (!NT_SUCCESS(Status))
> -    {
> -        QXL_LOG_ASSERTION1("RegisterHWInfo failed with status 0x%X\n",
> -                           Status);
> -        return Status;
> -    }
> +        Status = m_pHWDevice->HWInit(m_DeviceInfo.TranslatedResourceList,
> &m_CurrentModes[0].DispInfo);
> +        if (!NT_SUCCESS(Status)) {
> +            DbgPrint(TRACE_LEVEL_ERROR, ("HWInit failed with status 0x%X\n",
> Status));
> +            break;
> +        }
>  
> -    PhysicAddress.QuadPart =
> m_CurrentModes[0].DispInfo.PhysicAddress.QuadPart;
> -    if (m_pHWDevice->GetId() == 0)
> -    {
> -         Status =
> m_DxgkInterface.DxgkCbAcquirePostDisplayOwnership(m_DxgkInterface.DeviceHandle,
> &(m_CurrentModes[0].DispInfo));
> -    }
> +        Status = RegisterHWInfo(m_pHWDevice->GetId());
> +        if (!NT_SUCCESS(Status)) {
> +            QXL_LOG_ASSERTION1("RegisterHWInfo failed with status 0x%X\n",
> +                Status);
> +            break;
> +        }
>  
> -    if (!NT_SUCCESS(Status) )
> -    {
> -        DbgPrint(TRACE_LEVEL_ERROR, ("DxgkCbAcquirePostDisplayOwnership
> failed with status 0x%X Width = %d\n",
> -                           Status, m_CurrentModes[0].DispInfo.Width));
> -        return STATUS_UNSUCCESSFUL;
> +        PhysicAddress.QuadPart =
> m_CurrentModes[0].DispInfo.PhysicAddress.QuadPart;
> +        if (m_pHWDevice->GetId() == 0) {
> +            Status =
> m_DxgkInterface.DxgkCbAcquirePostDisplayOwnership(m_DxgkInterface.DeviceHandle,
> &(m_CurrentModes[0].DispInfo));
> +        }
> +
> +        if (!NT_SUCCESS(Status)) {
> +            DbgPrint(TRACE_LEVEL_ERROR, ("DxgkCbAcquirePostDisplayOwnership
> failed with status 0x%X Width = %d\n",
> +                Status, m_CurrentModes[0].DispInfo.Width));
> +            Status = STATUS_UNSUCCESSFUL;
> +            break;
> +        }
> +    } while (0);
> +    if (!NT_SUCCESS(Status)) {
> +        return Status;
>      }
>  
> -    if (m_CurrentModes[0].DispInfo.Width == 0)
> -    {
> -        m_CurrentModes[0].DispInfo.Width = MIN_WIDTH_SIZE;
> +    if (m_CurrentModes[0].DispInfo.Width == 0) {
> +        m_CurrentModes[0].DispInfo.Width = MIN_WIDTH_SIZE;
>          m_CurrentModes[0].DispInfo.Height = MIN_HEIGHT_SIZE;
>          m_CurrentModes[0].DispInfo.Pitch =
>          BPPFromPixelFormat(D3DDDIFMT_R8G8B8) / 8;
>          m_CurrentModes[0].DispInfo.ColorFormat = D3DDDIFMT_R8G8B8;

All these are just style.

> @@ -186,11 +185,9 @@ NTSTATUS QxlDod::StartDevice(_In_  DXGK_START_INFO*
> pDxgkStartInfo,
>          if (PhysicAddress.QuadPart != 0L) {
>               m_CurrentModes[0].DispInfo.PhysicAddress.QuadPart =
>               PhysicAddress.QuadPart;
>          }
> -
>      }
> -
> -   *pNumberOfViews = MAX_VIEWS;
> -   *pNumberOfChildren = MAX_CHILDREN;
> +    *pNumberOfViews = MAX_VIEWS;
> +    *pNumberOfChildren = MAX_CHILDREN;

These indentation fix are fine.

>      m_Flags.DriverStarted = TRUE;
>      DbgPrint(TRACE_LEVEL_INFORMATION, ("<--- %s\n", __FUNCTION__));
>      return STATUS_SUCCESS;

Can Windows stop and start a device again?
In this case you would have a leak.

Frediano