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 <j...@suse.cz>
SUSE Labs, CR
_______________________________________________
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel
