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