Re: [edk2-platforms][PATCH v3 10/11] Ext4Pkg: Fixes build on MSVC

["Marvin Häuser" <mhaeuser@posteo.de>]

[-- Attachment #1: Type: text/plain, Size: 3712 bytes --]

On 27. Jan 2023, at 15:33, Pedro Falcato <pedro.falcato@gmail.com> wrote:
> 
> On Fri, Jan 27, 2023 at 9:29 AM Savva Mitrofanov <savvamtr@gmail.com <mailto:savvamtr@gmail.com>> wrote:
>> 
>> Accessing array using index of uint64 type makes MSVC compiler to
>> include `__allmul` function in NOOPT which is not referenced in IA32.
>> So we null-terminates string using ReadSize, which should be equal to
>> SymlinkSizeTmp after correct reading. Also adds missing MultU64x32
>> in Ext4Read.
>> 
>> Cc: Marvin Häuser <mhaeuser@posteo.de>
>> Cc: Pedro Falcato <pedro.falcato@gmail.com>
>> Cc: Vitaly Cheptsov <vit9696@protonmail.com>
>> Fixes: 7c46116b0e18 ("Ext4Pkg: Add ext2/3 support")
>> Fixes: e81432fbacb7 ("Ext4Pkg: Add symbolic links support")
>> Signed-off-by: Savva Mitrofanov <savvamtr@gmail.com>
>> ---
>> Features/Ext4Pkg/Ext4Dxe/Inode.c   |  4 ++--
>> Features/Ext4Pkg/Ext4Dxe/Symlink.c | 12 ++++++------
>> 2 files changed, 8 insertions(+), 8 deletions(-)
>> 
>> diff --git a/Features/Ext4Pkg/Ext4Dxe/Inode.c b/Features/Ext4Pkg/Ext4Dxe/Inode.c
>> index 90e3eb88f523..8db051d3c444 100644
>> --- a/Features/Ext4Pkg/Ext4Dxe/Inode.c
>> +++ b/Features/Ext4Pkg/Ext4Dxe/Inode.c
>> @@ -152,7 +152,7 @@ Ext4Read (
>>       } else {
>>         // Uninitialized extents behave exactly the same as file holes, except they have
>>         // blocks already allocated to them.
>> -        HoleLen = (Ext4GetExtentLength (&Extent) * Partition->BlockSize) - HoleOff;
>> +        HoleLen = MultU64x32 (Ext4GetExtentLength (&Extent), Partition->BlockSize) - HoleOff;
>>       }
>> 
>>       WasRead = HoleLen > RemainingRead ? RemainingRead : (UINTN)HoleLen;
>> @@ -166,7 +166,7 @@ Ext4Read (
>>                            Partition->BlockSize
>>                            );
>>       ExtentLengthBytes  = Extent.ee_len * Partition->BlockSize;
>> -      ExtentLogicalBytes = (UINT64)Extent.ee_block * Partition->BlockSize;
>> +      ExtentLogicalBytes = MultU64x32 ((UINT64)Extent.ee_block, Partition->BlockSize);
>>       ExtentOffset       = CurrentSeek - ExtentLogicalBytes;
>>       ExtentMayRead      = (UINTN)(ExtentLengthBytes - ExtentOffset);
>> 
>> diff --git a/Features/Ext4Pkg/Ext4Dxe/Symlink.c b/Features/Ext4Pkg/Ext4Dxe/Symlink.c
>> index 19b357ac6ba0..8b1511a38b55 100644
>> --- a/Features/Ext4Pkg/Ext4Dxe/Symlink.c
>> +++ b/Features/Ext4Pkg/Ext4Dxe/Symlink.c
>> @@ -1,7 +1,7 @@
>> /** @file
>>   Symbolic links routines
>> 
>> -  Copyright (c) 2022 Savva Mitrofanov All rights reserved.
>> +  Copyright (c) 2022-2023 Savva Mitrofanov All rights reserved.
>>   SPDX-License-Identifier: BSD-2-Clause-Patent
>> **/
>> 
>> @@ -155,11 +155,6 @@ Ext4ReadSlowSymlink (
>>     return Status;
>>   }
>> 
>> -  //
>> -  // Add null-terminator
>> -  //
>> -  SymlinkTmp[SymlinkSizeTmp] = '\0';
>> -
>>   if (SymlinkSizeTmp != ReadSize) {
>>     DEBUG ((
>>       DEBUG_FS,
>> @@ -168,6 +163,11 @@ Ext4ReadSlowSymlink (
>>     return EFI_VOLUME_CORRUPTED;
>>   }
>> 
>> +  //
>> +  // Add null-terminator
>> +  //
> Sidenote: please don't use this comment style, I know it's prevalent
> in EDK2 and EDK2 platforms but Ext4Pkg just does:
> // Comment
> 
> Also, why are you bringing this null-termination down here?

I don't think there's a strong functional reason, but it adheres to the principle of not touching malformed data if at all possible. Not sure it needs to be part of this particular commit, but why not, it causes less noise this way.

>> +  SymlinkTmp[ReadSize] = '\0';
>> +
>>   *AsciiSymlinkSize = SymlinkAllocateSize;
>>   *AsciiSymlink     = SymlinkTmp;
>> 
>> --
>> 2.39.0
>> 
> 
> 
> -- 
> Pedro


[-- Attachment #2: Type: text/html, Size: 12062 bytes --]