On 1 September 2015 at 16:59, Leif Lindholm <[email protected]> wrote:
> 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 <[email protected]>
>> ---
>> 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 <[email protected]>
>
> /
> Leif
_______________________________________________
edk2-devel mailing list
[email protected]
https://lists.01.org/mailman/listinfo/edk2-devel
