[edk2-devel] [Patch v2 0/3] Ext4Pkg: Add Ext4Pkg

Michael D Kinney michael.d.kinney at intel.com
Wed Aug 11 16:43:25 UTC 2021


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 at gmail.com>
> Sent: Saturday, August 7, 2021 3:06 PM
> To: devel at edk2.groups.io
> Cc: Pedro Falcato <pedro.falcato at gmail.com>; Leif Lindholm <leif at nuviainc.com>; Kinney, Michael D
> <michael.d.kinney at intel.com>; Bret Barkelew <Bret.Barkelew at 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 at nuviainc.com>
> Cc: Michael D Kinney <michael.d.kinney at intel.com>
> Cc: Bret Barkelew <Bret.Barkelew at 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/1813853
Group Owner: devel+owner at edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [edk2-devel-archive at redhat.com]
-=-=-=-=-=-=-=-=-=-=-=-






More information about the edk2-devel-archive mailing list