Hi Pedro,
Really good updates. Here are a few comments on V2:
1) I see use of "inline" on some functions. Those are not required
and should be removed. The compiler options used for release
builds always maximize optimization with inlining used by the
compiler if that provides the best optimization.
2) Code style issue for function declarations.
Align types and parameter names in columns
2 spaces between type name and parameter name.
Here is one example (please review all functions):
EFI_STATUS
Ext4ReadDiskIo (
IN EXT4_PARTITION *Partition,
OUT VOID *Buffer,
IN UINTN Length,
IN UINT64 Offset
);
Should be:
EFI_STATUS
Ext4ReadDiskIo (
IN EXT4_PARTITION *Partition,
OUT VOID *Buffer,
IN UINTN Length,
IN UINT64 Offset
);
3) Code style issue.
EFIAPI should always be on its own line.
Here is one example (please review all functions):
EFI_STATUS EFIAPI
Ext4Close (
IN EFI_FILE_PROTOCOL *This
);
Should be:
EFI_STATUS
EFIAPI
Ext4Close (
IN EFI_FILE_PROTOCOL *This
);
4) Some @param comments missing [in], [out], or [in out] decorator
5) What is #if DEBUG_EXTENT_CACHE. We try not to use any ifdefs
and instead convert them to PCD Feature Flags if we need build
time enable/disable of a feature.
I also see a couple #if 0 statements. Those should also be
removed or converted to a PCD Feature Flag if it is something
that is an optional feature. Or converted to DEBUG_CODE()
if it is something that should only be enabled when DEBUG_CODE()
feature is enabled.
6) I see a 12 TODO comments. Can you write up a separate email
with the questions you need answered to address each of those
TODOs? The version committed should not have any remaining
TODO comments. Instead, if there are additional features that
need to be considered for the future, those should be entered
as TianoCore Bugzillas.
7) I see a few places where there are commented out DEBUG() messages
These should either be removed or changed to a debug level that
is normally disabled (DEBUG_VERBOSE or DEBUG_FS) and can be
optionally enabled to debug a specific issue. Here is an example:
EFI_STATUS
Ext4Read (
IN EXT4_PARTITION *Partition,
IN EXT4_FILE *File,
OUT VOID *Buffer,
IN UINT64 Offset,
IN OUT UINTN *Length
)
{
// DEBUG((DEBUG_INFO, "Ext4Read[Offset %lu, Length %lu]\n", Offset, *Length));
EXT4_INODE *Inode;
8) All function local variables must be declared at the beginning of
a function. Ext4Read() is an example where I see additional variables
being declared in the scope of a while and if/else statements.
9) I see places where @file is followed by the brief description of the file
on the same line. The brief description of a file should go on the next line.
10) I see one "HACK!" comment. Looks like it is related to an assumption for
read-only vs read/write capabilities in this driver. Perhaps update the
comment to state that the current implementation assumes read-only and
enter a TianoCore Bugzilla feature request to add read/write support
in the future and note the places in the current driver where read-only
assumptions need to be addressed to add read/write capability.
Thanks,
Mike
> -----Original Message-----
> From: Pedro Falcato <pedro.falcato@gmail.com>
> Sent: Saturday, August 7, 2021 3:06 PM
> To: devel@edk2.groups.io
> Cc: Pedro Falcato <pedro.falcato@gmail.com>; Leif Lindholm <leif@nuviainc.com>; Kinney, Michael D
> <michael.d.kinney@intel.com>; Bret Barkelew <Bret.Barkelew@microsoft.com>
> Subject: [Patch v2 0/3] Ext4Pkg: Add Ext4Pkg
>
> This patch-set adds Ext4Pkg, a package designed to hold various drivers and
> utilities related to the EXT4 filesystem.
>
> Right now, it holds a single read-only UEFI EXT4 driver (Ext4Dxe), which consumes the
> DISK_IO, BLOCK_IO and DISK_IO2 protocols and produce EFI_FILE_PROTOCOL and
> EFI_SIMPLE_FILE_SYSTEM_PROTOCOL; this driver allows the mounting of EXT4 partitions and
> the reading of their contents.
>
> Relevant RFC discussion, which includes a more in-depth walkthrough of EXT4 internals and
> driver limitations is available at https://edk2.groups.io/g/devel/topic/84368561.
>
> This patch set is version 2 and attempts to address issues raised by the
> community in v1's code review.
>
> Cc: Leif Lindholm <leif@nuviainc.com>
> Cc: Michael D Kinney <michael.d.kinney@intel.com>
> Cc: Bret Barkelew <Bret.Barkelew@microsoft.com>
>
> Pedro Falcato (3):
> Ext4Pkg: Add Ext4Pkg.dec and Ext4Pkg.uni.
> Ext4Pkg: Add Ext4Dxe driver.
> Ext4Pkg: Add .DSC file.
>
> Features/Ext4Pkg/Ext4Dxe/BlockGroup.c | 207 +++++
> Features/Ext4Pkg/Ext4Dxe/Collation.c | 171 ++++
> Features/Ext4Pkg/Ext4Dxe/Crc16.c | 74 ++
> Features/Ext4Pkg/Ext4Dxe/Crc32c.c | 83 ++
> Features/Ext4Pkg/Ext4Dxe/Directory.c | 513 +++++++++++
> Features/Ext4Pkg/Ext4Dxe/DiskUtil.c | 106 +++
> Features/Ext4Pkg/Ext4Dxe/Ext4Disk.h | 455 ++++++++++
> Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.c | 789 +++++++++++++++++
> Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.h | 1136 +++++++++++++++++++++++++
> Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.inf | 149 ++++
> Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.uni | 15 +
> Features/Ext4Pkg/Ext4Dxe/Extents.c | 618 ++++++++++++++
> Features/Ext4Pkg/Ext4Dxe/File.c | 786 +++++++++++++++++
> Features/Ext4Pkg/Ext4Dxe/Inode.c | 467 ++++++++++
> Features/Ext4Pkg/Ext4Dxe/Partition.c | 122 +++
> Features/Ext4Pkg/Ext4Dxe/Superblock.c | 281 ++++++
> Features/Ext4Pkg/Ext4Pkg.dec | 17 +
> Features/Ext4Pkg/Ext4Pkg.dsc | 68 ++
> Features/Ext4Pkg/Ext4Pkg.uni | 14 +
> 19 files changed, 6071 insertions(+)
> create mode 100644 Features/Ext4Pkg/Ext4Dxe/BlockGroup.c
> create mode 100644 Features/Ext4Pkg/Ext4Dxe/Collation.c
> create mode 100644 Features/Ext4Pkg/Ext4Dxe/Crc16.c
> create mode 100644 Features/Ext4Pkg/Ext4Dxe/Crc32c.c
> create mode 100644 Features/Ext4Pkg/Ext4Dxe/Directory.c
> create mode 100644 Features/Ext4Pkg/Ext4Dxe/DiskUtil.c
> create mode 100644 Features/Ext4Pkg/Ext4Dxe/Ext4Disk.h
> create mode 100644 Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.c
> create mode 100644 Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.h
> create mode 100644 Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.inf
> create mode 100644 Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.uni
> create mode 100644 Features/Ext4Pkg/Ext4Dxe/Extents.c
> create mode 100644 Features/Ext4Pkg/Ext4Dxe/File.c
> create mode 100644 Features/Ext4Pkg/Ext4Dxe/Inode.c
> create mode 100644 Features/Ext4Pkg/Ext4Dxe/Partition.c
> create mode 100644 Features/Ext4Pkg/Ext4Dxe/Superblock.c
> create mode 100644 Features/Ext4Pkg/Ext4Pkg.dec
> create mode 100644 Features/Ext4Pkg/Ext4Pkg.dsc
> create mode 100644 Features/Ext4Pkg/Ext4Pkg.uni
>
> --
> 2.32.0
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#79113): https://edk2.groups.io/g/devel/message/79113
Mute This Topic: https://groups.io/mt/84737427/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-