[Cluster-devel] [PATCH 1/2] gfs2_edit savemeta: Fix up saving of dinodes/symlinks

[Cluster-devel] [PATCH 1/2] gfs2_edit savemeta: Fix up saving of dinodes/symlinks

[Andrew Price]

Factor out the code that decides whether to save the dinode contents,
improve its error reporting (and don't exit) and make sure contents of
symlink dinodes are saved.

Signed-off-by: Andrew Price <anprice@redhat.com>
---
 gfs2/edit/savemeta.c | 52 +++++++++++++++++++++++++++++++++-------------------
 1 file changed, 33 insertions(+), 19 deletions(-)

diff --git a/gfs2/edit/savemeta.c b/gfs2/edit/savemeta.c
index dee405bc..6ae38274 100644
--- a/gfs2/edit/savemeta.c
+++ b/gfs2/edit/savemeta.c
@@ -190,6 +190,38 @@ static const char *anthropomorphize(unsigned long long inhuman_value)
 	return out_val;
 }
 
+static int di_save_len(struct gfs2_buffer_head *bh, uint64_t owner)
+{
+	struct gfs2_inode *inode;
+	struct gfs2_dinode *dn;
+	int len;
+
+	if (sbd.gfs1)
+		inode = lgfs2_gfs_inode_get(&sbd, bh);
+	else
+		inode = lgfs2_inode_get(&sbd, bh);
+
+	if (inode == NULL) {
+		fprintf(stderr, "Error reading inode at %"PRIu64": %s\n",
+		        bh->b_blocknr, strerror(errno));
+		return 0; /* Skip the block */
+	}
+	dn = &inode->i_di;
+	len = sbd.bsize;
+
+	/* Do not save (user) data from stuffed inodes unless they are
+	   indirect pointers, dirents, symlinks or fs internal data */
+	if (dn->di_height == 0 &&
+	    !S_ISDIR(dn->di_mode) &&
+	    !S_ISLNK(dn->di_mode) &&
+	    (sbd.gfs1 && dn->__pad1 != GFS_FILE_DIR) &&
+	    !block_is_systemfile(owner))
+		len = sizeof(struct gfs2_dinode);
+
+	inode_put(&inode);
+	return len;
+}
+
 /*
  * get_gfs_struct_info - get block type and structure length
  *
@@ -205,7 +237,6 @@ static int get_gfs_struct_info(struct gfs2_buffer_head *lbh, uint64_t owner,
                                int *block_type, int *gstruct_len)
 {
 	struct gfs2_meta_header mh;
-	struct gfs2_inode *inode;
 
 	if (block_type != NULL)
 		*block_type = 0;
@@ -229,24 +260,7 @@ static int get_gfs_struct_info(struct gfs2_buffer_head *lbh, uint64_t owner,
 		*gstruct_len = sbd.bsize;
 		break;
 	case GFS2_METATYPE_DI:   /* 4 (disk inode) */
-		if (sbd.gfs1) {
-			inode = lgfs2_gfs_inode_get(&sbd, lbh);
-		} else {
-			inode = lgfs2_inode_get(&sbd, lbh);
-		}
-		if (inode == NULL) {
-			perror("Error reading inode");
-			exit(-1);
-		}
-		if (S_ISDIR(inode->i_di.di_mode) ||
-		     (sbd.gfs1 && inode->i_di.__pad1 == GFS_FILE_DIR))
-			*gstruct_len = sbd.bsize;
-		else if (!inode->i_di.di_height && !block_is_systemfile(owner) &&
-			 !S_ISDIR(inode->i_di.di_mode))
-			*gstruct_len = sizeof(struct gfs2_dinode);
-		else
-			*gstruct_len = sbd.bsize;
-		inode_put(&inode);
+		*gstruct_len = di_save_len(lbh, owner);
 		break;
 	case GFS2_METATYPE_IN:   /* 5 (indir inode blklst) */
 		*gstruct_len = sbd.bsize; /*sizeof(struct gfs_indirect);*/
-- 
2.13.5

[Cluster-devel] [PATCH 2/2] gfs2_edit savemeta: Use size_t for saved structure lengths

[Andrew Price]

Although the value range can fit in an int it's better to use an
unsigned type consistent with sizeof and memcpy.

Signed-off-by: Andrew Price <anprice@redhat.com>
---
 gfs2/edit/savemeta.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/gfs2/edit/savemeta.c b/gfs2/edit/savemeta.c
index 6ae38274..7ff1b8e4 100644
--- a/gfs2/edit/savemeta.c
+++ b/gfs2/edit/savemeta.c
@@ -190,11 +190,11 @@ static const char *anthropomorphize(unsigned long long inhuman_value)
 	return out_val;
 }
 
-static int di_save_len(struct gfs2_buffer_head *bh, uint64_t owner)
+static size_t di_save_len(struct gfs2_buffer_head *bh, uint64_t owner)
 {
 	struct gfs2_inode *inode;
 	struct gfs2_dinode *dn;
-	int len;
+	size_t len;
 
 	if (sbd.gfs1)
 		inode = lgfs2_gfs_inode_get(&sbd, bh);
@@ -234,7 +234,7 @@ static int di_save_len(struct gfs2_buffer_head *bh, uint64_t owner)
  *          -1 if this isn't gfs metadata.
  */
 static int get_gfs_struct_info(struct gfs2_buffer_head *lbh, uint64_t owner,
-                               int *block_type, int *gstruct_len)
+                               int *block_type, size_t *gstruct_len)
 {
 	struct gfs2_meta_header mh;
 
@@ -423,8 +423,8 @@ static int savemetaclose(struct metafd *mfd)
 static int save_bh(struct metafd *mfd, struct gfs2_buffer_head *savebh, uint64_t owner, int *blktype)
 {
 	struct saved_metablock *savedata;
+	size_t blklen;
 	size_t outsz;
-	int blklen;
 
 	/* If this isn't metadata and isn't a system file, we don't want it.
 	   Note that we're checking "owner" here rather than blk.  That's
-- 
2.13.5

[Cluster-devel] [PATCH 1/2] gfs2_edit savemeta: Fix up saving of dinodes/symlinks

[Bob Peterson]

----- Original Message -----
| Factor out the code that decides whether to save the dinode contents,
| improve its error reporting (and don't exit) and make sure contents of
| symlink dinodes are saved.
| 
| Signed-off-by: Andrew Price <anprice@redhat.com>
| ---
|  gfs2/edit/savemeta.c | 52
|  +++++++++++++++++++++++++++++++++-------------------
|  1 file changed, 33 insertions(+), 19 deletions(-)
| 
Hi,

Both patches look good. ACK

Regards,

Bob Peterson
Red Hat File Systems

[Cluster-devel] [PATCH 1/2] gfs2_edit savemeta: Fix up saving of dinodes/symlinks

[Andrew Price]

On Thu, Aug 17, 2017 at 06:10:26PM +0100, Andrew Price wrote:
> Factor out the code that decides whether to save the dinode contents,
> improve its error reporting (and don't exit) and make sure contents of
> symlink dinodes are saved.
> 
> Signed-off-by: Andrew Price <anprice@redhat.com>
> ---
>  gfs2/edit/savemeta.c | 52 +++++++++++++++++++++++++++++++++-------------------
>  1 file changed, 33 insertions(+), 19 deletions(-)
> 
> diff --git a/gfs2/edit/savemeta.c b/gfs2/edit/savemeta.c
> index dee405bc..6ae38274 100644
> --- a/gfs2/edit/savemeta.c
> +++ b/gfs2/edit/savemeta.c
> @@ -190,6 +190,38 @@ static const char *anthropomorphize(unsigned long long inhuman_value)
>  	return out_val;
>  }
>  
> +static int di_save_len(struct gfs2_buffer_head *bh, uint64_t owner)
> +{
> +	struct gfs2_inode *inode;
> +	struct gfs2_dinode *dn;
> +	int len;
> +
> +	if (sbd.gfs1)
> +		inode = lgfs2_gfs_inode_get(&sbd, bh);
> +	else
> +		inode = lgfs2_inode_get(&sbd, bh);
> +
> +	if (inode == NULL) {
> +		fprintf(stderr, "Error reading inode at %"PRIu64": %s\n",
> +		        bh->b_blocknr, strerror(errno));
> +		return 0; /* Skip the block */
> +	}
> +	dn = &inode->i_di;
> +	len = sbd.bsize;
> +
> +	/* Do not save (user) data from stuffed inodes unless they are
> +	   indirect pointers, dirents, symlinks or fs internal data */
> +	if (dn->di_height == 0 &&
> +	    !S_ISDIR(dn->di_mode) &&
> +	    !S_ISLNK(dn->di_mode) &&
> +	    (sbd.gfs1 && dn->__pad1 != GFS_FILE_DIR) &&

Just spotted that the logic is wrong here. I'll fix that up.

Andy

> +	    !block_is_systemfile(owner))
> +		len = sizeof(struct gfs2_dinode);
> +
> +	inode_put(&inode);
> +	return len;
> +}
> +
>  /*
>   * get_gfs_struct_info - get block type and structure length
>   *
> @@ -205,7 +237,6 @@ static int get_gfs_struct_info(struct gfs2_buffer_head *lbh, uint64_t owner,
>                                 int *block_type, int *gstruct_len)
>  {
>  	struct gfs2_meta_header mh;
> -	struct gfs2_inode *inode;
>  
>  	if (block_type != NULL)
>  		*block_type = 0;
> @@ -229,24 +260,7 @@ static int get_gfs_struct_info(struct gfs2_buffer_head *lbh, uint64_t owner,
>  		*gstruct_len = sbd.bsize;
>  		break;
>  	case GFS2_METATYPE_DI:   /* 4 (disk inode) */
> -		if (sbd.gfs1) {
> -			inode = lgfs2_gfs_inode_get(&sbd, lbh);
> -		} else {
> -			inode = lgfs2_inode_get(&sbd, lbh);
> -		}
> -		if (inode == NULL) {
> -			perror("Error reading inode");
> -			exit(-1);
> -		}
> -		if (S_ISDIR(inode->i_di.di_mode) ||
> -		     (sbd.gfs1 && inode->i_di.__pad1 == GFS_FILE_DIR))
> -			*gstruct_len = sbd.bsize;
> -		else if (!inode->i_di.di_height && !block_is_systemfile(owner) &&
> -			 !S_ISDIR(inode->i_di.di_mode))
> -			*gstruct_len = sizeof(struct gfs2_dinode);
> -		else
> -			*gstruct_len = sbd.bsize;
> -		inode_put(&inode);
> +		*gstruct_len = di_save_len(lbh, owner);
>  		break;
>  	case GFS2_METATYPE_IN:   /* 5 (indir inode blklst) */
>  		*gstruct_len = sbd.bsize; /*sizeof(struct gfs_indirect);*/
> -- 
> 2.13.5
>