From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from list by lists.gnu.org with archive (Exim 4.71) id 1a8OUn-0007QV-Oh for mharc-grub-devel@gnu.org; Mon, 14 Dec 2015 03:28:37 -0500 Received: from eggs.gnu.org ([2001:4830:134:3::10]:51872) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1a8OUl-0007QM-0r for grub-devel@gnu.org; Mon, 14 Dec 2015 03:28:36 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1a8OUf-0007LI-W8 for grub-devel@gnu.org; Mon, 14 Dec 2015 03:28:34 -0500 Received: from mail-lf0-x234.google.com ([2a00:1450:4010:c07::234]:35177) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1a8OUf-0007L2-Kx for grub-devel@gnu.org; Mon, 14 Dec 2015 03:28:29 -0500 Received: by lfdl133 with SMTP id l133so113692214lfd.2 for ; Mon, 14 Dec 2015 00:28:28 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20120113; h=subject:to:references:from:message-id:date:user-agent:mime-version :in-reply-to:content-type:content-transfer-encoding; bh=hfF9EiPAP9V1ixHQssgtOHGr2DZ6gAR9a0V9CENx/Ic=; b=E/1Pl8BwIxBtWgvwKCLWAtlGRNqpqFRyOKM9j6KcGrG2eCf4yHHTAb3GYBLbcOtCYo VWg9SwiLJ/h0scDVJcxBuqsaxjDPjbnVscbYo1qz+twePQgOO1MPsSg9MNvCxgp5luBO xEk+v9/9o+YP5RZ8MoK6POBhEb/Rea5rJIWEV5B1TsGttPP2Hk02RhzVu5R0sA8UmUrq EjPnIp3CocMGPPmLKQxdUCu9RrXl1uP0kbR/QX3tQwALcO4ib7Ub2QPOHjt7sNpTgBkp /c2p9fnLG8MbC2TVE+OjczMIil0Yd5SlP/W+2nMwIK8WJPsFPz5rM7I40I1GRt2lSH66 K0KQ== X-Received: by 10.25.153.146 with SMTP id b140mr12521006lfe.33.1450081708596; Mon, 14 Dec 2015 00:28:28 -0800 (PST) Received: from [192.168.1.41] (ppp91-76-25-247.pppoe.mtu-net.ru. [91.76.25.247]) by smtp.gmail.com with ESMTPSA id ui5sm2314206lbc.12.2015.12.14.00.28.27 (version=TLSv1/SSLv3 cipher=OTHER); Mon, 14 Dec 2015 00:28:27 -0800 (PST) Subject: Re: [PATCH v3] F2FS support To: The development of GNU GRUB , linux-f2fs-devel@lists.sourceforge.net References: <1427185140-41120-1-git-send-email-jaegeuk@kernel.org> <20150403224908.GB25673@jaegeuk-mac02.mot.com> <20151119212824.GA11666@jaegeuk.local> From: Andrei Borzenkov X-Enigmail-Draft-Status: N1110 Message-ID: <566E7DAA.4010202@gmail.com> Date: Mon, 14 Dec 2015 11:28:26 +0300 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.4.0 MIME-Version: 1.0 In-Reply-To: <20151119212824.GA11666@jaegeuk.local> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8bit X-detected-operating-system: by eggs.gnu.org: GNU/Linux 2.2.x-3.x [generic] X-Received-From: 2a00:1450:4010:c07::234 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: Mon, 14 Dec 2015 08:28:36 -0000 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. > +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) ... > +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". > +} > + > +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. ... > + > +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. ... > + > +/* 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.