[PATCH RFC 0/10] xfs: add verifier context structure

* [PATCH RFC 0/10] xfs: add verifier context structure
@ 2018-12-05 21:01 Eric Sandeen
  2018-12-05 21:02 ` [PATCH 01/10] xfs: change xfs_attr3_rmt_hdr_ok to return bool Eric Sandeen
                   ` (9 more replies)
  0 siblings, 10 replies; 20+ messages in thread
From: Eric Sandeen @ 2018-12-05 21:01 UTC (permalink / raw)
  To: linux-xfs

This patchset adds and uses an "xfs verifier context" structure
in the callchains that lead into metadata verifiers and other
validators.

The high-level goal of this is twofold:

1) It lets us return whatever information we want about the point
   of failure by populating the structure.  We /could/ add file, line,
   function, code address, buffer offset, text description, whatever
   we'd like.  For now it only contains the same info as we have today,
   failaddr and errno.  For many of these options, we could easily
   encapsulate it in a macro used at each failure point.

1a) this hopefully may lead to more consistent error messages that don't
    change depending on the compiler used, etc (which IMHO is a problem
    with failaddr today).

2) By passing the verifier context by reference, we can move the
   verifiers back to booleans, instead of having the somewhat odd
   bool / failaddr_t dichotomy depending on what we're calling.  This
   then lets us condense some of the code.

So now we have macros that let us do things like:

	return XFS_CORRUPTED_RETURN(vc);
or
	return XFS_VERIFIED_RETURN(vc);

which populate the context and return true or false.  We can also
turn things like:

        if (xfs_sb_version_hascrc(&mp->m_sb) &&
            !xfs_buf_verify_cksum(bp, XFS_AGF_CRC_OFF))
                xfs_verifier_error(bp, -EFSBADCRC, __this_address);
        else {
                fa = xfs_agf_verify(bp);
                if (XFS_TEST_ERROR(fa, mp, XFS_ERRTAG_ALLOC_READ_AGF))
                        xfs_verifier_error(bp, -EFSCORRUPTED, fa);
        }

into:

       if ((xfs_sb_version_hascrc(&mp->m_sb) &&
                       !xfs_buf_verify_cksum(vc, bp, XFS_AGF_CRC_OFF)) ||
           XFS_TEST_ERROR(!xfs_agf_verify(vc, bp),
                       mp, XFS_ERRTAG_ALLOC_READ_AGF))
                xfs_verifier_error(bp, vc);

(is that better?)
(I've wondered about moving the hascrc check into the verify_cksum f'n)

when the failaddr return is changed to a bool.  Because all of the info
is in vc, we don't have to explicitly pass in the errno because the failure
point set it; this cleans up some other places as well.

There are definitely things to improve; my (ab)use of macros probably
isn't great, we should maybe have a verifier context initializer and
checks that it was used properly (I had one bug in development where
I forgot to set anything into the vc at failure and got back a garbage
errno), it could maybe be pushed into more validators, maybe "context"
is a bad name, etc.  Bikedshed away!

But I wanted to throw this out there to see if people thought it was
worth continuing on, and see what suggestions people have. 
(the first patch or two are cleanups that could go now, I think).

If the general approach seems good, I'll clean it up and then propose
something that may be a bit more consistent but still as useful as the
failaddr approach we have today.

Thanks,
-Eric

^ permalink raw reply	[flat|nested] 20+ messages in thread