Re: [PATCH v4 rebased 1/2] fs/erofs: Add support for EROFS

From: Yifan Zhao <zhaoyifan@sjtu.edu.cn>
To: Daniel Kiper <dkiper@net-space.pl>
Cc: grub-devel@gnu.org
Subject: Re: [PATCH v4 rebased 1/2] fs/erofs: Add support for EROFS
Date: Sat, 2 Mar 2024 12:41:30 +0800	[thread overview]
Message-ID: <fae6e7c9-c905-4314-96ad-36aecbbf9d06@sjtu.edu.cn> (raw)
In-Reply-To: <20240229175954.2nj2u2uer3d6mz4m@tomti.i.net-space.pl>


On 2024/3/1 1:59, Daniel Kiper wrote:
> On Mon, Feb 19, 2024 at 02:22:23PM +0800, Yifan Zhao wrote:
>> EROFS [1] is a lightweight read-only filesystem designed for performance
>> which has already been shipped in most Linux distributions as well as widely
>> used in several scenarios, such as Android system partitions, container
>> images, and rootfs for embedded devices.
>>
>> This patch brings EROFS uncompressed support. Now, it's possible to boot
>> directly through GRUB with an EROFS rootfs.
>>
>> EROFS compressed files will be supported later since it has more work to
>> polish.
>>
>> [1] https://erofs.docs.kernel.org
>>
>> Reviewed-by: Glenn Washburn <development@efficientek.com>
>> Reviewed-by: Gao Xiang <hsiangkao@linux.alibaba.com>
> Did they review the patch set here? If no these lines should be dropped.
> If yes they should go behind Signed-off-by below...
In the latest version previous RVB will be dropped.
>> Signed-off-by: Yifan Zhao <zhaoyifan@sjtu.edu.cn>
>> ---
>> Resend a rebased version of v4 as a thread and friendly ping...
>>
>>   INSTALL                     |   8 +-
>>   Makefile.util.def           |   1 +
>>   docs/grub.texi              |   3 +-
>>   grub-core/Makefile.core.def |   5 +
>>   grub-core/fs/erofs.c        | 968 ++++++++++++++++++++++++++++++++++++
>>   grub-core/kern/misc.c       |  14 +
>>   include/grub/misc.h         |   1 +
>>   7 files changed, 995 insertions(+), 5 deletions(-)
>>   create mode 100644 grub-core/fs/erofs.c
>>
>> diff --git a/INSTALL b/INSTALL
>> index 8d9207c84..545015ba2 100644
>> --- a/INSTALL
>> +++ b/INSTALL
>> @@ -77,15 +77,15 @@ Prerequisites for make-check:
>>
>>   * If running a Linux kernel the following modules must be loaded:
>>     - fuse, loop
>> -  - btrfs, ext4, f2fs, fat, hfs, hfsplus, jfs, mac-roman, minix, nilfs2,
>> +  - btrfs, erofs, ext4, f2fs, fat, hfs, hfsplus, jfs, mac-roman, minix, nilfs2,
>>       reiserfs, udf, xfs
>>     - On newer kernels, the exfat kernel modules may be used instead of the
>>       exfat FUSE filesystem
>>   * The following are Debian named packages required mostly for the full
>>     suite of filesystem testing (but some are needed by other tests as well):
>> -  - btrfs-progs, dosfstools, e2fsprogs, exfat-utils, f2fs-tools, genromfs,
>> -    hfsprogs, jfsutils, nilfs-tools, ntfs-3g, reiserfsprogs, squashfs-tools,
>> -    reiserfsprogs, udftools, xfsprogs, zfs-fuse
>> +  - btrfs-progs, dosfstools, erofs-utils, e2fsprogs, exfat-utils, f2fs-tools,
> erofs-utils should go after e2fsprogs.
Fixed.
>> +    genromfs, hfsprogs, jfsutils, nilfs-tools, ntfs-3g, reiserfsprogs,
>> +    squashfs-tools, reiserfsprogs, udftools, xfsprogs, zfs-fuse
>>     - exfat-fuse, if not using the exfat kernel module
>>     - gzip, lzop, xz-utils
>>     - attr, cpio, g++, gawk, parted, recode, tar, util-linux
>> diff --git a/Makefile.util.def b/Makefile.util.def
>> index 9432365a9..8d3bc107f 100644
>> --- a/Makefile.util.def
>> +++ b/Makefile.util.def
>> @@ -98,6 +98,7 @@ library = {
>>     common = grub-core/fs/cpio_be.c;
>>     common = grub-core/fs/odc.c;
>>     common = grub-core/fs/newc.c;
>> +  common = grub-core/fs/erofs.c;
>>     common = grub-core/fs/ext2.c;
>>     common = grub-core/fs/fat.c;
>>     common = grub-core/fs/exfat.c;
>> diff --git a/docs/grub.texi b/docs/grub.texi
>> index a225f9a88..396f711df 100644
>> --- a/docs/grub.texi
>> +++ b/docs/grub.texi
>> @@ -353,6 +353,7 @@ blocklist notation. The currently supported filesystem types are @dfn{Amiga
>>   Fast FileSystem (AFFS)}, @dfn{AtheOS fs}, @dfn{BeFS},
>>   @dfn{BtrFS} (including raid0, raid1, raid10, gzip and lzo),
>>   @dfn{cpio} (little- and big-endian bin, odc and newc variants),
>> +@dfn{EROFS} (only uncompressed support for now),
>>   @dfn{Linux ext2/ext3/ext4}, @dfn{DOS FAT12/FAT16/FAT32},
>>   @dfn{exFAT}, @dfn{F2FS}, @dfn{HFS}, @dfn{HFS+},
>>   @dfn{ISO9660} (including Joliet, Rock-ridge and multi-chunk files),
>> @@ -6267,7 +6268,7 @@ assumed to be encoded in UTF-8.
>>   NTFS, JFS, UDF, HFS+, exFAT, long filenames in FAT, Joliet part of
>>   ISO9660 are treated as UTF-16 as per specification. AFS and BFS are read
>>   as UTF-8, again according to specification. BtrFS, cpio, tar, squash4, minix,
>> -minix2, minix3, ROMFS, ReiserFS, XFS, ext2, ext3, ext4, FAT (short names),
>> +minix2, minix3, ROMFS, ReiserFS, XFS, EROFS, ext2, ext3, ext4, FAT (short names),
>>   F2FS, RockRidge part of ISO9660, nilfs2, UFS1, UFS2 and ZFS are assumed
>>   to be UTF-8. This might be false on systems configured with legacy charset
>>   but as long as the charset used is superset of ASCII you should be able to
>> diff --git a/grub-core/Makefile.core.def b/grub-core/Makefile.core.def
>> index 1571421d7..5aaeaef0c 100644
>> --- a/grub-core/Makefile.core.def
>> +++ b/grub-core/Makefile.core.def
>> @@ -1438,6 +1438,11 @@ module = {
>>     common = fs/odc.c;
>>   };
>>
>> +module = {
>> +  name = erofs;
>> +  common = fs/erofs.c;
>> +};
>> +
>>   module = {
>>     name = ext2;
>>     common = fs/ext2.c;
>> diff --git a/grub-core/fs/erofs.c b/grub-core/fs/erofs.c
>> new file mode 100644
>> index 000000000..de57aaa5e
>> --- /dev/null
>> +++ b/grub-core/fs/erofs.c
>> @@ -0,0 +1,968 @@
>> +/* erofs.c - Enhanced Read-Only File System */
>> +/*
>> + *  GRUB  --  GRand Unified Bootloader
>> + *  Copyright (C) 2023 Free Software Foundation, Inc.
> s/2023/2024/
Fixed.
>> + *
>> + *  GRUB is free software: you can redistribute it and/or modify
>> + *  it under the terms of the GNU General Public License as published by
>> + *  the Free Software Foundation, either version 3 of the License, or
>> + *  (at your option) any later version.
>> + *
>> + *  GRUB is distributed in the hope that it will be useful,
>> + *  but WITHOUT ANY WARRANTY; without even the implied warranty of
>> + *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>> + *  GNU General Public License for more details.
>> + *
>> + *  You should have received a copy of the GNU General Public License
>> + *  along with GRUB.  If not, see <http://www.gnu.org/licenses/>.
>> + */
>> +
>> +#include <grub/disk.h>
>> +#include <grub/dl.h>
>> +#include <grub/err.h>
>> +#include <grub/file.h>
>> +#include <grub/fs.h>
>> +#include <grub/fshelp.h>
>> +#include <grub/misc.h>
>> +#include <grub/mm.h>
>> +#include <grub/safemath.h>
>> +#include <grub/types.h>
>> +
>> +GRUB_MOD_LICENSE ("GPLv3+");
>> +
>> +#define EROFS_SUPER_OFFSET (1024)
>> +#define EROFS_MAGIC 0xE0F5E1E2
> Please be consistent and drop brackets from all plain numbers.
Fixed.
>
>> +#define EROFS_ISLOTBITS (5)
> Additionally, please align assignments in definition groups properly, e.g.:
>
> #define EROFS_SUPER_OFFSET	1024
> #define EROFS_MAGIC		0xE0F5E1E2
> #define EROFS_ISLOTBITS		5
>
> Please use tabs for alignment.
>
>> +#define EROFS_FEATURE_INCOMPAT_CHUNKED_FILE 0x00000004
>> +#define EROFS_ALL_FEATURE_INCOMPAT (EROFS_FEATURE_INCOMPAT_CHUNKED_FILE)
> Ditto...
>
> #define EROFS_FEATURE_INCOMPAT_CHUNKED_FILE	0x00000004
> #define EROFS_ALL_FEATURE_INCOMPAT		EROFS_FEATURE_INCOMPAT_CHUNKED_FILE
>
> ...and below please...
Fixed.
>> +struct grub_erofs_super
>> +{
>> +  grub_uint32_t magic;
>> +  grub_uint32_t checksum;
>> +  grub_uint32_t feature_compat;
>> +  grub_uint8_t log2_blksz;
>> +  grub_uint8_t sb_extslots;
>> +
>> +  grub_uint16_t root_nid;
>> +  grub_uint64_t inos;
>> +
>> +  grub_uint64_t build_time;
>> +  grub_uint32_t build_time_nsec;
>> +  grub_uint32_t blocks;
>> +  grub_uint32_t meta_blkaddr;
>> +  grub_uint32_t xattr_blkaddr;
>> +  grub_uint8_t uuid[16];
>> +  grub_uint8_t volume_name[16];
>> +  grub_uint32_t feature_incompat;
>> +
>> +  union
>> +  {
>> +    grub_uint16_t available_compr_algs;
>> +    grub_uint16_t lz4_max_distance;
>> +  } GRUB_PACKED u1;
>> +
>> +  grub_uint16_t extra_devices;
>> +  grub_uint16_t devt_slotoff;
>> +  grub_uint8_t log2_dirblksz;
>> +  grub_uint8_t xattr_prefix_count;
>> +  grub_uint32_t xattr_prefix_start;
>> +  grub_uint64_t packed_nid;
>> +  grub_uint8_t reserved2[24];
>> +} GRUB_PACKED;
>> +
>> +#define EROFS_INODE_LAYOUT_COMPACT 0
>> +#define EROFS_INODE_LAYOUT_EXTENDED 1
>> +
>> +enum
>> +{
>> +  EROFS_INODE_FLAT_PLAIN = 0,
>> +  EROFS_INODE_COMPRESSED_FULL = 1,
>> +  EROFS_INODE_FLAT_INLINE = 2,
>> +  EROFS_INODE_COMPRESSED_COMPACT = 3,
>> +  EROFS_INODE_CHUNK_BASED = 4,
>> +  EROFS_INODE_DATALAYOUT_MAX
>> +};
> I think this should be defines instead of enum.
Yes, use #define instead of enum now.
>
>> +#define EROFS_I_VERSION_MASKS 0x01
>> +#define EROFS_I_DATALAYOUT_MASKS 0x07
>> +
>> +#define EROFS_I_VERSION_BIT 0
>> +#define EROFS_I_DATALAYOUT_BIT 1
>> +
>> +struct grub_erofs_inode_chunk_info
>> +{
>> +  grub_uint16_t format;
>> +  grub_uint16_t reserved;
>> +} GRUB_PACKED;
>> +
>> +#define EROFS_CHUNK_FORMAT_BLKBITS_MASK 0x001F
>> +#define EROFS_CHUNK_FORMAT_INDEXES 0x0020
>> +
>> +#define EROFS_BLOCK_MAP_ENTRY_SIZE 4
>> +
>> +#define EROFS_NULL_ADDR -1
>> +
>> +struct grub_erofs_inode_chunk_index
>> +{
>> +  grub_uint16_t advise;
>> +  grub_uint16_t device_id;
>> +  grub_uint32_t blkaddr;
>> +};
>> +
>> +union grub_erofs_inode_i_u
>> +{
>> +  grub_uint32_t compressed_blocks;
>> +  grub_uint32_t raw_blkaddr;
>> +
>> +  grub_uint32_t rdev;
>> +
>> +  struct grub_erofs_inode_chunk_info c;
>> +};
>> +
>> +struct grub_erofs_inode_compact
>> +{
>> +  grub_uint16_t i_format;
>> +
>> +  grub_uint16_t i_xattr_icount;
>> +  grub_uint16_t i_mode;
>> +  grub_uint16_t i_nlink;
>> +  grub_uint32_t i_size;
>> +  grub_uint32_t i_reserved;
>> +
>> +  union grub_erofs_inode_i_u i_u;
>> +
>> +  grub_uint32_t i_ino;
>> +  grub_uint16_t i_uid;
>> +  grub_uint16_t i_gid;
>> +  grub_uint32_t i_reserved2;
>> +} GRUB_PACKED;
>> +
>> +struct grub_erofs_inode_extended
>> +{
>> +  grub_uint16_t i_format;
>> +
>> +  grub_uint16_t i_xattr_icount;
>> +  grub_uint16_t i_mode;
>> +  grub_uint16_t i_reserved;
>> +  grub_uint64_t i_size;
>> +
>> +  union grub_erofs_inode_i_u i_u;
>> +
>> +  grub_uint32_t i_ino;
>> +
>> +  grub_uint32_t i_uid;
>> +  grub_uint32_t i_gid;
>> +  grub_uint64_t i_mtime;
>> +  grub_uint32_t i_mtime_nsec;
>> +  grub_uint32_t i_nlink;
>> +  grub_uint8_t i_reserved2[16];
>> +} GRUB_PACKED;
>> +
>> +enum
>> +{
>> +  EROFS_FT_UNKNOWN,
>> +  EROFS_FT_REG_FILE,
>> +  EROFS_FT_DIR,
>> +  EROFS_FT_CHRDEV,
>> +  EROFS_FT_BLKDEV,
>> +  EROFS_FT_FIFO,
>> +  EROFS_FT_SOCK,
>> +  EROFS_FT_SYMLINK,
>> +  EROFS_FT_MAX
>> +};
> Ditto...
>
>> +#define EROFS_NAME_LEN 255
>> +
>> +struct grub_erofs_dirent
>> +{
>> +  grub_uint64_t nid;
>> +  grub_uint16_t nameoff;
>> +  grub_uint8_t file_type;
>> +  grub_uint8_t reserved;
>> +} GRUB_PACKED;
>> +
>> +#define EROFS_MAP_MAPPED 0x02
>> +
>> +struct grub_erofs_map_blocks
>> +{
>> +  grub_off_t m_pa, m_la;
>> +  grub_off_t m_plen, m_llen;
> Should not you use grub_uint64_t instead of grub_off_t here?
> And if yes then...
>
>    grub_uint64_t m_pa;
>    grub_uint64_t m_la;
>    grub_uint64_t m_plen;
>    grub_uint64_t m_llen
>
> Simply speaking, please be consistent with definitions format...
Fixed. grub_off_t => grub_uint64_t
>
>> +  grub_uint32_t m_flags;
>> +};
>> +
>> +struct grub_erofs_xattr_ibody_header
>> +{
>> +  grub_uint32_t h_reserved;
>> +  grub_uint8_t h_shared_count;
>> +  grub_uint8_t h_reserved2[7];
>> +  grub_uint32_t h_shared_xattrs[0];
>> +};
>> +
>> +struct grub_fshelp_node
>> +{
>> +  struct grub_erofs_data *data;
>> +  struct grub_erofs_inode_extended inode;
>> +
>> +  grub_uint64_t ino;
>> +  grub_uint8_t inode_type;
>> +  grub_uint8_t inode_datalayout;
>> +
>> +  /* if the inode has been read into memory? */
>> +  bool inode_read;
> s/inode_read/inode_loaded/?
Fixed. `inode_read` has been renamed to `inode_loaded`.
>
>> +};
>> +
>> +struct grub_erofs_data
>> +{
>> +  grub_disk_t disk;
>> +  struct grub_erofs_super sb;
>> +
>> +  struct grub_fshelp_node inode;
>> +};
>> +
>> +#define erofs_blocksz(data) (1u << data->sb.log2_blksz)
> This probably would be better...
>
>    #define erofs_blocksz(data) (((grub_uint32_t) 1) << data->sb.log2_blksz)
Fixed.
>
>> +
>> +static inline grub_uint64_t
> Please drop "inline" from all places here. Today's compilers are smart
> enough to do proper optimizations themselves and we should not put
> things down to their throats if it is not really needed.
Fixed. All inline keywords have been dropped.
>> +erofs_iloc (grub_fshelp_node_t node)
>> +{
>> +  struct grub_erofs_super *sb = &node->data->sb;
>> +
>> +  return (grub_le_to_cpu32 (sb->meta_blkaddr) << sb->log2_blksz) +
>> +	 (node->ino << EROFS_ISLOTBITS);
> I think this could be in one line. In general I am OK with lines a bit
> longer than 80 chars. Please fix things like that one everywhere.
Fixed, together with other lines slightly longer than 80 chars.
>> +}
>> +
>> +static grub_err_t
>> +grub_erofs_read_inode (struct grub_erofs_data *data, grub_fshelp_node_t node)
>> +{
>> +  struct grub_erofs_inode_compact *dic;
>> +  grub_err_t err;
>> +  grub_uint64_t addr = erofs_iloc (node);
>> +
>> +  dic = (struct grub_erofs_inode_compact *) &node->inode;
>> +
>> +  err = grub_disk_read (data->disk, addr >> GRUB_DISK_SECTOR_BITS,
>> +			addr & (GRUB_DISK_SECTOR_SIZE - 1),
>> +			sizeof (struct grub_erofs_inode_compact), dic);
>> +  if (err)
> I prefer "if (err != GRUB_ERR_NONE)". Please fix this everywhere...
Done.
>
>> +    return err;
>> +
>> +  node->inode_type =
>> +      (dic->i_format >> EROFS_I_VERSION_BIT) & EROFS_I_VERSION_MASKS;
>> +  node->inode_datalayout =
>> +      (dic->i_format >> EROFS_I_DATALAYOUT_BIT) & EROFS_I_DATALAYOUT_MASKS;
> Again this 4 lines can be put in two lines...
Done.
>
>> +  switch (node->inode_type)
>> +    {
>> +    case EROFS_INODE_LAYOUT_EXTENDED:
>> +      addr += sizeof (struct grub_erofs_inode_compact);
>> +      err = grub_disk_read (
>> +	  data->disk, addr >> GRUB_DISK_SECTOR_BITS,
>> +	  addr & (GRUB_DISK_SECTOR_SIZE - 1),
>> +	  sizeof (struct grub_erofs_inode_extended) -
>> +	      sizeof (struct grub_erofs_inode_compact),
>> +	  (char *) dic + sizeof (struct grub_erofs_inode_compact));
>> +      if (err)
>> +	return err;
>> +      break;
>> +    case EROFS_INODE_LAYOUT_COMPACT:
>> +      break;
>> +    default:
>> +      return grub_error (GRUB_ERR_BAD_FS,
>> +			 "invalid inode version %u @ inode %" PRIuGRUB_UINT64_T,
>> +			 node->inode_type, node->ino);
>> +    }
>> +
>> +  node->inode_read = true;
>> +
>> +  return 0;
>> +}
>> +
>> +static inline grub_uint64_t
>> +erofs_inode_size (grub_fshelp_node_t node)
>> +{
>> +  return node->inode_type == EROFS_INODE_LAYOUT_COMPACT
>> +	     ? sizeof (struct grub_erofs_inode_compact)
>> +	     : sizeof (struct grub_erofs_inode_extended);
>> +}
>> +
>> +static inline grub_uint64_t
>> +erofs_inode_file_size (grub_fshelp_node_t node)
>> +{
>> +  struct grub_erofs_inode_compact *dic =
>> +      (struct grub_erofs_inode_compact *) &node->inode;
>> +
>> +  return node->inode_type == EROFS_INODE_LAYOUT_COMPACT
>> +	     ? grub_le_to_cpu32 (dic->i_size)
>> +	     : grub_le_to_cpu64 (node->inode.i_size);
>> +}
>> +
>> +static inline grub_uint32_t
>> +erofs_inode_xattr_ibody_size (grub_fshelp_node_t node)
>> +{
>> +  grub_uint16_t cnt = grub_le_to_cpu16 (node->inode.i_xattr_icount);
>> +
>> +  return cnt ? sizeof (struct grub_erofs_xattr_ibody_header) +
>> +		   (cnt - 1) * sizeof (grub_uint32_t)
>> +	     : 0;
>> +}
>> +
>> +static inline grub_uint64_t
>> +erofs_inode_mtime (grub_fshelp_node_t node)
>> +{
>> +  return node->inode_type == EROFS_INODE_LAYOUT_COMPACT
>> +	     ? grub_le_to_cpu64 (node->data->sb.build_time)
>> +	     : grub_le_to_cpu64 (node->inode.i_mtime);
>> +}
>> +
>> +static grub_err_t
>> +grub_erofs_map_blocks_flatmode (grub_fshelp_node_t node,
>> +				struct grub_erofs_map_blocks *map)
> Please drop "grub_" prefix from all functions which are not exported
> outside of this module or not listed in the grub_erofs_fs struct below.
Fixed.
>> +{
>> +  grub_off_t nblocks, lastblk, file_size;
> I am not convinced we should use grub_off_t in the EROFS at all.
Fixed.
>
>> +  grub_off_t tailendpacking =
>> +      (node->inode_datalayout == EROFS_INODE_FLAT_INLINE) ? 1 : 0;
> Really, grub_off_t for something which looks like a bool. If it is not
> a bool then variable requires better naming or a comment.
Fixed. This variable is of bool type now.
>
>> +  grub_uint32_t blocksz = erofs_blocksz (node->data);
>> +
>> +  file_size = erofs_inode_file_size (node);
>> +  nblocks = (file_size + blocksz - 1) >> node->data->sb.log2_blksz;
>> +  lastblk = nblocks - tailendpacking;
>> +
>> +  map->m_flags = EROFS_MAP_MAPPED;
>> +
>> +  if (map->m_la < (lastblk * blocksz))
>> +    {
>> +      map->m_pa =
>> +	  grub_le_to_cpu32 (node->inode.i_u.raw_blkaddr) * blocksz + map->m_la;
>> +      map->m_plen = lastblk * blocksz - map->m_la;
> Should not we use safe math here? Could you verify all math in this code
> and replace unsafe operators with safe ones where needed?
Fixed with safe math now together with other places where overflow may 
happen.
>
>> +    }
>> +  else if (tailendpacking)
>> +    {
>> +      map->m_pa = erofs_iloc (node) + erofs_inode_size (node) +
>> +		  erofs_inode_xattr_ibody_size (node) + (map->m_la % blocksz);
> Ditto and below please...
>
>> +      map->m_plen = file_size - map->m_la;
>> +
>> +      if (((map->m_pa % blocksz) + map->m_plen) > blocksz)
>> +	return grub_error (
>> +	    GRUB_ERR_BAD_FS,
>> +	    "inline data cross block boundary @ inode %" PRIuGRUB_UINT64_T,
>> +	    node->ino);
>> +    }
>> +  else
>> +    return grub_error (GRUB_ERR_BAD_FS,
>> +		       "invalid map->m_la=%" PRIuGRUB_UINT64_T
>> +		       " @ inode %" PRIuGRUB_UINT64_T,
>> +		       map->m_la, node->ino);
>> +
>> +  map->m_llen = map->m_plen;
>> +  return GRUB_ERR_NONE;
>> +}
>> +
>> +static grub_err_t
>> +grub_erofs_map_blocks_chunkmode (grub_fshelp_node_t node,
>> +				 struct grub_erofs_map_blocks *map)
>> +{
>> +  grub_uint16_t chunk_format = grub_le_to_cpu16 (node->inode.i_u.c.format);
>> +  grub_off_t unit, pos;
>> +  grub_uint64_t chunknr;
>> +  grub_uint8_t chunkbits;
>> +  grub_err_t err;
>> +
>> +  if (chunk_format & EROFS_CHUNK_FORMAT_INDEXES)
>> +    unit = sizeof (struct grub_erofs_inode_chunk_index);
>> +  else
>> +    unit = EROFS_BLOCK_MAP_ENTRY_SIZE;
>> +
>> +  chunkbits = node->data->sb.log2_blksz +
>> +	      (chunk_format & EROFS_CHUNK_FORMAT_BLKBITS_MASK);
>> +
>> +  chunknr = map->m_la >> chunkbits;
>> +  pos = ALIGN_UP (erofs_iloc (node) + erofs_inode_size (node) +
>> +		      erofs_inode_xattr_ibody_size (node),
>> +		  unit);
>> +  pos += chunknr * unit;
>> +
>> +  map->m_la = chunknr << chunkbits;
>> +  map->m_plen = grub_min (1ULL << chunkbits,
>> +			  ALIGN_UP (erofs_inode_file_size (node) - map->m_la,
>> +				    erofs_blocksz (node->data)));
>> +
>> +  if (chunk_format & EROFS_CHUNK_FORMAT_INDEXES)
>> +    {
>> +      struct grub_erofs_inode_chunk_index idx;
>> +      grub_uint32_t blkaddr;
>> +
>> +      err = grub_disk_read (node->data->disk, pos >> GRUB_DISK_SECTOR_BITS,
>> +			    pos & (GRUB_DISK_SECTOR_SIZE - 1), unit, &idx);
>> +      if (err)
>> +	return err;
>> +
>> +      blkaddr = grub_le_to_cpu32 (idx.blkaddr);
>> +
>> +      if (blkaddr == (grub_uint32_t) EROFS_NULL_ADDR)
>> +	{
>> +	  map->m_pa = 0;
>> +	  map->m_flags = 0;
>> +	}
>> +      else
>> +	{
>> +	  map->m_pa = blkaddr << node->data->sb.log2_blksz;
>> +	  map->m_flags = EROFS_MAP_MAPPED;
>> +	}
>> +    }
>> +  else
>> +    {
>> +      grub_uint32_t blkaddr_le, blkaddr;
>> +
>> +      err =
>> +	  grub_disk_read (node->data->disk, pos >> GRUB_DISK_SECTOR_BITS,
>> +			  pos & (GRUB_DISK_SECTOR_SIZE - 1), unit, &blkaddr_le);
>> +      if (err)
>> +	return err;
>> +
>> +      blkaddr = grub_le_to_cpu32 (blkaddr_le);
>> +      if (blkaddr == (grub_uint32_t) EROFS_NULL_ADDR)
>> +	{
>> +	  map->m_pa = 0;
>> +	  map->m_flags = 0;
>> +	}
>> +      else
>> +	{
>> +	  map->m_pa = blkaddr << node->data->sb.log2_blksz;
>> +	  map->m_flags = EROFS_MAP_MAPPED;
>> +	}
>> +    }
>> +
>> +  map->m_llen = map->m_plen;
>> +  return GRUB_ERR_NONE;
>> +}
>> +
>> +static grub_err_t
>> +grub_erofs_map_blocks (grub_fshelp_node_t node,
>> +		       struct grub_erofs_map_blocks *map)
>> +{
>> +  if (map->m_la >= erofs_inode_file_size (node))
>> +    {
>> +      map->m_llen = map->m_plen = 0;
>> +      map->m_pa = 0;
>> +      map->m_flags = 0;
>> +      return GRUB_ERR_NONE;
>> +    }
>> +
>> +  if (node->inode_datalayout != EROFS_INODE_CHUNK_BASED)
>> +    return grub_erofs_map_blocks_flatmode (node, map);
>> +  else
>> +    return grub_erofs_map_blocks_chunkmode (node, map);
>> +}
>> +
>> +static grub_err_t
>> +grub_erofs_read_raw_data (grub_fshelp_node_t node, char *buf, grub_off_t size,
>> +			  grub_off_t offset, grub_off_t *bytes)
> Again, please rethink grub_off_t usage in this code...
Fixed.
>
>> +{
>> +  struct grub_erofs_map_blocks map;
> Why not "struct grub_erofs_map_blocks map = {0};"?
> Then you can drop grub_memset() below...
Fixed with other similar issues.
>
>> +  grub_err_t err;
>> +
>> +  if (bytes)
>> +    *bytes = 0;
>> +
>> +  if (!node->inode_read)
>> +    {
>> +      err = grub_erofs_read_inode (node->data, node);
>> +      if (err)
>> +	return err;
>> +    }
>> +
>> +  grub_memset (&map, 0, sizeof (map));
> ... i.e. here...
>
>> +
>> +  grub_off_t cur = offset;
>> +  while (cur < offset + size)
>> +    {
>> +      char *const estart = buf + cur - offset;
>> +      grub_off_t eend, moff = 0;
>> +
>> +      map.m_la = cur;
>> +      err = grub_erofs_map_blocks (node, &map);
>> +      if (err)
>> +	return err;
>> +
>> +      eend = grub_min (offset + size, map.m_la + map.m_llen);
>> +      if (!(map.m_flags & EROFS_MAP_MAPPED))
>> +	{
>> +	  if (!map.m_llen)
>> +	    {
>> +	      /* reached EOF */
>> +	      grub_memset (estart, 0, offset + size - cur);
>> +	      cur = offset + size;
>> +	      continue;
>> +	    }
>> +
>> +	  /* Hole */
>> +	  grub_memset (estart, 0, eend - cur);
>> +	  cur = eend;
>> +	  if (bytes)
>> +	    *bytes += eend - cur;
>> +	  continue;
>> +	}
>> +
>> +      if (cur > map.m_la)
>> +	{
>> +	  moff = cur - map.m_la;
>> +	  map.m_la = cur;
>> +	}
>> +
>> +      err = grub_disk_read (node->data->disk,
>> +			    (map.m_pa + moff) >> GRUB_DISK_SECTOR_BITS,
>> +			    (map.m_pa + moff) & (GRUB_DISK_SECTOR_SIZE - 1),
>> +			    eend - map.m_la, estart);
>> +      if (err)
>> +	return err;
>> +
>> +      if (bytes)
>> +	*bytes += eend - map.m_la;
>> +
>> +      cur = eend;
>> +    }
>> +
>> +  return GRUB_ERR_NONE;
>> +}
>> +
>> +static int
>> +grub_erofs_iterate_dir (grub_fshelp_node_t dir,
>> +			grub_fshelp_iterate_dir_hook_t hook, void *hook_data)
>> +{
>> +  grub_off_t offset = 0, file_size;
>> +  grub_err_t err;
>> +  grub_uint32_t blocksz = erofs_blocksz (dir->data);
>> +  char *buf;
>> +
>> +  if (!dir->inode_read)
>> +    {
>> +      err = grub_erofs_read_inode (dir->data, dir);
>> +      if (err)
>> +	return 0;
>> +    }
>> +
>> +  file_size = erofs_inode_file_size (dir);
>> +  buf = grub_malloc (blocksz);
>> +  if (!buf)
>> +    {
>> +      grub_error (GRUB_ERR_OUT_OF_MEMORY, "out of memory");
>> +      return 0;
>> +    }
>> +
>> +  while (offset < file_size)
>> +    {
>> +      grub_off_t maxsize = grub_min (blocksz, file_size - offset);
>> +      struct grub_erofs_dirent *de = (void *) buf, *end;
>> +      grub_uint16_t nameoff;
>> +
>> +      err = grub_erofs_read_raw_data (dir, buf, maxsize, offset, NULL);
>> +      if (err)
>> +	goto not_found;
>> +
>> +      nameoff = grub_le_to_cpu16 (de->nameoff);
>> +      if (nameoff < sizeof (struct grub_erofs_dirent) || nameoff > blocksz)
>> +	{
>> +	  grub_error (GRUB_ERR_BAD_FS,
>> +		      "invalid de[0].nameoff %u @ inode %" PRIuGRUB_UINT64_T,
>> +		      nameoff, dir->ino);
>> +	  goto not_found;
>> +	}
>> +
>> +      end = (struct grub_erofs_dirent *) ((char *) de + nameoff);
> I suppose grub_uint8_t would be more appropriate instead of char here.
Fixed.
>
>> +      while (de < end)
>> +	{
>> +	  struct grub_fshelp_node *fdiro;
>> +	  enum grub_fshelp_filetype type;
>> +	  char filename[EROFS_NAME_LEN + 1];
>> +	  unsigned int de_namelen;
>> +	  const char *de_name;
>> +
>> +	  fdiro = grub_malloc (sizeof (struct grub_fshelp_node));
>> +	  if (!fdiro)
>> +	    {
>> +	      grub_error (GRUB_ERR_OUT_OF_MEMORY, "out of memory");
>> +	      goto not_found;
>> +	    }
>> +
>> +	  fdiro->data = dir->data;
>> +	  fdiro->ino = grub_le_to_cpu64 (de->nid);
>> +	  fdiro->inode_read = false;
>> +
>> +	  nameoff = grub_le_to_cpu16 (de->nameoff);
>> +	  de_name = buf + nameoff;
>> +	  if (de + 1 >= end)
>> +	    de_namelen = grub_strnlen (de_name, maxsize - nameoff);
> The grub_strnlen() returns grub_size_t type. So, I think it would be
> better if de_namelen is defined as grub_size_t.
Fixed.
>> +	  else
>> +	    de_namelen = grub_le_to_cpu16 (de[1].nameoff) - nameoff;
>> +
>> +	  grub_memcpy (filename, de_name, de_namelen);
>> +	  filename[de_namelen] = '\0';
>> +
>> +	  switch (grub_le_to_cpu16 (de->file_type))
>> +	    {
>> +	    case EROFS_FT_REG_FILE:
>> +	    case EROFS_FT_BLKDEV:
>> +	    case EROFS_FT_CHRDEV:
>> +	    case EROFS_FT_FIFO:
>> +	    case EROFS_FT_SOCK:
>> +	      type = GRUB_FSHELP_REG;
>> +	      break;
>> +	    case EROFS_FT_DIR:
>> +	      type = GRUB_FSHELP_DIR;
>> +	      break;
>> +	    case EROFS_FT_SYMLINK:
>> +	      type = GRUB_FSHELP_SYMLINK;
>> +	      break;
>> +	    case EROFS_FT_UNKNOWN:
>> +	    default:
>> +	      type = GRUB_FSHELP_UNKNOWN;
>> +	      break;
> You do not need break here.
Fixed.
>> +	    }
>> +
>> +	  if (hook (filename, type, fdiro, hook_data))
>> +	    {
>> +	      grub_free (buf);
>> +	      return 1;
>> +	    }
>> +
>> +	  ++de;
>> +	}
>> +
>> +      offset += maxsize;
>> +    }
>> +
>> +not_found:
> Please add space before labels here and below.
Fixed, with other similar issues.
>
>> +  grub_free (buf);
>> +  return 0;
>> +}
>> +
>> +static char *
>> +grub_erofs_read_symlink (grub_fshelp_node_t node)
>> +{
>> +  char *symlink;
>> +  grub_size_t sz;
>> +
>> +  if (!node->inode_read)
>> +    {
>> +      grub_erofs_read_inode (node->data, node);
>> +      if (grub_errno)
>> +	return NULL;
>> +    }
>> +
>> +  if (grub_add (erofs_inode_file_size (node), 1, &sz))
>> +    {
>> +      grub_error (GRUB_ERR_OUT_OF_RANGE, N_ ("overflow is detected"));
>> +      return NULL;
>> +    }
>> +
>> +  symlink = grub_malloc (sz);
>> +  if (!symlink)
>> +    return NULL;
>> +
>> +  grub_erofs_read_raw_data (node, symlink, sz - 1, 0, NULL);
>> +  if (grub_errno)
> Why do you check grub_errno instead of return value here?
Fixed with other similar issues.
>
>> +    {
>> +      grub_free (symlink);
>> +      return NULL;
>> +    }
>> +
>> +  symlink[sz - 1] = '\0';
>> +  return symlink;
>> +}
>> +
>> +static struct grub_erofs_data *
>> +grub_erofs_mount (grub_disk_t disk, bool read_root)
>> +{
>> +  struct grub_erofs_super sb;
>> +  grub_err_t err;
>> +  struct grub_erofs_data *data;
>> +  grub_uint32_t feature;
>> +
>> +  err = grub_disk_read (disk, EROFS_SUPER_OFFSET >> GRUB_DISK_SECTOR_BITS, 0,
>> +			sizeof (sb), &sb);
>> +  if (grub_errno == GRUB_ERR_OUT_OF_RANGE)
>> +    grub_error (GRUB_ERR_BAD_FS, "not a valid erofs filesystem");
>> +  if (err)
>> +    return NULL;
>> +  if (sb.magic != grub_cpu_to_le32_compile_time (EROFS_MAGIC))
>> +    {
>> +      grub_error (GRUB_ERR_BAD_FS, "not a valid erofs filesystem");
>> +      return NULL;
>> +    }
>> +
>> +  feature = grub_le_to_cpu32 (sb.feature_incompat);
>> +  if (feature & ~EROFS_ALL_FEATURE_INCOMPAT)
>> +    {
>> +      grub_error (GRUB_ERR_BAD_FS, "unsupported features: 0x%x",
>> +		  feature & ~EROFS_ALL_FEATURE_INCOMPAT);
>> +      return NULL;
>> +    }
>> +
>> +  data = grub_malloc (sizeof (*data));
>> +  if (!data)
>> +    {
>> +      grub_error (GRUB_ERR_OUT_OF_MEMORY, "out of memory");
>> +      return NULL;
>> +    }
>> +
>> +  data->disk = disk;
>> +  data->sb = sb;
>> +
>> +  if (read_root)
>> +    {
>> +      data->inode.data = data;
>> +      data->inode.ino = grub_le_to_cpu16 (sb.root_nid);
>> +      err = grub_erofs_read_inode (data, &data->inode);
>> +      if (err)
>> +	{
>> +	  grub_free (data);
>> +	  return NULL;
>> +	}
>> +    }
>> +
>> +  return data;
>> +}
>> +
>> +/* Context for grub_erofs_dir. */
>> +struct grub_erofs_dir_ctx
>> +{
>> +  grub_fs_dir_hook_t hook;
>> +  void *hook_data;
>> +  struct grub_erofs_data *data;
>> +};
>> +
>> +/* Helper for grub_erofs_dir. */
>> +static int
>> +grub_erofs_dir_iter (const char *filename, enum grub_fshelp_filetype filetype,
>> +		     grub_fshelp_node_t node, void *data)
>> +{
>> +  struct grub_erofs_dir_ctx *ctx = data;
>> +  struct grub_dirhook_info info;
>> +
>> +  grub_memset (&info, 0, sizeof (info));
> Again, you can drop grub_memset() if you do
>    struct grub_dirhook_info info = {0};
>
> Please fix similar issues where possible.
>
>> +  if (!node->inode_read)
>> +    {
>> +      grub_erofs_read_inode (ctx->data, node);
>> +      grub_errno = GRUB_ERR_NONE;
>> +    }
>> +
>> +  if (node->inode_read)
>> +    {
>> +      info.mtimeset = 1;
>> +      info.mtime = erofs_inode_mtime (node);
>> +    }
>> +
>> +  info.dir = ((filetype & GRUB_FSHELP_TYPE_MASK) == GRUB_FSHELP_DIR);
>> +  grub_free (node);
>> +  return ctx->hook (filename, &info, ctx->hook_data);
>> +}
>> +
>> +static grub_err_t
>> +grub_erofs_dir (grub_device_t device, const char *path, grub_fs_dir_hook_t hook,
>> +		void *hook_data)
>> +{
>> +  grub_fshelp_node_t fdiro = NULL;
>> +  struct grub_erofs_dir_ctx ctx = {
>> +      .hook = hook,
>> +      .hook_data = hook_data,
> You can drop surplus "," from the end of the line.
Fixed.
>
>> +  };
>> +
>> +  ctx.data = grub_erofs_mount (device->disk, true);
>> +  if (!ctx.data)
>> +    goto fail;
>> +
>> +  grub_fshelp_find_file (path, &ctx.data->inode, &fdiro, grub_erofs_iterate_dir,
>> +			 grub_erofs_read_symlink, GRUB_FSHELP_DIR);
>> +  if (grub_errno)
>> +    goto fail;
>> +
>> +  grub_erofs_iterate_dir (fdiro, grub_erofs_dir_iter, &ctx);
>> +
>> +fail:
> Lack of space before label.
>
>> +  if (fdiro != &ctx.data->inode)
>> +    grub_free (fdiro);
>> +  grub_free (ctx.data);
>> +
>> +  return grub_errno;
>> +}
>> +
>> +static grub_err_t
>> +grub_erofs_open (grub_file_t file, const char *name)
>> +{
>> +  struct grub_erofs_data *data;
>> +  struct grub_fshelp_node *fdiro = 0;
>> +  grub_err_t err;
>> +
>> +  data = grub_erofs_mount (file->device->disk, true);
>> +  if (!data)
>> +    {
>> +      err = grub_errno;
>> +      goto fail;
>> +    }
>> +
>> +  err =
>> +      grub_fshelp_find_file (name, &data->inode, &fdiro, grub_erofs_iterate_dir,
> Please merge these two lines into one...
>
>> +			     grub_erofs_read_symlink, GRUB_FSHELP_REG);
>> +  if (err)
>> +    goto fail;
>> +
>> +  if (!fdiro->inode_read)
>> +    {
>> +      err = grub_erofs_read_inode (data, fdiro);
>> +      if (err)
>> +	goto fail;
>> +    }
>> +
>> +  grub_memcpy (&data->inode, fdiro, sizeof (*fdiro));
>> +  grub_free (fdiro);
>> +
>> +  file->data = data;
>> +  file->size = erofs_inode_file_size (&data->inode);
>> +
>> +  return GRUB_ERR_NONE;
>> +
>> +fail:
>> +  if (fdiro != &data->inode)
>> +    grub_free (fdiro);
>> +  grub_free (data);
>> +
>> +  return err;
>> +}
>> +
>> +static grub_ssize_t
>> +grub_erofs_read (grub_file_t file, char *buf, grub_size_t len)
>> +{
>> +  struct grub_erofs_data *data = file->data;
>> +  struct grub_fshelp_node *inode = &data->inode;
>> +  grub_off_t off = file->offset, ret = 0;
>> +  grub_off_t file_size;
>> +
>> +  if (!inode->inode_read)
>> +    {
>> +      grub_erofs_read_inode (data, inode);
>> +      if (grub_errno)
>> +	{
>> +	  ret = 0;
> This looks redundant.
Fixed.
>
>> +	  grub_error (GRUB_ERR_IO, "cannot read @ inode %" PRIuGRUB_UINT64_T,
>> +		      inode->ino);
>> +	  goto end;
>> +	}
>> +    }
>> +
>> +  file_size = erofs_inode_file_size (inode);
>> +
>> +  if (off >= file_size)
>> +    goto end;
>> +
>> +  if (off + len > file_size)
>> +    len = file_size - off;
>> +
>> +  grub_erofs_read_raw_data (inode, buf, len, off, &ret);
>> +  if (grub_errno)
> Why do not check grub_erofs_read_raw_data() return value?
>
>> +    {
>> +      ret = 0;
> Again, this looks redundant.
>
>> +      grub_error (GRUB_ERR_IO, "cannot read file @ inode %" PRIuGRUB_UINT64_T,
>> +		  inode->ino);
>> +      goto end;
>> +    }
>> +
>> +end:
>> +  return ret;
>> +}
>> +
>> +static grub_err_t
>> +grub_erofs_close (grub_file_t file)
>> +{
>> +  grub_free (file->data);
>> +
>> +  return GRUB_ERR_NONE;
>> +}
>> +
>> +static grub_err_t
>> +grub_erofs_uuid (grub_device_t device, char **uuid)
>> +{
>> +  struct grub_erofs_data *data;
>> +
>> +  grub_errno = GRUB_ERR_NONE;
>> +  data = grub_erofs_mount (device->disk, false);
>> +
>> +  if (data)
>> +    *uuid = grub_xasprintf (
>> +	"%02x%02x%02x%02x-%02x%02x-%02x%02x-%02x%02x-%02x%02x%02x%02x%02x%"
>> +	"02x",
>> +	data->sb.uuid[0], data->sb.uuid[1], data->sb.uuid[2], data->sb.uuid[3],
>> +	data->sb.uuid[4], data->sb.uuid[5], data->sb.uuid[6], data->sb.uuid[7],
>> +	data->sb.uuid[8], data->sb.uuid[9], data->sb.uuid[10],
>> +	data->sb.uuid[11], data->sb.uuid[12], data->sb.uuid[13],
>> +	data->sb.uuid[14], data->sb.uuid[15]);
> Could not you use "%pG" format string?
Fixed, using `grub_packed_guid_t` and %pG instead.
>
>> +  else
>> +    *uuid = NULL;
>> +
>> +  grub_free (data);
>> +
>> +  return grub_errno;
>> +}
>> +
>> +static grub_err_t
>> +grub_erofs_label (grub_device_t device, char **label)
>> +{
>> +  struct grub_erofs_data *data;
>> +
>> +  grub_errno = GRUB_ERR_NONE;
>> +  data = grub_erofs_mount (device->disk, false);
>> +
>> +  if (data)
>> +    *label = grub_strndup ((char *) data->sb.volume_name,
>> +			   sizeof (data->sb.volume_name));
>> +  else
>> +    *label = NULL;
>> +
>> +  grub_free (data);
>> +
>> +  return grub_errno;
> I would not assume grub_errno will not change after grub_strndup() and
> grub_free() calls.
Fixed by reorganizing the logic here and below.
>> +}
>> +
>> +static grub_err_t
>> +grub_erofs_mtime (grub_device_t device, grub_int64_t *tm)
>> +{
>> +  struct grub_erofs_data *data;
>> +
>> +  grub_errno = GRUB_ERR_NONE;
>> +  data = grub_erofs_mount (device->disk, false);
>> +
>> +  if (!data)
>> +    *tm = 0;
>> +  else
>> +    *tm = grub_le_to_cpu64 (data->sb.build_time);
>> +
>> +  grub_free (data);
>> +
>> +  return grub_errno;
> Ditto...
>
> Daniel

_______________________________________________
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel