Thank you for the review.
On Mon, Dec 14, 2015 at 11:28:26AM +0300, Andrei Borzenkov wrote:
> 20.11.2015 00:28, Jaegeuk Kim пишет:
> > Hello,
> >
> > Change log from v2:
> > o Enhance the code quality suggested by Andrei
> >
> > Sorry for the long delay.
> > Could you please check this patch?
> >
> > Thank you so much,
> >
>
> Thank you for continuing to work on it!
>
> ...
>
> > +
> > +static inline int
> > +grub_generic_test_bit (int nr, const grub_uint32_t *addr)
> > +{
> > + return 1UL & (addr[nr / 32] >> (nr & 31));
> > +}
> > +
>
> As already discussed this code is wrong on big-endian platform. On-disk
> bitmap is little-endian and kernel explicitly uses test_bit_le() here.
Got it.
>
> > +static inline char *
> > +__inline_addr (struct grub_f2fs_inode *inode)
> > +{
> > + return (char *)&inode->i_addr[1];
> > +}
> > +
> > +static inline grub_uint64_t
> > +grub_f2fs_file_size (struct grub_f2fs_inode *inode)
> > +{
> > + 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))
>
> This can use grub_cpu_to_le64_compile_time (1)
Done.
>
> ...
>
> > +static inline int
> > +grub_f2fs_test_bit (grub_uint32_t nr, const char *p)
> > +{
> > + int mask;
> > +
> > + p += (nr >> 3);
> > + mask = 1 << (7 - (nr & 0x07));
> > + return (mask & *p) != 0;
>
> This is really just "return mask & *p".
Okay.
>
> > +}
> > +
> > +static int
> > +grub_f2fs_sanity_check_sb (struct grub_f2fs_superblock *sb)
> > +{
> > + grub_uint32_t log_sectorsize, log_sectors_per_block;
> > +
> > + if (sb->magic != grub_cpu_to_le32_compile_time (F2FS_SUPER_MAGIC))
> > + return -1;
> > +
> > + if (sb->log_blocksize != grub_cpu_to_le32_compile_time (F2FS_BLK_BITS))
> > + return -1;
> > +
> > + log_sectorsize = grub_le_to_cpu32 (sb->log_sectorsize);
> > + log_sectors_per_block = grub_le_to_cpu32 (sb->log_sectors_per_block);
> > +
> > + if (log_sectorsize > F2FS_MAX_LOG_SECTOR_SIZE)
> > + return -1;
> > +
> > + if (log_sectorsize < F2FS_MIN_LOG_SECTOR_SIZE)
> > + return -1;
> > +
> > + if (log_sectors_per_block + log_sectorsize != F2FS_MAX_LOG_SECTOR_SIZE)
>
> This sounds like it should actually be F2FS_BLK_BITS; at least assuming
> that F2FS_MAX_LOG_SECTOR_SIZE may differ from F2FS_BLK_BITS.
>
Yup. In order to avoid confusion, I removed F2FS_MAX_LOG_SECTOR_SIZE.
> ...
>
> > +
> > +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;
> > + grub_off_t filesize = grub_f2fs_file_size (inode);
> > + char *inline_addr = __inline_addr (inode);
> > +
> > + if (inode->i_inline & F2FS_INLINE_DATA)
> > + {
> > + if (pos > filesize || filesize > MAX_INLINE_DATA)
> > + {
> > + grub_error (GRUB_ERR_BAD_FS, "corrupted inline_data: need fsck");
>
>
> Sorry for confusion, my fault. pos > filesize was OK, but filesize >
> MAX_INLINE_DATA not.
If filesize is over MAX_INLINE_DATA, this file is corrupted as well.
It will cause boundary violation during memcpy below.
>
> ...
>
> > +
> > +/* TODO: mkfs.f2fs stores label in a wrong way. Should be fixed. */
> > +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;
> > + pchTempPtr++;
> > + pwTempPtr++;
> > + }
> > + *pwTempPtr = '\0';
> > + return;
> > +}
>
> Sorry, I do not see how it can work on both big and little endian
> platforms. What byte order is used for on-disk label? Why cannot you use
> grub_utf16_to_utf8 as I asked last time?
>
> Also please add bundary check, do not rely on correct content.
Got it. I refactored this here and in f2fs-tools as well.
Could you check v4?
Thanks,
>
>
>
> ------------------------------------------------------------------------------
> _______________________________________________
> Linux-f2fs-devel mailing list
> linux-f2fs-de...@lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
_______________________________________________
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel
