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