* [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 related [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 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
* [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 related [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
* [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 related [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 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 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 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 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
* [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 related [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 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 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 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
* [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 related [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 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
* [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 related [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
* [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 related [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 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
* [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 related [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: 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