On Thu, 20 Jul 2023 21:14:22 +0800 <zhaoyi...@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 <developm...@efficientek.com> > > Sent: Monday, July 17, 2023 11:51 AM > > To: Yifan Zhao <zhaoyi...@sjtu.edu.cn> > > Cc: The development of GNU GRUB <grub-devel@gnu.org>; > > hsiang...@linux.alibaba.com; jeffl...@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 <zhaoyi...@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 <zhaoyi...@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'. */ > _______________________________________________ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel
