All of lore.kernel.org
 help / color / mirror / Atom feed
* misc xfsprogs cleanups after the 5.7 merge
@ 2020-05-09 17:01 Christoph Hellwig
  2020-05-09 17:01 ` [PATCH 1/8] libxfs: use tabs instead of spaces in div_u64 Christoph Hellwig
                   ` (8 more replies)
  0 siblings, 9 replies; 30+ messages in thread
From: Christoph Hellwig @ 2020-05-09 17:01 UTC (permalink / raw)
  To: sandeen; +Cc: linux-xfs

Hi Eric,

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

^ permalink raw reply	[flat|nested] 30+ messages in thread

* [PATCH 1/8] libxfs: use tabs instead of spaces in div_u64
  2020-05-09 17:01 misc xfsprogs cleanups after the 5.7 merge Christoph Hellwig
@ 2020-05-09 17:01 ` Christoph Hellwig
  2020-05-09 17:06   ` Darrick J. Wong
  2020-05-09 17:08   ` Eric Sandeen
  2020-05-09 17:01 ` [PATCH 2/8] db: fix a comment in scan_freelist Christoph Hellwig
                   ` (7 subsequent siblings)
  8 siblings, 2 replies; 30+ messages in thread
From: Christoph Hellwig @ 2020-05-09 17:01 UTC (permalink / raw)
  To: sandeen; +Cc: linux-xfs

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


^ permalink raw reply	[flat|nested] 30+ messages in thread

* [PATCH 2/8] db: fix a comment in scan_freelist
  2020-05-09 17:01 misc xfsprogs cleanups after the 5.7 merge Christoph Hellwig
  2020-05-09 17:01 ` [PATCH 1/8] libxfs: use tabs instead of spaces in div_u64 Christoph Hellwig
@ 2020-05-09 17:01 ` Christoph Hellwig
  2020-05-09 17:06   ` Darrick J. Wong
  2020-05-09 17:01 ` [PATCH 3/8] db: add a comment to agfl_crc_flds Christoph Hellwig
                   ` (6 subsequent siblings)
  8 siblings, 1 reply; 30+ messages in thread
From: Christoph Hellwig @ 2020-05-09 17:01 UTC (permalink / raw)
  To: sandeen; +Cc: linux-xfs

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


^ permalink raw reply	[flat|nested] 30+ messages in thread

* [PATCH 3/8] db: add a comment to agfl_crc_flds
  2020-05-09 17:01 misc xfsprogs cleanups after the 5.7 merge Christoph Hellwig
  2020-05-09 17:01 ` [PATCH 1/8] libxfs: use tabs instead of spaces in div_u64 Christoph Hellwig
  2020-05-09 17:01 ` [PATCH 2/8] db: fix a comment in scan_freelist Christoph Hellwig
@ 2020-05-09 17:01 ` Christoph Hellwig
  2020-05-09 17:07   ` Darrick J. Wong
  2020-05-09 17:01 ` [PATCH 4/8] db: cleanup attr_set_f and attr_remove_f Christoph Hellwig
                   ` (5 subsequent siblings)
  8 siblings, 1 reply; 30+ messages in thread
From: Christoph Hellwig @ 2020-05-09 17:01 UTC (permalink / raw)
  To: sandeen; +Cc: linux-xfs

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


^ permalink raw reply	[flat|nested] 30+ messages in thread

* [PATCH 4/8] db: cleanup attr_set_f and attr_remove_f
  2020-05-09 17:01 misc xfsprogs cleanups after the 5.7 merge Christoph Hellwig
                   ` (2 preceding siblings ...)
  2020-05-09 17:01 ` [PATCH 3/8] db: add a comment to agfl_crc_flds Christoph Hellwig
