From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from list by lists.gnu.org with archive (Exim 4.71) id 1Yob1C-0005L7-0N for mharc-grub-devel@gnu.org; Sat, 02 May 2015 13:15:58 -0400 Received: from eggs.gnu.org ([2001:4830:134:3::10]:44750) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Yob18-0005KO-6i for grub-devel@gnu.org; Sat, 02 May 2015 13:15:56 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Yob14-0006ww-Td for grub-devel@gnu.org; Sat, 02 May 2015 13:15:54 -0400 Received: from mail-lb0-x234.google.com ([2a00:1450:4010:c04::234]:33073) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Yob14-0006wb-FJ for grub-devel@gnu.org; Sat, 02 May 2015 13:15:50 -0400 Received: by lbbzk7 with SMTP id zk7so81788258lbb.0 for ; Sat, 02 May 2015 10:15:49 -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=PajHFP9V/b485z2FA8fybVLZjE/tWQz2XebDImxKctI=; b=A4y+h+wgbRfBVpLkFZ01lthGyWlG4hmlEebfBg8i6DvU8hmoce/xXFHSyaRLWCKuIq DHzWeFbH6IJeQcKzaKirrNOvZLU3mBw+Ul0cf5MfZJNXJ6zhlVoCxvncRGtfjgMRp6ro Gozo0D51Vosr/YM0npT8/BoTm2uzgOlTL/bu2fQ25fEEaskDZsLr+vXhShEahb7DInce W/XW8g+pIm7PCaprMI5j5GHcvcFKf5GdU70ZpiDneTSAGu91BV4ZowQytN2p4UsdzqKb gQu1IwS4+CFAIebSEz5Fd32B1mMFD34o16TGvwU6zbgO6odQocDXgtxLUl6lJBs7eFv2 a1dw== X-Received: by 10.112.144.69 with SMTP id sk5mr12733798lbb.6.1430586949224; Sat, 02 May 2015 10:15:49 -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 4sm2138377lai.36.2015.05.02.10.15.47 (version=TLSv1.2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Sat, 02 May 2015 10:15:48 -0700 (PDT) Date: Sat, 2 May 2015 20:15:45 +0300 From: Andrei Borzenkov To: Jaegeuk Kim Subject: Re: [PATCH v2] F2FS support Message-ID: <20150502201545.1ac3fead@opensuse.site> In-Reply-To: <20150403224908.GB25673@jaegeuk-mac02.mot.com> References: <1427185140-41120-1-git-send-email-jaegeuk@kernel.org> <20150403224908.GB25673@jaegeuk-mac02.mot.com> X-Mailer: Claws Mail 3.11.0 (GTK+ 2.24.27; 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:c04::234 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, 02 May 2015 17:15:56 -0000 Sorry for delay. =D0=92 Fri, 3 Apr 2015 15:49:08 -0700 Jaegeuk Kim =D0=BF=D0=B8=D1=88=D0=B5=D1=82: >=20 > This patch changes: >=20 > * 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 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 =3D 9, > + FI_INLINE_DATA =3D 10, > + FI_INLINE_DENTRY =3D 11, > + FI_DATA_EXIST =3D 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 =3D nr >> 3; #ifdef WORDS_BIGENDIAN byte ^=3D 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 =3D &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 =3D data->disk; > + grub_err_t err; > + > + /* Read first super block. */ > + err =3D 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 =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_le_to_cpu32 (*(grub_uint32_t *)((char *)cp_block + crc_of= fset)); 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 =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_le_to_cpu32 (*(grub_uint32_t *)((char *)cp_block + crc_of= fset)); Ditto. > + if (!grub_f2fs_crc_valid (crc, cp_block, crc_offset)) > + 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"); 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 =3D 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 =3D disk; > + > + err =3D grub_f2fs_read_sb (data, F2FS_SUPER_OFFSET); > + if (err) > + { > + err =3D 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 superb= lock)"); > + goto fail; > + } > + } You should check for err !=3D 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 =3D=3D GRUB_ERR_NONE) grub_error (GRUB_ERR_BAD_FS, ...) 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; > + > + data->nat_bitmap =3D __nat_bitmap_ptr (data); > + > + err =3D get_nat_journal (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: > + 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 =3D &(node->inode.i); Why extra parens? > + grub_off_t filesize =3D grub_f2fs_file_size (inode); > + char *inline_addr =3D __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 =3D 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 =3D 0; i < ctx->max;) > + { > + char *filename; > + enum grub_fshelp_filetype type =3D GRUB_FSHELP_UNKNOWN; > + enum FILE_TYPE ftype; > + int name_len; > + int ret; > + > + if (grub_generic_test_bit (i, ctx->bitmap) =3D=3D 0) > + { > + i++; > + continue; > + } > + > + ftype =3D ctx->dentry[i].file_type; > + name_len =3D grub_le_to_cpu16 (ctx->dentry[i].name_len); > + filename =3D 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 =3D grub_malloc (sizeof (struct grub_fshelp_node)); > + if (!fdiro) > + { > + grub_free(filename); > + 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; > + > + ret =3D ctx->hook (filename, type, fdiro, ctx->hook_data); > + grub_free(filename); > + if (ret) > + return 1; > + > + i +=3D (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 =3D in_buf; > + grub_uint8_t *pwTempPtr =3D out_buf; > + > + while (*pchTempPtr !=3D '\0') > + { > + *pwTempPtr =3D (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 =3D '\0'; > + return; > +} > + > +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)); > + if (*label) > + grub_f2fs_unicode_to_ascii ((grub_uint8_t *) (*label), > + data->sblock.volume_name); See above. > + } > + else > + *label =3D NULL; > + > + grub_free (data); > + grub_dl_unref (my_mod); > + return grub_errno; > +} > + ...