From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from list by lists.gnu.org with archive (Exim 4.71) id 1YsAXN-00045V-Tl for mharc-grub-devel@gnu.org; Tue, 12 May 2015 09:47:57 -0400 Received: from eggs.gnu.org ([2001:4830:134:3::10]:50299) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YsAXJ-0003yr-7W for grub-devel@gnu.org; Tue, 12 May 2015 09:47:54 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1YsAXC-0005yA-Ry for grub-devel@gnu.org; Tue, 12 May 2015 09:47:53 -0400 Received: from cantor2.suse.de ([195.135.220.15]:43538 helo=mx2.suse.de) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YsAXC-0005sj-JI for grub-devel@gnu.org; Tue, 12 May 2015 09:47:46 -0400 X-Virus-Scanned: by amavisd-new at test-mx.suse.de Received: from relay1.suse.de (charybdis-ext.suse.de [195.135.220.254]) by mx2.suse.de (Postfix) with ESMTP id 648EAAD5C; Tue, 12 May 2015 13:47:44 +0000 (UTC) Received: by quack.suse.cz (Postfix, from userid 1000) id A7C228281E; Tue, 12 May 2015 15:47:40 +0200 (CEST) Date: Tue, 12 May 2015 15:47:40 +0200 From: Jan Kara To: The development of GNU GRUB Subject: Re: [PATCH 4/4] xfs: V5 filesystem format support Message-ID: <20150512134740.GA20095@quack.suse.cz> References: <1405351291-24767-1-git-send-email-jack@suse.cz> <1405351291-24767-5-git-send-email-jack@suse.cz> <20150512082340.78aa6ee5@opensuse.site> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20150512082340.78aa6ee5@opensuse.site> User-Agent: Mutt/1.5.21 (2010-09-15) X-detected-operating-system: by eggs.gnu.org: GNU/Linux 2.2.x-3.x (no timestamps) [generic] X-Received-From: 195.135.220.15 Cc: Andrei Borzenkov , Jan Kara 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: Tue, 12 May 2015 13:47:55 -0000 On Tue 12-05-15 08:23:40, Andrei Borzenkov wrote: ... > > @@ -123,17 +144,6 @@ struct grub_xfs_inode > > grub_uint16_t unused3; > > grub_uint8_t fork_offset; > > grub_uint8_t unused4[17]; > > - union > > - { > > - char raw[156]; > > - struct dir > > - { > > - struct grub_xfs_dir_header dirhead; > > - struct grub_xfs_dir_entry direntry[1]; > > - } dir; > > - grub_xfs_extent extents[XFS_INODE_EXTENTS]; > > - struct grub_xfs_btree_root btree; > > - } GRUB_PACKED data; > > Can we make it grub_uint8_t xfs_v3_extra[76] or similar to avoid magic > numbers later when computing data offset? Well, depending on fs version the size of "base" inode is different. I have added defines like: #define XFS_V2_INODE_SIZE sizeof(struct grub_xfs_inode) #define XFS_V3_INODE_SIZE (XFS_V2_INODE_SIZE + 76) That should make things clearer. > > } GRUB_PACKED; > > > > struct grub_xfs_dirblock_tail > > @@ -157,6 +167,8 @@ struct grub_xfs_data > > int pos; > > int bsize; > > grub_uint32_t agsize; > > + unsigned int hasftype:1; > > + unsigned int hascrc:1; > > struct grub_fshelp_node diropen; > > }; > > > > @@ -164,6 +176,24 @@ static grub_dl_t my_mod; > > > > > > > > +static int grub_xfs_sb_hascrc(struct grub_xfs_data *data) > > +{ > > + return (grub_be_to_cpu16(data->sblock.version) & XFS_SB_VERSION_NUMBITS) == 5; > > Could you define name for magic constant? Done. > Also to avoid run-time computation: > data->sblock.version & > grub_cpu_to_b16_compile_time(XFS_SB_VERSION_NUMBITS) == > grub_cpu_to_b16_compile_time(5) OK. ... > > @@ -261,6 +279,96 @@ grub_xfs_inode_size(struct grub_xfs_data *data) > > return 1 << data->sblock.log2_inode; > > } > > > > +static void * > > +grub_xfs_inode_data(struct grub_xfs_inode *inode) > > +{ > > + if (inode->version <= 2) > > + return ((char *)inode) + 100; > > That is sizeof(struct grub_xfs_inode) in your patch, right? > > > + return ((char *)inode) + 176; > > Please avoid magic numbers. At least give it meaningful name, or as > suggested above just define structure to contain it. Used defines above. > > +} > > + > > +static struct grub_xfs_dir_entry * > > +grub_xfs_inline_de(struct grub_xfs_dir_header *head) > > +{ > > + /* > > + * With small inode numbers the header is 4 bytes smaller because of > > + * smaller parent pointer > > + */ > > + return (void *)(((char *)head) + sizeof(struct grub_xfs_dir_header) - > > + (head->largeino ? 0 : sizeof(grub_uint32_t))); > > +} > > + > > +static grub_uint8_t * > > +grub_xfs_inline_de_inopos(struct grub_xfs_data *data, > > + struct grub_xfs_dir_entry *de) > > +{ > > + return ((grub_uint8_t *)(de + 1)) + de->len - 1 + > The outer parens are somehow confusing. I can remove them but I find it better to explicitely show where the typecast happens with parenthesis... > > > + (data->hasftype ? 1 : 0); > > +} > > + > > +static struct grub_xfs_dir_entry * > > +grub_xfs_inline_next_de(struct grub_xfs_data *data, > > + struct grub_xfs_dir_header *head, > > + struct grub_xfs_dir_entry *de) > > +{ > > + char *p = (char *)de + sizeof(struct grub_xfs_dir_entry) - 1 + de->len; > > + > > + p += head->largeino ? sizeof(grub_uint64_t) : sizeof(grub_uint32_t); > > + if (data->hasftype) > > + p++; > > + > > + return (struct grub_xfs_dir_entry *)p; > > +} > > + > > +static struct grub_xfs_dirblock_tail * > > +grub_xfs_dir_tail(struct grub_xfs_data *data, void *dirblock) > > +{ > > + int dirblksize = 1 << (data->sblock.log2_bsize + data->sblock.log2_dirblk); > > + > > + return (struct grub_xfs_dirblock_tail *) > > + ((char *)dirblock + dirblksize - sizeof (struct grub_xfs_dirblock_tail)); > > +} > > + > > +static struct grub_xfs_dir2_entry * > > +grub_xfs_first_de(struct grub_xfs_data *data, void *dirblock) > > +{ > > + if (data->hascrc) > > + return (struct grub_xfs_dir2_entry *)((char *)dirblock + 64); > > + return (struct grub_xfs_dir2_entry *)((char *)dirblock + 16); > > +} > > + > > +static inline int > > +grub_xfs_round_dirent_size (int len) > > +{ > > + return (len + 7) & ~7; > > +} > > ALIGN_UP? > > > + > > +static struct grub_xfs_dir2_entry * > > +grub_xfs_next_de(struct grub_xfs_data *data, struct grub_xfs_dir2_entry *de) > > +{ > > + int size = sizeof (struct grub_xfs_dir2_entry) + de->len + 2 /* Tag */; > > + > > + if (data->hasftype) > > + size++; /* File type */ > > + return (struct grub_xfs_dir2_entry *) > > + (((char *)de) + grub_xfs_round_dirent_size (size)); > > What's wrong with ALIGN_UP (size, 8)? Nothing. I wasn't aware grub has the macro. Replaced. > > +} > > + > > +static grub_uint64_t * > > +grub_xfs_btree_keys(struct grub_xfs_data *data, > > + struct grub_xfs_btree_node *leaf) > > +{ > > + char *p = (char *)(leaf + 1); > > + > > + if (data->hascrc) > > + p += 48; /* crc, uuid, ... */ > > + /* > > + * We have to first type to void * to avoid complaints about possible > > + * alignment issues on some architectures > > + */ > > + return (grub_uint64_t *)(void *)p; > > Leaving it as grub_uint64_t keys and using &keys[6] would avoid this > warning as well, not? Also having keys[0] will likely simplify other > places as well (we do require GCC in any case). Well, the trouble with this is that we'd need two structures defined - one for crc-enabled fs and one for old fs. That seemed like a wasted effort to me when we could do: if (data->hascrc) p += 48; /* crc, uuid, ... */ like the above. The same holds for inodes, directory entries, etc. I'd prefer not to bloat the code with structure definitions we don't actually use but if you really insisted, I could do that. So what do you think? Honza -- Jan Kara SUSE Labs, CR