[PATCH 0/4] Support for XFS v5 superblock

[PATCH 0/4] Support for XFS v5 superblock

[Jan Kara]

  Hello,

  XFS has recently added some on disk format changes which break booting
from XFS partition using grub (grub is unable to read the filesystem).
This patch set implements support for the new XFS on disk format.

                                                                Honza

[PATCH 1/4] xfs: Add helper for inode size

[Jan Kara]

Signed-off-by: Jan Kara <jack@suse.cz>
---
 grub-core/fs/xfs.c | 17 +++++++++++------
 1 file changed, 11 insertions(+), 6 deletions(-)

diff --git a/grub-core/fs/xfs.c b/grub-core/fs/xfs.c
index 16ffd3f1ebd9..a2fc942707c1 100644
--- a/grub-core/fs/xfs.c
+++ b/grub-core/fs/xfs.c
@@ -255,6 +255,11 @@ grub_xfs_inode_offset (struct grub_xfs_data *data,
 	  data->sblock.log2_inode);
 }
 
+static inline int
+grub_xfs_inode_size(struct grub_xfs_data *data)
+{
+	return 1 << data->sblock.log2_inode;
+}
 
 static grub_err_t
 grub_xfs_read_inode (struct grub_xfs_data *data, grub_uint64_t ino,
@@ -264,8 +269,8 @@ grub_xfs_read_inode (struct grub_xfs_data *data, grub_uint64_t ino,
   int offset = grub_xfs_inode_offset (data, ino);
 
   /* Read the inode.  */
-  if (grub_disk_read (data->disk, block, offset,
-		      1 << data->sblock.log2_inode, inode))
+  if (grub_disk_read (data->disk, block, offset, grub_xfs_inode_size(data),
+		      inode))
     return grub_errno;
 
   if (grub_strncmp ((char *) inode->magic, "IN", 2))
@@ -297,7 +302,7 @@ grub_xfs_read_block (grub_fshelp_node_t node, grub_disk_addr_t fileblock)
       if (node->inode.fork_offset)
 	recoffset = (node->inode.fork_offset - 1) / 2;
       else
-	recoffset = ((1 << node->data->sblock.log2_inode)
+	recoffset = (grub_xfs_inode_size(node->data)
 		     - ((char *) &node->inode.data.btree.keys
 			- (char *) &node->inode))
 	  / (2 * sizeof (grub_uint64_t));
@@ -458,7 +463,7 @@ static int iterate_dir_call_hook (grub_uint64_t ino, const char *filename,
 
   fdiro = grub_malloc (sizeof (struct grub_fshelp_node)
 		       - sizeof (struct grub_xfs_inode)
-		       + (1 << ctx->diro->data->sblock.log2_inode) + 1);
+		       + grub_xfs_inode_size(ctx->diro->data) + 1);
   if (!fdiro)
     {
       grub_print_error ();
@@ -684,7 +689,7 @@ grub_xfs_mount (grub_disk_t disk)
   data = grub_realloc (data,
 		       sizeof (struct grub_xfs_data)
 		       - sizeof (struct grub_xfs_inode)
-		       + (1 << data->sblock.log2_inode) + 1);
+		       + grub_xfs_inode_size(data) + 1);
 
   if (! data)
     goto fail;
@@ -802,7 +807,7 @@ grub_xfs_open (struct grub_file *file, const char *name)
       grub_memcpy (&data->diropen, fdiro,
 		   sizeof (struct grub_fshelp_node)
 		   - sizeof (struct grub_xfs_inode)
-		   + (1 << data->sblock.log2_inode));
+		   + grub_xfs_inode_size(data));
       grub_free (fdiro);
     }
 
-- 
1.8.1.4

[PATCH 2/4] xfs: Fix termination loop for directory iteration

[Jan Kara]

Directory iteration used wrong position (sizeof wrong structure) for
termination of iteration inside a directory block. Luckily the position
ended up being wrong by just 1 byte and directory entries are larger so
things worked out fine in practice. But fix the problem anyway.

Signed-off-by: Jan Kara <jack@suse.cz>
---
 grub-core/fs/xfs.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/grub-core/fs/xfs.c b/grub-core/fs/xfs.c
index a2fc942707c1..ef3bc787e968 100644
--- a/grub-core/fs/xfs.c
+++ b/grub-core/fs/xfs.c
@@ -608,8 +608,7 @@ grub_xfs_iterate_dir (grub_fshelp_node_t dir,
 		       - grub_be_to_cpu32 (tail->leaf_stale));
 
 	    /* Iterate over all entries within this block.  */
-	    while (pos < (dirblk_size
-			  - (int) sizeof (struct grub_xfs_dir2_entry)))
+	    while (pos < tail_start)
 	      {
 		struct grub_xfs_dir2_entry *direntry;
 		grub_uint8_t *freetag;
-- 
1.8.1.4

[PATCH 3/4] xfs: Convert inode numbers to cpu endianity immediately after reading

[Jan Kara]

Currently XFS driver converted inode numbers to native endianity only
when using them to compute inode position. Although this works, it is
somewhat confusing. So convert inode numbers when reading them from disk
structures as every other field.

Signed-off-by: Jan Kara <jack@suse.cz>
---
 grub-core/fs/xfs.c | 13 ++++++-------
 1 file changed, 6 insertions(+), 7 deletions(-)

diff --git a/grub-core/fs/xfs.c b/grub-core/fs/xfs.c
index ef3bc787e968..7e247a32df5c 100644
--- a/grub-core/fs/xfs.c
+++ b/grub-core/fs/xfs.c
@@ -180,14 +180,14 @@ static inline grub_uint64_t
 GRUB_XFS_INO_INOINAG (struct grub_xfs_data *data,
 		      grub_uint64_t ino)
 {
-  return (grub_be_to_cpu64 (ino) & ((1LL << GRUB_XFS_INO_AGBITS (data)) - 1));
+  return (ino & ((1LL << GRUB_XFS_INO_AGBITS (data)) - 1));
 }
 
 static inline grub_uint64_t
 GRUB_XFS_INO_AG (struct grub_xfs_data *data,
 		 grub_uint64_t ino)
 {
-  return (grub_be_to_cpu64 (ino) >> GRUB_XFS_INO_AGBITS (data));
+  return (ino >> GRUB_XFS_INO_AGBITS (data));
 }
 
 static inline grub_disk_addr_t
@@ -511,13 +511,12 @@ grub_xfs_iterate_dir (grub_fshelp_node_t dir,
 	if (smallino)
 	  {
 	    parent = grub_be_to_cpu32 (diro->inode.data.dir.dirhead.parent.i4);
-	    parent = grub_cpu_to_be64 (parent);
 	    /* The header is a bit smaller than usual.  */
 	    de = (struct grub_xfs_dir_entry *) ((char *) de - 4);
 	  }
 	else
 	  {
-	    parent = diro->inode.data.dir.dirhead.parent.i8;
+	    parent = grub_be_to_cpu64(diro->inode.data.dir.dirhead.parent.i8);
 	  }
 
 	/* Synthesize the direntries for `.' and `..'.  */
@@ -550,7 +549,6 @@ grub_xfs_iterate_dir (grub_fshelp_node_t dir,
 		| (((grub_uint64_t) inopos[5]) << 16)
 		| (((grub_uint64_t) inopos[6]) << 8)
 		| (((grub_uint64_t) inopos[7]) << 0);
-	    ino = grub_cpu_to_be64 (ino);
 
 	    c = de->name[de->len];
 	    de->name[de->len] = '\0';
@@ -632,7 +630,8 @@ grub_xfs_iterate_dir (grub_fshelp_node_t dir,
 		   is not used by GRUB.  So it can be overwritten.  */
 		filename[direntry->len] = '\0';
 
-		if (iterate_dir_call_hook (direntry->inode, filename, &ctx))
+		if (iterate_dir_call_hook (grub_be_to_cpu64(direntry->inode), 
+					   filename, &ctx))
 		  {
 		    grub_free (dirblock);
 		    return 1;
@@ -694,7 +693,7 @@ grub_xfs_mount (grub_disk_t disk)
     goto fail;
 
   data->diropen.data = data;
-  data->diropen.ino = data->sblock.rootino;
+  data->diropen.ino = grub_be_to_cpu64(data->sblock.rootino);
   data->diropen.inode_read = 1;
   data->bsize = grub_be_to_cpu32 (data->sblock.bsize);
   data->agsize = grub_be_to_cpu32 (data->sblock.agsize);
-- 
1.8.1.4

[PATCH 4/4] xfs: V5 filesystem format support

[Jan Kara]

Add support for new XFS on disk format. We have to handle optional
filetype fields in directory entries, additional CRC, LSN, UUID entries
in some structures, etc.

Signed-off-by: Jan Kara <jack@suse.cz>
---
 grub-core/fs/xfs.c | 249 +++++++++++++++++++++++++++++++++++++----------------
 1 file changed, 177 insertions(+), 72 deletions(-)

diff --git a/grub-core/fs/xfs.c b/grub-core/fs/xfs.c
index 7e247a32df5c..7f2582d33e02 100644
--- a/grub-core/fs/xfs.c
+++ b/grub-core/fs/xfs.c
@@ -34,6 +34,15 @@ GRUB_MOD_LICENSE ("GPLv3+");
 #define XFS_INODE_FORMAT_EXT	2
 #define XFS_INODE_FORMAT_BTREE	3
 
+/* Superblock version field flags */
+#define XFS_SB_VERSION_NUMBITS          0x000f
+#define XFS_SB_VERSION_MOREBITSBIT      0x8000
+
+/* features2 field flags */
+#define XFS_SB_VERSION2_FTYPE           0x00000200      /* inode type in dir */
+
+/* incompat feature flags */
+#define XFS_SB_FEAT_INCOMPAT_FTYPE      (1 << 0)        /* filetype in dirent */
 
 struct grub_xfs_sblock
 {
@@ -45,7 +54,9 @@ struct grub_xfs_sblock
   grub_uint64_t rootino;
   grub_uint8_t unused3[20];
   grub_uint32_t agsize;
-  grub_uint8_t unused4[20];
+  grub_uint8_t unused4[12];
+  grub_uint16_t version;
+  grub_uint8_t unused5[6];
   grub_uint8_t label[12];
   grub_uint8_t log2_bsize;
   grub_uint8_t log2_sect;
@@ -54,12 +65,19 @@ struct grub_xfs_sblock
   grub_uint8_t log2_agblk;
   grub_uint8_t unused6[67];
   grub_uint8_t log2_dirblk;
+  grub_uint8_t unused7[7];
+  grub_uint32_t features2;
+  grub_uint8_t unused8[4];
+  grub_uint32_t sb_features_compat;
+  grub_uint32_t sb_features_ro_compat;
+  grub_uint32_t sb_features_incompat;
+  grub_uint32_t sb_features_log_incompat;
 } GRUB_PACKED;
 
 struct grub_xfs_dir_header
 {
   grub_uint8_t count;
-  grub_uint8_t smallino;
+  grub_uint8_t largeino;
   union
   {
     grub_uint32_t i4;
@@ -67,14 +85,16 @@ struct grub_xfs_dir_header
   } GRUB_PACKED parent;
 } GRUB_PACKED;
 
+/* Structure for directory entry inlined in the inode */
 struct grub_xfs_dir_entry
 {
   grub_uint8_t len;
   grub_uint16_t offset;
   char name[1];
-  /* Inode number follows, 32 bits.  */
+  /* Inode number follows, 32 / 64 bits.  */
 } GRUB_PACKED;
 
+/* Structure for directory entry in a block */
 struct grub_xfs_dir2_entry
 {
   grub_uint64_t inode;
@@ -90,7 +110,8 @@ struct grub_xfs_btree_node
   grub_uint16_t numrecs;
   grub_uint64_t left;
   grub_uint64_t right;
-  grub_uint64_t keys[1];
+  /* In V5 here follow crc, uuid, etc. */
+  /* Then follow keys and block pointers */
 }  GRUB_PACKED;
 
 struct grub_xfs_btree_root
@@ -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;
 } 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;
 
 \f
 
+static int grub_xfs_sb_hascrc(struct grub_xfs_data *data)
+{
+  return (grub_be_to_cpu16(data->sblock.version) & XFS_SB_VERSION_NUMBITS) == 5;
+}
+
+static int grub_xfs_sb_hasftype(struct grub_xfs_data *data)
+{
+  grub_uint32_t version = grub_be_to_cpu16(data->sblock.version);
+
+  if ((version & XFS_SB_VERSION_NUMBITS) == 5 &&
+      grub_be_to_cpu32(data->sblock.sb_features_incompat) & XFS_SB_FEAT_INCOMPAT_FTYPE)
+    return 1;
+  if (version & XFS_SB_VERSION_MOREBITSBIT &&
+      grub_be_to_cpu32(data->sblock.features2) & XFS_SB_VERSION2_FTYPE)
+    return 1;
+  return 0;
+}
+
 /* Filetype information as used in inodes.  */
 #define FILETYPE_INO_MASK	0170000
 #define FILETYPE_INO_REG	0100000
@@ -219,18 +249,6 @@ GRUB_XFS_EXTENT_SIZE (grub_xfs_extent *exts, int ex)
   return (grub_be_to_cpu32 (exts[ex][3]) & ((1 << 21) - 1));
 }
 
-static inline int
-GRUB_XFS_ROUND_TO_DIRENT (int pos)
-{
-  return ((((pos) + 8 - 1) / 8) * 8);
-}
-
-static inline int
-GRUB_XFS_NEXT_DIRENT (int pos, int len)
-{
-  return (pos) + GRUB_XFS_ROUND_TO_DIRENT (8 + 1 + len + 2);
-}
-
 \f
 static inline grub_uint64_t
 grub_xfs_inode_block (struct grub_xfs_data *data,
@@ -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;
+	return ((char *)inode) + 176;
+}
+
+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 +
+		 (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;
+}
+
+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));
+}
+
+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;
+}
+
 static grub_err_t
 grub_xfs_read_inode (struct grub_xfs_data *data, grub_uint64_t ino,
 		     struct grub_xfs_inode *inode)
@@ -268,6 +376,9 @@ grub_xfs_read_inode (struct grub_xfs_data *data, grub_uint64_t ino,
   grub_uint64_t block = grub_xfs_inode_block (data, ino);
   int offset = grub_xfs_inode_offset (data, ino);
 
+  grub_dprintf("xfs", "Reading inode (%llu) - %llu, %d\n",
+	       (unsigned long long) ino,
+	       (unsigned long long) block, offset);
   /* Read the inode.  */
   if (grub_disk_read (data->disk, block, offset, grub_xfs_inode_size(data),
 		      inode))
@@ -290,6 +401,7 @@ grub_xfs_read_block (grub_fshelp_node_t node, grub_disk_addr_t fileblock)
 
   if (node->inode.format == XFS_INODE_FORMAT_BTREE)
     {
+      struct grub_xfs_btree_root *root;
       const grub_uint64_t *keys;
       int recoffset;
 
@@ -297,15 +409,15 @@ grub_xfs_read_block (grub_fshelp_node_t node, grub_disk_addr_t fileblock)
       if (leaf == 0)
         return 0;
 
-      nrec = grub_be_to_cpu16 (node->inode.data.btree.numrecs);
-      keys = &node->inode.data.btree.keys[0];
+      root = grub_xfs_inode_data(&node->inode);
+      nrec = grub_be_to_cpu16 (root->numrecs);
+      keys = &root->keys[0];
       if (node->inode.fork_offset)
 	recoffset = (node->inode.fork_offset - 1) / 2;
       else
 	recoffset = (grub_xfs_inode_size(node->data)
-		     - ((char *) &node->inode.data.btree.keys
-			- (char *) &node->inode))
-	  / (2 * sizeof (grub_uint64_t));
+		     - ((char *) keys - (char *) &node->inode))
+				/ (2 * sizeof (grub_uint64_t));
       do
         {
           int i;
@@ -327,7 +439,10 @@ grub_xfs_read_block (grub_fshelp_node_t node, grub_disk_addr_t fileblock)
                               0, node->data->bsize, leaf))
             return 0;
 
-          if (grub_strncmp ((char *) leaf->magic, "BMAP", 4))
+	  if ((!node->data->hascrc &&
+	       grub_strncmp ((char *) leaf->magic, "BMAP", 4)) ||
+	      (node->data->hascrc &&
+	       grub_strncmp ((char *) leaf->magic, "BMA3", 4)))
             {
               grub_free (leaf);
               grub_error (GRUB_ERR_BAD_FS, "not a correct XFS BMAP node");
@@ -335,8 +450,8 @@ grub_xfs_read_block (grub_fshelp_node_t node, grub_disk_addr_t fileblock)
             }
 
           nrec = grub_be_to_cpu16 (leaf->numrecs);
-          keys = &leaf->keys[0];
-	  recoffset = ((node->data->bsize - ((char *) &leaf->keys
+          keys = grub_xfs_btree_keys(node->data, leaf);
+	  recoffset = ((node->data->bsize - ((char *) keys
 					     - (char *) leaf))
 		       / (2 * sizeof (grub_uint64_t)));
 	}
@@ -346,7 +461,7 @@ grub_xfs_read_block (grub_fshelp_node_t node, grub_disk_addr_t fileblock)
   else if (node->inode.format == XFS_INODE_FORMAT_EXT)
     {
       nrec = grub_be_to_cpu32 (node->inode.nextents);
-      exts = &node->inode.data.extents[0];
+      exts = grub_xfs_inode_data(&node->inode);
     }
   else
     {
@@ -404,7 +519,7 @@ grub_xfs_read_symlink (grub_fshelp_node_t node)
   switch (node->inode.format)
     {
     case XFS_INODE_FORMAT_INO:
-      return grub_strndup (node->inode.data.raw, size);
+      return grub_strndup (grub_xfs_inode_data(&node->inode), size);
 
     case XFS_INODE_FORMAT_EXT:
       {
@@ -501,23 +616,18 @@ grub_xfs_iterate_dir (grub_fshelp_node_t dir,
     {
     case XFS_INODE_FORMAT_INO:
       {
-	struct grub_xfs_dir_entry *de = &diro->inode.data.dir.direntry[0];
-	int smallino = !diro->inode.data.dir.dirhead.smallino;
+	struct grub_xfs_dir_header *head = grub_xfs_inode_data(&diro->inode);
+	struct grub_xfs_dir_entry *de = grub_xfs_inline_de(head);
+	int smallino = !head->largeino;
 	int i;
 	grub_uint64_t parent;
 
 	/* If small inode numbers are used to pack the direntry, the
 	   parent inode number is small too.  */
 	if (smallino)
-	  {
-	    parent = grub_be_to_cpu32 (diro->inode.data.dir.dirhead.parent.i4);
-	    /* The header is a bit smaller than usual.  */
-	    de = (struct grub_xfs_dir_entry *) ((char *) de - 4);
-	  }
+	  parent = grub_be_to_cpu32 (head->parent.i4);
 	else
-	  {
-	    parent = grub_be_to_cpu64(diro->inode.data.dir.dirhead.parent.i8);
-	  }
+	  parent = grub_be_to_cpu64 (head->parent.i8);
 
 	/* Synthesize the direntries for `.' and `..'.  */
 	if (iterate_dir_call_hook (diro->ino, ".", &ctx))
@@ -526,12 +636,10 @@ grub_xfs_iterate_dir (grub_fshelp_node_t dir,
 	if (iterate_dir_call_hook (parent, "..", &ctx))
 	  return 1;
 
-	for (i = 0; i < diro->inode.data.dir.dirhead.count; i++)
+	for (i = 0; i < head->count; i++)
 	  {
 	    grub_uint64_t ino;
-	    grub_uint8_t *inopos = (((grub_uint8_t *) de)
-			    + sizeof (struct grub_xfs_dir_entry)
-			    + de->len - 1);
+	    grub_uint8_t *inopos = grub_xfs_inline_de_inopos(dir->data, de);
 	    grub_uint8_t c;
 
 	    /* inopos might be unaligned.  */
@@ -556,10 +664,7 @@ grub_xfs_iterate_dir (grub_fshelp_node_t dir,
 	      return 1;
 	    de->name[de->len] = c;
 
-	    de = ((struct grub_xfs_dir_entry *)
-		  (((char *) de)+ sizeof (struct grub_xfs_dir_entry) + de->len
-		   + ((smallino ? sizeof (grub_uint32_t)
-		       : sizeof (grub_uint64_t))) - 1));
+	    de = grub_xfs_inline_next_de(dir->data, head, de);
 	  }
 	break;
       }
@@ -586,15 +691,11 @@ grub_xfs_iterate_dir (grub_fshelp_node_t dir,
 		    >> dirblk_log2);
 	     blk++)
 	  {
-	    /* The header is skipped, the first direntry is stored
-	       from byte 16.  */
-	    int pos = 16;
+	    struct grub_xfs_dir2_entry *direntry =
+					grub_xfs_first_de(dir->data, dirblock);
 	    int entries;
-	    int tail_start = (dirblk_size
-			      - sizeof (struct grub_xfs_dirblock_tail));
-
-	    struct grub_xfs_dirblock_tail *tail;
-	    tail = (struct grub_xfs_dirblock_tail *) &dirblock[tail_start];
+	    struct grub_xfs_dirblock_tail *tail =
+					grub_xfs_dir_tail(dir->data, dirblock);
 
 	    numread = grub_xfs_read_file (dir, 0, 0,
 					  blk << dirblk_log2,
@@ -606,13 +707,11 @@ grub_xfs_iterate_dir (grub_fshelp_node_t dir,
 		       - grub_be_to_cpu32 (tail->leaf_stale));
 
 	    /* Iterate over all entries within this block.  */
-	    while (pos < tail_start)
+	    while ((char *)direntry < (char *)tail)
 	      {
-		struct grub_xfs_dir2_entry *direntry;
 		grub_uint8_t *freetag;
 		char *filename;
 
-		direntry = (struct grub_xfs_dir2_entry *) &dirblock[pos];
 		freetag = (grub_uint8_t *) direntry;
 
 		if (grub_get_unaligned16 (freetag) == 0XFFFF)
@@ -620,14 +719,16 @@ grub_xfs_iterate_dir (grub_fshelp_node_t dir,
 		    grub_uint8_t *skip = (freetag + sizeof (grub_uint16_t));
 
 		    /* This entry is not used, go to the next one.  */
-		    pos += grub_be_to_cpu16 (grub_get_unaligned16 (skip));
+		    direntry = (struct grub_xfs_dir2_entry *)
+				(((char *)direntry) +
+				grub_be_to_cpu16 (grub_get_unaligned16 (skip)));
 
 		    continue;
 		  }
 
-		filename = &dirblock[pos + sizeof (*direntry)];
-		/* The byte after the filename is for the tag, which
-		   is not used by GRUB.  So it can be overwritten.  */
+		filename = (char *)(direntry + 1);
+		/* The byte after the filename is for the filetype, padding, or
+		   tag, which is not used by GRUB.  So it can be overwritten. */
 		filename[direntry->len] = '\0';
 
 		if (iterate_dir_call_hook (grub_be_to_cpu64(direntry->inode), 
@@ -644,8 +745,7 @@ grub_xfs_iterate_dir (grub_fshelp_node_t dir,
 		  break;
 
 		/* Select the next directory entry.  */
-		pos = GRUB_XFS_NEXT_DIRENT (pos, direntry->len);
-		pos = GRUB_XFS_ROUND_TO_DIRENT (pos);
+		direntry = grub_xfs_next_de(dir->data, direntry);
 	      }
 	  }
 	grub_free (dirblock);
@@ -670,6 +770,7 @@ grub_xfs_mount (grub_disk_t disk)
   if (!data)
     return 0;
 
+  grub_dprintf("xfs", "Reading sb\n");
   /* Read the superblock.  */
   if (grub_disk_read (disk, 0, 0,
 		      sizeof (struct grub_xfs_sblock), &data->sblock))
@@ -697,9 +798,13 @@ grub_xfs_mount (grub_disk_t disk)
   data->diropen.inode_read = 1;
   data->bsize = grub_be_to_cpu32 (data->sblock.bsize);
   data->agsize = grub_be_to_cpu32 (data->sblock.agsize);
+  data->hasftype = grub_xfs_sb_hasftype(data);
+  data->hascrc = grub_xfs_sb_hascrc(data);
 
   data->disk = disk;
   data->pos = 0;
+  grub_dprintf("xfs", "Reading root ino %llu\n",
+	       (unsigned long long) grub_cpu_to_be64(data->sblock.rootino));
 
   grub_xfs_read_inode (data, data->diropen.ino, &data->diropen.inode);
 
-- 
1.8.1.4

Re: [PATCH 0/4] Support for XFS v5 superblock

[Jan Kara]

On Mon 14-07-14 17:21:27, Jan Kara wrote:
> 
>   Hello,
> 
>   XFS has recently added some on disk format changes which break booting
> from XFS partition using grub (grub is unable to read the filesystem).
> This patch set implements support for the new XFS on disk format.
  Ping? Anyone cares about the patches?

								Honza
-- 
Jan Kara <jack@suse.cz>
SUSE Labs, CR

Re: [PATCH 0/4] Support for XFS v5 superblock

[Vladimir 'φ-coder/phcoder' Serbinenko]

[-- Attachment #1: Type: text/plain, Size: 438 bytes --]

On 21.07.2014 19:31, Jan Kara wrote:
> On Mon 14-07-14 17:21:27, Jan Kara wrote:
>>
>>   Hello,
>>
>>   XFS has recently added some on disk format changes which break booting
>> from XFS partition using grub (grub is unable to read the filesystem).
>> This patch set implements support for the new XFS on disk format.
>   Ping? Anyone cares about the patches?
> 
I'll go over the patches this weekend.
> 								Honza
> 



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 213 bytes --]

Re: [PATCH 0/4] Support for XFS v5 superblock

[Jan Kara]

On Mon 14-07-14 17:21:27, Jan Kara wrote:
> 
>   Hello,
> 
>   XFS has recently added some on disk format changes which break booting
> from XFS partition using grub (grub is unable to read the filesystem).
> This patch set implements support for the new XFS on disk format.
  Guys, could you please have a look at the patches? Or should I just
resend? Without these changes XFS & grub don't work together and people are
already starting to use the new ondisk format...

								Honza
-- 
Jan Kara <jack@suse.cz>
SUSE Labs, CR

Re: [PATCH 2/4] xfs: Fix termination loop for directory iteration

[Andrei Borzenkov]

В Mon, 14 Jul 2014 17:21:29 +0200
Jan Kara <jack@suse.cz> пишет:

> Directory iteration used wrong position (sizeof wrong structure) for
> termination of iteration inside a directory block. Luckily the position
> ended up being wrong by just 1 byte and directory entries are larger so
> things worked out fine in practice. But fix the problem anyway.
> 

Committed.

> Signed-off-by: Jan Kara <jack@suse.cz>
> ---
>  grub-core/fs/xfs.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/grub-core/fs/xfs.c b/grub-core/fs/xfs.c
> index a2fc942707c1..ef3bc787e968 100644
> --- a/grub-core/fs/xfs.c
> +++ b/grub-core/fs/xfs.c
> @@ -608,8 +608,7 @@ grub_xfs_iterate_dir (grub_fshelp_node_t dir,
>  		       - grub_be_to_cpu32 (tail->leaf_stale));
>  
>  	    /* Iterate over all entries within this block.  */
> -	    while (pos < (dirblk_size
> -			  - (int) sizeof (struct grub_xfs_dir2_entry)))
> +	    while (pos < tail_start)
>  	      {
>  		struct grub_xfs_dir2_entry *direntry;
>  		grub_uint8_t *freetag;

Re: [PATCH 1/4] xfs: Add helper for inode size

[Andrei Borzenkov]

В Mon, 14 Jul 2014 17:21:28 +0200
Jan Kara <jack@suse.cz> пишет:

> Signed-off-by: Jan Kara <jack@suse.cz>
> ---
>  grub-core/fs/xfs.c | 17 +++++++++++------
>  1 file changed, 11 insertions(+), 6 deletions(-)
> 
> diff --git a/grub-core/fs/xfs.c b/grub-core/fs/xfs.c
> index 16ffd3f1ebd9..a2fc942707c1 100644
> --- a/grub-core/fs/xfs.c
> +++ b/grub-core/fs/xfs.c
> @@ -255,6 +255,11 @@ grub_xfs_inode_offset (struct grub_xfs_data *data,
>  	  data->sblock.log2_inode);
>  }
>  
> +static inline int
> +grub_xfs_inode_size(struct grub_xfs_data *data)

This should be grub_size_t.

What is the reason to add it? It does not look like subsequent
patches modify this function like making it conditional on XFS version.

Re: [PATCH 1/4] xfs: Add helper for inode size

[Jan Kara]

On Mon 11-05-15 14:53:57, Andrei Borzenkov wrote:
> В Mon, 14 Jul 2014 17:21:28 +0200
> Jan Kara <jack@suse.cz> пишет:
> 
> > Signed-off-by: Jan Kara <jack@suse.cz>
> > ---
> >  grub-core/fs/xfs.c | 17 +++++++++++------
> >  1 file changed, 11 insertions(+), 6 deletions(-)
> > 
> > diff --git a/grub-core/fs/xfs.c b/grub-core/fs/xfs.c
> > index 16ffd3f1ebd9..a2fc942707c1 100644
> > --- a/grub-core/fs/xfs.c
> > +++ b/grub-core/fs/xfs.c
> > @@ -255,6 +255,11 @@ grub_xfs_inode_offset (struct grub_xfs_data *data,
> >  	  data->sblock.log2_inode);
> >  }
> >  
> > +static inline int
> > +grub_xfs_inode_size(struct grub_xfs_data *data)
> 
> This should be grub_size_t.
  OK.

> What is the reason to add it? It does not look like subsequent
> patches modify this function like making it conditional on XFS version.
  This is just a cleanup so that it's clearer that 1 <<
data->sblock.log2_inode is actually inode size. But if you don't like it,
you can just skip it. So should I send update patch? Thanks for having a
look!

								Honza
-- 
Jan Kara <jack@suse.cz>
SUSE Labs, CR

Re: [PATCH 3/4] xfs: Convert inode numbers to cpu endianity immediately after reading

[Andrei Borzenkov]

В Mon, 14 Jul 2014 17:21:30 +0200
Jan Kara <jack@suse.cz> пишет:

> Currently XFS driver converted inode numbers to native endianity only
> when using them to compute inode position. Although this works, it is
> somewhat confusing. So convert inode numbers when reading them from disk
> structures as every other field.
> 

Not sure whether it is better, but committed to avoid risk breaking
followup patch.

> Signed-off-by: Jan Kara <jack@suse.cz>
> ---
>  grub-core/fs/xfs.c | 13 ++++++-------
>  1 file changed, 6 insertions(+), 7 deletions(-)
> 
> diff --git a/grub-core/fs/xfs.c b/grub-core/fs/xfs.c
> index ef3bc787e968..7e247a32df5c 100644
> --- a/grub-core/fs/xfs.c
> +++ b/grub-core/fs/xfs.c
> @@ -180,14 +180,14 @@ static inline grub_uint64_t
>  GRUB_XFS_INO_INOINAG (struct grub_xfs_data *data,
>  		      grub_uint64_t ino)
>  {
> -  return (grub_be_to_cpu64 (ino) & ((1LL << GRUB_XFS_INO_AGBITS (data)) - 1));
> +  return (ino & ((1LL << GRUB_XFS_INO_AGBITS (data)) - 1));
>  }
>  
>  static inline grub_uint64_t
>  GRUB_XFS_INO_AG (struct grub_xfs_data *data,
>  		 grub_uint64_t ino)
>  {
> -  return (grub_be_to_cpu64 (ino) >> GRUB_XFS_INO_AGBITS (data));
> +  return (ino >> GRUB_XFS_INO_AGBITS (data));
>  }
>  
>  static inline grub_disk_addr_t
> @@ -511,13 +511,12 @@ grub_xfs_iterate_dir (grub_fshelp_node_t dir,
>  	if (smallino)
>  	  {
>  	    parent = grub_be_to_cpu32 (diro->inode.data.dir.dirhead.parent.i4);
> -	    parent = grub_cpu_to_be64 (parent);
>  	    /* The header is a bit smaller than usual.  */
>  	    de = (struct grub_xfs_dir_entry *) ((char *) de - 4);
>  	  }
>  	else
>  	  {
> -	    parent = diro->inode.data.dir.dirhead.parent.i8;
> +	    parent = grub_be_to_cpu64(diro->inode.data.dir.dirhead.parent.i8);
>  	  }
>  
>  	/* Synthesize the direntries for `.' and `..'.  */
> @@ -550,7 +549,6 @@ grub_xfs_iterate_dir (grub_fshelp_node_t dir,
>  		| (((grub_uint64_t) inopos[5]) << 16)
>  		| (((grub_uint64_t) inopos[6]) << 8)
>  		| (((grub_uint64_t) inopos[7]) << 0);
> -	    ino = grub_cpu_to_be64 (ino);
>  
>  	    c = de->name[de->len];
>  	    de->name[de->len] = '\0';
> @@ -632,7 +630,8 @@ grub_xfs_iterate_dir (grub_fshelp_node_t dir,
>  		   is not used by GRUB.  So it can be overwritten.  */
>  		filename[direntry->len] = '\0';
>  
> -		if (iterate_dir_call_hook (direntry->inode, filename, &ctx))
> +		if (iterate_dir_call_hook (grub_be_to_cpu64(direntry->inode), 
> +					   filename, &ctx))
>  		  {
>  		    grub_free (dirblock);
>  		    return 1;
> @@ -694,7 +693,7 @@ grub_xfs_mount (grub_disk_t disk)
>      goto fail;
>  
>    data->diropen.data = data;
> -  data->diropen.ino = data->sblock.rootino;
> +  data->diropen.ino = grub_be_to_cpu64(data->sblock.rootino);
>    data->diropen.inode_read = 1;
>    data->bsize = grub_be_to_cpu32 (data->sblock.bsize);
>    data->agsize = grub_be_to_cpu32 (data->sblock.agsize);

Re: [PATCH 4/4] xfs: V5 filesystem format support

[Andrei Borzenkov]

В Mon, 14 Jul 2014 17:21:31 +0200
Jan Kara <jack@suse.cz> пишет:

> Add support for new XFS on disk format. We have to handle optional
> filetype fields in directory entries, additional CRC, LSN, UUID entries
> in some structures, etc.
> 
> Signed-off-by: Jan Kara <jack@suse.cz>
> ---
>  grub-core/fs/xfs.c | 249 +++++++++++++++++++++++++++++++++++++----------------
>  1 file changed, 177 insertions(+), 72 deletions(-)
> 
> diff --git a/grub-core/fs/xfs.c b/grub-core/fs/xfs.c
> index 7e247a32df5c..7f2582d33e02 100644
> --- a/grub-core/fs/xfs.c
> +++ b/grub-core/fs/xfs.c
> @@ -34,6 +34,15 @@ GRUB_MOD_LICENSE ("GPLv3+");
>  #define XFS_INODE_FORMAT_EXT	2
>  #define XFS_INODE_FORMAT_BTREE	3
>  
> +/* Superblock version field flags */
> +#define XFS_SB_VERSION_NUMBITS          0x000f
> +#define XFS_SB_VERSION_MOREBITSBIT      0x8000
> +
> +/* features2 field flags */
> +#define XFS_SB_VERSION2_FTYPE           0x00000200      /* inode type in dir */
> +
> +/* incompat feature flags */
> +#define XFS_SB_FEAT_INCOMPAT_FTYPE      (1 << 0)        /* filetype in dirent */
>  
>  struct grub_xfs_sblock
>  {
> @@ -45,7 +54,9 @@ struct grub_xfs_sblock
>    grub_uint64_t rootino;
>    grub_uint8_t unused3[20];
>    grub_uint32_t agsize;
> -  grub_uint8_t unused4[20];
> +  grub_uint8_t unused4[12];
> +  grub_uint16_t version;
> +  grub_uint8_t unused5[6];
>    grub_uint8_t label[12];
>    grub_uint8_t log2_bsize;
>    grub_uint8_t log2_sect;
> @@ -54,12 +65,19 @@ struct grub_xfs_sblock
>    grub_uint8_t log2_agblk;
>    grub_uint8_t unused6[67];
>    grub_uint8_t log2_dirblk;
> +  grub_uint8_t unused7[7];
> +  grub_uint32_t features2;
> +  grub_uint8_t unused8[4];
> +  grub_uint32_t sb_features_compat;
> +  grub_uint32_t sb_features_ro_compat;
> +  grub_uint32_t sb_features_incompat;
> +  grub_uint32_t sb_features_log_incompat;
>  } GRUB_PACKED;
>  
>  struct grub_xfs_dir_header
>  {
>    grub_uint8_t count;
> -  grub_uint8_t smallino;
> +  grub_uint8_t largeino;
>    union
>    {
>      grub_uint32_t i4;
> @@ -67,14 +85,16 @@ struct grub_xfs_dir_header
>    } GRUB_PACKED parent;
>  } GRUB_PACKED;
>  
> +/* Structure for directory entry inlined in the inode */
>  struct grub_xfs_dir_entry
>  {
>    grub_uint8_t len;
>    grub_uint16_t offset;
>    char name[1];
> -  /* Inode number follows, 32 bits.  */
> +  /* Inode number follows, 32 / 64 bits.  */
>  } GRUB_PACKED;
>  
> +/* Structure for directory entry in a block */
>  struct grub_xfs_dir2_entry
>  {
>    grub_uint64_t inode;
> @@ -90,7 +110,8 @@ struct grub_xfs_btree_node
>    grub_uint16_t numrecs;
>    grub_uint64_t left;
>    grub_uint64_t right;
> -  grub_uint64_t keys[1];
> +  /* In V5 here follow crc, uuid, etc. */
> +  /* Then follow keys and block pointers */
>  }  GRUB_PACKED;
>  
>  struct grub_xfs_btree_root
> @@ -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?

>  } 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;
>  
>  \f
>  
> +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?

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)

> +}
> +
> +static int grub_xfs_sb_hasftype(struct grub_xfs_data *data)
> +{
> +  grub_uint32_t version = grub_be_to_cpu16(data->sblock.version);
> +
> +  if ((version & XFS_SB_VERSION_NUMBITS) == 5 &&
> +      grub_be_to_cpu32(data->sblock.sb_features_incompat) & XFS_SB_FEAT_INCOMPAT_FTYPE)
> +    return 1;
> +  if (version & XFS_SB_VERSION_MOREBITSBIT &&
> +      grub_be_to_cpu32(data->sblock.features2) & XFS_SB_VERSION2_FTYPE)
> +    return 1;
> +  return 0;
> +}
> +

Same. All are constants, so *_compile_time.

>  /* Filetype information as used in inodes.  */
>  #define FILETYPE_INO_MASK	0170000
>  #define FILETYPE_INO_REG	0100000
> @@ -219,18 +249,6 @@ GRUB_XFS_EXTENT_SIZE (grub_xfs_extent *exts, int ex)
>    return (grub_be_to_cpu32 (exts[ex][3]) & ((1 << 21) - 1));
>  }
>  
> -static inline int
> -GRUB_XFS_ROUND_TO_DIRENT (int pos)
> -{
> -  return ((((pos) + 8 - 1) / 8) * 8);
> -}
> -
> -static inline int
> -GRUB_XFS_NEXT_DIRENT (int pos, int len)
> -{
> -  return (pos) + GRUB_XFS_ROUND_TO_DIRENT (8 + 1 + len + 2);
> -}
> -
>  \f
>  static inline grub_uint64_t
>  grub_xfs_inode_block (struct grub_xfs_data *data,
> @@ -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.

> +}
> +
> +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.

> +		 (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)?

> +}
> +
> +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).

> +}
> +
>  static grub_err_t
>  grub_xfs_read_inode (struct grub_xfs_data *data, grub_uint64_t ino,
>  		     struct grub_xfs_inode *inode)
> @@ -268,6 +376,9 @@ grub_xfs_read_inode (struct grub_xfs_data *data, grub_uint64_t ino,
>    grub_uint64_t block = grub_xfs_inode_block (data, ino);
>    int offset = grub_xfs_inode_offset (data, ino);
>  
> +  grub_dprintf("xfs", "Reading inode (%llu) - %llu, %d\n",
> +	       (unsigned long long) ino,
> +	       (unsigned long long) block, offset);
>    /* Read the inode.  */
>    if (grub_disk_read (data->disk, block, offset, grub_xfs_inode_size(data),
>  		      inode))
> @@ -290,6 +401,7 @@ grub_xfs_read_block (grub_fshelp_node_t node, grub_disk_addr_t fileblock)
>  
>    if (node->inode.format == XFS_INODE_FORMAT_BTREE)
>      {
> +      struct grub_xfs_btree_root *root;
>        const grub_uint64_t *keys;
>        int recoffset;
>  
> @@ -297,15 +409,15 @@ grub_xfs_read_block (grub_fshelp_node_t node, grub_disk_addr_t fileblock)
>        if (leaf == 0)
>          return 0;
>  
> -      nrec = grub_be_to_cpu16 (node->inode.data.btree.numrecs);
> -      keys = &node->inode.data.btree.keys[0];
> +      root = grub_xfs_inode_data(&node->inode);
> +      nrec = grub_be_to_cpu16 (root->numrecs);
> +      keys = &root->keys[0];
>        if (node->inode.fork_offset)
>  	recoffset = (node->inode.fork_offset - 1) / 2;
>        else
>  	recoffset = (grub_xfs_inode_size(node->data)
> -		     - ((char *) &node->inode.data.btree.keys
> -			- (char *) &node->inode))
> -	  / (2 * sizeof (grub_uint64_t));
> +		     - ((char *) keys - (char *) &node->inode))
> +				/ (2 * sizeof (grub_uint64_t));
>        do
>          {
>            int i;
> @@ -327,7 +439,10 @@ grub_xfs_read_block (grub_fshelp_node_t node, grub_disk_addr_t fileblock)
>                                0, node->data->bsize, leaf))
>              return 0;
>  
> -          if (grub_strncmp ((char *) leaf->magic, "BMAP", 4))
> +	  if ((!node->data->hascrc &&
> +	       grub_strncmp ((char *) leaf->magic, "BMAP", 4)) ||
> +	      (node->data->hascrc &&
> +	       grub_strncmp ((char *) leaf->magic, "BMA3", 4)))
>              {
>                grub_free (leaf);
>                grub_error (GRUB_ERR_BAD_FS, "not a correct XFS BMAP node");
> @@ -335,8 +450,8 @@ grub_xfs_read_block (grub_fshelp_node_t node, grub_disk_addr_t fileblock)
>              }
>  
>            nrec = grub_be_to_cpu16 (leaf->numrecs);
> -          keys = &leaf->keys[0];
> -	  recoffset = ((node->data->bsize - ((char *) &leaf->keys
> +          keys = grub_xfs_btree_keys(node->data, leaf);
> +	  recoffset = ((node->data->bsize - ((char *) keys
>  					     - (char *) leaf))
>  		       / (2 * sizeof (grub_uint64_t)));
>  	}
> @@ -346,7 +461,7 @@ grub_xfs_read_block (grub_fshelp_node_t node, grub_disk_addr_t fileblock)
>    else if (node->inode.format == XFS_INODE_FORMAT_EXT)
>      {
>        nrec = grub_be_to_cpu32 (node->inode.nextents);
> -      exts = &node->inode.data.extents[0];
> +      exts = grub_xfs_inode_data(&node->inode);
>      }
>    else
>      {
> @@ -404,7 +519,7 @@ grub_xfs_read_symlink (grub_fshelp_node_t node)
>    switch (node->inode.format)
>      {
>      case XFS_INODE_FORMAT_INO:
> -      return grub_strndup (node->inode.data.raw, size);
> +      return grub_strndup (grub_xfs_inode_data(&node->inode), size);
>  
>      case XFS_INODE_FORMAT_EXT:
>        {
> @@ -501,23 +616,18 @@ grub_xfs_iterate_dir (grub_fshelp_node_t dir,
>      {
>      case XFS_INODE_FORMAT_INO:
>        {
> -	struct grub_xfs_dir_entry *de = &diro->inode.data.dir.direntry[0];
> -	int smallino = !diro->inode.data.dir.dirhead.smallino;
> +	struct grub_xfs_dir_header *head = grub_xfs_inode_data(&diro->inode);
> +	struct grub_xfs_dir_entry *de = grub_xfs_inline_de(head);
> +	int smallino = !head->largeino;
>  	int i;
>  	grub_uint64_t parent;
>  
>  	/* If small inode numbers are used to pack the direntry, the
>  	   parent inode number is small too.  */
>  	if (smallino)
> -	  {
> -	    parent = grub_be_to_cpu32 (diro->inode.data.dir.dirhead.parent.i4);
> -	    /* The header is a bit smaller than usual.  */
> -	    de = (struct grub_xfs_dir_entry *) ((char *) de - 4);
> -	  }
> +	  parent = grub_be_to_cpu32 (head->parent.i4);
>  	else
> -	  {
> -	    parent = grub_be_to_cpu64(diro->inode.data.dir.dirhead.parent.i8);
> -	  }
> +	  parent = grub_be_to_cpu64 (head->parent.i8);
>  
>  	/* Synthesize the direntries for `.' and `..'.  */
>  	if (iterate_dir_call_hook (diro->ino, ".", &ctx))
> @@ -526,12 +636,10 @@ grub_xfs_iterate_dir (grub_fshelp_node_t dir,
>  	if (iterate_dir_call_hook (parent, "..", &ctx))
>  	  return 1;
>  
> -	for (i = 0; i < diro->inode.data.dir.dirhead.count; i++)
> +	for (i = 0; i < head->count; i++)
>  	  {
>  	    grub_uint64_t ino;
> -	    grub_uint8_t *inopos = (((grub_uint8_t *) de)
> -			    + sizeof (struct grub_xfs_dir_entry)
> -			    + de->len - 1);
> +	    grub_uint8_t *inopos = grub_xfs_inline_de_inopos(dir->data, de);
>  	    grub_uint8_t c;
>  
>  	    /* inopos might be unaligned.  */
> @@ -556,10 +664,7 @@ grub_xfs_iterate_dir (grub_fshelp_node_t dir,
>  	      return 1;
>  	    de->name[de->len] = c;
>  
> -	    de = ((struct grub_xfs_dir_entry *)
> -		  (((char *) de)+ sizeof (struct grub_xfs_dir_entry) + de->len
> -		   + ((smallino ? sizeof (grub_uint32_t)
> -		       : sizeof (grub_uint64_t))) - 1));
> +	    de = grub_xfs_inline_next_de(dir->data, head, de);
>  	  }
>  	break;
>        }
> @@ -586,15 +691,11 @@ grub_xfs_iterate_dir (grub_fshelp_node_t dir,
>  		    >> dirblk_log2);
>  	     blk++)
>  	  {
> -	    /* The header is skipped, the first direntry is stored
> -	       from byte 16.  */
> -	    int pos = 16;
> +	    struct grub_xfs_dir2_entry *direntry =
> +					grub_xfs_first_de(dir->data, dirblock);
>  	    int entries;
> -	    int tail_start = (dirblk_size
> -			      - sizeof (struct grub_xfs_dirblock_tail));
> -
> -	    struct grub_xfs_dirblock_tail *tail;
> -	    tail = (struct grub_xfs_dirblock_tail *) &dirblock[tail_start];
> +	    struct grub_xfs_dirblock_tail *tail =
> +					grub_xfs_dir_tail(dir->data, dirblock);
>  
>  	    numread = grub_xfs_read_file (dir, 0, 0,
>  					  blk << dirblk_log2,
> @@ -606,13 +707,11 @@ grub_xfs_iterate_dir (grub_fshelp_node_t dir,
>  		       - grub_be_to_cpu32 (tail->leaf_stale));
>  
>  	    /* Iterate over all entries within this block.  */
> -	    while (pos < tail_start)
> +	    while ((char *)direntry < (char *)tail)
>  	      {
> -		struct grub_xfs_dir2_entry *direntry;
>  		grub_uint8_t *freetag;
>  		char *filename;
>  
> -		direntry = (struct grub_xfs_dir2_entry *) &dirblock[pos];
>  		freetag = (grub_uint8_t *) direntry;
>  
>  		if (grub_get_unaligned16 (freetag) == 0XFFFF)
> @@ -620,14 +719,16 @@ grub_xfs_iterate_dir (grub_fshelp_node_t dir,
>  		    grub_uint8_t *skip = (freetag + sizeof (grub_uint16_t));
>  
>  		    /* This entry is not used, go to the next one.  */
> -		    pos += grub_be_to_cpu16 (grub_get_unaligned16 (skip));
> +		    direntry = (struct grub_xfs_dir2_entry *)
> +				(((char *)direntry) +
> +				grub_be_to_cpu16 (grub_get_unaligned16 (skip)));
>  
>  		    continue;
>  		  }
>  
> -		filename = &dirblock[pos + sizeof (*direntry)];
> -		/* The byte after the filename is for the tag, which
> -		   is not used by GRUB.  So it can be overwritten.  */
> +		filename = (char *)(direntry + 1);
> +		/* The byte after the filename is for the filetype, padding, or
> +		   tag, which is not used by GRUB.  So it can be overwritten. */
>  		filename[direntry->len] = '\0';
>  
>  		if (iterate_dir_call_hook (grub_be_to_cpu64(direntry->inode), 
> @@ -644,8 +745,7 @@ grub_xfs_iterate_dir (grub_fshelp_node_t dir,
>  		  break;
>  
>  		/* Select the next directory entry.  */
> -		pos = GRUB_XFS_NEXT_DIRENT (pos, direntry->len);
> -		pos = GRUB_XFS_ROUND_TO_DIRENT (pos);
> +		direntry = grub_xfs_next_de(dir->data, direntry);
>  	      }
>  	  }
>  	grub_free (dirblock);
> @@ -670,6 +770,7 @@ grub_xfs_mount (grub_disk_t disk)
>    if (!data)
>      return 0;
>  
> +  grub_dprintf("xfs", "Reading sb\n");
>    /* Read the superblock.  */
>    if (grub_disk_read (disk, 0, 0,
>  		      sizeof (struct grub_xfs_sblock), &data->sblock))
> @@ -697,9 +798,13 @@ grub_xfs_mount (grub_disk_t disk)
>    data->diropen.inode_read = 1;
>    data->bsize = grub_be_to_cpu32 (data->sblock.bsize);
>    data->agsize = grub_be_to_cpu32 (data->sblock.agsize);
> +  data->hasftype = grub_xfs_sb_hasftype(data);
> +  data->hascrc = grub_xfs_sb_hascrc(data);
>  
>    data->disk = disk;
>    data->pos = 0;
> +  grub_dprintf("xfs", "Reading root ino %llu\n",
> +	       (unsigned long long) grub_cpu_to_be64(data->sblock.rootino));
>  
>    grub_xfs_read_inode (data, data->diropen.ino, &data->diropen.inode);
>  

Re: [PATCH 1/4] xfs: Add helper for inode size

[Andrei Borzenkov]

В Mon, 11 May 2015 14:15:48 +0200
Jan Kara <jack@suse.cz> пишет:

> On Mon 11-05-15 14:53:57, Andrei Borzenkov wrote:
> > В Mon, 14 Jul 2014 17:21:28 +0200
> > Jan Kara <jack@suse.cz> пишет:
> > 
> > > Signed-off-by: Jan Kara <jack@suse.cz>
> > > ---
> > >  grub-core/fs/xfs.c | 17 +++++++++++------
> > >  1 file changed, 11 insertions(+), 6 deletions(-)
> > > 
> > > diff --git a/grub-core/fs/xfs.c b/grub-core/fs/xfs.c
> > > index 16ffd3f1ebd9..a2fc942707c1 100644
> > > --- a/grub-core/fs/xfs.c
> > > +++ b/grub-core/fs/xfs.c
> > > @@ -255,6 +255,11 @@ grub_xfs_inode_offset (struct grub_xfs_data *data,
> > >  	  data->sblock.log2_inode);
> > >  }
> > >  
> > > +static inline int
> > > +grub_xfs_inode_size(struct grub_xfs_data *data)
> > 
> > This should be grub_size_t.
>   OK.
> 
> > What is the reason to add it? It does not look like subsequent
> > patches modify this function like making it conditional on XFS version.
>   This is just a cleanup so that it's clearer that 1 <<
> data->sblock.log2_inode is actually inode size. But if you don't like it,
> you can just skip it. So should I send update patch? Thanks for having a
> look!

Yes, send updated patch, it is less risky as series had been tested for
quite some time. But actually helper to compute allocation size with
comment would be quite useful; I stared for quite some time at

                  sizeof (struct grub_fshelp_node)
                   - sizeof (struct grub_xfs_inode)
                   + (1 << data->sblock.log2_inode));

trying to understand what it's about :)

Re: [PATCH 1/4] xfs: Add helper for inode size

[Jan Kara]

On Tue 12-05-15 08:26:26, Andrei Borzenkov wrote:
> В Mon, 11 May 2015 14:15:48 +0200
> Jan Kara <jack@suse.cz> пишет:
> 
> > On Mon 11-05-15 14:53:57, Andrei Borzenkov wrote:
> > > В Mon, 14 Jul 2014 17:21:28 +0200
> > > Jan Kara <jack@suse.cz> пишет:
> > > 
> > > > Signed-off-by: Jan Kara <jack@suse.cz>
> > > > ---
> > > >  grub-core/fs/xfs.c | 17 +++++++++++------
> > > >  1 file changed, 11 insertions(+), 6 deletions(-)
> > > > 
> > > > diff --git a/grub-core/fs/xfs.c b/grub-core/fs/xfs.c
> > > > index 16ffd3f1ebd9..a2fc942707c1 100644
> > > > --- a/grub-core/fs/xfs.c
> > > > +++ b/grub-core/fs/xfs.c
> > > > @@ -255,6 +255,11 @@ grub_xfs_inode_offset (struct grub_xfs_data *data,
> > > >  	  data->sblock.log2_inode);
> > > >  }
> > > >  
> > > > +static inline int
> > > > +grub_xfs_inode_size(struct grub_xfs_data *data)
> > > 
> > > This should be grub_size_t.
> >   OK.
> > 
> > > What is the reason to add it? It does not look like subsequent
> > > patches modify this function like making it conditional on XFS version.
> >   This is just a cleanup so that it's clearer that 1 <<
> > data->sblock.log2_inode is actually inode size. But if you don't like it,
> > you can just skip it. So should I send update patch? Thanks for having a
> > look!
> 
> Yes, send updated patch, it is less risky as series had been tested for
> quite some time. But actually helper to compute allocation size with
> comment would be quite useful; I stared for quite some time at
> 
>                   sizeof (struct grub_fshelp_node)
>                    - sizeof (struct grub_xfs_inode)
>                    + (1 << data->sblock.log2_inode));
> 
> trying to understand what it's about :)
  OK, I'll do that.

								Honza
-- 
Jan Kara <jack@suse.cz>
SUSE Labs, CR

Re: [PATCH 4/4] xfs: V5 filesystem format support

[Jan Kara]

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;
> >  
> >  \f
> >  
> > +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 <jack@suse.cz>
SUSE Labs, CR

Re: [PATCH 4/4] xfs: V5 filesystem format support

[Andrei Borzenkov]

В Tue, 12 May 2015 15:47:40 +0200
Jan Kara <jack@suse.cz> пишет:

> > > +{
> > > +	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...
> 

OK

> > +
> > > +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?

Why 2 structures? What I actually meant was

struct grub_xfs_btree_node
{
  grub_uint8_t magic[4];
  grub_uint16_t level;
  grub_uint16_t numrecs;
  grub_uint64_t left;
  grub_uint64_t right;
  grub_uint64_t keys[0];
}  GRUB_PACKED;

if (data->hascrc)
  return &leaf->keys[6]
else
  return &leaf->keys[0]

with suitable comment. It is not perfect either but at least leaves
compiler check in place.

Re: [PATCH 4/4] xfs: V5 filesystem format support

[Jan Kara]

On Wed 13-05-15 07:50:38, Andrei Borzenkov wrote:
> В Tue, 12 May 2015 15:47:40 +0200
> Jan Kara <jack@suse.cz> пишет:
> > > +
> > > > +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?
> 
> Why 2 structures? What I actually meant was
> 
> struct grub_xfs_btree_node
> {
>   grub_uint8_t magic[4];
>   grub_uint16_t level;
>   grub_uint16_t numrecs;
>   grub_uint64_t left;
>   grub_uint64_t right;
>   grub_uint64_t keys[0];
> }  GRUB_PACKED;
> 
> if (data->hascrc)
>   return &leaf->keys[6]
> else
>   return &leaf->keys[0]
> 
> with suitable comment. It is not perfect either but at least leaves
> compiler check in place.
  Ah, I see. I probably won't call it 'keys' but something like 'data' and
add a comment explaining what's going on.

								Honza
-- 
Jan Kara <jack@suse.cz>
SUSE Labs, CR