All the mail mirrored from lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 1/2] fs/erofs: Add support for EROFS
@ 2023-07-16 16:03 Yifan Zhao
  2023-07-17  3:51 ` Glenn Washburn
  0 siblings, 1 reply; 11+ messages in thread
From: Yifan Zhao @ 2023-07-16 16:03 UTC (permalink / raw
  To: grub-devel; +Cc: hsiangkao, jefflexu, Yifan Zhao

EROFS is a lightweight read-only filesystem designed for performance which
has already been 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 support will be developed later since it has more work
to polish.

Signed-off-by: Yifan Zhao <zhaoyifan@sjtu.edu.cn>
---
The link here shows how to boot the kernel from the EROFS partition with this patch.
https://precious-celery-715.notion.site/Demo-of-booting-kernel-in-a-EROFS-partition-c6d199f937a2489bafd03d3083c294b5

 INSTALL                     |   8 +-
 Makefile.util.def           |   1 +
 docs/grub.texi              |   4 +-
 grub-core/Makefile.core.def |   5 +
 grub-core/fs/erofs.c        | 971 ++++++++++++++++++++++++++++++++++++
 grub-core/kern/misc.c       |  14 +
 include/grub/misc.h         |   1 +
 7 files changed, 998 insertions(+), 6 deletions(-)
 create mode 100644 grub-core/fs/erofs.c

diff --git a/INSTALL b/INSTALL
index b93fe9c61..1939e4745 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,
+    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 1e9a13d3e..ca2a32a7f 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 b81267980..20c119c96 100644
--- a/docs/grub.texi
+++ b/docs/grub.texi
@@ -361,7 +361,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{Linux ext2/ext3/ext4}, @dfn{DOS FAT12/FAT16/FAT32},
+@dfn{EROFS}, @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),
 @dfn{JFS}, @dfn{Minix fs} (versions 1, 2 and 3), @dfn{nilfs2},
@@ -6212,7 +6212,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 d2cf29584..eccf9b6cf 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..f1366ce05
--- /dev/null
+++ b/grub-core/fs/erofs.c
@@ -0,0 +1,971 @@
+/* erofs.c - Enhanced Read-Only File System */
+/*
+ *  GRUB  --  GRand Unified Bootloader
+ *  Copyright (C) 2023 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 ROUND_UP(x, y) ({ grub_divmod64 (x + (y - 1), y, NULL) * y; })
+
+#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 blkszbits;
+  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 dirblkbits;
+  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
+};
+
+#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
+};
+
+#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;
+
+enum
+{
+  BH_Meta,
+  BH_Mapped,
+};
+
+#define EROFS_MAP_MAPPED (1 << BH_Mapped)
+
+struct grub_erofs_map_blocks
+{
+  grub_off_t m_pa, m_la;
+  grub_off_t m_plen, m_llen;
+  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;
+};
+
+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.blkszbits)
+
+static inline 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->blkszbits) +
+         (node->ino << EROFS_ISLOTBITS);
+}
+
+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)
+    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;
+
+  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)
+{
+  grub_off_t nblocks, lastblk, file_size;
+  grub_off_t tailendpacking =
+      (node->inode_datalayout == EROFS_INODE_FLAT_INLINE) ? 1 : 0;
+  grub_uint32_t blocksz = erofs_blocksz (node->data);
+
+  file_size = erofs_inode_file_size (node);
+  nblocks = grub_divmod64 (file_size + blocksz - 1, blocksz, NULL);
+  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;
+    }
+  else if (tailendpacking)
+    {
+      map->m_pa = erofs_iloc (node) + erofs_inode_size (node) +
+                  erofs_inode_xattr_ibody_size (node) + map->m_la % blocksz;
+      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,
+                         "internal error: 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.blkszbits +
+              (chunk_format & EROFS_CHUNK_FORMAT_BLKBITS_MASK);
+
+  chunknr = map->m_la >> chunkbits;
+  pos = ROUND_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,
+                          ROUND_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.blkszbits;
+          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.blkszbits;
+          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)
+{
+  struct grub_erofs_map_blocks map;
+  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));
+
+  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);
+      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);
+          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;
+            }
+
+          if (hook (filename, type, fdiro, hook_data))
+            {
+              grub_free (buf);
+              return 1;
+            }
+
+          ++de;
+        }
+
+      offset += maxsize;
+    }
+
+not_found:
+  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)
+    {
+      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));
+  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,
+  };
+
+  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:
+  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,
+                             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;
+          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)
+    {
+      ret = 0;
+      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_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]);
+  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_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;
+}
+
+static grub_err_t
+grub_erofs_mtime (grub_device_t device, grub_int64_t *tm)
+{
+  struct grub_erofs_data *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;
+}
+
+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 2890aad49..66cbb3c5c 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'. */
-- 
2.41.0



^ permalink raw reply related	[flat|nested] 11+ messages in thread

* Re: [PATCH 1/2] fs/erofs: Add support for EROFS
  2023-07-16 16:03 Yifan Zhao
@ 2023-07-17  3:51 ` Glenn Washburn
  2023-07-17  5:17   ` Gao Xiang
  0 siblings, 1 reply; 11+ messages in thread
From: Glenn Washburn @ 2023-07-17  3:51 UTC (permalink / raw
  To: Yifan Zhao; +Cc: The development of GNU GRUB, hsiangkao, jefflexu

On Mon, 17 Jul 2023 00:03:08 +0800
Yifan Zhao <zhaoyifan@sjtu.edu.cn> wrote:

> EROFS is a lightweight read-only filesystem designed for performance which
> has already been 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 support will be developed later since it has more work
> to polish.

I would expect that a very small minority are using EROFS without compression.
Does that also seem true to you? This is relevant because I believe GRUB is
in a feature freeze, but if there were a compelling reason to include this in
the upcoming release, Daniel might make an exception (just guessing). On the
one hand, in favor of including the current code without compression in the
next release is that its simple enough and doesn't touch existing code, so a
bug should have minor impact on the rest of GRUB. On the other hand, if there
is a low probability of its use, perhaps its not worth it. As it stands, it
seems that GRUB is being released every couple years.

When you create an updated patch series, it would be much appreciated if you
use --range-diff or --interdiff so that reviewers (me) can verify more easily
the changes.

> 
> Signed-off-by: Yifan Zhao <zhaoyifan@sjtu.edu.cn>
> ---
> The link here shows how to boot the kernel from the EROFS partition with this patch.
> https://precious-celery-715.notion.site/Demo-of-booting-kernel-in-a-EROFS-partition-c6d199f937a2489bafd03d3083c294b5
> 
>  INSTALL                     |   8 +-
>  Makefile.util.def           |   1 +
>  docs/grub.texi              |   4 +-
>  grub-core/Makefile.core.def |   5 +
>  grub-core/fs/erofs.c        | 971 ++++++++++++++++++++++++++++++++++++
>  grub-core/kern/misc.c       |  14 +
>  include/grub/misc.h         |   1 +
>  7 files changed, 998 insertions(+), 6 deletions(-)
>  create mode 100644 grub-core/fs/erofs.c
> 
> diff --git a/INSTALL b/INSTALL
> index b93fe9c61..1939e4745 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,
> +    genromfs, hfsprogs, jfsutils, nilfs-tools, ntfs-3g, reiserfsprogs,
> +    squashfs-tools, reiserfsprogs, udftools, xfsprogs, zfs-fuse

Thank you for updating the documentation. It would be nice to also update
docs/grub.texi where appropriate and note that there is no compression
support. I see two places that could use an update (case-insensitive search
for "btrfs" should find them).

>    - 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 1e9a13d3e..ca2a32a7f 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 b81267980..20c119c96 100644
> --- a/docs/grub.texi
> +++ b/docs/grub.texi
> @@ -361,7 +361,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{Linux ext2/ext3/ext4}, @dfn{DOS FAT12/FAT16/FAT32},
> +@dfn{EROFS}, @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),
>  @dfn{JFS}, @dfn{Minix fs} (versions 1, 2 and 3), @dfn{nilfs2},
> @@ -6212,7 +6212,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 d2cf29584..eccf9b6cf 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..f1366ce05
> --- /dev/null
> +++ b/grub-core/fs/erofs.c
> @@ -0,0 +1,971 @@
> +/* erofs.c - Enhanced Read-Only File System */
> +/*
> + *  GRUB  --  GRand Unified Bootloader
> + *  Copyright (C) 2023 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 ROUND_UP(x, y) ({ grub_divmod64 (x + (y - 1), y, NULL) * y; })

Is there a reason ALIGN_UP is not used instead of the above macro?

> +
> +#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 blkszbits;

Perhaps rename to "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 dirblkbits;

And maybe log2_dirblksz? And I notice that this is never used. I know nothing
about EROFS, but this looks like it has to do with directories. What is this
used for, if its not needed in the directory reading code below?

> +  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
> +};
> +
> +#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
> +};
> +
> +#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;
> +
> +enum
> +{
> +  BH_Meta,
> +  BH_Mapped,
> +};
> +
> +#define EROFS_MAP_MAPPED (1 << BH_Mapped)
> +
> +struct grub_erofs_map_blocks
> +{
> +  grub_off_t m_pa, m_la;
> +  grub_off_t m_plen, m_llen;
> +  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;
> +};
> +
> +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.blkszbits)
> +
> +static inline 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->blkszbits) +
> +         (node->ino << EROFS_ISLOTBITS);
> +}
> +
> +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),

8 spaces should be converted to tabs.

> +                        sizeof (struct grub_erofs_inode_compact), dic);
> +  if (err)
> +    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;
> +
> +  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)
> +{
> +  grub_off_t nblocks, lastblk, file_size;
> +  grub_off_t tailendpacking =
> +      (node->inode_datalayout == EROFS_INODE_FLAT_INLINE) ? 1 : 0;
> +  grub_uint32_t blocksz = erofs_blocksz (node->data);
> +
> +  file_size = erofs_inode_file_size (node);
> +  nblocks = grub_divmod64 (file_size + blocksz - 1, blocksz, NULL);

Would it not be better to do the following?

  nblocks = (file_size + (1U << node->data->sb.blkszbits)) >> node->data->sb.blkszbits;

And if so, then maybe create a erofs_log_blocksz() macro.

> +  lastblk = nblocks - tailendpacking;
> +
> +  map->m_flags = EROFS_MAP_MAPPED;
> +
> +  if (map->m_la < lastblk * blocksz)

s/lastblk * blocksz/(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;
> +    }
> +  else if (tailendpacking)
> +    {
> +      map->m_pa = erofs_iloc (node) + erofs_inode_size (node) +
> +                  erofs_inode_xattr_ibody_size (node) + map->m_la % blocksz;

s/map->m_la % blocksz/(map->m_la % blocksz)/

> +      map->m_plen = file_size - map->m_la;
> +
> +      if (map->m_pa % blocksz + map->m_plen > blocksz)

s/map->m_pa % blocksz + map->m_plen/((map->m_pa % blocksz) + map->m_plen)/

Add explicit parenthesis for the code below too.

> +        return grub_error (
> +            GRUB_ERR_BAD_FS,
> +            "inline data cross block boundary @ inode %" PRIuGRUB_UINT64_T,
> +            node->ino);
> +    }
> +  else
> +    {

This parenthesis is unneeded.

> +      return grub_error (GRUB_ERR_BAD_FS,
> +                         "internal error: 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.blkszbits +
> +              (chunk_format & EROFS_CHUNK_FORMAT_BLKBITS_MASK);
> +
> +  chunknr = map->m_la >> chunkbits;
> +  pos = ROUND_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,
> +                          ROUND_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)

It is preferred to have a space after the cast parenthesis, elsewhere also.

> +        {
> +          map->m_pa = 0;
> +          map->m_flags = 0;
> +        }
> +      else
> +        {
> +          map->m_pa = blkaddr << node->data->sb.blkszbits;
> +          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.blkszbits;
> +          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)
> +{
> +  struct grub_erofs_map_blocks map;
> +  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));
> +
> +  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);
> +      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);
> +          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;
> +            }
> +
> +          if (hook (filename, type, fdiro, hook_data))
> +            {
> +              grub_free (buf);
> +              return 1;
> +            }
> +
> +          ++de;
> +        }
> +
> +      offset += maxsize;
> +    }
> +
> +not_found:
> +  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)
> +    {
> +      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));
> +  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,
> +  };
> +
> +  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:
> +  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,
> +                             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;
> +          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)
> +    {
> +      ret = 0;
> +      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_erofs_mount (device->disk, false);

This should probably be changed to the following so that we don't return a previous error.

  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]);

I want to point out that grub_xasprintf can fail and not set grub_errno,
namely when calling grub_malloc. In this case this function *should not*
return GRUB_ERR_NONE (but likely will and if it doesn't it will be from
some previous failure unrelated to this function). I consider this a bug in
grub_xasprintf, so this ideally is correct. I think I'll send a patch for this.

Glenn

> +  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_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;
> +}
> +
> +static grub_err_t
> +grub_erofs_mtime (grub_device_t device, grub_int64_t *tm)
> +{
> +  struct grub_erofs_data *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;
> +}
> +
> +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 2890aad49..66cbb3c5c 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'. */


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 1/2] fs/erofs: Add support for EROFS
  2023-07-17  3:51 ` Glenn Washburn
@ 2023-07-17  5:17   ` Gao Xiang
  0 siblings, 0 replies; 11+ messages in thread
From: Gao Xiang @ 2023-07-17  5:17 UTC (permalink / raw
  To: development, Yifan Zhao; +Cc: The development of GNU GRUB, jefflexu

Hi Glenn,

On 2023/7/17 11:51, Glenn Washburn wrote:
> On Mon, 17 Jul 2023 00:03:08 +0800
> Yifan Zhao <zhaoyifan@sjtu.edu.cn> wrote:
> 
>> EROFS is a lightweight read-only filesystem designed for performance which
>> has already been 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 support will be developed later since it has more work
>> to polish.
> 
> I would expect that a very small minority are using EROFS without compression.
> Does that also seem true to you? This is relevant because I believe GRUB is
> in a feature freeze, but if there were a compelling reason to include this in
> the upcoming release, Daniel might make an exception (just guessing). On the
> one hand, in favor of including the current code without compression in the
> next release is that its simple enough and doesn't touch existing code, so a
> bug should have minor impact on the rest of GRUB. On the other hand, if there
> is a low probability of its use, perhaps its not worth it. As it stands, it

Thanks for the reply!

EROFS can have per-file compression, which means some files could be compressed,
the others are not.  Therefore, the majority of rootfs can still be compressed,
but leave the real kernel image uncompressed though, so we could still boot
systems from EROFS filesystems with EROFS rootfs images.

One barrier to support EROFS compression is that there is no public LZ4 library
in grub (Yifan said there is one in the ZFS filesystem, but no generic one.)

Personally I'd suggest support uncompressed EROFS for now since it removes
another filesystem dependency of EROFS rootfs so we don't need to add another
partition for this.


> seems that GRUB is being released every couple years.

Ok, I think if we missed this release, we will wait for another two years.

Thanks,
Gao Xiang


> 
> When you create an updated patch series, it would be much appreciated if you
> use --range-diff or --interdiff so that reviewers (me) can verify more easily
> the changes.
> 
>>
>> Signed-off-by: Yifan Zhao <zhaoyifan@sjtu.edu.cn>


^ permalink raw reply	[flat|nested] 11+ messages in thread

* RE: [PATCH 1/2] fs/erofs: Add support for EROFS
@ 2023-07-20 13:14 zhaoyifan
  2023-07-20 17:37 ` Glenn Washburn
  0 siblings, 1 reply; 11+ messages in thread
From: zhaoyifan @ 2023-07-20 13:14 UTC (permalink / raw
  To: development, hsiangkao; +Cc: grub-devel, jefflexu

Thank you for the reply!

As Xiang pointed out, we believe that uncompressed support for EROFS is still valuable, so I would be very grateful if this support could be merged into the GRUB mainline. 

By the way, we expect to bring support for the EROFS compression part within a few weeks. However, AFAIK, GRUB lacks generic support for lz4 compression algorithms. Would it be more appropriate to introduce a dedicated lz4 library for EROFS, like zfs, or generic lz4 support for grub?

> -----Original Message-----
> From: Glenn Washburn <development@efficientek.com>
> Sent: Monday, July 17, 2023 11:51 AM
> To: Yifan Zhao <zhaoyifan@sjtu.edu.cn>
> Cc: The development of GNU GRUB <grub-devel@gnu.org>;
> hsiangkao@linux.alibaba.com; jefflexu@linux.alibaba.com
> Subject: Re: [PATCH 1/2] fs/erofs: Add support for EROFS
> 
> On Mon, 17 Jul 2023 00:03:08 +0800
> Yifan Zhao <zhaoyifan@sjtu.edu.cn> wrote:
> 
> > EROFS is a lightweight read-only filesystem designed for performance
> > which has already been 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 support will be developed later since it has more
> > work to polish.
> 
> I would expect that a very small minority are using EROFS without
> compression.
> Does that also seem true to you? This is relevant because I believe GRUB is in
> a feature freeze, but if there were a compelling reason to include this in the
> upcoming release, Daniel might make an exception (just guessing). On the
> one hand, in favor of including the current code without compression in the
> next release is that its simple enough and doesn't touch existing code, so a
> bug should have minor impact on the rest of GRUB. On the other hand, if
> there is a low probability of its use, perhaps its not worth it. As it stands, it
> seems that GRUB is being released every couple years.
> 
> When you create an updated patch series, it would be much appreciated if
> you use --range-diff or --interdiff so that reviewers (me) can verify more
> easily the changes.
> 
> >
> > Signed-off-by: Yifan Zhao <zhaoyifan@sjtu.edu.cn>
> > ---
> > The link here shows how to boot the kernel from the EROFS partition with
> this patch.
> > https://precious-celery-715.notion.site/Demo-of-booting-kernel-in-a-ER
> > OFS-partition-c6d199f937a2489bafd03d3083c294b5
> >
> >  INSTALL                     |   8 +-
> >  Makefile.util.def           |   1 +
> >  docs/grub.texi              |   4 +-
> >  grub-core/Makefile.core.def |   5 +
> >  grub-core/fs/erofs.c        | 971 ++++++++++++++++++++++++++++++++++++
> >  grub-core/kern/misc.c       |  14 +
> >  include/grub/misc.h         |   1 +
> >  7 files changed, 998 insertions(+), 6 deletions(-)  create mode
> > 100644 grub-core/fs/erofs.c
> >
> > diff --git a/INSTALL b/INSTALL
> > index b93fe9c61..1939e4745 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,
> > +    genromfs, hfsprogs, jfsutils, nilfs-tools, ntfs-3g, reiserfsprogs,
> > +    squashfs-tools, reiserfsprogs, udftools, xfsprogs, zfs-fuse
> 
> Thank you for updating the documentation. It would be nice to also update
> docs/grub.texi where appropriate and note that there is no compression
> support. I see two places that could use an update (case-insensitive search
> for "btrfs" should find them).
> 

Thanks for pointing it out. I will note that EROFS support is incomplete currently. 

> >    - 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 1e9a13d3e..ca2a32a7f
> > 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
> > b81267980..20c119c96 100644
> > --- a/docs/grub.texi
> > +++ b/docs/grub.texi
> > @@ -361,7 +361,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{Linux ext2/ext3/ext4}, @dfn{DOS FAT12/FAT16/FAT32},
> > +@dfn{EROFS}, @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),  @dfn{JFS},
> > @dfn{Minix fs} (versions 1, 2 and 3), @dfn{nilfs2}, @@ -6212,7 +6212,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 d2cf29584..eccf9b6cf 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..f1366ce05
> > --- /dev/null
> > +++ b/grub-core/fs/erofs.c
> > @@ -0,0 +1,971 @@
> > +/* erofs.c - Enhanced Read-Only File System */
> > +/*
> > + *  GRUB  --  GRand Unified Bootloader
> > + *  Copyright (C) 2023 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 ROUND_UP(x, y) ({ grub_divmod64 (x + (y - 1), y, NULL) * y;
> > +})
> 
> Is there a reason ALIGN_UP is not used instead of the above macro?
> 

I miss the useful macro and will fix it in an updated patch.

> > +
> > +#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 blkszbits;
> 
> Perhaps rename to "log2_blksz".

Changes made as required.

> 
> > +  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 dirblkbits;
> 
> And maybe log2_dirblksz? And I notice that this is never used. I know nothing
> about EROFS, but this looks like it has to do with directories. What is this
> used for, if its not needed in the directory reading code below?

In a future version, EROFS will support directory block size being a multiple of base blksz, because large dir block size improves directory fanout efficiency.
Currently, this field is reserved and its value must be 0. Changed its name as you required.

> > +  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
> > +};
> > +
> > +#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
> > +};
> > +
> > +#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;
> > +
> > +enum
> > +{
> > +  BH_Meta,
> > +  BH_Mapped,
> > +};
> > +
> > +#define EROFS_MAP_MAPPED (1 << BH_Mapped)
> > +
> > +struct grub_erofs_map_blocks
> > +{
> > +  grub_off_t m_pa, m_la;
> > +  grub_off_t m_plen, m_llen;
> > +  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;
> > +};
> > +
> > +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.blkszbits)
> > +
> > +static inline 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->blkszbits) +
> > +         (node->ino << EROFS_ISLOTBITS); }
> > +
> > +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),
> 
> 8 spaces should be converted to tabs.
> 
> > +                        sizeof (struct grub_erofs_inode_compact),
> > + dic);  if (err)
> > +    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;
> > +
> > +  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) {
> > +  grub_off_t nblocks, lastblk, file_size;
> > +  grub_off_t tailendpacking =
> > +      (node->inode_datalayout == EROFS_INODE_FLAT_INLINE) ? 1 : 0;
> > +  grub_uint32_t blocksz = erofs_blocksz (node->data);
> > +
> > +  file_size = erofs_inode_file_size (node);  nblocks = grub_divmod64
> > + (file_size + blocksz - 1, blocksz, NULL);
> 
> Would it not be better to do the following?
> 
>   nblocks = (file_size + (1U << node->data->sb.blkszbits)) >> node->data-
> >sb.blkszbits;
> 

Yes, I think it would be

nblocks = (file_size + blocksz - 1) >> node->data->sb.blkszbits;

> And if so, then maybe create a erofs_log_blocksz() macro.
> 
> > +  lastblk = nblocks - tailendpacking;
> > +
> > +  map->m_flags = EROFS_MAP_MAPPED;
> > +
> > +  if (map->m_la < lastblk * blocksz)
> 
> s/lastblk * blocksz/(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;
> > +    }
> > +  else if (tailendpacking)
> > +    {
> > +      map->m_pa = erofs_iloc (node) + erofs_inode_size (node) +
> > +                  erofs_inode_xattr_ibody_size (node) + map->m_la %
> > + blocksz;
> 
> s/map->m_la % blocksz/(map->m_la % blocksz)/
> 
> > +      map->m_plen = file_size - map->m_la;
> > +
> > +      if (map->m_pa % blocksz + map->m_plen > blocksz)
> 
> s/map->m_pa % blocksz + map->m_plen/((map->m_pa % blocksz) + map-
> >m_plen)/
> 
> Add explicit parenthesis for the code below too.
> 

I'll add them.

> > +        return grub_error (
> > +            GRUB_ERR_BAD_FS,
> > +            "inline data cross block boundary @ inode %" PRIuGRUB_UINT64_T,
> > +            node->ino);
> > +    }
> > +  else
> > +    {
> 
> This parenthesis is unneeded.

Yep.

> 
> > +      return grub_error (GRUB_ERR_BAD_FS,
> > +                         "internal error: 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.blkszbits +
> > +              (chunk_format & EROFS_CHUNK_FORMAT_BLKBITS_MASK);
> > +
> > +  chunknr = map->m_la >> chunkbits;
> > +  pos = ROUND_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,
> > +                          ROUND_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)
> 
> It is preferred to have a space after the cast parenthesis, elsewhere also.

I've overlooked it. Thanks for pointing it out.

> > +        {
> > +          map->m_pa = 0;
> > +          map->m_flags = 0;
> > +        }
> > +      else
> > +        {
> > +          map->m_pa = blkaddr << node->data->sb.blkszbits;
> > +          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.blkszbits;
> > +          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) {
> > +  struct grub_erofs_map_blocks map;
> > +  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));
> > +
> > +  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);
> > +      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);
> > +          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;
> > +            }
> > +
> > +          if (hook (filename, type, fdiro, hook_data))
> > +            {
> > +              grub_free (buf);
> > +              return 1;
> > +            }
> > +
> > +          ++de;
> > +        }
> > +
> > +      offset += maxsize;
> > +    }
> > +
> > +not_found:
> > +  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)
> > +    {
> > +      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));  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,
> > +  };
> > +
> > +  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:
> > +  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,
> > +                             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;
> > +          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)
> > +    {
> > +      ret = 0;
> > +      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_erofs_mount (device->disk,
> > +false);
> 
> This should probably be changed to the following so that we don't return a
> previous error.

I overlooked that. Thanks.

>   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]);
> 
> I want to point out that grub_xasprintf can fail and not set grub_errno,
> namely when calling grub_malloc. In this case this function *should not*
> return GRUB_ERR_NONE (but likely will and if it doesn't it will be from some
> previous failure unrelated to this function). I consider this a bug in
> grub_xasprintf, so this ideally is correct. I think I'll send a patch for this.
> 
> Glenn

Thanks for the reminder. I think the code can be left unchanged here.


Thanks, 
Yifan Zhao



> > +  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_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;
> > +}
> > +
> > +static grub_err_t
> > +grub_erofs_mtime (grub_device_t device, grub_int64_t *tm) {
> > +  struct grub_erofs_data *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;
> > +}
> > +
> > +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
> > 2890aad49..66cbb3c5c 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'. */



^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 1/2] fs/erofs: Add support for EROFS
  2023-07-20 13:14 [PATCH 1/2] fs/erofs: Add support for EROFS zhaoyifan
@ 2023-07-20 17:37 ` Glenn Washburn
  2023-09-06  5:46   ` Gao Xiang
  0 siblings, 1 reply; 11+ messages in thread
From: Glenn Washburn @ 2023-07-20 17:37 UTC (permalink / raw
  To: zhaoyifan; +Cc: hsiangkao, grub-devel, jefflexu

On Thu, 20 Jul 2023 21:14:22 +0800
<zhaoyifan@sjtu.edu.cn> wrote:

> Thank you for the reply!
> 
> As Xiang pointed out, we believe that uncompressed support for EROFS is still valuable, so I would be very grateful if this support could be merged into the GRUB mainline. 

Yes, I believe this will get merged into master. The question is
"when", after or before release. Now that I know that the compression is
on a per-file basis and so this version can still be widely used (with
the condition that the kernel and initrd are uncompressed by EROFS), I
am in support of having this included in the upcoming release.
Ultimately the decision is Daniel's though.

> By the way, we expect to bring support for the EROFS compression part within a few weeks. However, AFAIK, GRUB lacks generic support for lz4 compression algorithms. Would it be more appropriate to introduce a dedicated lz4 library for EROFS, like zfs, or generic lz4 support for grub?

I have not looked into ZFS's usage of its internal library, nor how
hard it would be to make it a separate module. But I prefer the
separate module approach. Perhaps Daniel can speak to his preferred
method.

Glenn

> > -----Original Message-----
> > From: Glenn Washburn <development@efficientek.com>
> > Sent: Monday, July 17, 2023 11:51 AM
> > To: Yifan Zhao <zhaoyifan@sjtu.edu.cn>
> > Cc: The development of GNU GRUB <grub-devel@gnu.org>;
> > hsiangkao@linux.alibaba.com; jefflexu@linux.alibaba.com
> > Subject: Re: [PATCH 1/2] fs/erofs: Add support for EROFS
> > 
> > On Mon, 17 Jul 2023 00:03:08 +0800
> > Yifan Zhao <zhaoyifan@sjtu.edu.cn> wrote:
> > 
> > > EROFS is a lightweight read-only filesystem designed for performance
> > > which has already been 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 support will be developed later since it has more
> > > work to polish.
> > 
> > I would expect that a very small minority are using EROFS without
> > compression.
> > Does that also seem true to you? This is relevant because I believe GRUB is in
> > a feature freeze, but if there were a compelling reason to include this in the
> > upcoming release, Daniel might make an exception (just guessing). On the
> > one hand, in favor of including the current code without compression in the
> > next release is that its simple enough and doesn't touch existing code, so a
> > bug should have minor impact on the rest of GRUB. On the other hand, if
> > there is a low probability of its use, perhaps its not worth it. As it stands, it
> > seems that GRUB is being released every couple years.
> > 
> > When you create an updated patch series, it would be much appreciated if
> > you use --range-diff or --interdiff so that reviewers (me) can verify more
> > easily the changes.
> > 
> > >
> > > Signed-off-by: Yifan Zhao <zhaoyifan@sjtu.edu.cn>
> > > ---
> > > The link here shows how to boot the kernel from the EROFS partition with
> > this patch.
> > > https://precious-celery-715.notion.site/Demo-of-booting-kernel-in-a-ER
> > > OFS-partition-c6d199f937a2489bafd03d3083c294b5
> > >
> > >  INSTALL                     |   8 +-
> > >  Makefile.util.def           |   1 +
> > >  docs/grub.texi              |   4 +-
> > >  grub-core/Makefile.core.def |   5 +
> > >  grub-core/fs/erofs.c        | 971 ++++++++++++++++++++++++++++++++++++
> > >  grub-core/kern/misc.c       |  14 +
> > >  include/grub/misc.h         |   1 +
> > >  7 files changed, 998 insertions(+), 6 deletions(-)  create mode
> > > 100644 grub-core/fs/erofs.c
> > >
> > > diff --git a/INSTALL b/INSTALL
> > > index b93fe9c61..1939e4745 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,
> > > +    genromfs, hfsprogs, jfsutils, nilfs-tools, ntfs-3g, reiserfsprogs,
> > > +    squashfs-tools, reiserfsprogs, udftools, xfsprogs, zfs-fuse
> > 
> > Thank you for updating the documentation. It would be nice to also update
> > docs/grub.texi where appropriate and note that there is no compression
> > support. I see two places that could use an update (case-insensitive search
> > for "btrfs" should find them).
> > 
> 
> Thanks for pointing it out. I will note that EROFS support is incomplete currently. 
> 
> > >    - 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 1e9a13d3e..ca2a32a7f
> > > 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
> > > b81267980..20c119c96 100644
> > > --- a/docs/grub.texi
> > > +++ b/docs/grub.texi
> > > @@ -361,7 +361,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{Linux ext2/ext3/ext4}, @dfn{DOS FAT12/FAT16/FAT32},
> > > +@dfn{EROFS}, @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),  @dfn{JFS},
> > > @dfn{Minix fs} (versions 1, 2 and 3), @dfn{nilfs2}, @@ -6212,7 +6212,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 d2cf29584..eccf9b6cf 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..f1366ce05
> > > --- /dev/null
> > > +++ b/grub-core/fs/erofs.c
> > > @@ -0,0 +1,971 @@
> > > +/* erofs.c - Enhanced Read-Only File System */
> > > +/*
> > > + *  GRUB  --  GRand Unified Bootloader
> > > + *  Copyright (C) 2023 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 ROUND_UP(x, y) ({ grub_divmod64 (x + (y - 1), y, NULL) * y;
> > > +})
> > 
> > Is there a reason ALIGN_UP is not used instead of the above macro?
> > 
> 
> I miss the useful macro and will fix it in an updated patch.
> 
> > > +
> > > +#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 blkszbits;
> > 
> > Perhaps rename to "log2_blksz".
> 
> Changes made as required.
> 
> > 
> > > +  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 dirblkbits;
> > 
> > And maybe log2_dirblksz? And I notice that this is never used. I know nothing
> > about EROFS, but this looks like it has to do with directories. What is this
> > used for, if its not needed in the directory reading code below?
> 
> In a future version, EROFS will support directory block size being a multiple of base blksz, because large dir block size improves directory fanout efficiency.
> Currently, this field is reserved and its value must be 0. Changed its name as you required.
> 
> > > +  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
> > > +};
> > > +
> > > +#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
> > > +};
> > > +
> > > +#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;
> > > +
> > > +enum
> > > +{
> > > +  BH_Meta,
> > > +  BH_Mapped,
> > > +};
> > > +
> > > +#define EROFS_MAP_MAPPED (1 << BH_Mapped)
> > > +
> > > +struct grub_erofs_map_blocks
> > > +{
> > > +  grub_off_t m_pa, m_la;
> > > +  grub_off_t m_plen, m_llen;
> > > +  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;
> > > +};
> > > +
> > > +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.blkszbits)
> > > +
> > > +static inline 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->blkszbits) +
> > > +         (node->ino << EROFS_ISLOTBITS); }
> > > +
> > > +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),
> > 
> > 8 spaces should be converted to tabs.
> > 
> > > +                        sizeof (struct grub_erofs_inode_compact),
> > > + dic);  if (err)
> > > +    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;
> > > +
> > > +  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) {
> > > +  grub_off_t nblocks, lastblk, file_size;
> > > +  grub_off_t tailendpacking =
> > > +      (node->inode_datalayout == EROFS_INODE_FLAT_INLINE) ? 1 : 0;
> > > +  grub_uint32_t blocksz = erofs_blocksz (node->data);
> > > +
> > > +  file_size = erofs_inode_file_size (node);  nblocks = grub_divmod64
> > > + (file_size + blocksz - 1, blocksz, NULL);
> > 
> > Would it not be better to do the following?
> > 
> >   nblocks = (file_size + (1U << node->data->sb.blkszbits)) >> node->data-
> > >sb.blkszbits;
> > 
> 
> Yes, I think it would be
> 
> nblocks = (file_size + blocksz - 1) >> node->data->sb.blkszbits;
> 
> > And if so, then maybe create a erofs_log_blocksz() macro.
> > 
> > > +  lastblk = nblocks - tailendpacking;
> > > +
> > > +  map->m_flags = EROFS_MAP_MAPPED;
> > > +
> > > +  if (map->m_la < lastblk * blocksz)
> > 
> > s/lastblk * blocksz/(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;
> > > +    }
> > > +  else if (tailendpacking)
> > > +    {
> > > +      map->m_pa = erofs_iloc (node) + erofs_inode_size (node) +
> > > +                  erofs_inode_xattr_ibody_size (node) + map->m_la %
> > > + blocksz;
> > 
> > s/map->m_la % blocksz/(map->m_la % blocksz)/
> > 
> > > +      map->m_plen = file_size - map->m_la;
> > > +
> > > +      if (map->m_pa % blocksz + map->m_plen > blocksz)
> > 
> > s/map->m_pa % blocksz + map->m_plen/((map->m_pa % blocksz) + map-
> > >m_plen)/
> > 
> > Add explicit parenthesis for the code below too.
> > 
> 
> I'll add them.
> 
> > > +        return grub_error (
> > > +            GRUB_ERR_BAD_FS,
> > > +            "inline data cross block boundary @ inode %" PRIuGRUB_UINT64_T,
> > > +            node->ino);
> > > +    }
> > > +  else
> > > +    {
> > 
> > This parenthesis is unneeded.
> 
> Yep.
> 
> > 
> > > +      return grub_error (GRUB_ERR_BAD_FS,
> > > +                         "internal error: 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.blkszbits +
> > > +              (chunk_format & EROFS_CHUNK_FORMAT_BLKBITS_MASK);
> > > +
> > > +  chunknr = map->m_la >> chunkbits;
> > > +  pos = ROUND_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,
> > > +                          ROUND_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)
> > 
> > It is preferred to have a space after the cast parenthesis, elsewhere also.
> 
> I've overlooked it. Thanks for pointing it out.
> 
> > > +        {
> > > +          map->m_pa = 0;
> > > +          map->m_flags = 0;
> > > +        }
> > > +      else
> > > +        {
> > > +          map->m_pa = blkaddr << node->data->sb.blkszbits;
> > > +          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.blkszbits;
> > > +          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) {
> > > +  struct grub_erofs_map_blocks map;
> > > +  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));
> > > +
> > > +  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);
> > > +      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);
> > > +          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;
> > > +            }
> > > +
> > > +          if (hook (filename, type, fdiro, hook_data))
> > > +            {
> > > +              grub_free (buf);
> > > +              return 1;
> > > +            }
> > > +
> > > +          ++de;
> > > +        }
> > > +
> > > +      offset += maxsize;
> > > +    }
> > > +
> > > +not_found:
> > > +  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)
> > > +    {
> > > +      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));  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,
> > > +  };
> > > +
> > > +  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:
> > > +  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,
> > > +                             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;
> > > +          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)
> > > +    {
> > > +      ret = 0;
> > > +      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_erofs_mount (device->disk,
> > > +false);
> > 
> > This should probably be changed to the following so that we don't return a
> > previous error.
> 
> I overlooked that. Thanks.
> 
> >   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]);
> > 
> > I want to point out that grub_xasprintf can fail and not set grub_errno,
> > namely when calling grub_malloc. In this case this function *should not*
> > return GRUB_ERR_NONE (but likely will and if it doesn't it will be from some
> > previous failure unrelated to this function). I consider this a bug in
> > grub_xasprintf, so this ideally is correct. I think I'll send a patch for this.
> > 
> > Glenn
> 
> Thanks for the reminder. I think the code can be left unchanged here.
> 
> 
> Thanks, 
> Yifan Zhao
> 
> 
> 
> > > +  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_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;
> > > +}
> > > +
> > > +static grub_err_t
> > > +grub_erofs_mtime (grub_device_t device, grub_int64_t *tm) {
> > > +  struct grub_erofs_data *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;
> > > +}
> > > +
> > > +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
> > > 2890aad49..66cbb3c5c 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'. */
> 


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 1/2] fs/erofs: Add support for EROFS
  2023-07-20 17:37 ` Glenn Washburn
@ 2023-09-06  5:46   ` Gao Xiang
  2023-09-07 19:57     ` Daniel Kiper
  0 siblings, 1 reply; 11+ messages in thread
From: Gao Xiang @ 2023-09-06  5:46 UTC (permalink / raw
  To: Daniel Kiper; +Cc: grub-devel, jefflexu, Yifan Zhao, development

Hi Daniel,

On 2023/7/21 01:37, Glenn Washburn wrote:
> On Thu, 20 Jul 2023 21:14:22 +0800
> <zhaoyifan@sjtu.edu.cn> wrote:
> 
>> Thank you for the reply!
>>
>> As Xiang pointed out, we believe that uncompressed support for EROFS is still valuable, so I would be very grateful if this support could be merged into the GRUB mainline.
> 
> Yes, I believe this will get merged into master. The question is
> "when", after or before release. Now that I know that the compression is
> on a per-file basis and so this version can still be widely used (with
> the condition that the kernel and initrd are uncompressed by EROFS), I
> am in support of having this included in the upcoming release.
> Ultimately the decision is Daniel's though.

ping.. May I ask what's your opinion of EROFS uncompressed support
for GRUB.  I assume compressed EROFS support needs more work to do,
so could we support uncompressed support as the first step since
EROFS supports per-file compression configuration?

The v4 version is already finished with Glenn's RVB:
https://lists.gnu.org/archive/html/grub-devel/2023-07/msg00102.html

> 
>> By the way, we expect to bring support for the EROFS compression part within a few weeks. However, AFAIK, GRUB lacks generic support for lz4 compression algorithms. Would it be more appropriate to introduce a dedicated lz4 library for EROFS, like zfs, or generic lz4 support for grub?
> 
> I have not looked into ZFS's usage of its internal library, nor how
> hard it would be to make it a separate module. But I prefer the
> separate module approach. Perhaps Daniel can speak to his preferred> method.

same here.

Thanks,
Gao Xiang

> 
> Glenn
> 

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

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 1/2] fs/erofs: Add support for EROFS
  2023-09-06  5:46   ` Gao Xiang
@ 2023-09-07 19:57     ` Daniel Kiper
  2023-09-08  2:04       ` Gao Xiang
  0 siblings, 1 reply; 11+ messages in thread
From: Daniel Kiper @ 2023-09-07 19:57 UTC (permalink / raw
  To: Gao Xiang; +Cc: grub-devel, jefflexu, Yifan Zhao, development

Hi Gao,

On Wed, Sep 06, 2023 at 01:46:25PM +0800, Gao Xiang wrote:
> Hi Daniel,
>
> On 2023/7/21 01:37, Glenn Washburn wrote:
> > On Thu, 20 Jul 2023 21:14:22 +0800
> > <zhaoyifan@sjtu.edu.cn> wrote:
> >
> > > Thank you for the reply!
> > >
> > > As Xiang pointed out, we believe that uncompressed support for
> > > EROFS is still valuable, so I would be very grateful if this
> > > support could be merged into the GRUB mainline.
> >
> > Yes, I believe this will get merged into master. The question is
> > "when", after or before release. Now that I know that the compression is
> > on a per-file basis and so this version can still be widely used (with
> > the condition that the kernel and initrd are uncompressed by EROFS), I
> > am in support of having this included in the upcoming release.
> > Ultimately the decision is Daniel's though.
>
> ping.. May I ask what's your opinion of EROFS uncompressed support
> for GRUB.  I assume compressed EROFS support needs more work to do,
> so could we support uncompressed support as the first step since
> EROFS supports per-file compression configuration?
>
> The v4 version is already finished with Glenn's RVB:
> https://lists.gnu.org/archive/html/grub-devel/2023-07/msg00102.html

We are in the GRUB 2.12 release process and busy with it. I will take
a look at the patch set after the release. Sorry for delay.

> > > By the way, we expect to bring support for the EROFS compression
> > > part within a few weeks. However, AFAIK, GRUB lacks generic
> > > support for lz4 compression algorithms. Would it be more
> > > appropriate to introduce a dedicated lz4 library for EROFS, like
> > > zfs, or generic lz4 support for grub?
> >
> > I have not looked into ZFS's usage of its internal library, nor how
> > hard it would be to make it a separate module. But I prefer the
> > separate module approach. Perhaps Daniel can speak to his preferred> method.

I agree with Glenn, please put/move lz4 compression code to separate module.

Daniel

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

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 1/2] fs/erofs: Add support for EROFS
  2023-09-07 19:57     ` Daniel Kiper
@ 2023-09-08  2:04       ` Gao Xiang
  2023-12-25  7:26         ` Gao Xiang
  0 siblings, 1 reply; 11+ messages in thread
From: Gao Xiang @ 2023-09-08  2:04 UTC (permalink / raw
  To: Daniel Kiper; +Cc: grub-devel, jefflexu, Yifan Zhao, development

Hi,

On 2023/9/8 03:57, Daniel Kiper wrote:
> Hi Gao,
> 
> On Wed, Sep 06, 2023 at 01:46:25PM +0800, Gao Xiang wrote:
>> Hi Daniel,
>>
>> On 2023/7/21 01:37, Glenn Washburn wrote:
>>> On Thu, 20 Jul 2023 21:14:22 +0800
>>> <zhaoyifan@sjtu.edu.cn> wrote:
>>>
>>>> Thank you for the reply!
>>>>
>>>> As Xiang pointed out, we believe that uncompressed support for
>>>> EROFS is still valuable, so I would be very grateful if this
>>>> support could be merged into the GRUB mainline.
>>>
>>> Yes, I believe this will get merged into master. The question is
>>> "when", after or before release. Now that I know that the compression is
>>> on a per-file basis and so this version can still be widely used (with
>>> the condition that the kernel and initrd are uncompressed by EROFS), I
>>> am in support of having this included in the upcoming release.
>>> Ultimately the decision is Daniel's though.
>>
>> ping.. May I ask what's your opinion of EROFS uncompressed support
>> for GRUB.  I assume compressed EROFS support needs more work to do,
>> so could we support uncompressed support as the first step since
>> EROFS supports per-file compression configuration?
>>
>> The v4 version is already finished with Glenn's RVB:
>> https://lists.gnu.org/archive/html/grub-devel/2023-07/msg00102.html
> 
> We are in the GRUB 2.12 release process and busy with it. I will take
> a look at the patch set after the release. Sorry for delay.

Okay, thanks.

> 
>>>> By the way, we expect to bring support for the EROFS compression
>>>> part within a few weeks. However, AFAIK, GRUB lacks generic
>>>> support for lz4 compression algorithms. Would it be more
>>>> appropriate to introduce a dedicated lz4 library for EROFS, like
>>>> zfs, or generic lz4 support for grub?
>>>
>>> I have not looked into ZFS's usage of its internal library, nor how
>>> hard it would be to make it a separate module. But I prefer the
>>> separate module approach. Perhaps Daniel can speak to his preferred> method.
> 
> I agree with Glenn, please put/move lz4 compression code to separate module.

Thanks for the confirmation on this stuff!

Thanks,
Gao Xiang

> 
> Daniel

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

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 1/2] fs/erofs: Add support for EROFS
  2023-09-08  2:04       ` Gao Xiang
@ 2023-12-25  7:26         ` Gao Xiang
  2023-12-27 13:10           ` Daniel Kiper
  0 siblings, 1 reply; 11+ messages in thread
From: Gao Xiang @ 2023-12-25  7:26 UTC (permalink / raw
  To: Daniel Kiper, The development of GNU GRUB
  Cc: jefflexu, Yifan Zhao, development

Hi Daniel,

On Fri, Sep 08, 2023 at 10:04:53AM +0800, Gao Xiang wrote:
> Hi,
> 
> On 2023/9/8 03:57, Daniel Kiper wrote:
> > Hi Gao,
> > 
> > On Wed, Sep 06, 2023 at 01:46:25PM +0800, Gao Xiang wrote:
> > > Hi Daniel,
> > > 
> > > On 2023/7/21 01:37, Glenn Washburn wrote:
> > > > On Thu, 20 Jul 2023 21:14:22 +0800
> > > > <zhaoyifan@sjtu.edu.cn> wrote:
> > > > 
> > > > > Thank you for the reply!
> > > > > 
> > > > > As Xiang pointed out, we believe that uncompressed support for
> > > > > EROFS is still valuable, so I would be very grateful if this
> > > > > support could be merged into the GRUB mainline.
> > > > 
> > > > Yes, I believe this will get merged into master. The question is
> > > > "when", after or before release. Now that I know that the compression is
> > > > on a per-file basis and so this version can still be widely used (with
> > > > the condition that the kernel and initrd are uncompressed by EROFS), I
> > > > am in support of having this included in the upcoming release.
> > > > Ultimately the decision is Daniel's though.
> > > 
> > > ping.. May I ask what's your opinion of EROFS uncompressed support
> > > for GRUB.  I assume compressed EROFS support needs more work to do,
> > > so could we support uncompressed support as the first step since
> > > EROFS supports per-file compression configuration?
> > > 
> > > The v4 version is already finished with Glenn's RVB:
> > > https://lists.gnu.org/archive/html/grub-devel/2023-07/msg00102.html
> > 
> > We are in the GRUB 2.12 release process and busy with it. I will take
> > a look at the patch set after the release. Sorry for delay.
> 
> Okay, thanks.

It seems that GRUB 2.12 has been always released.  Is it possible to
consider this to the upstream?

Also EROFS has some more information on the webpages:
https://erofs.docs.kernel.org 

> 
> > 

...

> > > > 
> > > > I have not looked into ZFS's usage of its internal library, nor how
> > > > hard it would be to make it a separate module. But I prefer the
> > > > separate module approach. Perhaps Daniel can speak to his preferred> method.
> > 
> > I agree with Glenn, please put/move lz4 compression code to separate module.
> 
> Thanks for the confirmation on this stuff!

For EROFS support for compressed files, it still needs some work/review
(mainly seperate modules) to do anyway.  IMHO, we could address them
step-by-step.

Thanks,
Gao Xiang

> 
> Thanks,
> Gao Xiang
> 
> > 
> > Daniel

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

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 1/2] fs/erofs: Add support for EROFS
  2023-12-25  7:26         ` Gao Xiang
@ 2023-12-27 13:10           ` Daniel Kiper
  2023-12-27 15:21             ` Gao Xiang
  0 siblings, 1 reply; 11+ messages in thread
From: Daniel Kiper @ 2023-12-27 13:10 UTC (permalink / raw
  To: Gao Xiang; +Cc: The development of GNU GRUB, jefflexu, Yifan Zhao, development

Hi Gao,

On Mon, Dec 25, 2023 at 03:26:28PM +0800, Gao Xiang wrote:
> Hi Daniel,

[...]

> It seems that GRUB 2.12 has been always released.  Is it possible to
> consider this to the upstream?

Sure thing. Though there is a holiday season here. So, expect reviews in
a few weeks.

[...]

> > > > > I have not looked into ZFS's usage of its internal library, nor how
> > > > > hard it would be to make it a separate module. But I prefer the
> > > > > separate module approach. Perhaps Daniel can speak to his preferred> method.
> > >
> > > I agree with Glenn, please put/move lz4 compression code to separate module.
> >
> > Thanks for the confirmation on this stuff!
>
> For EROFS support for compressed files, it still needs some work/review
> (mainly seperate modules) to do anyway.  IMHO, we could address them
> step-by-step.

Yeah, I think it is good idea.

Daniel

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

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 1/2] fs/erofs: Add support for EROFS
  2023-12-27 13:10           ` Daniel Kiper
