public inbox for linux-xfs@vger.kernel.org
 help / color / mirror / Atom feed
* [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

end of thread, other threads:[~2018-12-17 18:39 UTC | newest]

Thread overview: 20+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2018-12-05 21:01 [PATCH RFC 0/10] xfs: add verifier context structure Eric Sandeen
2018-12-05 21:02 ` [PATCH 01/10] xfs: change xfs_attr3_rmt_hdr_ok to return bool Eric Sandeen
2018-12-07 13:36   ` Brian Foster
2018-12-17 18:23   ` Darrick J. Wong
2018-12-05 21:03 ` [PATCH 02/10] xfs: make checksum verifiers consistently return bools Eric Sandeen
2018-12-07 13:36   ` Brian Foster
2018-12-17 18:24   ` Darrick J. Wong
2018-12-05 21:03 ` [PATCH 03/10] xfs: pass a verifier context down verifier callchains Eric Sandeen
2018-12-17 18:29   ` Darrick J. Wong
2018-12-05 21:04 ` [PATCH 04/10] xfs: pass a verifier context to crc validation functions Eric Sandeen
2018-12-05 21:05 ` [PATCH 05/10] xfs: define new macros to set verifier context on return Eric Sandeen
2018-12-05 21:06 ` [PATCH 06/10] xfs: teach xfs_btree_[sl]block_verify_crc to populate verifier context Eric Sandeen
2018-12-05 21:08 ` [PATCH 07/10] xfs: change all verifiers to return booleans Eric Sandeen
2018-12-05 21:09 ` [PATCH 08/10] xfs: set failaddr into vc for checksum failures Eric Sandeen
2018-12-07 13:37   ` Brian Foster
2018-12-10 16:00     ` Eric Sandeen
2018-12-17 18:39       ` Darrick J. Wong
2018-12-05 21:11 ` [PATCH 09/10] xfs: add errno to verifier context and populate it Eric Sandeen
2018-12-07 13:41   ` Brian Foster
2018-12-05 21:11 ` [PATCH 10/10] xfs: condense crc and verifier checks where possible Eric Sandeen

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox