| Message ID | 20200205190455.1834330-1-billodo@redhat.com (mailing list archive) |
|---|---|
| State | Rejected |
| Headers | show |
| Series | xfs: xchk_xattr_listent() fix context->seen_enough to -ECANCELED | expand |
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 >
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 > > >
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; }
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(-)