misc xfsprogs cleanups after the 5.7 merge

misc xfsprogs cleanups after the 5.7 merge

[Christoph Hellwig]

Hi Eric,

this series contains the differences to my port that seem useful
to keep.

[PATCH 1/8] libxfs: use tabs instead of spaces in div_u64

[Christoph Hellwig]

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 libxfs/libxfs_priv.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/libxfs/libxfs_priv.h b/libxfs/libxfs_priv.h
index 8d717d91..5688284d 100644
--- a/libxfs/libxfs_priv.h
+++ b/libxfs/libxfs_priv.h
@@ -263,8 +263,8 @@ div_u64_rem(uint64_t dividend, uint32_t divisor, uint32_t *remainder)
  */
 static inline uint64_t div_u64(uint64_t dividend, uint32_t divisor)
 {
-        uint32_t remainder;
-        return div_u64_rem(dividend, divisor, &remainder);
+	uint32_t remainder;
+	return div_u64_rem(dividend, divisor, &remainder);
 }
 
 /**
-- 
2.26.2

[PATCH 2/8] db: fix a comment in scan_freelist

[Christoph Hellwig]

XFS_BUF_TO_AGFL_BNO has been renamed to open coded xfs_buf_to_agfl_bno.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 db/check.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/db/check.c b/db/check.c
index c9bafa8e..09f8f6c9 100644
--- a/db/check.c
+++ b/db/check.c
@@ -4075,7 +4075,7 @@ scan_freelist(
 		return;
 	}
 
-	/* open coded XFS_BUF_TO_AGFL_BNO */
+	/* open coded xfs_buf_to_agfl_bno */
 	state.count = 0;
 	state.agno = seqno;
 	libxfs_agfl_walk(mp, agf, iocur_top->bp, scan_agfl, &state);
-- 
2.26.2

[PATCH 3/8] db: add a comment to agfl_crc_flds

[Christoph Hellwig]

Explain the bno field that is not actually part of the structure
anymore.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 db/agfl.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/db/agfl.c b/db/agfl.c
index 45e4d6f9..ce7a2548 100644
--- a/db/agfl.c
+++ b/db/agfl.c
@@ -47,6 +47,7 @@ const field_t	agfl_crc_flds[] = {
 	{ "uuid", FLDT_UUID, OI(OFF(uuid)), C1, 0, TYP_NONE },
 	{ "lsn", FLDT_UINT64X, OI(OFF(lsn)), C1, 0, TYP_NONE },
 	{ "crc", FLDT_CRC, OI(OFF(crc)), C1, 0, TYP_NONE },
+	/* the bno array really is behind the actual structure */
 	{ "bno", FLDT_AGBLOCKNZ, OI(bitize(sizeof(struct xfs_agfl))),
 	  agfl_bno_size, FLD_ARRAY|FLD_COUNT, TYP_DATA },
 	{ NULL }
-- 
2.26.2

[PATCH 4/8] db: cleanup attr_set_f and attr_remove_f

[Christoph Hellwig]

Don't use local variables for information that is set in the da_args
structure.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 db/attrset.c | 67 ++++++++++++++++++++++------------------------------
 1 file changed, 28 insertions(+), 39 deletions(-)

diff --git a/db/attrset.c b/db/attrset.c
index 1ff2eb85..0a464983 100644
--- a/db/attrset.c
+++ b/db/attrset.c
@@ -67,10 +67,9 @@ attr_set_f(
 	int			argc,
 	char			**argv)
 {
-	struct xfs_inode	*ip = NULL;
-	struct xfs_da_args	args = { NULL };
-	char			*name, *value, *sp;
-	int			c, valuelen = 0;
+	struct xfs_da_args	args = { };
+	char			*sp;
+	int			c;
 
 	if (cur_typ == NULL) {
 		dbprintf(_("no current type\n"));
@@ -111,8 +110,9 @@ attr_set_f(
 
 		/* value length */
 		case 'v':
-			valuelen = (int)strtol(optarg, &sp, 0);
-			if (*sp != '\0' || valuelen < 0 || valuelen > 64*1024) {
+			args.valuelen = strtol(optarg, &sp, 0);
+			if (*sp != '\0' ||
+			    args.valuelen < 0 || args.valuelen > 64 * 1024) {
 				dbprintf(_("bad attr_set valuelen %s\n"), optarg);
 				return 0;
 			}
@@ -129,35 +129,29 @@ attr_set_f(
 		return 0;
 	}
 
-	name = argv[optind];
+	args.name = (const unsigned char *)argv[optind];
+	args.namelen = strlen(argv[optind]);
 
-	if (valuelen) {
-		value = (char *)memalign(getpagesize(), valuelen);
-		if (!value) {
-			dbprintf(_("cannot allocate buffer (%d)\n"), valuelen);
+	if (args.valuelen) {
+		args.value = memalign(getpagesize(), args.valuelen);
+		if (!args.value) {
+			dbprintf(_("cannot allocate buffer (%d)\n"),
+				args.valuelen);
 			goto out;
 		}
-		memset(value, 'v', valuelen);
-	} else {
-		value = NULL;
+		memset(args.value, 'v', args.valuelen);
 	}
 
-	if (libxfs_iget(mp, NULL, iocur_top->ino, 0, &ip,
+	if (libxfs_iget(mp, NULL, iocur_top->ino, 0, &args.dp,
 			&xfs_default_ifork_ops)) {
 		dbprintf(_("failed to iget inode %llu\n"),
 			(unsigned long long)iocur_top->ino);
 		goto out;
 	}
 
-	args.dp = ip;
-	args.name = (unsigned char *)name;
-	args.namelen = strlen(name);
-	args.value = value;
-	args.valuelen = valuelen;
-
 	if (libxfs_attr_set(&args)) {
 		dbprintf(_("failed to set attr %s on inode %llu\n"),
-			name, (unsigned long long)iocur_top->ino);
+			args.name, (unsigned long long)iocur_top->ino);
 		goto out;
 	}
 
@@ -166,10 +160,10 @@ attr_set_f(
 
 out:
 	mp->m_flags &= ~LIBXFS_MOUNT_COMPAT_ATTR;
-	if (ip)
-		libxfs_irele(ip);
-	if (value)
-		free(value);
+	if (args.dp)
+		libxfs_irele(args.dp);
+	if (args.value)
+		free(args.value);
 	return 0;
 }
 
@@ -178,9 +172,7 @@ attr_remove_f(
 	int			argc,
 	char			**argv)
 {
-	struct xfs_inode	*ip = NULL;
-	struct xfs_da_args	args = { NULL };
-	char			*name;
+	struct xfs_da_args	args = { };
 	int			c;
 
 	if (cur_typ == NULL) {
@@ -223,23 +215,20 @@ attr_remove_f(
 		return 0;
 	}
 
-	name = argv[optind];
+	args.name = (const unsigned char *)argv[optind];
+	args.namelen = strlen(argv[optind]);
 
-	if (libxfs_iget(mp, NULL, iocur_top->ino, 0, &ip,
+	if (libxfs_iget(mp, NULL, iocur_top->ino, 0, &args.dp,
 			&xfs_default_ifork_ops)) {
 		dbprintf(_("failed to iget inode %llu\n"),
 			(unsigned long long)iocur_top->ino);
 		goto out;
 	}
 
-	args.dp = ip;
-	args.name = (unsigned char *)name;
-	args.namelen = strlen(name);
-	args.value = NULL;
-
 	if (libxfs_attr_set(&args)) {
 		dbprintf(_("failed to remove attr %s from inode %llu\n"),
-			name, (unsigned long long)iocur_top->ino);
+			(unsigned char *)args.name,
+			(unsigned long long)iocur_top->ino);
 		goto out;
 	}
 
@@ -248,7 +237,7 @@ attr_remove_f(
 
 out:
 	mp->m_flags &= ~LIBXFS_MOUNT_COMPAT_ATTR;
-	if (ip)
-		libxfs_irele(ip);
+	if (args.dp)
+		libxfs_irele(args.dp);
 	return 0;
 }
-- 
2.26.2

[PATCH 5/8] db: validate name and namelen in attr_set_f and attr_remove_f

[Christoph Hellwig]

libxfs has stopped validating these parameters internally, so do it
in the xfs_db commands.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 db/attrset.c | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

diff --git a/db/attrset.c b/db/attrset.c
index 0a464983..e3575271 100644
--- a/db/attrset.c
+++ b/db/attrset.c
@@ -130,7 +130,16 @@ attr_set_f(
 	}
 
 	args.name = (const unsigned char *)argv[optind];
+	if (!args.name) {
+		dbprintf(_("invalid name\n"));
+		return 0;
+	}
+
 	args.namelen = strlen(argv[optind]);
+	if (args.namelen >= MAXNAMELEN) {
+		dbprintf(_("name too long\n"));
+		return 0;
+	}
 
 	if (args.valuelen) {
 		args.value = memalign(getpagesize(), args.valuelen);
@@ -216,7 +225,16 @@ attr_remove_f(
 	}
 
 	args.name = (const unsigned char *)argv[optind];
+	if (!args.name) {
+		dbprintf(_("invalid name\n"));
+		return 0;
+	}
+
 	args.namelen = strlen(argv[optind]);
+	if (args.namelen >= MAXNAMELEN) {
+		dbprintf(_("name too long\n"));
+		return 0;
+	}
 
 	if (libxfs_iget(mp, NULL, iocur_top->ino, 0, &args.dp,
 			&xfs_default_ifork_ops)) {
-- 
2.26.2

[PATCH 6/8] db: ensure that create and replace are exclusive in attr_set_f

[Christoph Hellwig]

Clear the other flag when applying the create or replace option,
as the low-level libxfs can't handle both at the same time.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 db/attrset.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/db/attrset.c b/db/attrset.c
index e3575271..b86ecec7 100644
--- a/db/attrset.c
+++ b/db/attrset.c
@@ -99,9 +99,11 @@ attr_set_f(
 		/* modifiers */
 		case 'C':
 			args.attr_flags |= XATTR_CREATE;
+			args.attr_flags &= ~XATTR_REPLACE;
 			break;
 		case 'R':
 			args.attr_flags |= XATTR_REPLACE;
+			args.attr_flags &= ~XATTR_CREATE;
 			break;
 
 		case 'n':
-- 
2.26.2

[PATCH 7/8] repair: cleanup build_agf_agfl

[Christoph Hellwig]

No need to have two variables for the AGFL block number array.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 repair/phase5.c | 7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/repair/phase5.c b/repair/phase5.c
index 17b57448..677297fe 100644
--- a/repair/phase5.c
+++ b/repair/phase5.c
@@ -2149,18 +2149,15 @@ build_agf_agfl(
 
 	/* setting to 0xff results in initialisation to NULLAGBLOCK */
 	memset(agfl, 0xff, mp->m_sb.sb_sectsize);
+	freelist = xfs_buf_to_agfl_bno(agfl_buf);
 	if (xfs_sb_version_hascrc(&mp->m_sb)) {
-		__be32 *agfl_bno = xfs_buf_to_agfl_bno(agfl_buf);
-
 		agfl->agfl_magicnum = cpu_to_be32(XFS_AGFL_MAGIC);
 		agfl->agfl_seqno = cpu_to_be32(agno);
 		platform_uuid_copy(&agfl->agfl_uuid, &mp->m_sb.sb_meta_uuid);
 		for (i = 0; i < libxfs_agfl_size(mp); i++)
-			agfl_bno[i] = cpu_to_be32(NULLAGBLOCK);
+			freelist[i] = cpu_to_be32(NULLAGBLOCK);
 	}
 
-	freelist = xfs_buf_to_agfl_bno(agfl_buf);
-
 	/*
 	 * do we have left-over blocks in the btree cursors that should
 	 * be used to fill the AGFL?
-- 
2.26.2

[PATCH 8/8] metadump: small cleanup for process_inode

[Christoph Hellwig]

Shorten a conditional to a single line.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 db/metadump.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/db/metadump.c b/db/metadump.c
index 14e7eaa7..e5cb3aa5 100644
--- a/db/metadump.c
+++ b/db/metadump.c
@@ -2415,8 +2415,7 @@ process_inode(
 	nametable_clear();
 
 	/* copy extended attributes if they exist and forkoff is valid */
-	if (success &&
-	    XFS_DFORK_DSIZE(dip, mp) < XFS_LITINO(mp)) {
+	if (success && XFS_DFORK_DSIZE(dip, mp) < XFS_LITINO(mp)) {
 		attr_data.remote_val_count = 0;
 		switch (dip->di_aformat) {
 			case XFS_DINODE_FMT_LOCAL:
-- 
2.26.2

Re: [PATCH 1/8] libxfs: use tabs instead of spaces in div_u64

[Darrick J. Wong]

On Sat, May 09, 2020 at 07:01:18PM +0200, Christoph Hellwig wrote:
> Signed-off-by: Christoph Hellwig <hch@lst.de>

Looks ok,
Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>

--D

> ---
>  libxfs/libxfs_priv.h | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/libxfs/libxfs_priv.h b/libxfs/libxfs_priv.h
> index 8d717d91..5688284d 100644
> --- a/libxfs/libxfs_priv.h
> +++ b/libxfs/libxfs_priv.h
> @@ -263,8 +263,8 @@ div_u64_rem(uint64_t dividend, uint32_t divisor, uint32_t *remainder)
>   */
>  static inline uint64_t div_u64(uint64_t dividend, uint32_t divisor)
>  {
> -        uint32_t remainder;
> -        return div_u64_rem(dividend, divisor, &remainder);
> +	uint32_t remainder;
> +	return div_u64_rem(dividend, divisor, &remainder);
>  }
>  
>  /**
> -- 
> 2.26.2
> 

Re: [PATCH 2/8] db: fix a comment in scan_freelist

[Darrick J. Wong]

On Sat, May 09, 2020 at 07:01:19PM +0200, Christoph Hellwig wrote:
> XFS_BUF_TO_AGFL_BNO has been renamed to open coded xfs_buf_to_agfl_bno.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>

LGTM
Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>

--D

> ---
>  db/check.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/db/check.c b/db/check.c
> index c9bafa8e..09f8f6c9 100644
> --- a/db/check.c
> +++ b/db/check.c
> @@ -4075,7 +4075,7 @@ scan_freelist(
>  		return;
>  	}
>  
> -	/* open coded XFS_BUF_TO_AGFL_BNO */
> +	/* open coded xfs_buf_to_agfl_bno */
>  	state.count = 0;
>  	state.agno = seqno;
>  	libxfs_agfl_walk(mp, agf, iocur_top->bp, scan_agfl, &state);
> -- 
> 2.26.2
> 

Re: [PATCH 3/8] db: add a comment to agfl_crc_flds

[Darrick J. Wong]

On Sat, May 09, 2020 at 07:01:20PM +0200, Christoph Hellwig wrote:
> Explain the bno field that is not actually part of the structure
> anymore.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>  db/agfl.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/db/agfl.c b/db/agfl.c
> index 45e4d6f9..ce7a2548 100644
> --- a/db/agfl.c
> +++ b/db/agfl.c
> @@ -47,6 +47,7 @@ const field_t	agfl_crc_flds[] = {
>  	{ "uuid", FLDT_UUID, OI(OFF(uuid)), C1, 0, TYP_NONE },
>  	{ "lsn", FLDT_UINT64X, OI(OFF(lsn)), C1, 0, TYP_NONE },
>  	{ "crc", FLDT_CRC, OI(OFF(crc)), C1, 0, TYP_NONE },
> +	/* the bno array really is behind the actual structure */

Er... the bno array comes /after/ the actual structure, right?

--D

>  	{ "bno", FLDT_AGBLOCKNZ, OI(bitize(sizeof(struct xfs_agfl))),
>  	  agfl_bno_size, FLD_ARRAY|FLD_COUNT, TYP_DATA },
>  	{ NULL }
> -- 
> 2.26.2
> 

Re: [PATCH 4/8] db: cleanup attr_set_f and attr_remove_f

[Darrick J. Wong]

On Sat, May 09, 2020 at 07:01:21PM +0200, Christoph Hellwig wrote:
> Don't use local variables for information that is set in the da_args
> structure.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>

Seems like a good cleanup,
Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>

--D

> ---
>  db/attrset.c | 67 ++++++++++++++++++++++------------------------------
>  1 file changed, 28 insertions(+), 39 deletions(-)
> 
> diff --git a/db/attrset.c b/db/attrset.c
> index 1ff2eb85..0a464983 100644
> --- a/db/attrset.c
> +++ b/db/attrset.c
> @@ -67,10 +67,9 @@ attr_set_f(
>  	int			argc,
>  	char			**argv)
>  {
> -	struct xfs_inode	*ip = NULL;
> -	struct xfs_da_args	args = { NULL };
> -	char			*name, *value, *sp;
> -	int			c, valuelen = 0;
> +	struct xfs_da_args	args = { };
> +	char			*sp;
> +	int			c;
>  
>  	if (cur_typ == NULL) {
>  		dbprintf(_("no current type\n"));
> @@ -111,8 +110,9 @@ attr_set_f(
>  
>  		/* value length */
>  		case 'v':
> -			valuelen = (int)strtol(optarg, &sp, 0);
> -			if (*sp != '\0' || valuelen < 0 || valuelen > 64*1024) {
> +			args.valuelen = strtol(optarg, &sp, 0);
> +			if (*sp != '\0' ||
> +			    args.valuelen < 0 || args.valuelen > 64 * 1024) {
>  				dbprintf(_("bad attr_set valuelen %s\n"), optarg);
>  				return 0;
>  			}
> @@ -129,35 +129,29 @@ attr_set_f(
>  		return 0;
>  	}
>  
> -	name = argv[optind];
> +	args.name = (const unsigned char *)argv[optind];
> +	args.namelen = strlen(argv[optind]);
>  
> -	if (valuelen) {
> -		value = (char *)memalign(getpagesize(), valuelen);
> -		if (!value) {
> -			dbprintf(_("cannot allocate buffer (%d)\n"), valuelen);
> +	if (args.valuelen) {
> +		args.value = memalign(getpagesize(), args.valuelen);
> +		if (!args.value) {
> +			dbprintf(_("cannot allocate buffer (%d)\n"),
> +				args.valuelen);
>  			goto out;
>  		}
> -		memset(value, 'v', valuelen);
> -	} else {
> -		value = NULL;
> +		memset(args.value, 'v', args.valuelen);
>  	}
>  
> -	if (libxfs_iget(mp, NULL, iocur_top->ino, 0, &ip,
> +	if (libxfs_iget(mp, NULL, iocur_top->ino, 0, &args.dp,
>  			&xfs_default_ifork_ops)) {
>  		dbprintf(_("failed to iget inode %llu\n"),
>  			(unsigned long long)iocur_top->ino);
>  		goto out;
>  	}
>  
> -	args.dp = ip;
> -	args.name = (unsigned char *)name;
> -	args.namelen = strlen(name);
> -	args.value = value;
> -	args.valuelen = valuelen;
> -
>  	if (libxfs_attr_set(&args)) {
>  		dbprintf(_("failed to set attr %s on inode %llu\n"),
> -			name, (unsigned long long)iocur_top->ino);
> +			args.name, (unsigned long long)iocur_top->ino);
>  		goto out;
>  	}
>  
> @@ -166,10 +160,10 @@ attr_set_f(
>  
>  out:
>  	mp->m_flags &= ~LIBXFS_MOUNT_COMPAT_ATTR;
> -	if (ip)
> -		libxfs_irele(ip);
> -	if (value)
> -		free(value);
> +	if (args.dp)
> +		libxfs_irele(args.dp);
> +	if (args.value)
> +		free(args.value);
>  	return 0;
>  }
>  
> @@ -178,9 +172,7 @@ attr_remove_f(
>  	int			argc,
>  	char			**argv)
>  {
> -	struct xfs_inode	*ip = NULL;
> -	struct xfs_da_args	args = { NULL };
> -	char			*name;
> +	struct xfs_da_args	args = { };
>  	int			c;
>  
>  	if (cur_typ == NULL) {
> @@ -223,23 +215,20 @@ attr_remove_f(
>  		return 0;
>  	}
>  
> -	name = argv[optind];
> +	args.name = (const unsigned char *)argv[optind];
> +	args.namelen = strlen(argv[optind]);
>  
> -	if (libxfs_iget(mp, NULL, iocur_top->ino, 0, &ip,
> +	if (libxfs_iget(mp, NULL, iocur_top->ino, 0, &args.dp,
>  			&xfs_default_ifork_ops)) {
>  		dbprintf(_("failed to iget inode %llu\n"),
>  			(unsigned long long)iocur_top->ino);
>  		goto out;
>  	}
>  
> -	args.dp = ip;
> -	args.name = (unsigned char *)name;
> -	args.namelen = strlen(name);
> -	args.value = NULL;
> -
>  	if (libxfs_attr_set(&args)) {
>  		dbprintf(_("failed to remove attr %s from inode %llu\n"),
> -			name, (unsigned long long)iocur_top->ino);
> +			(unsigned char *)args.name,
> +			(unsigned long long)iocur_top->ino);
>  		goto out;
>  	}
>  
> @@ -248,7 +237,7 @@ attr_remove_f(
>  
>  out:
>  	mp->m_flags &= ~LIBXFS_MOUNT_COMPAT_ATTR;
> -	if (ip)
> -		libxfs_irele(ip);
> +	if (args.dp)
> +		libxfs_irele(args.dp);
>  	return 0;
>  }
> -- 
> 2.26.2
> 

Re: [PATCH 1/8] libxfs: use tabs instead of spaces in div_u64

[Eric Sandeen]

On 5/9/20 12:01 PM, Christoph Hellwig wrote:
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---

oh that's embarrassing....  Thanks for this and the other fixes,
I had meant to go through the differences but tough to keep up
with you. ;)

-Eric

>  libxfs/libxfs_priv.h | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/libxfs/libxfs_priv.h b/libxfs/libxfs_priv.h
> index 8d717d91..5688284d 100644
> --- a/libxfs/libxfs_priv.h
> +++ b/libxfs/libxfs_priv.h
> @@ -263,8 +263,8 @@ div_u64_rem(uint64_t dividend, uint32_t divisor, uint32_t *remainder)
>   */
>  static inline uint64_t div_u64(uint64_t dividend, uint32_t divisor)
>  {
> -        uint32_t remainder;
> -        return div_u64_rem(dividend, divisor, &remainder);
> +	uint32_t remainder;
> +	return div_u64_rem(dividend, divisor, &remainder);
>  }
>  
>  /**
> 

Re: [PATCH 5/8] db: validate name and namelen in attr_set_f and attr_remove_f

[Darrick J. Wong]

On Sat, May 09, 2020 at 07:01:22PM +0200, Christoph Hellwig wrote:
> libxfs has stopped validating these parameters internally, so do it
> in the xfs_db commands.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>

IIRC the VFS checks these parameters so that libxfs doesn't have to,
right?

If so,
Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>

--D

> ---
>  db/attrset.c | 18 ++++++++++++++++++
>  1 file changed, 18 insertions(+)
> 
> diff --git a/db/attrset.c b/db/attrset.c
> index 0a464983..e3575271 100644
> --- a/db/attrset.c
> +++ b/db/attrset.c
> @@ -130,7 +130,16 @@ attr_set_f(
>  	}
>  
>  	args.name = (const unsigned char *)argv[optind];
> +	if (!args.name) {
> +		dbprintf(_("invalid name\n"));
> +		return 0;
> +	}
> +
>  	args.namelen = strlen(argv[optind]);
> +	if (args.namelen >= MAXNAMELEN) {
> +		dbprintf(_("name too long\n"));
> +		return 0;
> +	}
>  
>  	if (args.valuelen) {
>  		args.value = memalign(getpagesize(), args.valuelen);
> @@ -216,7 +225,16 @@ attr_remove_f(
>  	}
>  
>  	args.name = (const unsigned char *)argv[optind];
> +	if (!args.name) {
> +		dbprintf(_("invalid name\n"));
> +		return 0;
> +	}
> +
>  	args.namelen = strlen(argv[optind]);
> +	if (args.namelen >= MAXNAMELEN) {
> +		dbprintf(_("name too long\n"));
> +		return 0;
> +	}
>  
>  	if (libxfs_iget(mp, NULL, iocur_top->ino, 0, &args.dp,
>  			&xfs_default_ifork_ops)) {
> -- 
> 2.26.2
> 

Re: [PATCH 6/8] db: ensure that create and replace are exclusive in attr_set_f

[Darrick J. Wong]

On Sat, May 09, 2020 at 07:01:23PM +0200, Christoph Hellwig wrote:
> Clear the other flag when applying the create or replace option,
> as the low-level libxfs can't handle both at the same time.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>

Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>

--D

> ---
>  db/attrset.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/db/attrset.c b/db/attrset.c
> index e3575271..b86ecec7 100644
> --- a/db/attrset.c
> +++ b/db/attrset.c
> @@ -99,9 +99,11 @@ attr_set_f(
>  		/* modifiers */
>  		case 'C':
>  			args.attr_flags |= XATTR_CREATE;
> +			args.attr_flags &= ~XATTR_REPLACE;
>  			break;
>  		case 'R':
>  			args.attr_flags |= XATTR_REPLACE;
> +			args.attr_flags &= ~XATTR_CREATE;
>  			break;
>  
>  		case 'n':
> -- 
> 2.26.2
> 

Re: [PATCH 3/8] db: add a comment to agfl_crc_flds

[Christoph Hellwig]

On Sat, May 09, 2020 at 10:07:12AM -0700, Darrick J. Wong wrote:
> On Sat, May 09, 2020 at 07:01:20PM +0200, Christoph Hellwig wrote:
> > Explain the bno field that is not actually part of the structure
> > anymore.
> > 
> > Signed-off-by: Christoph Hellwig <hch@lst.de>
> > ---
> >  db/agfl.c | 1 +
> >  1 file changed, 1 insertion(+)
> > 
> > diff --git a/db/agfl.c b/db/agfl.c
> > index 45e4d6f9..ce7a2548 100644
> > --- a/db/agfl.c
> > +++ b/db/agfl.c
> > @@ -47,6 +47,7 @@ const field_t	agfl_crc_flds[] = {
> >  	{ "uuid", FLDT_UUID, OI(OFF(uuid)), C1, 0, TYP_NONE },
> >  	{ "lsn", FLDT_UINT64X, OI(OFF(lsn)), C1, 0, TYP_NONE },
> >  	{ "crc", FLDT_CRC, OI(OFF(crc)), C1, 0, TYP_NONE },
> > +	/* the bno array really is behind the actual structure */
> 
> Er... the bno array comes /after/ the actual structure, right?

Yes.  That's what I mean, but after seems to be less confusing.

Re: [PATCH 7/8] repair: cleanup build_agf_agfl

[Darrick J. Wong]

On Sat, May 09, 2020 at 07:01:24PM +0200, Christoph Hellwig wrote:
> No need to have two variables for the AGFL block number array.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>  repair/phase5.c | 7 ++-----
>  1 file changed, 2 insertions(+), 5 deletions(-)
> 
> diff --git a/repair/phase5.c b/repair/phase5.c
> index 17b57448..677297fe 100644
> --- a/repair/phase5.c
> +++ b/repair/phase5.c
> @@ -2149,18 +2149,15 @@ build_agf_agfl(
>  
>  	/* setting to 0xff results in initialisation to NULLAGBLOCK */
>  	memset(agfl, 0xff, mp->m_sb.sb_sectsize);

/me wonders why this memset isn't sufficient to null out the freelist,
but a better cleanup would be to rip all this out in favor of adapting
the nearly identical functions in xfs_ag.c.

In the meantime we don't need duplicate variables, and:

Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>

--D

> +	freelist = xfs_buf_to_agfl_bno(agfl_buf);
>  	if (xfs_sb_version_hascrc(&mp->m_sb)) {
> -		__be32 *agfl_bno = xfs_buf_to_agfl_bno(agfl_buf);
> -
>  		agfl->agfl_magicnum = cpu_to_be32(XFS_AGFL_MAGIC);
>  		agfl->agfl_seqno = cpu_to_be32(agno);
>  		platform_uuid_copy(&agfl->agfl_uuid, &mp->m_sb.sb_meta_uuid);
>  		for (i = 0; i < libxfs_agfl_size(mp); i++)
> -			agfl_bno[i] = cpu_to_be32(NULLAGBLOCK);
> +			freelist[i] = cpu_to_be32(NULLAGBLOCK);
>  	}
>  
> -	freelist = xfs_buf_to_agfl_bno(agfl_buf);
> -
>  	/*
>  	 * do we have left-over blocks in the btree cursors that should
>  	 * be used to fill the AGFL?
> -- 
> 2.26.2
> 

Re: [PATCH 8/8] metadump: small cleanup for process_inode

[Darrick J. Wong]

On Sat, May 09, 2020 at 07:01:25PM +0200, Christoph Hellwig wrote:
> Shorten a conditional to a single line.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>

Looks good,
Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>

--D

> ---
>  db/metadump.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/db/metadump.c b/db/metadump.c
> index 14e7eaa7..e5cb3aa5 100644
> --- a/db/metadump.c
> +++ b/db/metadump.c
> @@ -2415,8 +2415,7 @@ process_inode(
>  	nametable_clear();
>  
>  	/* copy extended attributes if they exist and forkoff is valid */
> -	if (success &&
> -	    XFS_DFORK_DSIZE(dip, mp) < XFS_LITINO(mp)) {
> +	if (success && XFS_DFORK_DSIZE(dip, mp) < XFS_LITINO(mp)) {
>  		attr_data.remote_val_count = 0;
>  		switch (dip->di_aformat) {
>  			case XFS_DINODE_FMT_LOCAL:
> -- 
> 2.26.2
> 

Re: [PATCH 5/8] db: validate name and namelen in attr_set_f and attr_remove_f

[Christoph Hellwig]

On Sat, May 09, 2020 at 10:09:09AM -0700, Darrick J. Wong wrote:
> On Sat, May 09, 2020 at 07:01:22PM +0200, Christoph Hellwig wrote:
> > libxfs has stopped validating these parameters internally, so do it
> > in the xfs_db commands.
> > 
> > Signed-off-by: Christoph Hellwig <hch@lst.de>
> 
> IIRC the VFS checks these parameters so that libxfs doesn't have to,
> right?

Yes.  But we don't have the VFS in xfsprogs :)

Re: [PATCH 4/8] db: cleanup attr_set_f and attr_remove_f

[Eric Sandeen]

On 5/9/20 12:01 PM, Christoph Hellwig wrote:
> Don't use local variables for information that is set in the da_args
> structure.

I'm on the fence about this one; Darrick had missed setting a couple
of necessary structure members, so I actually see some value in assigning them
all right before we call into libxfs_attr_set .... it makes it very clear what's
being sent in to libxfs_attr_set.

-Eric

> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>  db/attrset.c | 67 ++++++++++++++++++++++------------------------------
>  1 file changed, 28 insertions(+), 39 deletions(-)
> 
> diff --git a/db/attrset.c b/db/attrset.c
> index 1ff2eb85..0a464983 100644
> --- a/db/attrset.c
> +++ b/db/attrset.c
> @@ -67,10 +67,9 @@ attr_set_f(
>  	int			argc,
>  	char			**argv)
>  {
> -	struct xfs_inode	*ip = NULL;
> -	struct xfs_da_args	args = { NULL };
> -	char			*name, *value, *sp;
> -	int			c, valuelen = 0;
> +	struct xfs_da_args	args = { };
> +	char			*sp;
> +	int			c;
>  
>  	if (cur_typ == NULL) {
>  		dbprintf(_("no current type\n"));
> @@ -111,8 +110,9 @@ attr_set_f(
>  
>  		/* value length */
>  		case 'v':
> -			valuelen = (int)strtol(optarg, &sp, 0);
> -			if (*sp != '\0' || valuelen < 0 || valuelen > 64*1024) {
> +			args.valuelen = strtol(optarg, &sp, 0);
> +			if (*sp != '\0' ||
> +			    args.valuelen < 0 || args.valuelen > 64 * 1024) {
>  				dbprintf(_("bad attr_set valuelen %s\n"), optarg);
>  				return 0;
>  			}
> @@ -129,35 +129,29 @@ attr_set_f(
>  		return 0;
>  	}
>  
> -	name = argv[optind];
> +	args.name = (const unsigned char *)argv[optind];
> +	args.namelen = strlen(argv[optind]);
>  
> -	if (valuelen) {
> -		value = (char *)memalign(getpagesize(), valuelen);
> -		if (!value) {
> -			dbprintf(_("cannot allocate buffer (%d)\n"), valuelen);
> +	if (args.valuelen) {
> +		args.value = memalign(getpagesize(), args.valuelen);
> +		if (!args.value) {
> +			dbprintf(_("cannot allocate buffer (%d)\n"),
> +				args.valuelen);
>  			goto out;
>  		}
> -		memset(value, 'v', valuelen);
> -	} else {
> -		value = NULL;
> +		memset(args.value, 'v', args.valuelen);
>  	}
>  
> -	if (libxfs_iget(mp, NULL, iocur_top->ino, 0, &ip,
> +	if (libxfs_iget(mp, NULL, iocur_top->ino, 0, &args.dp,
>  			&xfs_default_ifork_ops)) {
>  		dbprintf(_("failed to iget inode %llu\n"),
>  			(unsigned long long)iocur_top->ino);
>  		goto out;
>  	}
>  
> -	args.dp = ip;
> -	args.name = (unsigned char *)name;
> -	args.namelen = strlen(name);
> -	args.value = value;
> -	args.valuelen = valuelen;
> -
>  	if (libxfs_attr_set(&args)) {
>  		dbprintf(_("failed to set attr %s on inode %llu\n"),
> -			name, (unsigned long long)iocur_top->ino);
> +			args.name, (unsigned long long)iocur_top->ino);
>  		goto out;
>  	}
>  
> @@ -166,10 +160,10 @@ attr_set_f(
>  
>  out:
>  	mp->m_flags &= ~LIBXFS_MOUNT_COMPAT_ATTR;
> -	if (ip)
> -		libxfs_irele(ip);
> -	if (value)
> -		free(value);
> +	if (args.dp)
> +		libxfs_irele(args.dp);
> +	if (args.value)
> +		free(args.value);
>  	return 0;
>  }
>  
> @@ -178,9 +172,7 @@ attr_remove_f(
>  	int			argc,
>  	char			**argv)
>  {
> -	struct xfs_inode	*ip = NULL;
> -	struct xfs_da_args	args = { NULL };
> -	char			*name;
> +	struct xfs_da_args	args = { };
>  	int			c;
>  
>  	if (cur_typ == NULL) {
> @@ -223,23 +215,20 @@ attr_remove_f(
>  		return 0;
>  	}
>  
> -	name = argv[optind];
> +	args.name = (const unsigned char *)argv[optind];
> +	args.namelen = strlen(argv[optind]);
>  
> -	if (libxfs_iget(mp, NULL, iocur_top->ino, 0, &ip,
> +	if (libxfs_iget(mp, NULL, iocur_top->ino, 0, &args.dp,
>  			&xfs_default_ifork_ops)) {
>  		dbprintf(_("failed to iget inode %llu\n"),
>  			(unsigned long long)iocur_top->ino);
>  		goto out;
>  	}
>  
> -	args.dp = ip;
> -	args.name = (unsigned char *)name;
> -	args.namelen = strlen(name);
> -	args.value = NULL;
> -
>  	if (libxfs_attr_set(&args)) {
>  		dbprintf(_("failed to remove attr %s from inode %llu\n"),
> -			name, (unsigned long long)iocur_top->ino);
> +			(unsigned char *)args.name,
> +			(unsigned long long)iocur_top->ino);
>  		goto out;
>  	}
>  
> @@ -248,7 +237,7 @@ attr_remove_f(
>  
>  out:
>  	mp->m_flags &= ~LIBXFS_MOUNT_COMPAT_ATTR;
> -	if (ip)
> -		libxfs_irele(ip);
> +	if (args.dp)
> +		libxfs_irele(args.dp);
>  	return 0;
>  }
> 

Re: [PATCH 3/8] db: add a comment to agfl_crc_flds

[Eric Sandeen]

On 5/9/20 12:10 PM, Christoph Hellwig wrote:
> On Sat, May 09, 2020 at 10:07:12AM -0700, Darrick J. Wong wrote:
>> On Sat, May 09, 2020 at 07:01:20PM +0200, Christoph Hellwig wrote:
>>> Explain the bno field that is not actually part of the structure
>>> anymore.
>>>
>>> Signed-off-by: Christoph Hellwig <hch@lst.de>
>>> ---
>>>  db/agfl.c | 1 +
>>>  1 file changed, 1 insertion(+)
>>>
>>> diff --git a/db/agfl.c b/db/agfl.c
>>> index 45e4d6f9..ce7a2548 100644
>>> --- a/db/agfl.c
>>> +++ b/db/agfl.c
>>> @@ -47,6 +47,7 @@ const field_t	agfl_crc_flds[] = {
>>>  	{ "uuid", FLDT_UUID, OI(OFF(uuid)), C1, 0, TYP_NONE },
>>>  	{ "lsn", FLDT_UINT64X, OI(OFF(lsn)), C1, 0, TYP_NONE },
>>>  	{ "crc", FLDT_CRC, OI(OFF(crc)), C1, 0, TYP_NONE },
>>> +	/* the bno array really is behind the actual structure */
>>
>> Er... the bno array comes /after/ the actual structure, right?
> 
> Yes.  That's what I mean, but after seems to be less confusing.
so:

/* the bno array is after the actual structure */

right?  I can just do that on merge.

Re: [PATCH 7/8] repair: cleanup build_agf_agfl

[Eric Sandeen]

On 5/9/20 12:11 PM, Darrick J. Wong wrote:
> On Sat, May 09, 2020 at 07:01:24PM +0200, Christoph Hellwig wrote:
>> No need to have two variables for the AGFL block number array.
>>
>> Signed-off-by: Christoph Hellwig <hch@lst.de>
>> ---
>>  repair/phase5.c | 7 ++-----
>>  1 file changed, 2 insertions(+), 5 deletions(-)
>>
>> diff --git a/repair/phase5.c b/repair/phase5.c
>> index 17b57448..677297fe 100644
>> --- a/repair/phase5.c
>> +++ b/repair/phase5.c
>> @@ -2149,18 +2149,15 @@ build_agf_agfl(
>>  
>>  	/* setting to 0xff results in initialisation to NULLAGBLOCK */
>>  	memset(agfl, 0xff, mp->m_sb.sb_sectsize);
> 
> /me wonders why this memset isn't sufficient to null out the freelist,
> but a better cleanup would be to rip all this out in favor of adapting
> the nearly identical functions in xfs_ag.c.

probably because xfs_agflblock_init is

a) static and
b) expects a aghdr_init_data *id arg which isn't too convenient here I guess

Might be nice to factor xfs_agflblock_init to call a helper that this and
build_agf_agfl can both use though, I'll give that a whirl.

-Eric

> In the meantime we don't need duplicate variables, and:
> 
> Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>
> 
> --D
> 
>> +	freelist = xfs_buf_to_agfl_bno(agfl_buf);
>>  	if (xfs_sb_version_hascrc(&mp->m_sb)) {
>> -		__be32 *agfl_bno = xfs_buf_to_agfl_bno(agfl_buf);
>> -
>>  		agfl->agfl_magicnum = cpu_to_be32(XFS_AGFL_MAGIC);
>>  		agfl->agfl_seqno = cpu_to_be32(agno);
>>  		platform_uuid_copy(&agfl->agfl_uuid, &mp->m_sb.sb_meta_uuid);
>>  		for (i = 0; i < libxfs_agfl_size(mp); i++)
>> -			agfl_bno[i] = cpu_to_be32(NULLAGBLOCK);
>> +			freelist[i] = cpu_to_be32(NULLAGBLOCK);
>>  	}
>>  
>> -	freelist = xfs_buf_to_agfl_bno(agfl_buf);
>> -
>>  	/*
>>  	 * do we have left-over blocks in the btree cursors that should
>>  	 * be used to fill the AGFL?
>> -- 
>> 2.26.2
>>
> 

Re: misc xfsprogs cleanups after the 5.7 merge

[Eric Sandeen]

On 5/9/20 12:01 PM, Christoph Hellwig wrote:
> Hi Eric,
> 
> this series contains the differences to my port that seem useful
> to keep.

I staged all of these except:

11538453 New          [4/8] db: cleanup attr_set_f and attr_remove_f
11538455 New          [5/8] db: validate name and namelen in attr_set_f and attr_remove_f
11538457 New          [6/8] db: ensure that create and replace are exclusive in attr_set_f

because i'm not sure about patch 4/8; because the structure bits that need
initialization differ a bit between set and remove, and because the
initial backport missed a couple initializations, I feel like initializing
the structure just before the libxfs call might be more obvious in terms of
what's getting passed in.

(however, if we keep that I should make the attr_filter variable work the same way)

patches 5 and 6 just depend on the decision re: patch 4.

thanks,
-Eric

Re: [PATCH 3/8] db: add a comment to agfl_crc_flds

[Darrick J. Wong]

On Sat, May 09, 2020 at 12:32:01PM -0500, Eric Sandeen wrote:
> On 5/9/20 12:10 PM, Christoph Hellwig wrote:
> > On Sat, May 09, 2020 at 10:07:12AM -0700, Darrick J. Wong wrote:
> >> On Sat, May 09, 2020 at 07:01:20PM +0200, Christoph Hellwig wrote:
> >>> Explain the bno field that is not actually part of the structure
> >>> anymore.
> >>>
> >>> Signed-off-by: Christoph Hellwig <hch@lst.de>
> >>> ---
> >>>  db/agfl.c | 1 +
> >>>  1 file changed, 1 insertion(+)
> >>>
> >>> diff --git a/db/agfl.c b/db/agfl.c
> >>> index 45e4d6f9..ce7a2548 100644
> >>> --- a/db/agfl.c
> >>> +++ b/db/agfl.c
> >>> @@ -47,6 +47,7 @@ const field_t	agfl_crc_flds[] = {
> >>>  	{ "uuid", FLDT_UUID, OI(OFF(uuid)), C1, 0, TYP_NONE },
> >>>  	{ "lsn", FLDT_UINT64X, OI(OFF(lsn)), C1, 0, TYP_NONE },
> >>>  	{ "crc", FLDT_CRC, OI(OFF(crc)), C1, 0, TYP_NONE },
> >>> +	/* the bno array really is behind the actual structure */
> >>
> >> Er... the bno array comes /after/ the actual structure, right?
> > 
> > Yes.  That's what I mean, but after seems to be less confusing.
> so:
> 
> /* the bno array is after the actual structure */
> 
> right?  I can just do that on merge.
> 

With that changed,
Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>

--D

Re: [PATCH 4/8] db: cleanup attr_set_f and attr_remove_f

[Christoph Hellwig]

On Sat, May 09, 2020 at 12:23:42PM -0500, Eric Sandeen wrote:
> On 5/9/20 12:01 PM, Christoph Hellwig wrote:
> > Don't use local variables for information that is set in the da_args
> > structure.
> 
> I'm on the fence about this one; Darrick had missed setting a couple
> of necessary structure members, so I actually see some value in assigning them
> all right before we call into libxfs_attr_set .... it makes it very clear what's
> being sent in to libxfs_attr_set.

But using additional local variables doesn't help with initialing
the fields, it actually makes it easier to miss, which I guess is
what happened.  I find the code much easier to verify without the
extra variables.

Re: [PATCH 3/8] db: add a comment to agfl_crc_flds

[Christoph Hellwig]

On Sat, May 09, 2020 at 12:32:01PM -0500, Eric Sandeen wrote:
> > Yes.  That's what I mean, but after seems to be less confusing.
> so:
> 
> /* the bno array is after the actual structure */
> 
> right?  I can just do that on merge.

Looks ok.

Re: [PATCH 4/8] db: cleanup attr_set_f and attr_remove_f

[Eric Sandeen]

On 5/10/20 2:11 AM, Christoph Hellwig wrote:
> On Sat, May 09, 2020 at 12:23:42PM -0500, Eric Sandeen wrote:
>> On 5/9/20 12:01 PM, Christoph Hellwig wrote:
>>> Don't use local variables for information that is set in the da_args
>>> structure.
>>
>> I'm on the fence about this one; Darrick had missed setting a couple
>> of necessary structure members, so I actually see some value in assigning them
>> all right before we call into libxfs_attr_set .... it makes it very clear what's
>> being sent in to libxfs_attr_set.
> 
> But using additional local variables doesn't help with initialing
> the fields, it actually makes it easier to miss, which I guess is
> what happened.  I find the code much easier to verify without the
> extra variables.

They seem a bit extraneous, but my problem is I can't keep track of how much
of the args structure is actually filled out when it's spread out over dozens
of lines ....  

*shrug* I dunno. Maybe darrick can cast the tie-breaking vote.  ;)

Re: [PATCH 4/8] db: cleanup attr_set_f and attr_remove_f

[Darrick J. Wong]

On Sun, May 10, 2020 at 09:09:14AM -0500, Eric Sandeen wrote:
> On 5/10/20 2:11 AM, Christoph Hellwig wrote:
> > On Sat, May 09, 2020 at 12:23:42PM -0500, Eric Sandeen wrote:
> >> On 5/9/20 12:01 PM, Christoph Hellwig wrote:
> >>> Don't use local variables for information that is set in the da_args
> >>> structure.
> >>
> >> I'm on the fence about this one; Darrick had missed setting a couple
> >> of necessary structure members, so I actually see some value in assigning them
> >> all right before we call into libxfs_attr_set .... it makes it very clear what's
> >> being sent in to libxfs_attr_set.
> > 
> > But using additional local variables doesn't help with initialing
> > the fields, it actually makes it easier to miss, which I guess is
> > what happened.  I find the code much easier to verify without the
> > extra variables.
> 
> They seem a bit extraneous, but my problem is I can't keep track of how much
> of the args structure is actually filled out when it's spread out over dozens
> of lines ....  
> 
> *shrug* I dunno. Maybe darrick can cast the tie-breaking vote.  ;)

I mean... I /did/ already RVB this one... :)


--D

Re: [PATCH 4/8] db: cleanup attr_set_f and attr_remove_f

[Eric Sandeen]

On 5/10/20 10:41 PM, Darrick J. Wong wrote:
> On Sun, May 10, 2020 at 09:09:14AM -0500, Eric Sandeen wrote:
>> On 5/10/20 2:11 AM, Christoph Hellwig wrote:
>>> On Sat, May 09, 2020 at 12:23:42PM -0500, Eric Sandeen wrote:
>>>> On 5/9/20 12:01 PM, Christoph Hellwig wrote:
>>>>> Don't use local variables for information that is set in the da_args
>>>>> structure.
>>>>
>>>> I'm on the fence about this one; Darrick had missed setting a couple
>>>> of necessary structure members, so I actually see some value in assigning them
>>>> all right before we call into libxfs_attr_set .... it makes it very clear what's
>>>> being sent in to libxfs_attr_set.
>>>
>>> But using additional local variables doesn't help with initialing
>>> the fields, it actually makes it easier to miss, which I guess is
>>> what happened.  I find the code much easier to verify without the
>>> extra variables.
>>
>> They seem a bit extraneous, but my problem is I can't keep track of how much
>> of the args structure is actually filled out when it's spread out over dozens
>> of lines ....  
>>
>> *shrug* I dunno. Maybe darrick can cast the tie-breaking vote.  ;)
> 
> I mean... I /did/ already RVB this one... :)

Before I raised the question, but *shrug* ok, I guess nobody else sees it my way
so I'll merge it as is, not worth haggling over any further.

thanks for all the cleanups, Christoph.

-Eric