@ 2023-12-27 15:21             ` Gao Xiang
  0 siblings, 0 replies; 11+ messages in thread
From: Gao Xiang @ 2023-12-27 15:21 UTC (permalink / raw
  To: Daniel Kiper, Gao Xiang
  Cc: The development of GNU GRUB, jefflexu, Yifan Zhao, development



On 2023/12/27 21:10, Daniel Kiper wrote:
> Hi Gao,
> 
> On Mon, Dec 25, 2023 at 03:26:28PM +0800, Gao Xiang wrote:
>> Hi Daniel,
> 
> [...]
> 
>> It seems that GRUB 2.12 has been always released.  Is it possible to
>> consider this to the upstream?
> 
> Sure thing. Though there is a holiday season here. So, expect reviews in
> a few weeks.

Thanks. That would be great helpful!

Thanks,
Gao Xiang

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

^ permalink raw reply	[flat|nested] 11+ messages in thread

end of thread, other threads:[~2023-12-27 15:27 UTC | newest]

Thread overview: 11+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-07-20 13:14 [PATCH 1/2] fs/erofs: Add support for EROFS zhaoyifan
2023-07-20 17:37 ` Glenn Washburn
2023-09-06  5:46   ` Gao Xiang
2023-09-07 19:57     ` Daniel Kiper
2023-09-08  2:04       ` Gao Xiang
2023-12-25  7:26         ` Gao Xiang
2023-12-27 13:10           ` Daniel Kiper
2023-12-27 15:21             ` Gao Xiang
  -- strict thread matches above, loose matches on Subject: below --
2023-07-16 16:03 Yifan Zhao
2023-07-17  3:51 ` Glenn Washburn
2023-07-17  5:17   ` Gao Xiang

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.