On Fri, Feb 21, 2014 at 3:58 PM, Laszlo Ersek <ler...@redhat.com> wrote:
> In QemuVideoControllerDriverStart():
> - remove redundant zero-initialization of:
> - Private->Handle (2 locations)
> - Private->GopDevicePath (when at devpath end)
>
> - remove fields used for error handling only:
> - PciAttributesSaved
>
> - tigthen scope of temporaries:
> - MmioDesc
> - AcpiDeviceNode
>
> - supplement missing error checks:
> - AppendDevicePathNode() can fail with out-of-memory (2 locations)
> - when installing GopDevicePath
> - retval of QemuVideoGraphicsOutputConstructor() (can justifiedly fail
> with out-of-resources)
>
> - plug leaks on error:
> - free GopDevicePath (AppendDevicePathNode() allocates dynamically)
> - uninstall GopDevicePath
> - free Private->ModeData
> - call QemuVideoGraphicsOutputDestructor()
> - uninstall GOP
>
> In QemuVideoGraphicsOutputConstructor(), called by Start():
> - supplement missing error checks:
> - QemuVideoGraphicsOutputSetMode() retval (it can fail with
> out-of-resources)
>
> - plug leaks on error:
> - free Mode->Info
> - free Mode
>
> In QemuVideoCirrusModeSetup() and QemuVideoBochsModeSetup(), both called
> by Start():
> - supplement missing error checks:
> - AllocatePool() can fail in both
>
> In QemuVideoGraphicsOutputDestructor(), called by Start() on the error
> path:
> - plug leaks:
> - free Private->LineBuffer, which is allocated in
> Start() -> Constructor() -> SetMode()
>
> In QemuVideoGraphicsOutputSetMode(), called by Start() indirectly:
> - remove redundant zero-assignment to:
> - Private->LineBuffer
>
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Laszlo Ersek <ler...@redhat.com>
> ---
> OvmfPkg/QemuVideoDxe/Driver.c | 207
> +++++++++++++++++++++-----------------
> OvmfPkg/QemuVideoDxe/Gop.c | 23 ++++-
> OvmfPkg/QemuVideoDxe/Initialize.c | 6 ++
> 3 files changed, 140 insertions(+), 96 deletions(-)
>
> diff --git a/OvmfPkg/QemuVideoDxe/Driver.c b/OvmfPkg/QemuVideoDxe/Driver.c
> index b253ec7..b1105f4 100644
> --- a/OvmfPkg/QemuVideoDxe/Driver.c
> +++ b/OvmfPkg/QemuVideoDxe/Driver.c
> @@ -203,29 +203,23 @@ QemuVideoControllerDriverStart (
> {
> EFI_STATUS Status;
> QEMU_VIDEO_PRIVATE_DATA *Private;
> - BOOLEAN PciAttributesSaved;
> EFI_DEVICE_PATH_PROTOCOL *ParentDevicePath;
> - ACPI_ADR_DEVICE_PATH AcpiDeviceNode;
> PCI_TYPE00 Pci;
> QEMU_VIDEO_CARD *Card;
> - EFI_ACPI_ADDRESS_SPACE_DESCRIPTOR *MmioDesc;
> EFI_PCI_IO_PROTOCOL *ChildPciIo;
>
> - PciAttributesSaved = FALSE;
> //
> // Allocate Private context data for GOP inteface.
> //
> Private = AllocateZeroPool (sizeof (QEMU_VIDEO_PRIVATE_DATA));
> if (Private == NULL) {
> - Status = EFI_OUT_OF_RESOURCES;
> - goto Error;
> + return EFI_OUT_OF_RESOURCES;
> }
>
> //
> // Set up context record
> //
> Private->Signature = QEMU_VIDEO_PRIVATE_DATA_SIGNATURE;
> - Private->Handle = NULL;
>
> //
> // Open PCI I/O Protocol
> @@ -239,7 +233,7 @@ QemuVideoControllerDriverStart (
> EFI_OPEN_PROTOCOL_BY_DRIVER
> );
> if (EFI_ERROR (Status)) {
> - goto Error;
> + goto FreePrivate;
> }
>
> //
> @@ -253,13 +247,16 @@ QemuVideoControllerDriverStart (
> &Pci
> );
> if (EFI_ERROR (Status)) {
> - goto Error;
> + goto ClosePciIo;
> }
>
> + //
> + // Determine card variant.
> + //
> Card = QemuVideoDetect(Pci.Hdr.VendorId, Pci.Hdr.DeviceId);
> if (Card == NULL) {
> Status = EFI_DEVICE_ERROR;
> - goto Error;
> + goto ClosePciIo;
> }
> Private->Variant = Card->Variant;
>
> @@ -274,10 +271,12 @@ QemuVideoControllerDriverStart (
> );
>
> if (EFI_ERROR (Status)) {
> - goto Error;
> + goto ClosePciIo;
> }
> - PciAttributesSaved = TRUE;
>
> + //
> + // Set new PCI attributes
> + //
> Status = Private->PciIo->Attributes (
> Private->PciIo,
> EfiPciIoAttributeOperationEnable,
> @@ -285,13 +284,15 @@ QemuVideoControllerDriverStart (
> NULL
> );
> if (EFI_ERROR (Status)) {
> - goto Error;
> + goto ClosePciIo;
> }
>
> //
> // Check whenever the qemu stdvga mmio bar is present (qemu 1.3+).
> //
> if (Private->Variant == QEMU_VIDEO_BOCHS_MMIO) {
> + EFI_ACPI_ADDRESS_SPACE_DESCRIPTOR *MmioDesc;
> +
> Status = Private->PciIo->GetBarAttributes (
> Private->PciIo,
> PCI_BAR_IDX2,
> @@ -322,7 +323,7 @@ QemuVideoControllerDriverStart (
> if ((BochsId & 0xFFF0) != VBE_DISPI_ID0) {
> DEBUG ((EFI_D_INFO, "QemuVideo: BochsID mismatch (got 0x%x)\n",
> BochsId));
> Status = EFI_DEVICE_ERROR;
> - goto Error;
> + goto RestoreAttributes;
> }
> }
>
> @@ -335,13 +336,15 @@ QemuVideoControllerDriverStart (
> (VOID **) &ParentDevicePath
> );
> if (EFI_ERROR (Status)) {
> - goto Error;
> + goto RestoreAttributes;
> }
>
> //
> // Set Gop Device Path
> //
> if (RemainingDevicePath == NULL) {
> + ACPI_ADR_DEVICE_PATH AcpiDeviceNode;
> +
> ZeroMem (&AcpiDeviceNode, sizeof (ACPI_ADR_DEVICE_PATH));
> AcpiDeviceNode.Header.Type = ACPI_DEVICE_PATH;
> AcpiDeviceNode.Header.SubType = ACPI_ADR_DP;
> @@ -352,31 +355,37 @@ QemuVideoControllerDriverStart (
> ParentDevicePath,
> (EFI_DEVICE_PATH_PROTOCOL *)
> &AcpiDeviceNode
> );
> + if (Private->GopDevicePath == NULL) {
> + Status = EFI_OUT_OF_RESOURCES;
> + goto RestoreAttributes;
> + }
> } else if (!IsDevicePathEnd (RemainingDevicePath)) {
> //
> // If RemainingDevicePath isn't the End of Device Path Node,
> // only scan the specified device by RemainingDevicePath
> //
> Private->GopDevicePath = AppendDevicePathNode (ParentDevicePath,
> RemainingDevicePath);
> - } else {
> - //
> - // If RemainingDevicePath is the End of Device Path Node,
> - // don't create child device and return EFI_SUCCESS
> - //
> - Private->GopDevicePath = NULL;
> + if (Private->GopDevicePath == NULL) {
> + Status = EFI_OUT_OF_RESOURCES;
> + goto RestoreAttributes;
> + }
> }
> -
> +
> + //
> + // Create new child handle and install the device path protocol on it only
> if
> + // RemainingDevicePath equals NULL, or doesn't point to the End of Device
> + // Path Node.
> + //
> if (Private->GopDevicePath != NULL) {
> - //
> - // Creat child handle and device path protocol firstly
> - //
> - Private->Handle = NULL;
> Status = gBS->InstallMultipleProtocolInterfaces (
> &Private->Handle,
> &gEfiDevicePathProtocolGuid,
> Private->GopDevicePath,
> NULL
> );
> + if (EFI_ERROR (Status)) {
> + goto FreeGopDevicePath;
> + }
> }
>
> //
> @@ -397,84 +406,96 @@ QemuVideoControllerDriverStart (
> break;
> }
> if (EFI_ERROR (Status)) {
> - goto Error;
> + goto UninstallGopDevicePath;
> }
>
> + //
> + // If RemainingDevicePath points to the End of Device Path Node, then we
> + // haven't created a child handle, and we're done.
> + //
> if (Private->GopDevicePath == NULL) {
> - //
> - // If RemainingDevicePath is the End of Device Path Node,
> - // don't create child device and return EFI_SUCCESS
> - //
> - Status = EFI_SUCCESS;
> - } else {
> + return EFI_SUCCESS;
> + }
>
> - //
> - // Start the GOP software stack.
> - //
> - Status = QemuVideoGraphicsOutputConstructor (Private);
> - ASSERT_EFI_ERROR (Status);
> + //
> + // Start the GOP software stack.
> + //
> + Status = QemuVideoGraphicsOutputConstructor (Private);
> + if (EFI_ERROR (Status)) {
> + goto FreeModeData;
> + }
>
> - Status = gBS->InstallMultipleProtocolInterfaces (
> - &Private->Handle,
> - &gEfiGraphicsOutputProtocolGuid,
> - &Private->GraphicsOutput,
> - NULL
> - );
> - if (EFI_ERROR (Status)) {
> - goto Error;
> - }
> -
> - Status = gBS->OpenProtocol (
> - Controller,
> - &gEfiPciIoProtocolGuid,
> - (VOID **) &ChildPciIo,
> - This->DriverBindingHandle,
> - Private->Handle,
> - EFI_OPEN_PROTOCOL_BY_CHILD_CONTROLLER
> + Status = gBS->InstallMultipleProtocolInterfaces (
> + &Private->Handle,
> + &gEfiGraphicsOutputProtocolGuid,
> + &Private->GraphicsOutput,
> + NULL
> );
> -
> - if (EFI_ERROR (Status)) {
> - goto Error;
> - }
> + if (EFI_ERROR (Status)) {
> + goto DestructQemuVideoGraphics;
> }
>
> -Error:
> + //
> + // Reference parent handle from child handle.
> + //
> + Status = gBS->OpenProtocol (
> + Controller,
> + &gEfiPciIoProtocolGuid,
> + (VOID **) &ChildPciIo,
> + This->DriverBindingHandle,
> + Private->Handle,
> + EFI_OPEN_PROTOCOL_BY_CHILD_CONTROLLER
> + );
> if (EFI_ERROR (Status)) {
> - if (Private) {
> - if (Private->PciIo) {
> - if (PciAttributesSaved == TRUE) {
> - //
> - // Restore original PCI attributes
> - //
> - Private->PciIo->Attributes (
> - Private->PciIo,
> - EfiPciIoAttributeOperationSet,
> - Private->OriginalPciAttributes,
> - NULL
> - );
> - }
> - //
> - // Close the PCI I/O Protocol
> - //
> - gBS->CloseProtocol (
> - Controller,
> - &gEfiPciIoProtocolGuid,
> - This->DriverBindingHandle,
> - Controller
> - );
> -
> - gBS->CloseProtocol (
> - Controller,
> - &gEfiPciIoProtocolGuid,
> - This->DriverBindingHandle,
> - Private->Handle
> - );
> - }
> -
> - gBS->FreePool (Private);
> - }
> + goto UninstallGop;
> }
>
> + //
> + // When adding further code that can fail, the error handling label to jump
> + // to is CloseChildRef.
> + //
> + return EFI_SUCCESS;
> +
> +#if 0 // in order to suppress the compiler warning about the unused label
> +CloseChildRef:
> +#endif
> + gBS->CloseProtocol (Controller, &gEfiPciIoProtocolGuid,
> + This->DriverBindingHandle, Private->Handle);
It seems like there is a good chance of getting an unreachable code
warning here too.
It is a bit rare to leave #if 0 code around our edk2 tree. Maybe we
could just let the future developer deal with this other cleanup if a
new error path is added?
If you remove this and the comment above, then:
Reviewed-by: Jordan Justen <jordan.l.jus...@intel.com>
> +
> +UninstallGop:
> + gBS->UninstallProtocolInterface (Private->Handle,
> + &gEfiGraphicsOutputProtocolGuid, &Private->GraphicsOutput);
> +
> +DestructQemuVideoGraphics:
> + QemuVideoGraphicsOutputDestructor (Private);
> +
> +FreeModeData:
> + FreePool (Private->ModeData);
> +
> +UninstallGopDevicePath:
> + //
> + // Handles the case transparently when Private->Handle and
> + // Private->GopDevicePath are NULL.
> + //
> + gBS->UninstallProtocolInterface (Private->Handle,
> + &gEfiDevicePathProtocolGuid, Private->GopDevicePath);
> +
> +FreeGopDevicePath:
> + if (Private->GopDevicePath != NULL) {
> + FreePool (Private->GopDevicePath);
> + }
> +
> +RestoreAttributes:
> + Private->PciIo->Attributes (Private->PciIo, EfiPciIoAttributeOperationSet,
> + Private->OriginalPciAttributes, NULL);
> +
> +ClosePciIo:
> + gBS->CloseProtocol (Controller, &gEfiPciIoProtocolGuid,
> + This->DriverBindingHandle, Controller);
> +
> +FreePrivate:
> + FreePool (Private);
> +
> return Status;
> }
>
> diff --git a/OvmfPkg/QemuVideoDxe/Gop.c b/OvmfPkg/QemuVideoDxe/Gop.c
> index 30aac7f..1b7db32 100644
> --- a/OvmfPkg/QemuVideoDxe/Gop.c
> +++ b/OvmfPkg/QemuVideoDxe/Gop.c
> @@ -176,7 +176,6 @@ Routine Description:
> gBS->FreePool (Private->LineBuffer);
> }
>
> - Private->LineBuffer = NULL;
> Private->LineBuffer = AllocatePool (4 * ModeData->HorizontalResolution);
> if (Private->LineBuffer == NULL) {
> return EFI_OUT_OF_RESOURCES;
> @@ -321,13 +320,14 @@ QemuVideoGraphicsOutputConstructor (
> if (EFI_ERROR (Status)) {
> return Status;
> }
> +
> Status = gBS->AllocatePool (
> EfiBootServicesData,
> sizeof (EFI_GRAPHICS_OUTPUT_MODE_INFORMATION),
> (VOID **) &Private->GraphicsOutput.Mode->Info
> );
> if (EFI_ERROR (Status)) {
> - return Status;
> + goto FreeMode;
> }
> Private->GraphicsOutput.Mode->MaxMode = (UINT32) Private->MaxMode;
> Private->GraphicsOutput.Mode->Mode =
> GRAPHICS_OUTPUT_INVALIDE_MODE_NUMBER;
> @@ -337,7 +337,11 @@ QemuVideoGraphicsOutputConstructor (
> //
> // Initialize the hardware
> //
> - GraphicsOutput->SetMode (GraphicsOutput, 0);
> + Status = GraphicsOutput->SetMode (GraphicsOutput, 0);
> + if (EFI_ERROR (Status)) {
> + goto FreeInfo;
> + }
> +
> DrawLogo (
> Private,
>
> Private->ModeData[Private->GraphicsOutput.Mode->Mode].HorizontalResolution,
> @@ -345,6 +349,15 @@ QemuVideoGraphicsOutputConstructor (
> );
>
> return EFI_SUCCESS;
> +
> +FreeInfo:
> + FreePool (Private->GraphicsOutput.Mode->Info);
> +
> +FreeMode:
> + FreePool (Private->GraphicsOutput.Mode);
> + Private->GraphicsOutput.Mode = NULL;
> +
> + return Status;
> }
>
> EFI_STATUS
> @@ -363,6 +376,10 @@ Returns:
>
> --*/
> {
> + if (Private->LineBuffer != NULL) {
> + FreePool (Private->LineBuffer);
> + }
> +
> if (Private->GraphicsOutput.Mode != NULL) {
> if (Private->GraphicsOutput.Mode->Info != NULL) {
> gBS->FreePool (Private->GraphicsOutput.Mode->Info);
> diff --git a/OvmfPkg/QemuVideoDxe/Initialize.c
> b/OvmfPkg/QemuVideoDxe/Initialize.c
> index 305797b..3774478 100644
> --- a/OvmfPkg/QemuVideoDxe/Initialize.c
> +++ b/OvmfPkg/QemuVideoDxe/Initialize.c
> @@ -176,6 +176,9 @@ QemuVideoCirrusModeSetup (
> Private->ModeData = AllocatePool (
> sizeof (Private->ModeData[0]) *
> QEMU_VIDEO_CIRRUS_MODE_COUNT
> );
> + if (Private->ModeData == NULL) {
> + return EFI_OUT_OF_RESOURCES;
> + }
> ModeData = Private->ModeData;
> VideoMode = &QemuVideoCirrusModes[0];
> for (Index = 0; Index < QEMU_VIDEO_CIRRUS_MODE_COUNT; Index ++) {
> @@ -228,6 +231,9 @@ QemuVideoBochsModeSetup (
> Private->ModeData = AllocatePool (
> sizeof (Private->ModeData[0]) *
> QEMU_VIDEO_BOCHS_MODE_COUNT
> );
> + if (Private->ModeData == NULL) {
> + return EFI_OUT_OF_RESOURCES;
> + }
> ModeData = Private->ModeData;
> VideoMode = &QemuVideoBochsModes[0];
> for (Index = 0; Index < QEMU_VIDEO_BOCHS_MODE_COUNT; Index ++) {
> --
> 1.8.3.1
>
>
>
> ------------------------------------------------------------------------------
> Managing the Performance of Cloud-Based Applications
> Take advantage of what the Cloud has to offer - Avoid Common Pitfalls.
> Read the Whitepaper.
> http://pubads.g.doubleclick.net/gampad/clk?id=121054471&iu=/4140/ostg.clktrk
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/edk2-devel
------------------------------------------------------------------------------
Flow-based real-time traffic analytics software. Cisco certified tool.
Monitor traffic, SLAs, QoS, Medianet, WAAS etc. with NetFlow Analyzer
Customize your own dashboards, set traffic alerts and generate reports.
Network behavioral analysis & security monitoring. All-in-one tool.
http://pubads.g.doubleclick.net/gampad/clk?id=126839071&iu=/4140/ostg.clktrk
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/edk2-devel
