Re: [PATCH 1/8] ArmPlatformPkg/NorFlashDxe: correct NumOfLba vararg type in EraseBlocks()

[Laszlo Ersek <lersek@redhat.com>]

On 05/18/17 19:21, Jordan Justen wrote:
> On 2017-05-18 08:04:20, Laszlo Ersek wrote:
>> According to the PI spec, Volume 3,
>> EFI_FIRMWARE_VOLUME_BLOCK2_PROTOCOL.EraseBlocks():
>>
>>> The variable argument list is a list of tuples. Each tuple describes a
>>> range of LBAs to erase and consists of the following:
>>> * An EFI_LBA that indicates the starting LBA
>>> * A UINTN that indicates the number of blocks to erase
>>
>> (NB, in edk2, EFI_FIRMWARE_VOLUME_BLOCK_PROTOCOL is a typedef to
>> EFI_FIRMWARE_VOLUME_BLOCK2_PROTOCOL.)
>>
>> In this driver, the NumOfLba local variable is defined with type UINTN,
>> but the TYPE argument passed to VA_ARG() is UINT32. Fix the mismatch.
>>
>> In addition, update the DEBUG macro invocation where NumOfLba is formatted
>> with the %d conversion specifier: UINTN values should be converted to
>> UINT64 and printed with %Lu or %Lx for portability between 32-bit and
>> 64-bit.
>>
>> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>> Cc: Jordan Justen <jordan.l.justen@intel.com>
>> Cc: Leif Lindholm <leif.lindholm@linaro.org>
>> Reported-by: Jordan Justen <jordan.l.justen@intel.com>
>> Contributed-under: TianoCore Contribution Agreement 1.0
>> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
>> ---
>>  ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashFvbDxe.c | 12 +++++++++---
>>  1 file changed, 9 insertions(+), 3 deletions(-)
>>
>> diff --git a/ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashFvbDxe.c b/ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashFvbDxe.c
>> index 12a861267a86..3ea858f46ffb 100644
>> --- a/ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashFvbDxe.c
>> +++ b/ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashFvbDxe.c
>> @@ -628,10 +628,16 @@ FvbEraseBlocks (
>>      }
>>  
>>      // How many Lba blocks are we requested to erase?
>> -    NumOfLba = VA_ARG (Args, UINT32);
>> +    NumOfLba = VA_ARG (Args, UINTN);
>>  
>>      // All blocks must be within range
>> -    DEBUG ((DEBUG_BLKIO, "FvbEraseBlocks: Check if: ( StartingLba=%ld + NumOfLba=%d - 1 ) > LastBlock=%ld.\n", Instance->StartLba + StartingLba, NumOfLba, Instance->Media.LastBlock));
>> +    DEBUG ((
>> +      DEBUG_BLKIO,
>> +      "FvbEraseBlocks: Check if: ( StartingLba=%ld + NumOfLba=%Lu - 1 ) > LastBlock=%ld.\n",
> 
> Notably this is still > 80 columns. Maybe?
> 
>       "FvbEraseBlocks: Check if: ( StartingLba=%ld + NumOfLba=%Lu - 1 ) "
>       "> LastBlock=%ld.\n",

This file ("ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashFvbDxe.c") has
extremely long lines, the longest one (line 774) has 172 columns. I
broke up the above DEBUG so that it would at least fit in 120 chars per
line (which is the "second level" recommendation in the coding spec).

Also, the format string looked messy enough for me not to want to break
it up.

Finally, Leif has expressed a preference earlier for keeping DEBUG
format strings on one line, if memory serves. (I certainly don't follow
that in OvmfPkg and ArmVirtPkg, but ArmPlatformPkg is not my co-turf :) )

I think I'd like to leave the format string like this.

> 
> I noticed the capital 'L' in "%Lu". Doesn't make a difference since
> it's not hex...

PrintLib handes "l" and "L" interchangeably. In
BasePrintLibSPrintMarker(), file
"MdePkg/Library/BasePrintLib/PrintLibInternal.c", we have

        case 'L':
        case 'l':
          Flags |= LONG_TYPE;
          break;

and all hex characters are always printed upper-case (see "mHexStr").

Instead, I use the "L" length modifier rather than "l" because:
- in ISO C, "l" (in %lx, %ld, %lu) stands for "long", and "L" is not
valid for integers,
- while in edk2, both "l" and "L" stand for 64-bit (which is a different
question from "long").

In other words, "l" has conflicting meanings between ISO C and edk2
(long vs. 64-bit), while "L" is uniquely defined (it is undefined in ISO
C, for integers, and in edk2 it stands for 64-bit).

> The need for this debug message seems dubious to me, but I guess I
> might not be familiar with situation.
> 
> Thanks for noticing and fixing this bug in all the EDK II drivers.

For noticing the bug (outside of OvmfPkg/EmuVariableFvbRuntimeDxe, where
I originally fixed it), the credit goes to you :) The patches have the
right Reported-by: tag.

> 
> Series Reviewed-by: Jordan Justen <jordan.l.justen@intel.com>

Thanks!
Laszlo

>> +      Instance->StartLba + StartingLba,
>> +      (UINT64)NumOfLba,
>> +      Instance->Media.LastBlock
>> +      ));
>>      if ((NumOfLba == 0) || ((Instance->StartLba + StartingLba + NumOfLba - 1) > Instance->Media.LastBlock)) {
>>        VA_END (Args);
>>        DEBUG ((EFI_D_ERROR, "FvbEraseBlocks: ERROR - Lba range goes past the last Lba.\n"));
>> @@ -656,7 +662,7 @@ FvbEraseBlocks (
>>      }
>>  
>>      // How many Lba blocks are we requested to erase?
>> -    NumOfLba = VA_ARG (Args, UINT32);
>> +    NumOfLba = VA_ARG (Args, UINTN);
>>  
>>      // Go through each one and erase it
>>      while (NumOfLba > 0) {
>> -- 
>> 2.9.3
>>
>>