Sorry for delay.
В Fri, 3 Apr 2015 15:49:08 -0700
Jaegeuk Kim <jaeg...@kernel.org> пишет:
>
> This patch changes:
>
> * 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.
>
Drop file list, it is available from git.
...
> diff --git a/grub-core/fs/f2fs.c b/grub-core/fs/f2fs.c
> new file mode 100644
> index 0000000..e6b8386
> --- /dev/null
> +++ b/grub-core/fs/f2fs.c
> +
...
> +#define JENTRY_SIZE 13
sizeof (struct grub_f2fs_nat_jent), right?
...
> +
> +#define NAT_ENTRY_SIZE 9
That's sizeof (struct grub_f2fs_nat_entry)?
...
> +#define ver_after (a, b) ((long long)((a) - (b)) > 0)
This macro is not used anywhere
> +#define F2FS_NAME_LEN 255
> +#define F2FS_SLOT_LEN 8
> +#define NR_DENTRY_IN_BLOCK 214
> +#define SIZE_OF_DIR_ENTRY 11 /* by byte */
sizeof (struct grub_f2fs_dir_entry)?
...
> +enum
> + {
> + FI_INLINE_XATTR = 9,
> + FI_INLINE_DATA = 10,
> + FI_INLINE_DENTRY = 11,
> + FI_DATA_EXIST = 18,
> + };
> +
This does not match subsequent usage; you use them with i_inline, not
i_flags.
...
> +
> +static inline int
> +grub_generic_test_bit (int nr, const grub_uint32_t *addr)
> +{
> + return 1UL & (addr[nr / 32] >> (nr & 31));
> +}
> +
This is used only in grub_f2fs_check_dentries() with on-disk bitmap.
On-disk bitmap is little-endian; code is wrong on big-endian system.
Also dentry_bitmap is not multiple of 4 bytes as you replied earlier.
You should rather compute correct byte address instead and make all
parameters grub_uint8_t *. This will also avoid all those casts later.
That's really just
grub_uint8_t *addr;
byte = nr >> 3;
#ifdef WORDS_BIGENDIAN
byte ^= 3;
#endif
return addr[byte] & (1 << nr & 7);
...
> +
> +static inline int
> +__inode_inline_set (struct grub_f2fs_inode *inode, int flag)
> +{
> + return inode->i_inline & flag;
> +}
> +
Function is completely redundant if you really want to check i_inline;
just use it directly. Then definition of flags is wrong.
If you mean inode->i_flags (as implied by passed flag values) this
should obviously be (1 << flag) as adjusted by system byte order.
Out of curiosity - it appears those flags are present in both i_inline
and i_flags; which one is authoritative?
> +static inline grub_uint32_t
> +__nat_bitmap_size (struct grub_f2fs_data *data)
> +{
> + struct grub_f2fs_checkpoint *ckpt = &data->ckpt;
> +
> + return grub_le_to_cpu32 (ckpt->nat_ver_bitmap_bytesize);
> +}
This function is not used anywhere.
...
> +
> +static inline grub_uint32_t
> +__get_node_id (struct grub_f2fs_node *rn, int off, int i)
> +{
> + if (i)
Judging by usage something like "first" would probably be more
appropriate.
> + return grub_le_to_cpu32 (rn->i.i_nid[off - NODE_DIR1_BLOCK]);
> + return grub_le_to_cpu32 (rn->in.nid[off]);
> +}
> +
> +static inline grub_err_t
> +grub_f2fs_block_read (struct grub_f2fs_data *data, grub_uint32_t blkaddr,
> void *buf)
> +{
> + return grub_disk_read (data->disk, blkaddr << F2FS_BLK_SEC_BITS,
I suspect coverity will complain about overflow here; cast of blkaddr
to grub_disk_addr_t should silence it.
> + 0, F2FS_BLKSIZE, buf);
> +}
> +
...
> +
> +static grub_err_t
Not sure if it is needed here; see comment below at caller.
> +grub_f2fs_read_sb (struct grub_f2fs_data *data, grub_uint64_t offset)
> +{
> + grub_disk_t disk = data->disk;
> + grub_err_t err;
> +
> + /* Read first super block. */
> + err = grub_disk_read (disk, offset >> GRUB_DISK_SECTOR_BITS, 0,
Make parameter grub_disk_addr_t and compute it in caller. It is
constant, there is no need to compute it at run time.
> + 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_le_to_cpu32 (*(grub_uint32_t *)((char *)cp_block + crc_offset));
I understand that it /should/ be hardcoded to 4092, but then please
either check that crc_offset *is* 4092 before or use
grub_get_unaligned. Otherwise it crashes on archs that do not support
unaligned access.
> + if (!grub_f2fs_crc_valid (crc, cp_block, crc_offset))
> + goto invalid_cp1;
> +
> + 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_le_to_cpu32 (*(grub_uint32_t *)((char *)cp_block + crc_offset));
Ditto.
> + if (!grub_f2fs_crc_valid (crc, cp_block, crc_offset))
> + 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");
Trailing "\n" is not needed.
> +
> + grub_memcpy (&data->ckpt, cur_page, F2FS_BLKSIZE);
> +
> + grub_free (cp1);
> + grub_free (cp2);
> + return 0;
> +}
> +
...
> +
> +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));
Is it needed to be zalloc? Structure is large and it runs every time
file is accessed. Most of it is immediately overwritten, may be
explicitly initialize what remains?
> + if (!data)
> + return NULL;
> +
> + data->disk = disk;
> +
> + err = grub_f2fs_read_sb (data, F2FS_SUPER_OFFSET);
> + if (err)
> + {
> + err = grub_f2fs_read_sb (data, F2FS_BLKSIZE + F2FS_SUPER_OFFSET);
As mentioned just compute disk address here, it is static.
> + if (err)
> + {
> + grub_error (GRUB_ERR_BAD_FS, "not a F2FS filesystem (no
> superblock)");
> + goto fail;
> + }
> + }
You should check for err != GRUB_ERR_BAD_FS here, otherwise error from
grub_disk_read is lost. Alternatively just return 0/1 from read_sb, so
that
if (grub_f2fs_read_sb)
if (grub_f2fs_read_sb)
if (grub_errno == GRUB_ERR_NONE)
grub_error (GRUB_ERR_BAD_FS, ...)
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;
> +
> + data->nat_bitmap = __nat_bitmap_ptr (data);
> +
> + err = get_nat_journal (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:
> + grub_free (data);
> + return NULL;
> +}
> +
...
> +
> +static grub_ssize_t
> +grub_f2fs_read_file (grub_fshelp_node_t node,
> + grub_disk_read_hook_t read_hook, void *read_hook_data,
> + grub_off_t pos, grub_size_t len, char *buf)
> +{
> + struct grub_f2fs_inode *inode = &(node->inode.i);
Why extra parens?
> + grub_off_t filesize = grub_f2fs_file_size (inode);
> + char *inline_addr = __inline_addr (inode);
> +
> + if (__inode_inline_set (&node->inode.i, FI_INLINE_DATA))
Just inode, you already have it.
> + {
> + if (pos > filesize || filesize > MAX_INLINE_DATA)
> + {
> + grub_error (GRUB_ERR_OUT_OF_RANGE,
> + N_("attempt to read past the end of file"));
If filesize > MAX_INLINE_DATA at this point we really deal with
corrupted filesystem so this should be GRUB_ERR_BAD_FS.
> + return -1;
> + }
> + if (pos + len > filesize)
len > filesize - pos is probably more coverity-friendly.
> + len = filesize - pos;
> +
> + grub_memcpy (buf + pos, inline_addr + pos, len);
> + return len;
> + }
> +
> + return grub_fshelp_read_file (node->data->disk, node,
> + read_hook, read_hook_data,
> + pos, len, buf, grub_f2fs_get_block,
> + filesize,
> + F2FS_BLK_SEC_BITS, 0);
> +}
> +
...
> +
> +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;
> + enum grub_fshelp_filetype type = GRUB_FSHELP_UNKNOWN;
> + enum FILE_TYPE ftype;
> + int name_len;
> + int ret;
> +
> + if (grub_generic_test_bit (i, ctx->bitmap) == 0)
> + {
> + i++;
> + continue;
> + }
> +
> + ftype = ctx->dentry[i].file_type;
> + name_len = grub_le_to_cpu16 (ctx->dentry[i].name_len);
> + filename = grub_zalloc (name_len + 1);
It is overwritten on next line, do you really need grub_zalloc only for
the trailing zero?
> + if (!filename)
> + return 0;
> +
> + grub_memcpy (filename, ctx->filename[i], name_len);
> +
> + fdiro = grub_malloc (sizeof (struct grub_fshelp_node));
> + if (!fdiro)
> + {
> + grub_free(filename);
> + 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;
> +
> + ret = ctx->hook (filename, type, fdiro, ctx->hook_data);
> + grub_free(filename);
> + if (ret)
> + return 1;
> +
> + i += (name_len + F2FS_SLOT_LEN - 1) / F2FS_SLOT_LEN;
> + }
> + return 0;
> +}
> +
...
> +
> +static void
> +grub_f2fs_unicode_to_ascii (grub_uint8_t *out_buf, grub_uint16_t *in_buf)
> +{
> + grub_uint16_t *pchTempPtr = in_buf;
> + grub_uint8_t *pwTempPtr = out_buf;
> +
> + while (*pchTempPtr != '\0')
> + {
> + *pwTempPtr = (grub_uint8_t) *pchTempPtr;
This is not byte order safe and it does not convert to ASCII as name
suggests. If you are going to convert to 8 bit encoding why not simply
use grub_utf16_to_utf8?
> + pchTempPtr++;
> + pwTempPtr++;
> + }
> + *pwTempPtr = '\0';
> + return;
> +}
> +
> +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));
> + if (*label)
> + grub_f2fs_unicode_to_ascii ((grub_uint8_t *) (*label),
> + data->sblock.volume_name);
See above.
> + }
> + else
> + *label = NULL;
> +
> + grub_free (data);
> + grub_dl_unref (my_mod);
> + return grub_errno;
> +}
> +
...
_______________________________________________
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel
