Hi Andrei,
On Sat, Mar 28, 2015 at 10:31:55AM +0300, Andrei Borzenkov wrote:
> В Tue, 24 Mar 2015 01:19:00 -0700
> Jaegeuk Kim <jaeg...@kernel.org> пишет:
>
...
> > +/* 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.
Fixed.
>
> ...
> > +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?
I remained this, since this can be non-multiple of 4 bytes.
>
> > + 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;
> > +
...
> > +
> > +#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?
Removed this.
>
> ...
> > +
> > +static inline int
> > +__test_bit (int nr, grub_uint32_t *addr)
> > +{
> > + return 1UL & (addr[nr / 32] >> (nr & (31)));
> Extra parenthesis (31)
Fixed.
>
> > +}
> > +
>
> 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.
I added:
grub_f2fs_general_test_bit
grub_f2fs_test_bit
Both of them keep bit streams without endian conversion, and the difference is
beyond handling the order of bit stream indices.
>
> > +static inline char *
> > +__inline_addr (struct grub_f2fs_inode *inode)
> > +{
> > + return (char *)&(inode->i_addr[1]);
> Redundant parens around inode->
Fixed.
>
> > +}
> > +
> > +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.
Changed to grub_f2fs_file_size.
>
> > +{
> > + 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 = &data->ckpt;
> > + grub_uint64_t ckpt_version = grub_le_to_cpu64 (ckpt->checkpoint_ver);
> > + grub_uint32_t start_addr = 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 = &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 = &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 = grub_le_to_cpu32 (ckpt->ckpt_flags);
> > + return ckpt_flags & f;
>
> All flags are constant so you can simply do
>
> ckpt->ckpt_flags & grub_cpu_to_le32_compile_time (FLAG)
>
> in place to avoid extra calls. This makes function redundant.
Fixed.
>
> > +}
> > +
> > +static inline int
> > +__inode_flag_set (struct grub_f2fs_inode *inode, int flag)
Fixed to refer i_inline. This was a bug.
...
> > + * 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.
Fixed.
>
> > +{
> > + int i;
> > + unsigned char *p = (unsigned char *)buf;
> > +
> > + while (len--)
> > + {
> > + crc ^= *p++;
> > + for (i = 0; i < 8; i++)
> > + crc = (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 = 0;
> > +
> > + cal_crc = grub_f2fs_cal_crc32 (F2FS_SUPER_MAGIC, buf, len);
> > +
> > + return (cal_crc == blk_crc) ? 1 : 0;
> > +}
> > +
> > +static inline int
> > +grub_f2fs_test_bit (grub_uint32_t nr, const char *p)
> > +{
> > + int mask;
> > + char *addr = (char *)p;
>
> Why cast? We are not going to modify it, right?
Right.
>
> > +
> > + addr += (nr >> 3);
> > + mask = 1 << (7 - (nr & 0x07));
> > + return (mask & *addr) != 0;
> > +}
> > +
> > +static int
> > +grub_f2fs_sanity_check_sb (struct grub_f2fs_superblock *sb)
> > +{
> > + unsigned int blocksize;
> > +
> > + if (F2FS_SUPER_MAGIC != grub_le_to_cpu32 (sb->magic))
>
> sb->magic != grub_cpu_to_le32_compile_time (F2FS_SUPER_MAGIC)
Fixed.
>
> > + return -1;
> > +
> > + blocksize = 1 << grub_le_to_cpu32 (sb->log_blocksize);
> > + if (blocksize != F2FS_BLKSIZE)
>
> sb->log_blksize != grub_cpu_to_le32_compile_time (F2FS_BLK_BITS)
Fixed.
>
> > + 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) != F2FS_MAX_LOG_SECTOR_SIZE)
>
> Should not it be F2FS_BLKSIZE? At least it sounds logical. Also please
> convert log_sectorsize just once.
Fixed.
>
> > + return -1;
> > +
> > + return 0;
> > +}
> > +
> > +static grub_err_t
> > +grub_f2fs_read_sb (struct grub_f2fs_data *data, int block)
> > +{
> > + grub_disk_t disk = data->disk;
> > + grub_uint64_t offset;
> > + grub_err_t err;
> > +
> > + if (block == 0)
> > + offset = F2FS_SUPER_OFFSET;
> > + else
> > + offset = 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 = F2FS_SUPER_OFFEST;
> if (secondary)
> offset += F2FS_BLKSIZE;
Fixed as your latest comment.
>
> > + /* Read first super block. */
> > + err = 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 = 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 = 0, pre_version = 0;
> > + grub_uint32_t crc = 0;
> > + grub_uint32_t crc_offset;
> > + grub_err_t err;
> > +
> > + /* Read the 1st cp block in this CP pack */
> > + cp_page_1 = grub_malloc (F2FS_BLKSIZE);
> > + if (!cp_page_1)
> > + return NULL;
> > +
> > + err = grub_f2fs_block_read (data, cp_addr, cp_page_1);
> > + if (err)
> > + goto invalid_cp1;
> > +
> > + cp_block = (struct grub_f2fs_checkpoint *)cp_page_1;
> > + crc_offset = grub_le_to_cpu32 (cp_block->checksum_offset);
> > + if (crc_offset >= F2FS_BLKSIZE)
> > + goto invalid_cp1;
> > +
> > + crc = *(grub_uint32_t *)((char *)cp_block + crc_offset);
>
> Is unaligned access possible here? If yes, it probably should be
> grub_get_unaligned32.
No. It was hard-coded as 4092 from mkfs.
>
> > + if (!grub_f2fs_crc_valid (crc, cp_block, crc_offset))
> > + goto invalid_cp1;
> > +
>
> Should not CRC be converted from LE?
Fixed.
>
> > + pre_version = grub_le_to_cpu64 (cp_block->checkpoint_ver);
> > +
> > + /* Read the 2nd cp block in this CP pack */
> > + cp_page_2 = grub_malloc (F2FS_BLKSIZE);
> > + if (!cp_page_2)
> > + goto invalid_cp1;
> > +
> > + cp_addr += grub_le_to_cpu32 (cp_block->cp_pack_total_block_count) - 1;
> > +
> > + err = grub_f2fs_block_read (data, cp_addr, cp_page_2);
> > + if (err)
> > + goto invalid_cp2;
> > +
> > + cp_block = (struct grub_f2fs_checkpoint *)cp_page_2;
> > + crc_offset = grub_le_to_cpu32 (cp_block->checksum_offset);
> > + if (crc_offset >= F2FS_BLKSIZE)
> > + goto invalid_cp2;
> > +
> > + crc = *(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 = grub_le_to_cpu64 (cp_block->checkpoint_ver);
> > + if (cur_version == pre_version)
> > + {
> > + *version = 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 = 0, cp2_version = 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 = data->cp_blkaddr;
> > + cp1 = 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 += data->blocks_per_seg;
> > + cp2 = validate_checkpoint (data, cp_start_blk_no, &cp2_version);
> > + if (!cp2 && grub_errno)
> > + {
> > + grub_free (cp1);
> > + return grub_errno;
> > + }
> > +
> > + if (cp1 && cp2)
> > + cur_page = (cp2_version > cp1_version) ? cp2 : cp1;
> > + else if (cp1)
> > + cur_page = cp1;
> > + else if (cp2)
> > + cur_page = 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
Fixed.
>
> > +get_nat_journal (struct grub_f2fs_data *data)
> > +{
> > + grub_uint32_t block;
> > + char *buf;
> > + grub_err_t err;
> > +
> > + buf = grub_malloc (F2FS_BLKSIZE);
> > + if (!buf)
> > + return grub_errno;
> > +
> > + if (__ckpt_flag_set (&data->ckpt, CP_COMPACT_SUM_FLAG))
> > + block = __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.
Fixed.
>
> > + block = __sum_blk_addr (data, NR_CURSEG_TYPE, CURSEG_HOT_DATA);
> > + else
> > + block = __sum_blk_addr (data, NR_CURSEG_DATA_TYPE, CURSEG_HOT_DATA);
> > +
> > + err = 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 = ADDRS_PER_BLOCK;
> > + grub_uint32_t dptrs_per_blk = NIDS_PER_BLOCK;
> > + grub_uint32_t indirect_blks = ADDRS_PER_BLOCK * NIDS_PER_BLOCK;
> > + grub_uint32_t dindirect_blks = indirect_blks * NIDS_PER_BLOCK;
> > + grub_uint32_t direct_index = DEF_ADDRS_PER_INODE;
> > + int n = 0;
> > + int level = 0;
> > +
> > + if (__inode_flag_set (inode, FI_INLINE_XATTR))
> > + direct_index -= F2FS_INLINE_XATTR_ADDRS;
> > +
> > + noffset[0] = 0;
> > +
> > + if (block < direct_index)
> > + {
> > + offset[n] = block;
> > + goto got;
> > + }
> > +
> > + block -= direct_index;
> > + if (block < direct_blks)
> > + {
> > + offset[n++] = NODE_DIR1_BLOCK;
> > + noffset[n] = 1;
> > + offset[n] = block;
> > + level = 1;
> > + goto got;
> > + }
> > +
> > + block -= direct_blks;
> > + if (block < direct_blks)
> > + {
> > + offset[n++] = NODE_DIR2_BLOCK;
> > + noffset[n] = 2;
> > + offset[n] = block;
> > + level = 1;
> > + goto got;
> > + }
> > +
> > + block -= direct_blks;
> > + if (block < indirect_blks)
> > + {
> > + offset[n++] = NODE_IND1_BLOCK;
> > + noffset[n] = 3;
> > + offset[n++] = block / direct_blks;
> > + noffset[n] = 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.
It is not out of bound access, since it decreases *block* at every condition
checks.
This function should hit only one of if {} conditions.
>
> > + offset[n] = block % direct_blks;
> > + level = 2;
> > + goto got;
> > + }
> > +
> > + block -= indirect_blks;
> > + if (block < indirect_blks)
> > + {
> > + offset[n++] = NODE_IND2_BLOCK;
> > + noffset[n] = 4 + dptrs_per_blk;
> > + offset[n++] = block / direct_blks;
> > + noffset[n] = 5 + dptrs_per_blk + offset[n - 1];
> > + offset[n] = block % direct_blks;
> > + level = 2;
> > + goto got;
> > + }
> > +
> > + block -= indirect_blks;
> > + if (block < dindirect_blks)
> > + {
> > + offset[n++] = NODE_DIND_BLOCK;
> > + noffset[n] = 5 + (dptrs_per_blk * 2);
> > + offset[n++] = block / indirect_blks;
> > + noffset[n] = 6 + (dptrs_per_blk * 2) +
> > + offset[n - 1] * (dptrs_per_blk + 1);
> > + offset[n++] = (block / direct_blks) % dptrs_per_blk;
> > + noffset[n] = 7 + (dptrs_per_blk * 2) +
> > + offset[n - 2] * (dptrs_per_blk + 1) +
> > + offset[n - 1];
> > + offset[n] = block % direct_blks;
> > + level = 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 = grub_malloc (__nat_bitmap_size (data));
> > + if (!data->nat_bitmap)
> > + return grub_errno;
> > +
> > + version_bitmap = __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?
Fixed not to allocate and copying this.
>
> > + err = 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 = 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.
Fixed.
>
> > +}
> > +
> > +static struct grub_f2fs_data *
> > +grub_f2fs_mount (grub_disk_t disk)
> > +{
> > + struct grub_f2fs_data *data;
> > + grub_err_t err;
> > +
> > + data = grub_zalloc (sizeof (*data));
> > + if (!data)
> > + return NULL;
> > +
> > + data->disk = disk;
> > +
> > + err = grub_f2fs_read_sb (data, 0);
> > + if (err)
> > + {
> > + err = 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.
Fixed.
>
> > + goto fail;
> > + }
> > + }
> > +
> > + data->root_ino = grub_le_to_cpu32 (data->sblock.root_ino);
> > + data->cp_blkaddr = grub_le_to_cpu32 (data->sblock.cp_blkaddr);
> > + data->nat_blkaddr = grub_le_to_cpu32 (data->sblock.nat_blkaddr);
> > + data->blocks_per_seg = 1 <<
> > + grub_le_to_cpu32 (data->sblock.log_blocks_per_seg);
> > +
> > + err = grub_f2fs_read_cp (data);
> > + if (err)
> > + goto fail;
> > +
> > + err = load_nat_info (data);
> > + if (err)
> > + goto fail;
> > +
> > + data->diropen.data = data;
> > + data->diropen.ino = data->root_ino;
> > + data->diropen.inode_read = 1;
> > + data->inode = &data->diropen.inode;
> > +
> > + err = 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).
Removed all grub_frees.
>
> > + 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_ofs)
>
> 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?
Good suggestion.
Changed to grub_f2fs_get_block.
>
> > +{
> > + struct grub_f2fs_data *data = node->data;
> > + struct grub_f2fs_inode *inode = &node->inode.i;
> > + grub_uint32_t offset[4], noffset[4], nids[4];
>
> See above about overflow in grub_get_inode_path.
No error.
>
> > + struct grub_f2fs_node *node_block;
> > + grub_uint32_t block_addr = -1;
> > + int level, i;
> > +
> > + level = grub_get_node_path (inode, block_ofs, offset, noffset);
> > + if (level == 0)
> > + return grub_le_to_cpu32 (inode->i_addr[offset[0]]);
> > +
> > + node_block = grub_malloc (F2FS_BLKSIZE);
> > + if (!node_block)
> > + return -1;
> > +
> > + nids[1] = __get_node_id (&node->inode, offset[0], 1);
> > +
> > + /* get indirect or direct nodes */
> > + for (i = 1; i <= level; i++)
> > + {
> > + grub_f2fs_read_node (data, nids[i], node_block);
> > + if (grub_errno)
> > + goto fail;
> > +
> > + if (i < level)
> > + nids[i + 1] = __get_node_id (node_block, offset[i], 0);
> > + }
> > +
> > + block_addr = 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 = node;
> > +
> > + if (!diro->inode_read)
> > + {
> > + grub_f2fs_read_node (diro->data, diro->ino, &diro->inode);
> > + if (grub_errno)
> > + return 0;
> > + }
> > +
> > + symlink = 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?
What is short read?
When I refer the other filesystem, it seems that it doesn't need to return
errors.
>
> > + symlink[__i_size (&diro->inode.i)] = '\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 = 0; i < ctx->max;)
> > + {
> > + char filename[F2FS_NAME_LEN + 1];
>
> Could we avoid large stack allocations?
Fixed to allocate dynamically.
>
> > + enum grub_fshelp_filetype type = GRUB_FSHELP_UNKNOWN;
> > + enum FILE_TYPE ftype;
> > + int name_len;
> > +
> > + if (__test_bit (i, ctx->bitmap) == 0)
>
> grub_f2fs_test_bit_le32?
>
> > + {
> > + i++;
> > + continue;
> > + }
> > +
> > + ftype = ctx->dentry[i].file_type;
> > + name_len = grub_le_to_cpu16 (ctx->dentry[i].name_len);
> > + grub_memcpy (filename, ctx->filename[i], name_len);
> > + filename[name_len] = '\0';
> > +
> > + fdiro = grub_malloc (sizeof (struct grub_fshelp_node));
> > + if (!fdiro)
> > + return 0;
> > +
> > + if (ftype == F2FS_FT_DIR)
> > + type = GRUB_FSHELP_DIR;
> > + else if (ftype == F2FS_FT_SYMLINK)
> > + type = GRUB_FSHELP_SYMLINK;
> > + else if (ftype == F2FS_FT_REG_FILE)
> > + type = GRUB_FSHELP_REG;
> > +
> > + fdiro->data = ctx->data;
> > + fdiro->ino = grub_le_to_cpu32 (ctx->dentry[i].ino);
> > + fdiro->inode_read = 0;
> > +
> > + if (ctx->hook (filename, type, fdiro, ctx->hook_data))
> > + return 1;
> > +
> > + i += (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 = (struct grub_fshelp_node *) dir;
> > + struct grub_f2fs_inode *inode;
> > + struct grub_f2fs_dir_iter_ctx ctx = {
> > + .data = diro->data,
> > + .hook = hook,
> > + .hook_data = hook_data
> > + };
> > + grub_off_t fpos = 0;
> > +
> > + if (!diro->inode_read)
> > + {
> > + grub_f2fs_read_node (diro->data, diro->ino, &diro->inode);
> > + if (grub_errno)
> > + return 0;
> > + }
> > +
> > + inode = &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 = 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 = (struct grub_f2fs_dentry_block *) buf;
> > +
> > + ctx.bitmap = (grub_uint32_t *) de_blk->dentry_bitmap;
> > + ctx.dentry = de_blk->dentry;
> > + ctx.filename = de_blk->filename;
> > + ctx.max = NR_DENTRY_IN_BLOCK;
> > +
> > + if (grub_f2fs_check_dentries (&ctx))
> > + return 1;
>
> memory leak
Fixed.
>
> > +
> > + grub_free (buf);
> > +
> > + fpos += 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 = {
> > + .hook = hook,
> > + .hook_data = hook_data
> > + };
> > + struct grub_fshelp_node *fdiro = 0;
> > +
> > + grub_dl_ref (my_mod);
> > +
> > + ctx.data = 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 != &ctx.data->diropen)
> > + grub_free (fdiro);
> > + if (ctx.data)
> > + grub_free (ctx.data->nat_bitmap);
>
> Triple free :)
Removed nat_bitmap entirely. :)
>
> > + 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 = NULL;
> > + struct grub_fshelp_node *fdiro = 0;
> > +
> > + grub_dl_ref (my_mod);
> > +
> > + data = 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?
Not a big deal. Fixed.
>
> > + grub_free (fdiro);
> > +
> > + file->size = __i_size (&(data->inode->i));
> > + file->data = data;
> > + file->offset = 0;
> > +
> > + return 0;
> > +
> > +fail:
> > + if (fdiro != &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 = (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 = (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 = device->disk;
> > +
> > + grub_dl_ref (my_mod);
> > +
> > + data = grub_f2fs_mount (disk);
> > + if (data)
> > + {
> > + *label = grub_zalloc (sizeof (data->sblock.volume_name));
> > + grub_utf16_to_utf8 ((grub_uint8_t *) (*label),
>
> malloc failure check?
Fxied.
>
> > + data->sblock.volume_name, 512);
>
> Where 512 comes from? Should it not be sizeof
> (data->sblock.volume_name) as well?
Fixed regarding to mkfs.f2fs handling.
>
> > + }
> > + else
> > + *label = 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=8
> > # OS LIMITATION: It could go up to 32768 but Linux rejects sector
> > sizes > 4096
> > MAXLOGSECSIZE=12;;
> > - xxfs)
> > + xxfs|xf2fs)
> > MINLOGSECSIZE=9
> > # OS LIMITATION: GNU/Linux doesn't accept > 4096
> > MAXLOGSECSIZE=12;;
> > @@ -135,6 +135,10 @@ for
> > ((LOGSECSIZE=MINLOGSECSIZE;LOGSECSIZE<=MAXLOGSECSIZE;LOGSECSIZE=LOGSECSIZE +
> > fi
> > MAXBLKSIZE=4096
> > ;;
> > + xf2fs)
> > + MINBLKSIZE=$SECSIZE
> > + # OS Limitation: GNU/Linux doesn't accept > 4096
> > + MAXBLKSIZE=4096;;
> > xsquash*)
> > MINBLKSIZE=4096
> > MAXBLKSIZE=1048576;;
> > @@ -256,7 +260,9 @@ for
> > ((LOGSECSIZE=MINLOGSECSIZE;LOGSECSIZE<=MAXLOGSECSIZE;LOGSECSIZE=LOGSECSIZE +
> > # FS LIMITATION: btrfs label is at most 255 UTF-8 chars
> > x"btrfs"*)
> > FSLABEL="grub_;/testé莭莽😁киритi
> > urewfceniuewruevrewnuuireurevueurnievrewfnerfcnevirivinrewvnirewnivrewiuvcrewvnuewvrrrewniuerwreiuviurewiuviurewnuvewnvrenurnunuvrevuurerejiremvreijnvcreivire
> > nverivnreivrevnureiorfnfrvoeoiroireoireoifrefoieroifoireoif";;
> > -
> > + # FS LIMITATION: btrfs label is at most 512 UTF-16 chars
>
> F2FS, not btrfs
>
> > + x"f2fs")
> > + FSLABEL="grub_;/testjaegeuk kim
>
> Could you leave initial part in place? This includes some funny UNICODE
> characters for a reason, actually. Unless this is not possible with
> f2fs?
I found that mkfs.f2fs doesn't handle utf conversion correctly.
So, for now, I'd like to add just few characters.
Please, v2 patch.
Thanks,
>
> f2fsaskdfjkasdlfajskdfjaksdjfkjaskjkjkzjkjckzjvkcjkjkjekqjkwejkqwrlkasdfjksadjflaskdhzxhvjzxchvjzkxchvjkhakjsdhfjkhqjkwehrjkhasjkdfhjkashdfjkhjzkxhcjkvzhxcjkvhzxjchvkzhxckjvhjzkxchvjkhzjkxchvjkzhxckjvhzkxjchvkjzxhckjvzxcjkvhjzxkchkvjhzxkjcvhjkhjkahsjkdhkjqhwekrjhakjsdfhkjashdkjzhxcvjkhzxcvzxcvggggggggggf";;
> > # FS LIMITATION: exfat is at most 15 UTF-16 chars
> > x"exfat")
> > FSLABEL="géт ;/莭莽😁кир";;
> > @@ -466,7 +472,7 @@ for
> > ((LOGSECSIZE=MINLOGSECSIZE;LOGSECSIZE<=MAXLOGSECSIZE;LOGSECSIZE=LOGSECSIZE +
> > # FIXME: Not sure about BtrFS, NTFS, JFS, AFS, UDF and SFS. Check
> > it.
> > # FS LIMITATION: as far as I know those FS don't store their last
> > modification date.
> > x"jfs_caseins" | x"jfs" | x"xfs"| x"btrfs"* | x"reiserfs_old" |
> > x"reiserfs" \
> > - | 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=y;;
> > @@ -745,6 +751,8 @@ for
> > ((LOGSECSIZE=MINLOGSECSIZE;LOGSECSIZE<=MAXLOGSECSIZE;LOGSECSIZE=LOGSECSIZE +
> > MOUNTDEVICE="/dev/mapper/grub_test-testvol"
> > MOUNTFS=ext2
> > "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)
_______________________________________________
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel
