On Tue, Sep 01, 2015 at 11:58:21AM +0200, Ard Biesheuvel wrote:
> In InitializeConsolePipe (), we clobber the Status variable in the
> error handling path that reports Status in its output. Instead,
> use a NULL check on the LocateProtocol () output argument.
>
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Ard Biesheuvel <ard.biesheu...@linaro.org>
> ---
> ArmPlatformPkg/Library/PlatformIntelBdsLib/IntelBdsPlatform.c | 10 +++++-----
> 1 file changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/ArmPlatformPkg/Library/PlatformIntelBdsLib/IntelBdsPlatform.c
> b/ArmPlatformPkg/Library/PlatformIntelBdsLib/IntelBdsPlatform.c
> index 739704727945..9ba95d989666 100644
> --- a/ArmPlatformPkg/Library/PlatformIntelBdsLib/IntelBdsPlatform.c
> +++ b/ArmPlatformPkg/Library/PlatformIntelBdsLib/IntelBdsPlatform.c
> @@ -154,12 +154,12 @@ InitializeConsolePipe (
> DEBUG_CODE_BEGIN();
> if (EFI_ERROR(Status)) {
> // We convert back to the text representation of the device Path
> - EFI_DEVICE_PATH_TO_TEXT_PROTOCOL* DevicePathToTextProtocol;
> - CHAR16* DevicePathTxt;
> - EFI_STATUS Status;
> + EFI_DEVICE_PATH_TO_TEXT_PROTOCOL *DevicePathToTextProtocol;
> + CHAR16 *DevicePathTxt;
Two of the above are unrelated coding style fixes.
Not going to be a total pain, but unless you feel like breaking it up
into a separate patch (which would be nicer), can you please add a
note to the commit message?
> - Status = gBS->LocateProtocol(&gEfiDevicePathToTextProtocolGuid,
> NULL, (VOID **)&DevicePathToTextProtocol);
> - if (!EFI_ERROR(Status)) {
> + DevicePathToTextProtocol = NULL;
> + gBS->LocateProtocol(&gEfiDevicePathToTextProtocolGuid, NULL, (VOID
> **) &DevicePathToTextProtocol);
> + if (DevicePathToTextProtocol != NULL) {
> DevicePathTxt = DevicePathToTextProtocol->ConvertDevicePathToText
> (DevicePath, TRUE, TRUE);
>
> DEBUG((EFI_D_ERROR,"Fail to start the console with the Device Path
> '%s'. (Error '%r')\n", DevicePathTxt, Status));
> --
> 1.9.1
That isn't actually clobbering though?
Shadowing a variable within the very function it is defined is clearly
something that should be fixed - so I approve of the patch, but am
confused by the commit message. Oh, you were talking about the error
path within the error path... Oh dear. Well.
Switch the commit message to (something more like):
---
ArmPlatformPkg/PlatformIntelBdsLib: fix and clean up error handling
InitializeConsolePipe () shadowed its own Status variable, and then
clobbered the top one before printing its error message. Instead,
use a NULL check on the LocateProtocol () output argument.
Also clean up coding style on the error path.
---
and you can have a
Reviewed-by: Leif Lindholm <leif.lindh...@linaro.org>
/
Leif
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
