From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from list by lists.gnu.org with archive (Exim 4.71) id 1YblE3-0004XD-7L for mharc-grub-devel@gnu.org; Sat, 28 Mar 2015 03:32:11 -0400 Received: from eggs.gnu.org ([2001:4830:134:3::10]:54456) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YblDy-0004MH-8g for grub-devel@gnu.org; Sat, 28 Mar 2015 03:32:09 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1YblDt-0006Sg-6T for grub-devel@gnu.org; Sat, 28 Mar 2015 03:32:06 -0400 Received: from mail-la0-x22a.google.com ([2a00:1450:4010:c03::22a]:36348) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YblDs-0006SI-KN for grub-devel@gnu.org; Sat, 28 Mar 2015 03:32:01 -0400 Received: by labe2 with SMTP id e2so85456213lab.3 for ; Sat, 28 Mar 2015 00:31:59 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20120113; h=date:from:to:cc:subject:message-id:in-reply-to:references :mime-version:content-type:content-transfer-encoding; bh=xlZG40t7AqHHIFBrjuRDbzyCOwEXVwCAgK0RTqplKds=; b=ifeUYQJ2ACrVx/eXvBQqV9ZE1nPWJnWN8cJsSxZMRMQ98lcQnrbw4wEUfPQboi4eFf XDFVcMPas0glgbNBskl/BwQ81/2xOnOL7ciEQSgLZmgWkYmCkjbQqbY7etXeOhOGg1fI KEnAO4zl2fnyswD9A0NIUOVGCeUcCazwcnAV/s6Rc1wF50iGykbzA9QevbxMMGBzr41A OcMOou9vaaFYsu+Hj+UPU9GXE3t34yVmPQIAHxd0mHEp0NzZjSJh00xjKqyBqfuq2RGu +xXG47Em4C+EEfUeQKvEg/vR+kZu29y/yUbqsOkHyHVBhuQSKCAAjM48c1cWEh8xhj3E Hiqw== X-Received: by 10.112.132.35 with SMTP id or3mr8992283lbb.85.1427527919677; Sat, 28 Mar 2015 00:31:59 -0700 (PDT) Received: from opensuse.site (ppp91-76-14-38.pppoe.mtu-net.ru. [91.76.14.38]) by mx.google.com with ESMTPSA id o8sm750897lal.24.2015.03.28.00.31.57 (version=TLSv1.2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Sat, 28 Mar 2015 00:31:58 -0700 (PDT) Date: Sat, 28 Mar 2015 10:31:55 +0300 From: Andrei Borzenkov To: Jaegeuk Kim Subject: Re: [PATCH] F2FS support Message-ID: <20150328103155.5c961fec@opensuse.site> In-Reply-To: <1427185140-41120-1-git-send-email-jaegeuk@kernel.org> References: <1427185140-41120-1-git-send-email-jaegeuk@kernel.org> X-Mailer: Claws Mail 3.11.0 (GTK+ 2.24.25; x86_64-suse-linux-gnu) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable X-detected-operating-system: by eggs.gnu.org: Error: Malformed IPv6 address (bad octet value). X-Received-From: 2a00:1450:4010:c03::22a Cc: grub-devel@gnu.org, linux-f2fs-devel@lists.sourceforge.net X-BeenThere: grub-devel@gnu.org X-Mailman-Version: 2.1.14 Precedence: list Reply-To: The development of GNU GRUB List-Id: The development of GNU GRUB List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Sat, 28 Mar 2015 07:32:09 -0000 =D0=92 Tue, 24 Mar 2015 01:19:00 -0700 Jaegeuk Kim =D0=BF=D0=B8=D1=88=D0=B5=D1=82: > * Makefile.util.def: Add f2fs.c. > * doc/grub.texi: Add f2fs description. > * grub-core/Makefile.core.def: Add f2fs module. > * grub-core/fs/f2fs.c: New file. > * tests/f2fs_test.in: New file. > * tests/util/grub-fs-tester.in: Add f2fs requirements. >=20 It's not the most useful commit message. Better would be short explanation of use cases and intended platforms. I'm curious here - F2FS is intended for raw flash access, on which platform(s) grub has access to such devices?=20 >=20 > diff --git a/ChangeLog-2015 b/ChangeLog-2015 We do not use ChangeLog any more, it is autogenerated from commits. This file is legacy before this change, do not change it. > diff --git a/grub-core/fs/f2fs.c b/grub-core/fs/f2fs.c > new file mode 100644 > index 0000000..40360d5 > --- /dev/null > +++ b/grub-core/fs/f2fs.c > @@ -0,0 +1,1321 @@ > +/* > + * f2fs.c - Flash-Friendly File System > + * > + * Written by Jaegeuk Kim > + * > + * Copyright (C) 2015 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 . > + */ > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > + > +GRUB_MOD_LICENSE ("GPLv3+"); > + > +/* F2FS Magic Number */ > +#define F2FS_SUPER_MAGIC 0xF2F52010 > + > +/* byte-size offset */ > +#define F2FS_SUPER_OFFSET 1024 > + > +/* 12 bits for 4096 bytes */ > +#define F2FS_MAX_LOG_SECTOR_SIZE 12 > + > +/* 9 bits for 512 bytes */ > +#define F2FS_MIN_LOG_SECTOR_SIZE 9 > + > +/* support only 4KB block */ > +#define F2FS_BLKSIZE 4096 (2 << F2FS_BLK_BITS)? > +#define F2FS_BLK_BITS 12 > +#define F2FS_BLK_SEC_BITS (3) It is confusing to have some defines parenthesized and some not. Could it be unified somehow? Also this can be computed from F2FS_BLK_BITS and GRUB_DISK_SECTOR_BITS - one magic number less. ... > +struct grub_f2fs_inline_dentry > +{ > + grub_uint8_t dentry_bitmap[INLINE_DENTRY_BITMAP_SIZE]; This is cast to grub_uint32_t everywhere. Can it be non-multiple of 4 bytes? If not, may be just define as such?=20 > + grub_uint8_t reserved[INLINE_RESERVED_SIZE]; > + struct grub_f2fs_dir_entry dentry[NR_INLINE_DENTRY]; > + grub_uint8_t filename[NR_INLINE_DENTRY][F2FS_SLOT_LEN]; > +} GRUB_PACKED; > + > +struct grub_f2fs_dentry_block { > + grub_uint8_t dentry_bitmap[SIZE_OF_DENTRY_BITMAP]; ditto > + grub_uint8_t reserved[SIZE_OF_RESERVED]; > + struct grub_f2fs_dir_entry dentry[NR_DENTRY_IN_BLOCK]; > + grub_uint8_t filename[NR_DENTRY_IN_BLOCK][F2FS_SLOT_LEN]; > +} GRUB_PACKED; > + ... > + > +#define ver_after (a, b) (typecheck (unsigned long long, a) && \ > + typecheck (unsigned long long, b) && \ > + ((long long)((a) - (b)) > 0)) > + Where typecheck definition comes from? ... > + > +static inline int > +__test_bit (int nr, grub_uint32_t *addr) > +{ > + return 1UL & (addr[nr / 32] >> (nr & (31))); Extra parenthesis (31) > +} > + It is used for dentry_bitmap which is kept in LE format and not converted as far as I can tell. This needs fixing for BE systems. Linux kernel is explicitly using test_bit_le here. This will also work for inode flags (just skip explicit conversion). There are two functions with more or less identical names. May be make them grub_f2fs_test_bit_le32 grub_f2fs_test_bit As a general comment - marking non-modified arguments as const everywhere would be good. > +static inline char * > +__inline_addr (struct grub_f2fs_inode *inode) > +{ > + return (char *)&(inode->i_addr[1]); Redundant parens around inode-> > +} > + > +static inline grub_uint64_t > +__i_size (struct grub_f2fs_inode *inode) Could we make it grub_f2fs_file_size or similar? i_size really does not tell much outside of linux kernel. > +{ > + return grub_le_to_cpu64 (inode->i_size); > +} > + > +static inline grub_uint32_t > +__start_cp_addr (struct grub_f2fs_data *data) > +{ > + struct grub_f2fs_checkpoint *ckpt =3D &data->ckpt; > + grub_uint64_t ckpt_version =3D grub_le_to_cpu64 (ckpt->checkpoint_ver); > + grub_uint32_t start_addr =3D data->cp_blkaddr; > + > + if (!(ckpt_version & 1)) > + return start_addr + data->blocks_per_seg; > + return start_addr; > +} > + > +static inline grub_uint32_t > +__start_sum_block (struct grub_f2fs_data *data) > +{ > + struct grub_f2fs_checkpoint *ckpt =3D &data->ckpt; > + > + return __start_cp_addr (data) + grub_le_to_cpu32 (ckpt->cp_pack_start_= sum); > +} > + > +static inline grub_uint32_t > +__sum_blk_addr (struct grub_f2fs_data *data, int base, int type) > +{ > + struct grub_f2fs_checkpoint *ckpt =3D &data->ckpt; > + > + return __start_cp_addr (data) + > + grub_le_to_cpu32 (ckpt->cp_pack_total_block_count) > + - (base + 1) + type; > +} > + > +static inline int > +__ckpt_flag_set (struct grub_f2fs_checkpoint *ckpt, unsigned int f) > +{ > + grub_uint32_t ckpt_flags =3D grub_le_to_cpu32 (ckpt->ckpt_flags); > + return ckpt_flags & f; All flags are constant so you can simply do=20 ckpt->ckpt_flags & grub_cpu_to_le32_compile_time (FLAG) in place to avoid extra calls. This makes function redundant. > +} > + > +static inline int > +__inode_flag_set (struct grub_f2fs_inode *inode, int flag) > +{ > + grub_uint32_t i_flags =3D grub_le_to_cpu32 (inode->i_flags); > + return __test_bit (flag, &i_flags); > +} grub_f2fs_test_bit_le32? > + > +/* > + * CRC32 > + */ > +#define CRCPOLY_LE 0xedb88320 > + > +static inline grub_uint32_t > +grub_f2fs_cal_crc32 (grub_uint32_t crc, void *buf, int len) Why crc is parameter here? This function is used exactly once with fixed value for initial crc. > +{ > + int i; > + unsigned char *p =3D (unsigned char *)buf; > + > + while (len--) > + { > + crc ^=3D *p++; > + for (i =3D 0; i < 8; i++) > + crc =3D (crc >> 1) ^ ((crc & 1) ? CRCPOLY_LE : 0); > + } > + return crc; > +} > + > +static inline int > +grub_f2fs_crc_valid (grub_uint32_t blk_crc, void *buf, int len) > +{ > + grub_uint32_t cal_crc =3D 0; > + > + cal_crc =3D grub_f2fs_cal_crc32 (F2FS_SUPER_MAGIC, buf, len); > + > + return (cal_crc =3D=3D blk_crc) ? 1 : 0; > +} > + > +static inline int > +grub_f2fs_test_bit (grub_uint32_t nr, const char *p) > +{ > + int mask; > + char *addr =3D (char *)p; Why cast? We are not going to modify it, right? > + > + addr +=3D (nr >> 3); > + mask =3D 1 << (7 - (nr & 0x07)); > + return (mask & *addr) !=3D 0; > +} > + > +static int > +grub_f2fs_sanity_check_sb (struct grub_f2fs_superblock *sb) > +{ > + unsigned int blocksize; > + > + if (F2FS_SUPER_MAGIC !=3D grub_le_to_cpu32 (sb->magic)) sb->magic !=3D grub_cpu_to_le32_compile_time (F2FS_SUPER_MAGIC) > + return -1; > + > + blocksize =3D 1 << grub_le_to_cpu32 (sb->log_blocksize); > + if (blocksize !=3D F2FS_BLKSIZE) sb->log_blksize !=3D grub_cpu_to_le32_compile_time (F2FS_BLK_BITS) > + return -1; > + > + if (grub_le_to_cpu32 (sb->log_sectorsize) > F2FS_MAX_LOG_SECTOR_SIZE) > + return -1; > + > + if (grub_le_to_cpu32 (sb->log_sectorsize) < F2FS_MIN_LOG_SECTOR_SIZE) > + return -1; > + > + if (grub_le_to_cpu32 (sb->log_sectors_per_block) + > + grub_le_to_cpu32 (sb->log_sectorsize) !=3D F2FS_MAX_LOG_SECTOR_SIZ= E) Should not it be F2FS_BLKSIZE? At least it sounds logical. Also please convert log_sectorsize just once. > + return -1; > + > + return 0; > +} > + > +static grub_err_t > +grub_f2fs_read_sb (struct grub_f2fs_data *data, int block) > +{ > + grub_disk_t disk =3D data->disk; > + grub_uint64_t offset; > + grub_err_t err; > + > + if (block =3D=3D 0) > + offset =3D F2FS_SUPER_OFFSET; > + else > + offset =3D F2FS_BLKSIZE + F2FS_SUPER_OFFSET; > + Please name it "secondary" or similar instead of "block" to avoid confusion. You do not really want to read arbitrary block, right? offset =3D F2FS_SUPER_OFFEST; if (secondary) offset +=3D F2FS_BLKSIZE; > + /* Read first super block. */ > + err =3D grub_disk_read (disk, offset >> GRUB_DISK_SECTOR_BITS, 0, > + sizeof (data->sblock), &data->sblock); > + if (err) > + return err; > + > + if (grub_f2fs_sanity_check_sb (&data->sblock)) > + err =3D GRUB_ERR_BAD_FS; > + > + return err; > +} > + > +static void * > +validate_checkpoint (struct grub_f2fs_data *data, grub_uint32_t cp_addr, > + grub_uint64_t *version) > +{ > + void *cp_page_1, *cp_page_2; > + struct grub_f2fs_checkpoint *cp_block; > + grub_uint64_t cur_version =3D 0, pre_version =3D 0; > + grub_uint32_t crc =3D 0; > + grub_uint32_t crc_offset; > + grub_err_t err; > + > + /* Read the 1st cp block in this CP pack */ > + cp_page_1 =3D grub_malloc (F2FS_BLKSIZE); > + if (!cp_page_1) > + return NULL; > + > + err =3D grub_f2fs_block_read (data, cp_addr, cp_page_1); > + if (err) > + goto invalid_cp1; > + > + cp_block =3D (struct grub_f2fs_checkpoint *)cp_page_1; > + crc_offset =3D grub_le_to_cpu32 (cp_block->checksum_offset); > + if (crc_offset >=3D F2FS_BLKSIZE) > + goto invalid_cp1; > + > + crc =3D *(grub_uint32_t *)((char *)cp_block + crc_offset); Is unaligned access possible here? If yes, it probably should be grub_get_unaligned32. > + if (!grub_f2fs_crc_valid (crc, cp_block, crc_offset)) > + goto invalid_cp1; > + Should not CRC be converted from LE? > + pre_version =3D grub_le_to_cpu64 (cp_block->checkpoint_ver); > + > + /* Read the 2nd cp block in this CP pack */ > + cp_page_2 =3D grub_malloc (F2FS_BLKSIZE); > + if (!cp_page_2) > + goto invalid_cp1; > + > + cp_addr +=3D grub_le_to_cpu32 (cp_block->cp_pack_total_block_count) - = 1; > + > + err =3D grub_f2fs_block_read (data, cp_addr, cp_page_2); > + if (err) > + goto invalid_cp2; > + > + cp_block =3D (struct grub_f2fs_checkpoint *)cp_page_2; > + crc_offset =3D grub_le_to_cpu32 (cp_block->checksum_offset); > + if (crc_offset >=3D F2FS_BLKSIZE) > + goto invalid_cp2; > + > + crc =3D *(grub_uint32_t *)((char *)cp_block + crc_offset); Ditto alignment. > + if (!grub_f2fs_crc_valid (crc, cp_block, crc_offset)) Ditto endianness. > + goto invalid_cp2; > + > + cur_version =3D grub_le_to_cpu64 (cp_block->checkpoint_ver); > + if (cur_version =3D=3D pre_version) > + { > + *version =3D cur_version; > + grub_free (cp_page_2); > + return cp_page_1; > + } > + > +invalid_cp2: > + grub_free (cp_page_2); > +invalid_cp1: > + grub_free (cp_page_1); > + return NULL; > +} > + > +static grub_err_t > +grub_f2fs_read_cp (struct grub_f2fs_data *data) > +{ > + void *cp1, *cp2, *cur_page; > + grub_uint64_t cp1_version =3D 0, cp2_version =3D 0; > + grub_uint64_t cp_start_blk_no; > + > + /* > + * Finding out valid cp block involves read both > + * sets (cp pack1 and cp pack 2) > + */ > + cp_start_blk_no =3D data->cp_blkaddr; > + cp1 =3D validate_checkpoint (data, cp_start_blk_no, &cp1_version); > + if (!cp1 && grub_errno) > + return grub_errno; > + > + /* The second checkpoint pack should start at the next segment */ > + cp_start_blk_no +=3D data->blocks_per_seg; > + cp2 =3D validate_checkpoint (data, cp_start_blk_no, &cp2_version); > + if (!cp2 && grub_errno) > + { > + grub_free (cp1); > + return grub_errno; > + } > + > + if (cp1 && cp2) > + cur_page =3D (cp2_version > cp1_version) ? cp2 : cp1; > + else if (cp1) > + cur_page =3D cp1; > + else if (cp2) > + cur_page =3D cp2; > + else > + return grub_error (GRUB_ERR_BAD_FS, "no checkpoints\n"); > + > + grub_memcpy (&data->ckpt, cur_page, F2FS_BLKSIZE); > + > + grub_free (cp1); > + grub_free (cp2); > + return 0; > +} > + > +static int static grub_error_t > +get_nat_journal (struct grub_f2fs_data *data) > +{ > + grub_uint32_t block; > + char *buf; > + grub_err_t err; > + > + buf =3D grub_malloc (F2FS_BLKSIZE); > + if (!buf) > + return grub_errno; > + > + if (__ckpt_flag_set (&data->ckpt, CP_COMPACT_SUM_FLAG)) > + block =3D __start_sum_block (data); > + else if (__ckpt_flag_set (&data->ckpt, CP_UMOUNT_FLAG)) As mentioned, use grub_cpu_to_leXX_compile_time to avoid run time conversion. > + block =3D __sum_blk_addr (data, NR_CURSEG_TYPE, CURSEG_HOT_DATA); > + else > + block =3D __sum_blk_addr (data, NR_CURSEG_DATA_TYPE, CURSEG_HOT_DATA= ); > + > + err =3D grub_f2fs_block_read (data, block, buf); > + if (err) > + goto fail; > + > + if (__ckpt_flag_set (&data->ckpt, CP_COMPACT_SUM_FLAG)) > + grub_memcpy (&data->nat_j, buf, SUM_JOURNAL_SIZE); > + else > + grub_memcpy (&data->nat_j, buf + SUM_ENTRIES_SIZE, SUM_JOURNAL_SIZE); > + > +fail: > + grub_free (buf); > + return err; > +} > + ... > +static int > +grub_get_node_path (struct grub_f2fs_inode *inode, grub_uint32_t block, > + grub_uint32_t offset[4], grub_uint32_t noffset[4]) > +{ > + grub_uint32_t direct_blks =3D ADDRS_PER_BLOCK; > + grub_uint32_t dptrs_per_blk =3D NIDS_PER_BLOCK; > + grub_uint32_t indirect_blks =3D ADDRS_PER_BLOCK * NIDS_PER_BLOCK; > + grub_uint32_t dindirect_blks =3D indirect_blks * NIDS_PER_BLOCK; > + grub_uint32_t direct_index =3D DEF_ADDRS_PER_INODE; > + int n =3D 0; > + int level =3D 0; > + > + if (__inode_flag_set (inode, FI_INLINE_XATTR)) > + direct_index -=3D F2FS_INLINE_XATTR_ADDRS; > + > + noffset[0] =3D 0; > + > + if (block < direct_index) > + { > + offset[n] =3D block; > + goto got; > + } > + > + block -=3D direct_index; > + if (block < direct_blks) > + { > + offset[n++] =3D NODE_DIR1_BLOCK; > + noffset[n] =3D 1; > + offset[n] =3D block; > + level =3D 1; > + goto got; > + } > + > + block -=3D direct_blks; > + if (block < direct_blks) > + { > + offset[n++] =3D NODE_DIR2_BLOCK; > + noffset[n] =3D 2; > + offset[n] =3D block; > + level =3D 1; > + goto got; > + } > + > + block -=3D direct_blks; > + if (block < indirect_blks) > + { > + offset[n++] =3D NODE_IND1_BLOCK; > + noffset[n] =3D 3; > + offset[n++] =3D block / direct_blks; > + noffset[n] =3D 4 + offset[n - 1]; That does not fit. You declared offset and noffset as arrays of four elements and pass arrays of four elements; here is out of bound access already. > + offset[n] =3D block % direct_blks; > + level =3D 2; > + goto got; > + } > + > + block -=3D indirect_blks; > + if (block < indirect_blks) > + { > + offset[n++] =3D NODE_IND2_BLOCK; > + noffset[n] =3D 4 + dptrs_per_blk; > + offset[n++] =3D block / direct_blks; > + noffset[n] =3D 5 + dptrs_per_blk + offset[n - 1]; > + offset[n] =3D block % direct_blks; > + level =3D 2; > + goto got; > + } > + > + block -=3D indirect_blks; > + if (block < dindirect_blks) > + { > + offset[n++] =3D NODE_DIND_BLOCK; > + noffset[n] =3D 5 + (dptrs_per_blk * 2); > + offset[n++] =3D block / indirect_blks; > + noffset[n] =3D 6 + (dptrs_per_blk * 2) + > + offset[n - 1] * (dptrs_per_blk + 1); > + offset[n++] =3D (block / direct_blks) % dptrs_per_blk; > + noffset[n] =3D 7 + (dptrs_per_blk * 2) + > + offset[n - 2] * (dptrs_per_blk + 1) + > + offset[n - 1]; > + offset[n] =3D block % direct_blks; > + level =3D 3; > + goto got; > + } > +got: > + return level; > +} > + > + > +static grub_err_t > +load_nat_info (struct grub_f2fs_data *data) > +{ > + void *version_bitmap; > + grub_err_t err; > + > + data->nat_bitmap =3D grub_malloc (__nat_bitmap_size (data)); > + if (!data->nat_bitmap) > + return grub_errno; > + > + version_bitmap =3D __nat_bitmap_ptr (data); > + > + /* copy version bitmap */ > + grub_memcpy (data->nat_bitmap, version_bitmap, __nat_bitmap_size (data= )); > + Any reason to actually copy it? Why is it not possible to just set pointer to source, which is available all the time anyway? > + err =3D get_nat_journal (data); > + if (err) > + grub_free (data->nat_bitmap); > + > + return err; > +} > + > +static grub_err_t > +grub_f2fs_read_node (struct grub_f2fs_data *data, > + grub_uint32_t nid, struct grub_f2fs_node *np) > +{ > + grub_uint32_t blkaddr; > + > + blkaddr =3D get_node_blkaddr (data, nid); > + if (!blkaddr) > + return grub_errno; > + > + return grub_f2fs_block_read (data, blkaddr, np); Is struct grub_f2fs_node guaranteed to always have the same size as F2FS block? Then adding char [F2FS_BLKSIZE] to union to make it obvious is better and ensures that it will always be at least this size. > +} > + > +static struct grub_f2fs_data * > +grub_f2fs_mount (grub_disk_t disk) > +{ > + struct grub_f2fs_data *data; > + grub_err_t err; > + > + data =3D grub_zalloc (sizeof (*data)); > + if (!data) > + return NULL; > + > + data->disk =3D disk; > + > + err =3D grub_f2fs_read_sb (data, 0); > + if (err) > + { > + err =3D grub_f2fs_read_sb (data, 1); > + if (err) > + { > + grub_error (GRUB_ERR_BAD_FS, "not a F2FS filesystem"); May be mentioning that superblock could not be read? In another place you already tell that checkpoints could not be found. It helps to troubleshoot issues. > + goto fail; > + } > + } > + > + data->root_ino =3D grub_le_to_cpu32 (data->sblock.root_ino); > + data->cp_blkaddr =3D grub_le_to_cpu32 (data->sblock.cp_blkaddr); > + data->nat_blkaddr =3D grub_le_to_cpu32 (data->sblock.nat_blkaddr); > + data->blocks_per_seg =3D 1 << > + grub_le_to_cpu32 (data->sblock.log_blocks_per_seg); > + > + err =3D grub_f2fs_read_cp (data); > + if (err) > + goto fail; > + > + err =3D load_nat_info (data); > + if (err) > + goto fail; > + > + data->diropen.data =3D data; > + data->diropen.ino =3D data->root_ino; > + data->diropen.inode_read =3D 1; > + data->inode =3D &data->diropen.inode; > + > + err =3D grub_f2fs_read_node (data, data->root_ino, data->inode); > + if (err) > + goto fail; > + > + return data; > + > +fail: > + if (data) > + grub_free (data->nat_bitmap); Double free after load_nat_info failure. Assuming that we do need to allocate anything at all (see above). > + grub_free (data); > + return NULL; > +} > + > +/* guarantee inline_data was handled by caller */ > +static grub_disk_addr_t > +grub_f2fs_read_block (grub_fshelp_node_t node, grub_disk_addr_t block_of= s) You have grub_f2fs_read_block and grub_f2fs_block_read. Could we make them more different and self-explaining? In particular, this one does not read anything, it returns disk address. grub_f2fs_map_file_block? > +{ > + struct grub_f2fs_data *data =3D node->data; > + struct grub_f2fs_inode *inode =3D &node->inode.i; > + grub_uint32_t offset[4], noffset[4], nids[4]; See above about overflow in grub_get_inode_path. > + struct grub_f2fs_node *node_block; > + grub_uint32_t block_addr =3D -1; > + int level, i; > + > + level =3D grub_get_node_path (inode, block_ofs, offset, noffset); > + if (level =3D=3D 0) > + return grub_le_to_cpu32 (inode->i_addr[offset[0]]); > + > + node_block =3D grub_malloc (F2FS_BLKSIZE); > + if (!node_block) > + return -1; > + > + nids[1] =3D __get_node_id (&node->inode, offset[0], 1); > + > + /* get indirect or direct nodes */ > + for (i =3D 1; i <=3D level; i++) > + { > + grub_f2fs_read_node (data, nids[i], node_block); > + if (grub_errno) > + goto fail; > + > + if (i < level) > + nids[i + 1] =3D __get_node_id (node_block, offset[i], 0); > + } > + > + block_addr =3D grub_le_to_cpu32 (node_block->dn.addr[offset[level]]); > +fail: > + grub_free (node_block); > + return block_addr; > +} > + ... > + > +static char * > +grub_f2fs_read_symlink (grub_fshelp_node_t node) > +{ > + char *symlink; > + struct grub_fshelp_node *diro =3D node; > + > + if (!diro->inode_read) > + { > + grub_f2fs_read_node (diro->data, diro->ino, &diro->inode); > + if (grub_errno) > + return 0; > + } > + > + symlink =3D grub_malloc (__i_size (&diro->inode.i) + 1); > + if (!symlink) > + return 0; > + > + grub_f2fs_read_file (diro, 0, 0, 0, __i_size (&diro->inode.i), symlink= ); > + if (grub_errno) > + { > + grub_free (symlink); > + return 0; > + } > + What about short read? Is this an error or not? > + symlink[__i_size (&diro->inode.i)] =3D '\0'; > + return symlink; > +} > + > +static int > +grub_f2fs_check_dentries (struct grub_f2fs_dir_iter_ctx *ctx) > +{ > + struct grub_fshelp_node *fdiro; > + int i; > + > + for (i =3D 0; i < ctx->max;) > + { > + char filename[F2FS_NAME_LEN + 1]; Could we avoid large stack allocations? > + enum grub_fshelp_filetype type =3D GRUB_FSHELP_UNKNOWN; > + enum FILE_TYPE ftype; > + int name_len; > + > + if (__test_bit (i, ctx->bitmap) =3D=3D 0) grub_f2fs_test_bit_le32? > + { > + i++; > + continue; > + } > + > + ftype =3D ctx->dentry[i].file_type; > + name_len =3D grub_le_to_cpu16 (ctx->dentry[i].name_len); > + grub_memcpy (filename, ctx->filename[i], name_len); > + filename[name_len] =3D '\0'; > + > + fdiro =3D grub_malloc (sizeof (struct grub_fshelp_node)); > + if (!fdiro) > + return 0; > + > + if (ftype =3D=3D F2FS_FT_DIR) > + type =3D GRUB_FSHELP_DIR; > + else if (ftype =3D=3D F2FS_FT_SYMLINK) > + type =3D GRUB_FSHELP_SYMLINK; > + else if (ftype =3D=3D F2FS_FT_REG_FILE) > + type =3D GRUB_FSHELP_REG; > + > + fdiro->data =3D ctx->data; > + fdiro->ino =3D grub_le_to_cpu32 (ctx->dentry[i].ino); > + fdiro->inode_read =3D 0; > + > + if (ctx->hook (filename, type, fdiro, ctx->hook_data)) > + return 1; > + > + i +=3D (name_len + F2FS_SLOT_LEN - 1) / F2FS_SLOT_LEN; > + } > + return 0; > +} > + ... > + > +static int > +grub_f2fs_iterate_dir (grub_fshelp_node_t dir, > + grub_fshelp_iterate_dir_hook_t hook, void *hook_data) > +{ > + struct grub_fshelp_node *diro =3D (struct grub_fshelp_node *) dir; > + struct grub_f2fs_inode *inode; > + struct grub_f2fs_dir_iter_ctx ctx =3D { > + .data =3D diro->data, > + .hook =3D hook, > + .hook_data =3D hook_data > + }; > + grub_off_t fpos =3D 0; > + > + if (!diro->inode_read) > + { > + grub_f2fs_read_node (diro->data, diro->ino, &diro->inode); > + if (grub_errno) > + return 0; > + } > + > + inode =3D &diro->inode.i; > + > + if (__inode_flag_set (inode, FI_INLINE_DENTRY)) > + return grub_f2fs_iterate_inline_dir (inode, &ctx); > + > + while (fpos < __i_size (inode)) > + { > + struct grub_f2fs_dentry_block *de_blk; > + char *buf; > + > + buf =3D grub_zalloc (F2FS_BLKSIZE); > + if (!buf) > + return 0; > + > + grub_f2fs_read_file (diro, 0, 0, fpos, F2FS_BLKSIZE, buf); > + if (grub_errno) > + { > + grub_free (buf); > + return 0; > + } > + > + de_blk =3D (struct grub_f2fs_dentry_block *) buf; > + > + ctx.bitmap =3D (grub_uint32_t *) de_blk->dentry_bitmap; > + ctx.dentry =3D de_blk->dentry; > + ctx.filename =3D de_blk->filename; > + ctx.max =3D NR_DENTRY_IN_BLOCK; > + > + if (grub_f2fs_check_dentries (&ctx)) > + return 1; memory leak > + > + grub_free (buf); > + > + fpos +=3D F2FS_BLKSIZE; > + } > + return 0; > +} > + ... > +static grub_err_t > +grub_f2fs_dir (grub_device_t device, const char *path, > + grub_fs_dir_hook_t hook, void *hook_data) > +{ > + struct grub_f2fs_dir_ctx ctx =3D { > + .hook =3D hook, > + .hook_data =3D hook_data > + }; > + struct grub_fshelp_node *fdiro =3D 0; > + > + grub_dl_ref (my_mod); > + > + ctx.data =3D grub_f2fs_mount (device->disk); > + if (!ctx.data) > + goto fail; > + > + grub_fshelp_find_file (path, &ctx.data->diropen, &fdiro, > + grub_f2fs_iterate_dir, grub_f2fs_read_symlink, > + GRUB_FSHELP_DIR); > + if (grub_errno) > + goto fail; > + > + grub_f2fs_iterate_dir (fdiro, grub_f2fs_dir_iter, &ctx); > + > +fail: > + if (fdiro !=3D &ctx.data->diropen) > + grub_free (fdiro); > + if (ctx.data) > + grub_free (ctx.data->nat_bitmap); Triple free :) > + grub_free (ctx.data); > + grub_dl_unref (my_mod); > + return grub_errno; > +} > + > + > +/* Open a file named NAME and initialize FILE. */ > +static grub_err_t > +grub_f2fs_open (struct grub_file *file, const char *name) > +{ > + struct grub_f2fs_data *data =3D NULL; > + struct grub_fshelp_node *fdiro =3D 0; > + > + grub_dl_ref (my_mod); > + > + data =3D grub_f2fs_mount (file->device->disk); > + if (!data) > + goto fail; > + > + grub_fshelp_find_file (name, &data->diropen, &fdiro, > + grub_f2fs_iterate_dir, grub_f2fs_read_symlink, > + GRUB_FSHELP_REG); > + if (grub_errno) > + goto fail; > + > + if (!fdiro->inode_read) > + { > + grub_f2fs_read_node (data, fdiro->ino, &fdiro->inode); > + if (grub_errno) > + goto fail; > + } > + > + grub_memcpy (data->inode, &fdiro->inode, F2FS_BLKSIZE); sizeof (*data->inode)? Or they can be different? > + grub_free (fdiro); > + > + file->size =3D __i_size (&(data->inode->i)); > + file->data =3D data; > + file->offset =3D 0; > + > + return 0; > + > +fail: > + if (fdiro !=3D &data->diropen) > + grub_free (fdiro); > + if (data) > + grub_free (data->nat_bitmap); Again. > + grub_free (data); > + > + grub_dl_unref (my_mod); > + > + return grub_errno; > +} > + > +static grub_ssize_t > +grub_f2fs_read (grub_file_t file, char *buf, grub_size_t len) > +{ > + struct grub_f2fs_data *data =3D (struct grub_f2fs_data *) file->data; > + > + return grub_f2fs_read_file (&data->diropen, > + file->read_hook, file->read_hook_data, > + file->offset, len, buf); > +} > + > +static grub_err_t > +grub_f2fs_close (grub_file_t file) > +{ > + struct grub_f2fs_data *data =3D (struct grub_f2fs_data *) file->data; > + > + if (data) > + grub_free (data->nat_bitmap); Again. > + grub_free (data); > + > + grub_dl_unref (my_mod); > + > + return GRUB_ERR_NONE; > +} > + > +static grub_err_t > +grub_f2fs_label (grub_device_t device, char **label) > +{ > + struct grub_f2fs_data *data; > + grub_disk_t disk =3D device->disk; > + > + grub_dl_ref (my_mod); > + > + data =3D grub_f2fs_mount (disk); > + if (data) > + { > + *label =3D grub_zalloc (sizeof (data->sblock.volume_name)); > + grub_utf16_to_utf8 ((grub_uint8_t *) (*label), malloc failure check? > + data->sblock.volume_name, 512); Where 512 comes from? Should it not be sizeof (data->sblock.volume_name) as well? > + } > + else > + *label =3D NULL; > + > + if (data) > + grub_free (data->nat_bitmap); Again. > + grub_free (data); > + grub_dl_unref (my_mod); > + return grub_errno; > +} > + ... > diff --git a/tests/util/grub-fs-tester.in b/tests/util/grub-fs-tester.in > index e9e85c2..acc35cc 100644 > --- a/tests/util/grub-fs-tester.in > +++ b/tests/util/grub-fs-tester.in > @@ -36,7 +36,7 @@ case x"$fs" in > MINLOGSECSIZE=3D8 > # OS LIMITATION: It could go up to 32768 but Linux rejects sector = sizes > 4096 > MAXLOGSECSIZE=3D12;; > - xxfs) > + xxfs|xf2fs) > MINLOGSECSIZE=3D9 > # OS LIMITATION: GNU/Linux doesn't accept > 4096 > MAXLOGSECSIZE=3D12;; > @@ -135,6 +135,10 @@ for ((LOGSECSIZE=3DMINLOGSECSIZE;LOGSECSIZE<=3DMAXLO= GSECSIZE;LOGSECSIZE=3DLOGSECSIZE + > fi > MAXBLKSIZE=3D4096 > ;; > + xf2fs) > + MINBLKSIZE=3D$SECSIZE > + # OS Limitation: GNU/Linux doesn't accept > 4096 > + MAXBLKSIZE=3D4096;; > xsquash*) > MINBLKSIZE=3D4096 > MAXBLKSIZE=3D1048576;; > @@ -256,7 +260,9 @@ for ((LOGSECSIZE=3DMINLOGSECSIZE;LOGSECSIZE<=3DMAXLOG= SECSIZE;LOGSECSIZE=3DLOGSECSIZE + > # FS LIMITATION: btrfs label is at most 255 UTF-8 chars > x"btrfs"*) > FSLABEL=3D"grub_;/test=C3=A9=F0=AF=A6=9B=F0=AF=A6=9D=F0=9F=98=81= =D0=BA=D0=B8=D1=80=D0=B8=D1=82i urewfceniuewruevrewnuuireurevueurnievrewfne= rfcnevirivinrewvnirewnivrewiuvcrewvnuewvrrrewniuerwreiuviurewiuviurewnuvewn= vrenurnunuvrevuurerejiremvreijnvcreivire nverivnreivrevnureiorfnfrvoeoiroir= eoireoifrefoieroifoireoif";; > - > + # FS LIMITATION: btrfs label is at most 512 UTF-16 chars F2FS, not btrfs > + x"f2fs") > + FSLABEL=3D"grub_;/testjaegeuk kim=20 Could you leave initial part in place? This includes some funny UNICODE characters for a reason, actually. Unless this is not possible with f2fs? f2fsaskdfjkasdlfajskdfjaksdjfkjaskjkjkzjkjckzjvkcjkjkjekqjkwejkqwrlkasdfjks= adjflaskdhzxhvjzxchvjzkxchvjkhakjsdhfjkhqjkwehrjkhasjkdfhjkashdfjkhjzkxhcjk= vzhxcjkvhzxjchvkzhxckjvhjzkxchvjkhzjkxchvjkzhxckjvhzkxjchvkjzxhckjvzxcjkvhj= zxkchkvjhzxkjcvhjkhjkahsjkdhkjqhwekrjhakjsdfhkjashdkjzhxcvjkhzxcvzxcvgggggg= ggggf";; > # FS LIMITATION: exfat is at most 15 UTF-16 chars > x"exfat") > FSLABEL=3D"g=C3=A9=D1=82 ;/=F0=AF=A6=9B=F0=AF=A6=9D=F0=9F=98=81=D0= =BA=D0=B8=D1=80";; > @@ -466,7 +472,7 @@ for ((LOGSECSIZE=3DMINLOGSECSIZE;LOGSECSIZE<=3DMAXLOG= SECSIZE;LOGSECSIZE=3DLOGSECSIZE + > # FIXME: Not sure about BtrFS, NTFS, JFS, AFS, UDF and SFS. Check i= t. > # FS LIMITATION: as far as I know those FS don't store their last modif= ication date. > x"jfs_caseins" | x"jfs" | x"xfs"| x"btrfs"* | x"reiserfs_old" | x"reis= erfs" \ > - | x"bfs" | x"afs" \ > + | x"bfs" | x"afs" | x"f2fs" \ > | x"tarfs" | x"cpio_"* | x"minix" | x"minix2" \ > | x"minix3" | x"ntfs"* | x"udf" | x"sfs"*) > NOFSTIME=3Dy;; > @@ -745,6 +751,8 @@ for ((LOGSECSIZE=3DMINLOGSECSIZE;LOGSECSIZE<=3DMAXLOG= SECSIZE;LOGSECSIZE=3DLOGSECSIZE + > MOUNTDEVICE=3D"/dev/mapper/grub_test-testvol" > MOUNTFS=3Dext2 > "mkfs.ext2" -L "$FSLABEL" -q "${MOUNTDEVICE}" ;; > + xf2fs) > + "mkfs.f2fs" -l "$FSLABEL" -q "${LODEVICES[0]}" ;; > xnilfs2) > "mkfs.nilfs2" -L "$FSLABEL" -b $BLKSIZE -q "${LODEVICES[0]}" ;; > xext2_old)