All the mail mirrored from lore.kernel.org
 help / color / mirror / Atom feed
From: Glenn Washburn <development@efficientek.com>
To: Yifan Zhao <zhaoyifan@sjtu.edu.cn>
Cc: grub-devel@gnu.org, dkiper@net-space.pl,
	Gao Xiang <hsiangkao@linux.alibaba.com>
Subject: Re: [PATCH v5 1/2] fs/erofs: Add support for EROFS
Date: Sat, 27 Apr 2024 01:42:35 -0500	[thread overview]
Message-ID: <20240427014235.4dc6653a@crass-HP-ZBook-15-G2> (raw)
In-Reply-To: <5f4db96f-35d2-405d-aca2-0c348beed86b@sjtu.edu.cn>

On Mon, 22 Apr 2024 12:20:35 +0800
Yifan Zhao <zhaoyifan@sjtu.edu.cn> wrote:

> 
> On 2024/4/22 11:15, Glenn Washburn wrote:
> > On Fri, 19 Apr 2024 01:12:40 +0800
> > Yifan Zhao <zhaoyifan@sjtu.edu.cn> wrote:
> >
> >> On 2024/4/18 16:16, Glenn Washburn wrote:
> >>> On Mon,  4 Mar 2024 01:15:54 +0800
> >>> Yifan Zhao <zhaoyifan@sjtu.edu.cn> 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
> >>>>
> >>>> Signed-off-by: Yifan Zhao <zhaoyifan@sjtu.edu.cn>
> >>>> ---
> >>>>    INSTALL                     |   8 +-
> >>>>    Makefile.util.def           |   1 +
> >>>>    docs/grub.texi              |   3 +-
> >>>>    grub-core/Makefile.core.def |   5 +
> >>>>    grub-core/fs/erofs.c        | 978 ++++++++++++++++++++++++++++++++++++
> >>>>    grub-core/kern/misc.c       |  14 +
> >>>>    include/grub/misc.h         |   1 +
> >>>>    7 files changed, 1005 insertions(+), 5 deletions(-)
> >>>>    create mode 100644 grub-core/fs/erofs.c
> >>>>
> >>>> diff --git a/INSTALL b/INSTALL
> >>>> index 8d9207c84..84030c9f4 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, e2fsprogs, erofs-utils, exfat-utils, f2fs-tools,
> >>>> +    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..34f16ba20
> >>>> --- /dev/null
> >>>> +++ b/grub-core/fs/erofs.c
> >>>> @@ -0,0 +1,978 @@
> >>>> +/* erofs.c - Enhanced Read-Only File System */
> >>>> +/*
> >>>> + *  GRUB  --  GRand Unified Bootloader
> >>>> + *  Copyright (C) 2024 Free Software Foundation, Inc.
> >>>> + *
> >>>> + *  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
> >>>> +#define EROFS_ISLOTBITS		5
> >>>> +
> >>>> +#define EROFS_FEATURE_INCOMPAT_CHUNKED_FILE	0x00000004
> >>>> +#define EROFS_ALL_FEATURE_INCOMPAT		EROFS_FEATURE_INCOMPAT_CHUNKED_FILE
> >>>> +
> >>>> +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_packed_guid_t uuid;
> >>>> +  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
> >>>> +
> >>>> +#define EROFS_INODE_FLAT_PLAIN		0
> >>>> +#define EROFS_INODE_COMPRESSED_FULL	1
> >>>> +#define EROFS_INODE_FLAT_INLINE		2
> >>>> +#define EROFS_INODE_COMPRESSED_COMPACT	3
> >>>> +#define EROFS_INODE_CHUNK_BASED		4
> >>>> +
> >>>> +#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_MAP_MAPPED		0x02
> >>>> +
> >>>> +#define EROFS_NULL_ADDR			1
> >>>> +#define EROFS_NAME_LEN			255
> >>>> +#define EROFS_MAX_LOG2_BLOCK_SIZE	16
> >>>> +
> >>>> +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;
> >>>> +
> >>>> +#define EROFS_FT_UNKNOWN	0
> >>>> +#define EROFS_FT_REG_FILE	1
> >>>> +#define EROFS_FT_DIR		2
> >>>> +#define EROFS_FT_CHRDEV		3
> >>>> +#define EROFS_FT_BLKDEV		4
> >>>> +#define EROFS_FT_FIFO		5
> >>>> +#define EROFS_FT_SOCK		6
> >>>> +#define EROFS_FT_SYMLINK	7
> >>>> +
> >>>> +struct grub_erofs_dirent
> >>>> +{
> >>>> +  grub_uint64_t nid;
> >>>> +  grub_uint16_t nameoff;
> >>>> +  grub_uint8_t file_type;
> >>>> +  grub_uint8_t reserved;
> >>>> +} GRUB_PACKED;
> >>>> +
> >>>> +struct grub_erofs_map_blocks
> >>>> +{
> >>>> +  grub_uint64_t m_pa;
> >>>> +  grub_uint64_t m_la;
> >>>> +  grub_uint64_t m_plen;
> >>>> +  grub_uint64_t m_llen;
> >>>> +  grub_uint32_t m_flags;
> >>> Since the names have been shortened, it would be nice to have some
> >>> comments noting what they mean. I'm guessing that 'l' is short for
> >>> 'logical', 'p' for 'physical', and 'a' for address.
> >> This understanding is correct. Comments have been added.
> >>>> +};
> >>>> +
> >>>> +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_loaded;
> >>>> +};
> >>>> +
> >>>> +struct grub_erofs_data
> >>>> +{
> >>>> +  grub_disk_t disk;
> >>>> +  struct grub_erofs_super sb;
> >>>> +
> >>>> +  struct grub_fshelp_node inode;
> >>>> +};
> >>>> +
> >>>> +#define erofs_blocksz(data) (((grub_uint32_t) 1) << data->sb.log2_blksz)
> >>>> +
> >>>> +static grub_uint64_t
> >>>> +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);
> >>>> +}
> >>>> +
> >>>> +static grub_err_t
> >>>> +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 != GRUB_ERR_NONE)
> >>>> +    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;
> >>> The use of dic->i_format assumes endianness here, correct? Should
> >>> "grub_le_to_cpu16 (dic->i_format)" be used? Please double check the rest
> >>> of the code to make sure its endian agnostic. Does this pass the
> >>> filesystem tests on a big endian machine?
> >> Thanks for pointing it out. Fixed and have checked the rest of code for
> >> endianness issues.
> >>>> +
> >>>> +  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));
> >>> Daniel mentioned in a previous review that grub_uint8_t would be better
> >>> than char. There are other casts that this applies to as well.
> >> Fixed, and this is the only 'char => grub_uint8_t' issue I've found so far.
> > I've noted some others below.
> >
> >>>> +      if (err != GRUB_ERR_NONE)
> >>>> +	return err;
> >>>> +      break;
> >>>> +    case EROFS_INODE_LAYOUT_COMPACT:
> >>>> +      break;
> >>>> +    default:
> >>>> +      return grub_error (GRUB_ERR_BAD_FS, "invalid type %u @ inode %" PRIuGRUB_UINT64_T,
> >>>> +			 node->inode_type, node->ino);
> >>>> +    }
> >>>> +
> >>>> +  node->inode_loaded = true;
> >>>> +
> >>>> +  return 0;
> >>>> +}
> >>>> +
> >>>> +static 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 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 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);
> >>>> +
> >>>> +  if (cnt == 0)
> >>>> +    return 0;
> >>>> +
> >>>> +  return sizeof (struct grub_erofs_xattr_ibody_header) + (cnt - 1) * sizeof (grub_uint32_t);
> >>> I would prefer explicit parenthesis here. It makes it more readable. In
> >>> general, I prefer not relying on operator precedence.
> >> Fixed.
> >>>> +}
> >>>> +
> >>>> +static 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
> >>>> +erofs_map_blocks_flatmode (grub_fshelp_node_t node,
> >>>> +			   struct grub_erofs_map_blocks *map)
> >>>> +{
> >>>> +  grub_uint64_t nblocks, lastblk, file_size;
> >>>> +  bool tailendpacking = (node->inode_datalayout == EROFS_INODE_FLAT_INLINE);
> >>>> +  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 ? 1 : 0);
> >>> Is this tertiary operator needed here? Or is it used to be very
> >>> explicit?
> >> Thanks. I think it's unnecessary to use tertiary operator here. Fixed.
> >>>> +
> >>>> +  map->m_flags = EROFS_MAP_MAPPED;
> >>>> +
> >>>> +  if (map->m_la < (lastblk * blocksz))
> >>>> +    {
> >>>> +      if (grub_mul (grub_le_to_cpu32 (node->inode.i_u.raw_blkaddr), blocksz, &map->m_pa) ||
> >>>> +	  grub_add (map->m_pa, map->m_la, &map->m_pa))
> >>>> +	return GRUB_ERR_OUT_OF_RANGE;
> >>>> +      if (grub_sub (lastblk * blocksz, map->m_la, &map->m_plen))
> >>>> +	return GRUB_ERR_OUT_OF_RANGE;
> >>>> +    }
> >>>> +  else if (tailendpacking)
> >>>> +    {
> >>>> +      if (grub_add (erofs_iloc (node), erofs_inode_size (node), &map->m_pa) ||
> >>>> +	  grub_add (map->m_pa, erofs_inode_xattr_ibody_size (node), &map->m_pa) ||
> >>>> +	  grub_add (map->m_pa, map->m_la % blocksz, &map->m_pa))
> >>>> +	return GRUB_ERR_OUT_OF_RANGE;
> >>>> +      if (grub_sub (file_size, map->m_la, &map->m_plen))
> >>>> +	return GRUB_ERR_OUT_OF_RANGE;
> >>>> +
> >>>> +      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
> >>>> +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_uint64_t unit, pos, 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);
> >>>> +  if (chunkbits > 64)
> >>>> +    return grub_error (GRUB_ERR_BAD_FS, "invalid chunkbits %u @ inode %" PRIuGRUB_UINT64_T,
> >>>> +		       chunkbits, node->ino);
> >>>> +
> >>>> +  chunknr = map->m_la >> chunkbits;
> >>>> +
> >>>> +  if (grub_add (erofs_iloc (node), erofs_inode_size (node), &pos) ||
> >>>> +      grub_add (pos, erofs_inode_xattr_ibody_size (node), &pos))
> >>>> +    return GRUB_ERR_OUT_OF_RANGE;
> >>>> +  pos = ALIGN_UP (pos, unit);
> >>>> +  if (grub_add (pos, chunknr * unit, &pos))
> >>>> +    return GRUB_ERR_OUT_OF_RANGE;
> >>>> +
> >>>> +  map->m_la = chunknr << chunkbits;
> >>>> +
> >>>> +  if (grub_sub (erofs_inode_file_size (node), map->m_la, &map->m_plen))
> >>>> +    return GRUB_ERR_OUT_OF_RANGE;
> >>>> +  map->m_plen = grub_min (((grub_uint64_t) 1) << chunkbits,
> >>>> +			  ALIGN_UP (map->m_plen, 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 != GRUB_ERR_NONE)
> >>>> +	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 != GRUB_ERR_NONE)
> >>>> +	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
> >>>> +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 erofs_map_blocks_flatmode (node, map);
> >>>> +  else
> >>>> +    return erofs_map_blocks_chunkmode (node, map);
> >>>> +}
> >>>> +
> >>>> +static grub_err_t
> >>>> +erofs_read_raw_data (grub_fshelp_node_t node, char *buf, grub_uint64_t size,
> > Seems like this should be a grub_uint8_t *.
> >
> >>>> +		     grub_uint64_t offset, grub_uint64_t *bytes)
> >>>> +{
> >>>> +  struct grub_erofs_map_blocks map = {0};
> >>>> +  grub_uint64_t cur;
> >>>> +  grub_err_t err;
> >>>> +
> >>>> +  if (bytes)
> >>>> +    *bytes = 0;
> >>>> +
> >>>> +  if (!node->inode_loaded)
> >>>> +    {
> >>>> +      err = erofs_read_inode (node->data, node);
> >>>> +      if (err != GRUB_ERR_NONE)
> >>>> +	return err;
> >>>> +    }
> >>>> +
> >>>> +  cur = offset;
> >>>> +  while (cur < offset + size)
> >>>> +    {
> >>>> +      char *const estart = buf + cur - offset;
> > And grub_uint8_t * here too.
> >
> >>>> +      grub_uint64_t eend, moff = 0;
> >>>> +
> >>>> +      map.m_la = cur;
> >>>> +      err = erofs_map_blocks (node, &map);
> >>>> +      if (err != GRUB_ERR_NONE)
> >>>> +	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);
> >>>> +	  if (bytes)
> >>>> +	    *bytes += eend - cur;
> >>>> +	  cur = eend;
> >>>> +	  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 != GRUB_ERR_NONE)
> >>>> +	return err;
> >>>> +
> >>>> +      if (bytes)
> >>>> +	*bytes += eend - map.m_la;
> >>>> +
> >>>> +      cur = eend;
> >>>> +    }
> >>>> +
> >>>> +  return GRUB_ERR_NONE;
> >>>> +}
> >>>> +
> >>>> +static int
> >>>> +erofs_iterate_dir (grub_fshelp_node_t dir, grub_fshelp_iterate_dir_hook_t hook,
> >>>> +		   void *hook_data)
> >>>> +{
> >>>> +  grub_uint64_t offset = 0, file_size;
> >>>> +  grub_uint32_t blocksz = erofs_blocksz (dir->data);
> >>>> +  char *buf;
> > And grub_uint8_t * here too.
> >
> >>>> +  grub_err_t err;
> >>>> +
> >>>> +  if (!dir->inode_loaded)
> >>>> +    {
> >>>> +      err = erofs_read_inode (dir->data, dir);
> >>>> +      if (err != GRUB_ERR_NONE)
> >>>> +	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_uint64_t maxsize = grub_min (blocksz, file_size - offset);
> >>>> +      struct grub_erofs_dirent *de = (void *) buf, *end;
> >>>> +      grub_uint16_t nameoff;
> >>>> +
> >>>> +      err = erofs_read_raw_data (dir, buf, maxsize, offset, NULL);
> >>>> +      if (err != GRUB_ERR_NONE)
> >>>> +	goto not_found;
> >>>> +
> >>>> +      nameoff = grub_le_to_cpu16 (de->nameoff);
> >>>> +      if (nameoff < sizeof (struct grub_erofs_dirent) || nameoff > maxsize)
> >>>> +	{
> >>>> +	  grub_error (GRUB_ERR_BAD_FS,
> >>>> +		      "invalid nameoff %u @ inode %" PRIuGRUB_UINT64_T,
> >>>> +		      nameoff, dir->ino);
> >>>> +	  goto not_found;
> >>>> +	}
> >>>> +
> >>>> +      end = (struct grub_erofs_dirent *) ((grub_uint8_t *) de + nameoff);
> >>>> +      while (de < end)
> >>>> +	{
> >>>> +	  struct grub_fshelp_node *fdiro;
> >>>> +	  enum grub_fshelp_filetype type;
> >>>> +	  char filename[EROFS_NAME_LEN + 1];
> >>>> +	  grub_size_t 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_loaded = false;
> >>>> +
> >>>> +	  nameoff = grub_le_to_cpu16 (de->nameoff);
> >>>> +	  if (nameoff < sizeof (struct grub_erofs_dirent) || nameoff > maxsize)
> >>>> +	    {
> >>>> +	      grub_error (GRUB_ERR_BAD_FS,
> >>>> +			  "invalid nameoff %u @ inode %" PRIuGRUB_UINT64_T,
> >>>> +			  nameoff, dir->ino);
> >>>> +	      goto not_found;
> >>>> +	    }
> >>>> +
> >>>> +	  de_name = buf + nameoff;
> >>>> +	  if (de + 1 >= end)
> >>>> +	    de_namelen = grub_strnlen (de_name, maxsize - nameoff);
> >>>> +	  else
> >>>> +	    de_namelen = grub_le_to_cpu16 (de[1].nameoff) - nameoff;
> >>>> +
> >>>> +	  if (nameoff + de_namelen > maxsize || de_namelen > EROFS_NAME_LEN)
> >>>> +	    {
> >>>> +	      grub_error (GRUB_ERR_BAD_FS,
> >>>> +			  "invalid de_namelen %" PRIuGRUB_SIZE
> >>>> +			  " @ inode %" PRIuGRUB_UINT64_T,
> >>>> +			  de_namelen, dir->ino);
> >>>> +	      goto not_found;
> >>>> +	    }
> >>>> +
> >>>> +	  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;
> >>>> +	    }
> >>>> +
> >>>> +	  if (hook (filename, type, fdiro, hook_data))
> >>>> +	    {
> >>>> +	      grub_free (buf);
> >>>> +	      return 1;
> >>>> +	    }
> >>>> +
> >>>> +	  ++de;
> >>>> +	}
> >>>> +
> >>>> +      offset += maxsize;
> >>>> +    }
> >>>> +
> >>>> + not_found:
> >>>> +  grub_free (buf);
> >>>> +  return 0;
> >>>> +}
> >>>> +
> >>>> +static char *
> >>>> +erofs_read_symlink (grub_fshelp_node_t node)
> >>>> +{
> >>>> +  char *symlink;
> >>>> +  grub_size_t sz;
> >>>> +  grub_err_t err;
> >>>> +
> >>>> +  if (!node->inode_loaded)
> >>>> +    {
> >>>> +      err = erofs_read_inode (node->data, node);
> >>>> +      if (err != GRUB_ERR_NONE)
> >>>> +	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;
> >>>> +
> >>>> +  err = erofs_read_raw_data (node, symlink, sz - 1, 0, NULL);
> >>>> +  if (err != GRUB_ERR_NONE)
> >>>> +    {
> >>>> +      grub_free (symlink);
> >>>> +      return NULL;
> >>>> +    }
> >>>> +
> >>>> +  symlink[sz - 1] = '\0';
> >>>> +  return symlink;
> >>>> +}
> >>>> +
> >>>> +static struct grub_erofs_data *
> >>>> +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 != GRUB_ERR_NONE)
> >>>> +    return NULL;
> >>>> +  if (sb.magic != grub_cpu_to_le32_compile_time (EROFS_MAGIC) ||
> >>>> +      grub_le_to_cpu32 (sb.log2_blksz) > EROFS_MAX_LOG2_BLOCK_SIZE)
> >>>> +    {
> >>>> +      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 = erofs_read_inode (data, &data->inode);
> >>>> +      if (err != GRUB_ERR_NONE)
> >>>> +	{
> >>>> +	  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
> >>>> +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 = {0};
> >>>> +
> >>>> +  if (!node->inode_loaded)
> >>>> +    {
> >>>> +      erofs_read_inode (ctx->data, node);
> >>>> +      grub_errno = GRUB_ERR_NONE;
> >>>> +    }
> >>>> +
> >>>> +  if (node->inode_loaded)
> >>>> +    {
> >>>> +      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;
> >>>> +  grub_err_t err;
> >>>> +  struct grub_erofs_dir_ctx ctx = {
> >>>> +      .hook = hook,
> >>>> +      .hook_data = hook_data
> >>>> +  };
> >>>> +
> >>>> +  ctx.data = erofs_mount (device->disk, true);
> >>>> +  if (!ctx.data)
> >>>> +    goto fail;
> >>>> +
> >>>> +  err = grub_fshelp_find_file (path, &ctx.data->inode, &fdiro, erofs_iterate_dir,
> >>>> +			       erofs_read_symlink, GRUB_FSHELP_DIR);
> >>>> +  if (err != GRUB_ERR_NONE)
> >>>> +    goto fail;
> >>>> +
> >>>> +  erofs_iterate_dir (fdiro, erofs_dir_iter, &ctx);
> >>>> +
> >>>> + fail:
> >>>> +  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;
> >>> s/0/NULL/
> >>>
> >>> Glenn
> >> Fixed.
> >>
> >>
> >> Thanks,
> >>
> >> Yifan Zhao
> >>
> >>>> +  grub_err_t err;
> >>>> +
> >>>> +  data = erofs_mount (file->device->disk, true);
> >>>> +  if (!data)
> >>>> +    {
> >>>> +      err = grub_errno;
> >>>> +      goto fail;
> >>>> +    }
> >>>> +
> >>>> +  err = grub_fshelp_find_file (name, &data->inode, &fdiro, erofs_iterate_dir,
> >>>> +			       erofs_read_symlink, GRUB_FSHELP_REG);
> >>>> +  if (err != GRUB_ERR_NONE)
> >>>> +    goto fail;
> >>>> +
> >>>> +  if (!fdiro->inode_loaded)
> >>>> +    {
> >>>> +      err = erofs_read_inode (data, fdiro);
> >>>> +      if (err != GRUB_ERR_NONE)
> >>>> +	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)
> > And grub_uint8_t * here too.
> 
> One additional point. I don't think this could be changed as the 
> interface declaration in `struct grub_fs` is `char*`.

Good point, keep as is.

Glenn

> 
> Other modifications as required.
> 
> 
> Thanks,
> 
> Yifan Zhao
> 
> >
> > These are all the ones I see.
> >
> > Glenn
> >
> >>>> +{
> >>>> +  struct grub_erofs_data *data = file->data;
> >>>> +  struct grub_fshelp_node *inode = &data->inode;
> >>>> +  grub_off_t off = file->offset;
> >>>> +  grub_uint64_t ret = 0, file_size;
> >>>> +  grub_err_t err;
> >>>> +
> >>>> +  if (!inode->inode_loaded)
> >>>> +    {
> >>>> +      err = erofs_read_inode (data, inode);
> >>>> +      if (err != GRUB_ERR_NONE)
> >>>> +	{
> >>>> +	  grub_error (GRUB_ERR_IO, "cannot read @ inode %" PRIuGRUB_UINT64_T, inode->ino);
> >>>> +	  return -1;
> >>>> +	}
> >>>> +    }
> >>>> +
> >>>> +  file_size = erofs_inode_file_size (inode);
> >>>> +
> >>>> +  if (off > file_size)
> >>>> +    {
> >>>> +      grub_error (GRUB_ERR_IO, "read past EOF @ inode %" PRIuGRUB_UINT64_T, inode->ino);
> >>>> +      return -1;
> >>>> +    }
> >>>> +  if (off == file_size)
> >>>> +    return 0;
> >>>> +
> >>>> +  if (off + len > file_size)
> >>>> +    len = file_size - off;
> >>>> +
> >>>> +  err = erofs_read_raw_data (inode, buf, len, off, &ret);
> >>>> +  if (err != GRUB_ERR_NONE)
> >>>> +    {
> >>>> +      grub_error (GRUB_ERR_IO, "cannot read file @ inode %" PRIuGRUB_UINT64_T, inode->ino);
> >>>> +      return -1;
> >>>> +    }
> >>>> +
> >>>> +  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;
> >>>> +
> >>>> +  data = erofs_mount (device->disk, false);
> >>>> +  if (!data)
> >>>> +    {
> >>>> +      *uuid = NULL;
> >>>> +      return grub_errno;
> >>>> +    }
> >>>> +
> >>>> +  *uuid = grub_xasprintf ("%pG", &data->sb.uuid);
> >>>> +
> >>>> +  grub_free (data);
> >>>> +
> >>>> +  return GRUB_ERR_NONE;
> >>>> +}
> >>>> +
> >>>> +static grub_err_t
> >>>> +grub_erofs_label (grub_device_t device, char **label)
> >>>> +{
> >>>> +  struct grub_erofs_data *data;
> >>>> +
> >>>> +  data = erofs_mount (device->disk, false);
> >>>> +  if (!data)
> >>>> +    {
> >>>> +      *label = NULL;
> >>>> +      return grub_errno;
> >>>> +    }
> >>>> +
> >>>> +  *label = grub_strndup ((char *) data->sb.volume_name, sizeof (data->sb.volume_name));
> >>>> +  if (!*label)
> >>>> +    return grub_errno;
> >>>> +
> >>>> +  grub_free (data);
> >>>> +
> >>>> +  return GRUB_ERR_NONE;
> >>>> +}
> >>>> +
> >>>> +static grub_err_t
> >>>> +grub_erofs_mtime (grub_device_t device, grub_int64_t *tm)
> >>>> +{
> >>>> +  struct grub_erofs_data *data;
> >>>> +
> >>>> +  data = erofs_mount (device->disk, false);
> >>>> +  if (!data)
> >>>> +    {
> >>>> +      *tm = 0;
> >>>> +      return grub_errno;
> >>>> +    }
> >>>> +
> >>>> +  *tm = grub_le_to_cpu64 (data->sb.build_time);
> >>>> +
> >>>> +  grub_free (data);
> >>>> +
> >>>> +  return GRUB_ERR_NONE;
> >>>> +}
> >>>> +
> >>>> +static struct grub_fs grub_erofs_fs = {
> >>>> +    .name = "erofs",
> >>>> +    .fs_dir = grub_erofs_dir,
> >>>> +    .fs_open = grub_erofs_open,
> >>>> +    .fs_read = grub_erofs_read,
> >>>> +    .fs_close = grub_erofs_close,
> >>>> +    .fs_uuid = grub_erofs_uuid,
> >>>> +    .fs_label = grub_erofs_label,
> >>>> +    .fs_mtime = grub_erofs_mtime,
> >>>> +#ifdef GRUB_UTIL
> >>>> +    .reserved_first_sector = 1,
> >>>> +    .blocklist_install = 0,
> >>>> +#endif
> >>>> +    .next = 0,
> >>>> +};
> >>>> +
> >>>> +GRUB_MOD_INIT (erofs)
> >>>> +{
> >>>> +  grub_fs_register (&grub_erofs_fs);
> >>>> +}
> >>>> +
> >>>> +GRUB_MOD_FINI (erofs)
> >>>> +{
> >>>> +  grub_fs_unregister (&grub_erofs_fs);
> >>>> +}
> >>>> \ No newline at end of file
> >>>> diff --git a/grub-core/kern/misc.c b/grub-core/kern/misc.c
> >>>> index 7cee5d75c..ecb27fa5a 100644
> >>>> --- a/grub-core/kern/misc.c
> >>>> +++ b/grub-core/kern/misc.c
> >>>> @@ -594,6 +594,20 @@ grub_strlen (const char *s)
> >>>>      return p - s;
> >>>>    }
> >>>>    
> >>>> +grub_size_t
> >>>> +grub_strnlen (const char *s, grub_size_t n)
> >>>> +{
> >>>> +  const char *p = s;
> >>>> +
> >>>> +  if (n == 0)
> >>>> +    return 0;
> >>>> +
> >>>> +  while (*p && n--)
> >>>> +    p++;
> >>>> +
> >>>> +  return p - s;
> >>>> +}
> >>>> +
> >>>>    static inline void
> >>>>    grub_reverse (char *str)
> >>>>    {
> >>>> diff --git a/include/grub/misc.h b/include/grub/misc.h
> >>>> index 1b35a167f..eb4aa55c1 100644
> >>>> --- a/include/grub/misc.h
> >>>> +++ b/include/grub/misc.h
> >>>> @@ -335,6 +335,7 @@ char *EXPORT_FUNC(grub_strdup) (const char *s) WARN_UNUSED_RESULT;
> >>>>    char *EXPORT_FUNC(grub_strndup) (const char *s, grub_size_t n) WARN_UNUSED_RESULT;
> >>>>    void *EXPORT_FUNC(grub_memset) (void *s, int c, grub_size_t n);
> >>>>    grub_size_t EXPORT_FUNC(grub_strlen) (const char *s) WARN_UNUSED_RESULT;
> >>>> +grub_size_t EXPORT_FUNC(grub_strnlen) (const char *s, grub_size_t n) WARN_UNUSED_RESULT;
> >>>>    
> >>>>    /* Replace all `ch' characters of `input' with `with' and copy the
> >>>>       result into `output'; return EOS address of `output'. */

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

  reply	other threads:[~2024-04-27  6:43 UTC|newest]

Thread overview: 14+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2024-03-03 17:15 [PATCH v5 0/2] Introduce EROFS support Yifan Zhao
2024-03-03 17:15 ` [PATCH v5 1/2] fs/erofs: Add support for EROFS Yifan Zhao
2024-03-06  2:18   ` Gao Xiang
2024-04-04 20:56     ` Daniel Kiper
2024-04-10 11:23       ` Gao Xiang
2024-04-18  8:16   ` Glenn Washburn
2024-04-18 17:12     ` Yifan Zhao
2024-04-22  3:15       ` Glenn Washburn
2024-04-22  3:35         ` Yifan Zhao
2024-04-22  4:20         ` Yifan Zhao
2024-04-27  6:42           ` Glenn Washburn [this message]
2024-03-03 17:15 ` [PATCH v5 2/2] fs/erofs: Add tests for EROFS in grub-fs-tester Yifan Zhao
2024-04-18  8:19   ` Glenn Washburn
2024-03-03 17:17 ` [PATCH v4~v2 Interdiff] Introduce EROFS support Yifan Zhao

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20240427014235.4dc6653a@crass-HP-ZBook-15-G2 \
    --to=development@efficientek.com \
    --cc=dkiper@net-space.pl \
    --cc=grub-devel@gnu.org \
    --cc=hsiangkao@linux.alibaba.com \
    --cc=zhaoyifan@sjtu.edu.cn \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.