* [PATCH] xfs: xchk_xattr_listent() fix context->seen_enough to -ECANCELED
@ 2020-02-05 19:04 Bill O'Donnell
2020-02-06 23:07 ` Darrick J. Wong
0 siblings, 1 reply; 3+ messages in thread
From: Bill O'Donnell @ 2020-02-05 19:04 UTC (permalink / raw)
To: linux-xfs
Commit e7ee96dfb8c (xfs: remove all *_ITER_ABORT values)
replaced *_ITER_ABORT values with -ECANCELED. The replacement
in the case of scrub/attr.c xchk_xattr_listent() is in
error (context->seen_enough = 1;). Instead of '1', use
the intended -ECANCELED.
Fixes: e7ee96dfb8c (xfs: remove all *_ITER_ABORT values)
Signed-off-by: Bill O'Donnell <billodo@redhat.com>
---
fs/xfs/scrub/attr.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/fs/xfs/scrub/attr.c b/fs/xfs/scrub/attr.c
index d9f0dd444b80..5d0590f78973 100644
--- a/fs/xfs/scrub/attr.c
+++ b/fs/xfs/scrub/attr.c
@@ -171,7 +171,7 @@ xchk_xattr_listent(
args.blkno);
fail_xref:
if (sx->sc->sm->sm_flags & XFS_SCRUB_OFLAG_CORRUPT)
- context->seen_enough = 1;
+ context->seen_enough = -ECANCELED;
return;
}
--
2.24.1
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH] xfs: xchk_xattr_listent() fix context->seen_enough to -ECANCELED
2020-02-05 19:04 [PATCH] xfs: xchk_xattr_listent() fix context->seen_enough to -ECANCELED Bill O'Donnell
@ 2020-02-06 23:07 ` Darrick J. Wong
2020-02-06 23:51 ` Bill O'Donnell
0 siblings, 1 reply; 3+ messages in thread
From: Darrick J. Wong @ 2020-02-06 23:07 UTC (permalink / raw)
To: Bill O'Donnell; +Cc: linux-xfs
On Wed, Feb 05, 2020 at 01:04:55PM -0600, Bill O'Donnell wrote:
> Commit e7ee96dfb8c (xfs: remove all *_ITER_ABORT values)
> replaced *_ITER_ABORT values with -ECANCELED. The replacement
> in the case of scrub/attr.c xchk_xattr_listent() is in
> error (context->seen_enough = 1;). Instead of '1', use
> the intended -ECANCELED.
>
> Fixes: e7ee96dfb8c (xfs: remove all *_ITER_ABORT values)
> Signed-off-by: Bill O'Donnell <billodo@redhat.com>
> ---
> fs/xfs/scrub/attr.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/fs/xfs/scrub/attr.c b/fs/xfs/scrub/attr.c
> index d9f0dd444b80..5d0590f78973 100644
> --- a/fs/xfs/scrub/attr.c
> +++ b/fs/xfs/scrub/attr.c
> @@ -171,7 +171,7 @@ xchk_xattr_listent(
> args.blkno);
> fail_xref:
> if (sx->sc->sm->sm_flags & XFS_SCRUB_OFLAG_CORRUPT)
> - context->seen_enough = 1;
Hmm. The attr list functions do:
if (context->seen_enough)
break;
to stop iteration of the attributes. Any nonzero value will work,
positive or negative. Further down in the scrub/attr.c, xchk_xattr
does:
/* Did our listent function try to return any errors? */
if (sx.context.seen_enough < 0)
error = sx.context.seen_enough;
Which means that if seen_enough is set to a negative value, we'll return
that negative value all the way back to userspace, which means that the
userspace buffer is not updated and xfs_scrub will think there was a
runtime error.
> + context->seen_enough = -ECANCELED;
So this will cause xfs_scrub to abort with "Operation Canceled" if it
found a corruption error. The patch I sent to the list had -ECANCELED,
but then I noticed the scrub breakage and changed it to 1 before
committing. Other parts of the attr code use 1 to stop an attr walk
without returning errors to userspace.
Perhaps it's time to replace that novel use of "1" (and audit all the
branching and whatnot) with -ECANCELED so that we can go on cargoculting
negative int errors in peace.
(*UGH* I remembered that I was the one who applied negative int error
semantics to seen_enough in the first place; before that, its meaning
was purely boolean. It's still screaming for a cleanup though...)
--D
> return;
> }
>
> --
> 2.24.1
>
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH] xfs: xchk_xattr_listent() fix context->seen_enough to -ECANCELED
2020-02-06 23:07 ` Darrick J. Wong
@ 2020-02-06 23:51 ` Bill O'Donnell
0 siblings, 0 replies; 3+ messages in thread
From: Bill O'Donnell @ 2020-02-06 23:51 UTC (permalink / raw)
To: Darrick J. Wong; +Cc: linux-xfs
On Thu, Feb 06, 2020 at 03:07:31PM -0800, Darrick J. Wong wrote:
> On Wed, Feb 05, 2020 at 01:04:55PM -0600, Bill O'Donnell wrote:
> > Commit e7ee96dfb8c (xfs: remove all *_ITER_ABORT values)
> > replaced *_ITER_ABORT values with -ECANCELED. The replacement
> > in the case of scrub/attr.c xchk_xattr_listent() is in
> > error (context->seen_enough = 1;). Instead of '1', use
> > the intended -ECANCELED.
> >
> > Fixes: e7ee96dfb8c (xfs: remove all *_ITER_ABORT values)
> > Signed-off-by: Bill O'Donnell <billodo@redhat.com>
> > ---
> > fs/xfs/scrub/attr.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/fs/xfs/scrub/attr.c b/fs/xfs/scrub/attr.c
> > index d9f0dd444b80..5d0590f78973 100644
> > --- a/fs/xfs/scrub/attr.c
> > +++ b/fs/xfs/scrub/attr.c
> > @@ -171,7 +171,7 @@ xchk_xattr_listent(
> > args.blkno);
> > fail_xref:
> > if (sx->sc->sm->sm_flags & XFS_SCRUB_OFLAG_CORRUPT)
> > - context->seen_enough = 1;
>
> Hmm. The attr list functions do:
>
> if (context->seen_enough)
> break;
>
> to stop iteration of the attributes. Any nonzero value will work,
> positive or negative. Further down in the scrub/attr.c, xchk_xattr
> does:
>
> /* Did our listent function try to return any errors? */
> if (sx.context.seen_enough < 0)
> error = sx.context.seen_enough;
>
> Which means that if seen_enough is set to a negative value, we'll return
> that negative value all the way back to userspace, which means that the
> userspace buffer is not updated and xfs_scrub will think there was a
> runtime error.
>
> > + context->seen_enough = -ECANCELED;
>
> So this will cause xfs_scrub to abort with "Operation Canceled" if it
> found a corruption error. The patch I sent to the list had -ECANCELED,
> but then I noticed the scrub breakage and changed it to 1 before
> committing. Other parts of the attr code use 1 to stop an attr walk
> without returning errors to userspace.
That is what had me confused.
>
> Perhaps it's time to replace that novel use of "1" (and audit all the
> branching and whatnot) with -ECANCELED so that we can go on cargoculting
> negative int errors in peace.
>
> (*UGH* I remembered that I was the one who applied negative int error
> semantics to seen_enough in the first place; before that, its meaning
> was purely boolean. It's still screaming for a cleanup though...)
Agreed.
Thanks-
Bill
> --D
>
> > return;
> > }
> >
> > --
> > 2.24.1
> >
>
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2020-02-06 23:51 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-02-05 19:04 [PATCH] xfs: xchk_xattr_listent() fix context->seen_enough to -ECANCELED Bill O'Donnell
2020-02-06 23:07 ` Darrick J. Wong
2020-02-06 23:51 ` Bill O'Donnell
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).