grub-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


From: Glenn Washburn
Subject: Re: [PATCH v5 1/2] fs/erofs: Add support for EROFS
Date: Sat, 27 Apr 2024 01:42:35 -0500

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'. */



reply via email to

[Prev in Thread] Current Thread [Next in Thread]