@ 2020-05-09 17:01 ` Christoph Hellwig
  2020-05-09 17:08   ` Darrick J. Wong
  2020-05-09 17:23   ` Eric Sandeen
  2020-05-09 17:01 ` [PATCH 5/8] db: validate name and namelen in " Christoph Hellwig
                   ` (4 subsequent siblings)
  8 siblings, 2 replies; 30+ messages in thread
From: Christoph Hellwig @ 2020-05-09 17:01 UTC (permalink / raw)
  To: sandeen; +Cc: linux-xfs

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


^ permalink raw reply	[flat|nested] 30+ messages in thread

* [PATCH 5/8] db: validate name and namelen in attr_set_f and attr_remove_f
  2020-05-09 17:01 misc xfsprogs cleanups after the 5.7 merge Christoph Hellwig
                   ` (3 preceding siblings ...)
  2020-05-09 17:01 ` [PATCH 4/8] db: cleanup attr_set_f and attr_remove_f Christoph Hellwig
@ 2020-05-09 17:01 ` Christoph Hellwig
  2020-05-09 17:09   ` Darrick J. Wong
  2020-05-09 17:01 ` [PATCH 6/8] db: ensure that create and replace are exclusive in attr_set_f Christoph Hellwig
                   ` (3 subsequent siblings)
  8 siblings, 1 reply; 30+ messages in thread
From: Christoph Hellwig @ 2020-05-09 17:01 UTC (permalink / raw)
  To: sandeen; +Cc: linux-xfs

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


^ permalink raw reply	[flat|nested] 30+ messages in thread

* [PATCH 6/8] db: ensure that create and replace are exclusive in attr_set_f
  2020-05-09 17:01 misc xfsprogs cleanups after the 5.7 merge Christoph Hellwig
                   ` (4 preceding siblings ...)
  2020-05-09 17:01 ` [PATCH 5/8] db: validate name and namelen in " Christoph Hellwig
@ 2020-05-09 17:01 ` Christoph Hellwig
  2020-05-09 17:09   ` Darrick J. Wong
  2020-05-09 17:01 ` [PATCH 7/8] repair: cleanup build_agf_agfl Christoph Hellwig
                   ` (2 subsequent siblings)
  8 siblings, 1 reply; 30+ messages in thread
From: Christoph Hellwig @ 2020-05-09 17:01 UTC (permalink / raw)
  To: sandeen; +Cc: linux-xfs

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


^ permalink raw reply	[flat|nested] 30+ messages in thread

* [PATCH 7/8] repair: cleanup build_agf_agfl
  2020-05-09 17:01 misc xfsprogs cleanups after the 5.7 merge Christoph Hellwig
                   ` (5 preceding siblings ...)
  2020-05-09 17:01 ` [PATCH 6/8] db: ensure that create and replace are exclusive in attr_set_f Christoph Hellwig
@ 2020-05-09 17:01 ` Christoph Hellwig
  2020-05-09 17:11   ` Darrick J. Wong
  2020-05-09 17:01 ` [PATCH 8/8] metadump: small cleanup for process_inode Christoph Hellwig
  2020-05-09 19:03 ` misc xfsprogs cleanups after the 5.7 merge Eric Sandeen
  8 siblings, 1 reply; 30+ messages in thread
From: Christoph Hellwig @ 2020-05-09 17:01 UTC (permalink / raw)
  To: sandeen; +Cc: linux-xfs

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


^ permalink raw reply	[flat|nested] 30+ messages in thread

* [PATCH 8/8] metadump: small cleanup for process_inode
  2020-05-09 17:01 misc xfsprogs cleanups after the 5.7 merge Christoph Hellwig
                   ` (6 preceding siblings ...)
  2020-05-09 17:01 ` [PATCH 7/8] repair: cleanup build_agf_agfl Christoph Hellwig
@ 2020-05-09 17:01 ` Christoph Hellwig
  2020-05-09 17:11   ` Darrick J. Wong
  2020-05-09 19:03 ` misc xfsprogs cleanups after the 5.7 merge Eric Sandeen
  8 siblings, 1 reply; 30+ messages in thread
From: Christoph Hellwig @ 2020-05-09 17:01 UTC (permalink / raw)
  To: sandeen; +Cc: linux-xfs

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


^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [PATCH 1/8] libxfs: use tabs instead of spaces in div_u64
  2020-05-09 17:01 ` [PATCH 1/8] libxfs: use tabs instead of spaces in div_u64 Christoph Hellwig
@ 2020-05-09 17:06   ` Darrick J. Wong
  2020-05-09 17:08   ` Eric Sandeen
  1 sibling, 0 replies; 30+ messages in thread
From: Darrick J. Wong @ 2020-05-09 17:06 UTC (permalink / raw)
  To: Christoph Hellwig; +Cc: sandeen, linux-xfs

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
> 

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [PATCH 2/8] db: fix a comment in scan_freelist
  2020-05-09 17:01 ` [PATCH 2/8] db: fix a comment in scan_freelist Christoph Hellwig
@ 2020-05-09 17:06   ` Darrick J. Wong
  0 siblings, 0 replies; 30+ messages in thread
From: Darrick J. Wong @ 2020-05-09 17:06 UTC (permalink / raw)
  To: Christoph Hellwig; +Cc: sandeen, linux-xfs

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
> 

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [PATCH 3/8] db: add a comment to agfl_crc_flds
  2020-05-09 17:01 ` [PATCH 3/8] db: add a comment to agfl_crc_flds Christoph Hellwig
@ 2020-05-09 17:07   ` Darrick J. Wong
  2020-05-09 17:10     ` Christoph Hellwig
  0 siblings, 1 reply; 30+ messages in thread
From: Darrick J. Wong @ 2020-05-09 17:07 UTC (permalink / raw)
  To: Christoph Hellwig; +Cc: sandeen, linux-xfs

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
> 

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [PATCH 4/8] db: cleanup attr_set_f and attr_remove_f
  2020-05-09 17:01 ` [PATCH 4/8] db: cleanup attr_set_f and attr_remove_f Christoph Hellwig
@ 2020-05-09 17:08   ` Darrick J. Wong
  2020-05-09 17:23   ` Eric Sandeen
  1 sibling, 0 replies; 30+ messages in thread
From: Darrick J. Wong @ 2020-05-09 17:08 UTC (permalink / raw)
  To: Christoph Hellwig; +Cc: sandeen, linux-xfs

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
> 

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [PATCH 1/8] libxfs: use tabs instead of spaces in div_u64
  2020-05-09 17:01 ` [PATCH 1/8] libxfs: use tabs instead of spaces in div_u64 Christoph Hellwig
  2020-05-09 17:06   ` Darrick J. Wong
@ 2020-05-09 17:08   ` Eric Sandeen
  1 sibling, 0 replies; 30+ messages in thread
From: Eric Sandeen @ 2020-05-09 17:08 UTC (permalink / raw)
  To: Christoph Hellwig; +Cc: linux-xfs

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);
>  }
>  
>  /**
> 

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [PATCH 5/8] db: validate name and namelen in attr_set_f and attr_remove_f
  2020-05-09 17:01 ` [PATCH 5/8] db: validate name and namelen in " Christoph Hellwig
@ 2020-05-09 17:09   ` Darrick J. Wong
  2020-05-09 17:12     ` Christoph Hellwig
  0 siblings, 1 reply; 30+ messages in thread
From: Darrick J. Wong @ 2020-05-09 17:09 UTC (permalink / raw)
  To: Christoph Hellwig; +Cc: sandeen, linux-xfs

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
> 

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [PATCH 6/8] db: ensure that create and replace are exclusive in attr_set_f
  2020-05-09 17:01 ` [PATCH 6/8] db: ensure that create and replace are exclusive in attr_set_f Christoph Hellwig
@ 2020-05-09 17:09   ` Darrick J. Wong
  0 siblings, 0 replies; 30+ messages in thread
From: Darrick J. Wong @ 2020-05-09 17:09 UTC (permalink / raw)
  To: Christoph Hellwig; +Cc: sandeen, linux-xfs

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
> 

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [PATCH 3/8] db: add a comment to agfl_crc_flds
  2020-05-09 17:07   ` Darrick J. Wong
@ 2020-05-09 17:10     ` Christoph Hellwig
  2020-05-09 17:32       ` Eric Sandeen
  0 siblings, 1 reply; 30+ messages in thread
From: Christoph Hellwig @ 2020-05-09 17:10 UTC (permalink / raw)
  To: Darrick J. Wong; +Cc: Christoph Hellwig, sandeen, linux-xfs

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.

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [PATCH 7/8] repair: cleanup build_agf_agfl
  2020-05-09 17:01 ` [PATCH 7/8] repair: cleanup build_agf_agfl Christoph Hellwig
@ 2020-05-09 17:11   ` Darrick J. Wong
  2020-05-09 17:47     ` Eric Sandeen
  0 siblings, 1 reply; 30+ messages in thread
From: Darrick J. Wong @ 2020-05-09 17:11 UTC (permalink / raw)
  To: Christoph Hellwig; +Cc: sandeen, linux-xfs

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
> 

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [PATCH 8/8] metadump: small cleanup for process_inode
  2020-05-09 17:01 ` [PATCH 8/8] metadump: small cleanup for process_inode Christoph Hellwig
@ 2020-05-09 17:11   ` Darrick J. Wong
  0 siblings, 0 replies; 30+ messages in thread
From: Darrick J. Wong @ 2020-05-09 17:11 UTC (permalink / raw)
  To: Christoph Hellwig; +Cc: sandeen, linux-xfs

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
> 

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [PATCH 5/8] db: validate name and namelen in attr_set_f and attr_remove_f
  2020-05-09 17:09   ` Darrick J. Wong
@ 2020-05-09 17:12     ` Christoph Hellwig
  0 siblings, 0 replies; 30+ messages in thread
From: Christoph Hellwig @ 2020-05-09 17:12 UTC (permalink / raw)
  To: Darrick J. Wong; +Cc: Christoph Hellwig, sandeen, linux-xfs

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 :)

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [PATCH 4/8] db: cleanup attr_set_f and attr_remove_f
  2020-05-09 17:01 ` [PATCH 4/8] db: cleanup attr_set_f and attr_remove_f Christoph Hellwig
  2020-05-09 17:08   ` Darrick J. Wong
@ 2020-05-09 17:23   ` Eric Sandeen
  2020-05-10  7:11     ` Christoph Hellwig
  1 sibling, 1 reply; 30+ messages in thread
From: Eric Sandeen @ 2020-05-09 17:23 UTC (permalink / raw)
  To: Christoph Hellwig; +Cc: linux-xfs

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;
>  }
> 

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [PATCH 3/8] db: add a comment to agfl_crc_flds
  2020-05-09 17:10     ` Christoph Hellwig
@ 2020-05-09 17:32       ` Eric Sandeen
  2020-05-09 19:18         ` Darrick J. Wong
  2020-05-10  7:11         ` Christoph Hellwig
  0 siblings, 2 replies; 30+ messages in thread
From: Eric Sandeen @ 2020-05-09 17:32 UTC (permalink / raw)
  To: Christoph Hellwig, Darrick J. Wong; +Cc: linux-xfs

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.


^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [PATCH 7/8] repair: cleanup build_agf_agfl
  2020-05-09 17:11   ` Darrick J. Wong
@ 2020-05-09 17:47     ` Eric Sandeen
  0 siblings, 0 replies; 30+ messages in thread
From: Eric Sandeen @ 2020-05-09 17:47 UTC (permalink / raw)
  To: Darrick J. Wong, Christoph Hellwig; +Cc: linux-xfs

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

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: misc xfsprogs cleanups after the 5.7 merge
  2020-05-09 17:01 misc xfsprogs cleanups after the 5.7 merge Christoph Hellwig
                   ` (7 preceding siblings ...)
  2020-05-09 17:01 ` [PATCH 8/8] metadump: small cleanup for process_inode Christoph Hellwig
@ 2020-05-09 19:03 ` Eric Sandeen
  8 siblings, 0 replies; 30+ messages in thread
From: Eric Sandeen @ 2020-05-09 19:03 UTC (permalink / raw)
  To: Christoph Hellwig; +Cc: linux-xfs

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

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [PATCH 3/8] db: add a comment to agfl_crc_flds
  2020-05-09 17:32       ` Eric Sandeen
@ 2020-05-09 19:18         ` Darrick J. Wong
  2020-05-10  7:11         ` Christoph Hellwig
  1 sibling, 0 replies; 30+ messages in thread
From: Darrick J. Wong @ 2020-05-09 19:18 UTC (permalink / raw)
  To: Eric Sandeen; +Cc: Christoph Hellwig, linux-xfs

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


^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [PATCH 4/8] db: cleanup attr_set_f and attr_remove_f
  2020-05-09 17:23   ` Eric Sandeen
@ 2020-05-10  7:11     ` Christoph Hellwig
  2020-05-10 14:09       ` Eric Sandeen
  0 siblings, 1 reply; 30+ messages in thread
From: Christoph Hellwig @ 2020-05-10  7:11 UTC (permalink / raw)
  To: Eric Sandeen; +Cc: Christoph Hellwig, linux-xfs

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.

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [PATCH 3/8] db: add a comment to agfl_crc_flds
  2020-05-09 17:32       ` Eric Sandeen
  2020-05-09 19:18         ` Darrick J. Wong
@ 2020-05-10  7:11         ` Christoph Hellwig
  1 sibling, 0 replies; 30+ messages in thread
From: Christoph Hellwig @ 2020-05-10  7:11 UTC (permalink / raw)
  To: Eric Sandeen; +Cc: Christoph Hellwig, Darrick J. Wong, linux-xfs

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.

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [PATCH 4/8] db: cleanup attr_set_f and attr_remove_f
  2020-05-10  7:11     ` Christoph Hellwig
@ 2020-05-10 14:09       ` Eric Sandeen
  2020-05-11  3:41         ` Darrick J. Wong
  0 siblings, 1 reply; 30+ messages in thread
From: Eric Sandeen @ 2020-05-10 14:09 UTC (permalink / raw)
  To: Christoph Hellwig; +Cc: linux-xfs

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.  ;)

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [PATCH 4/8] db: cleanup attr_set_f and attr_remove_f
  2020-05-10 14:09       ` Eric Sandeen
@ 2020-05-11  3:41         ` Darrick J. Wong
  2020-05-11 13:21           ` Eric Sandeen
  0 siblings, 1 reply; 30+ messages in thread
From: Darrick J. Wong @ 2020-05-11  3:41 UTC (permalink / raw)
  To: Eric Sandeen; +Cc: Christoph Hellwig, linux-xfs

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

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [PATCH 4/8] db: cleanup attr_set_f and attr_remove_f
  2020-05-11  3:41         ` Darrick J. Wong
@ 2020-05-11 13:21           ` Eric Sandeen
  0 siblings, 0 replies; 30+ messages in thread
From: Eric Sandeen @ 2020-05-11 13:21 UTC (permalink / raw)
  To: Darrick J. Wong; +Cc: Christoph Hellwig, linux-xfs

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

^ permalink raw reply	[flat|nested] 30+ messages in thread

end of thread, other threads:[~2020-05-11 13:21 UTC | newest]

Thread overview: 30+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-05-09 17:01 misc xfsprogs cleanups after the 5.7 merge Christoph Hellwig
2020-05-09 17:01 ` [PATCH 1/8] libxfs: use tabs instead of spaces in div_u64 Christoph Hellwig
2020-05-09 17:06   ` Darrick J. Wong
2020-05-09 17:08   ` Eric Sandeen
2020-05-09 17:01 ` [PATCH 2/8] db: fix a comment in scan_freelist Christoph Hellwig
2020-05-09 17:06   ` Darrick J. Wong
2020-05-09 17:01 ` [PATCH 3/8] db: add a comment to agfl_crc_flds Christoph Hellwig
2020-05-09 17:07   ` Darrick J. Wong
2020-05-09 17:10     ` Christoph Hellwig
2020-05-09 17:32       ` Eric Sandeen
2020-05-09 19:18         ` Darrick J. Wong
2020-05-10  7:11         ` Christoph Hellwig
2020-05-09 17:01 ` [PATCH 4/8] db: cleanup attr_set_f and attr_remove_f Christoph Hellwig
2020-05-09 17:08   ` Darrick J. Wong
2020-05-09 17:23   ` Eric Sandeen
2020-05-10  7:11     ` Christoph Hellwig
2020-05-10 14:09       ` Eric Sandeen
2020-05-11  3:41         ` Darrick J. Wong
2020-05-11 13:21           ` Eric Sandeen
2020-05-09 17:01 ` [PATCH 5/8] db: validate name and namelen in " Christoph Hellwig
2020-05-09 17:09   ` Darrick J. Wong
2020-05-09 17:12     ` Christoph Hellwig
2020-05-09 17:01 ` [PATCH 6/8] db: ensure that create and replace are exclusive in attr_set_f Christoph Hellwig
2020-05-09 17:09   ` Darrick J. Wong
2020-05-09 17:01 ` [PATCH 7/8] repair: cleanup build_agf_agfl Christoph Hellwig
2020-05-09 17:11   ` Darrick J. Wong
2020-05-09 17:47     ` Eric Sandeen
2020-05-09 17:01 ` [PATCH 8/8] metadump: small cleanup for process_inode Christoph Hellwig
2020-05-09 17:11   ` Darrick J. Wong
2020-05-09 19:03 ` misc xfsprogs cleanups after the 5.7 merge Eric Sandeen

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.