{"action":"closed","issue":{"url":"https://api.github.com/repos/wiredtiger/wiredtiger/issues/1435","labels_url":"https://api.github.com/repos/wiredtiger/wiredtiger/issues/1435/labels{/name}","comments_url":"https://api.github.com/repos/wiredtiger/wiredtiger/issues/1435/comments","events_url":"https://api.github.com/repos/wiredtiger/wiredtiger/issues/1435/events","html_url":"https://github.com/wiredtiger/wiredtiger/issues/1435","id":50985454,"number":1435,"title":"distinguish verification object failure from verification call failure","user":{"login":"keithbostic","id":1393186,"avatar_url":"https://avatars.githubusercontent.com/u/1393186?v=3","gravatar_id":"","url":"https://api.github.com/users/keithbostic","html_url":"https://github.com/keithbostic","followers_url":"https://api.github.com/users/keithbostic/followers","following_url":"https://api.github.com/users/keithbostic/following{/other_user}","gists_url":"https://api.github.com/users/keithbostic/gists{/gist_id}","starred_url":"https://api.github.com/users/keithbostic/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/keithbostic/subscriptions","organizations_url":"https://api.github.com/users/keithbostic/orgs","repos_url":"https://api.github.com/users/keithbostic/repos","events_url":"https://api.github.com/users/keithbostic/events{/privacy}","received_events_url":"https://api.github.com/users/keithbostic/received_events","type":"User","site_admin":false},"labels":[{"url":"https://api.github.com/repos/wiredtiger/wiredtiger/labels/Cleanup","name":"Cleanup","color":"DDDDDD"}],"state":"closed","locked":true,"assignee":{"login":"keithbostic","id":1393186,"avatar_url":"https://avatars.githubusercontent.com/u/1393186?v=3","gravatar_id":"","url":"https://api.github.com/users/keithbostic","html_url":"https://github.com/keithbostic","followers_url":"https://api.github.com/users/keithbostic/followers","following_url":"https://api.github.com/users/keithbostic/following{/other_user}","gists_url":"https://api.github.com/users/keithbostic/gists{/gist_id}","starred_url":"https://api.github.com/users/keithbostic/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/keithbostic/subscriptions","organizations_url":"https://api.github.com/users/keithbostic/orgs","repos_url":"https://api.github.com/users/keithbostic/repos","events_url":"https://api.github.com/users/keithbostic/events{/privacy}","received_events_url":"https://api.github.com/users/keithbostic/received_events","type":"User","site_admin":false},"milestone":null,"comments":2,"created_at":"2014-12-04T15:04:23Z","updated_at":"2015-05-01T01:34:15Z","closed_at":"2015-05-01T01:34:15Z","body":"There's currently no way to distinguish between a `WT_SESSION.verify` failure where the object being verified wasn't correct, and the verification code itself failed for some reason, and it's possible we'd report to an administrator that the file needs verification when it doesn't.\r\n\r\nFirst, there was initial email discussion about how to distinguish `WT_SESSION.verify` error returns. The two suggestions were a new error return (`WT_VERIFICATION_FAILED`), or a new argument to the `WT_SESSION.verify` call. We don't want to change the `WT_SESSION.verify` signature, so `WT_VERIFICATION_FAILED` is probably better.\r\n\r\n(We could also use `WT_ERROR` to mean the object failed verification, instead of adding `WT_VERIFICATION_FAILED`, but that implies ensuring `WT_SESSION.verify` never returns `WT_ERROR` itself, and I doubt that's possible.)\r\n\r\nSecond, this turns out to be a difficult problem. The verify code uses lots of other code throughout the library, not to mention system calls, and that code can return any error you can imagine. For example, if an internal page has a corrupted block-manager address, `__wt_vunpack_uint`, the code to unpack a 8B unsigned integer, will return `EINVAL`. Or, imagine the underlying file has been truncated and we have an invalid file offset: the pread documentation on my OS X 10.10.1 system claims pread may return `EINVAL` if the file offset is invalid.\r\n\r\nWe could map `EINVAL` to `WT_VERIFICATION_FAILED`, but assuming any `EINVAL` returned to verify from any lower-level function means the underlying file has failed verification is obviously flawed, and we're going to gradually accumulate a list of errors that imply verification failed without any reason to believe the list is complete or correct. Does `EIO` mean the object failed verification, or that the block manager's process group was orphaned? Got me.\r\n\r\nThinking about how I'd deal with this in the field, I'd use the error messages. For example, if the error message is something about \"Read failed\", or \"Input/output error\", then it's likely the object failed verification, if it's \"Too many open files in system\", then it's likely verification itself failed.\r\n\r\nIf it's automated in the field, then I think the best we can do is document that \"any failure from verify implies the object failed verification\", which is how the system currently works.\r\n\r\nIf we need to do better than that, my only idea is to document three possible returns:\r\n\r\n1. return of 0 means everything is OK,\r\n2. return of `WT_VERIFICATION_FAILED` means the object failed verification,\r\n3. return of anything else means something went wrong and the object may have failed verification, and you can use the associated error message to decide.\r\n\r\nThen, review the verification code and in the paths where we detect corruption and start driving in the `WT_VERIFICATION_FAILED` error. For example, we'd start making changes like this:\r\n\r\n```\r\n__block_buffer_to_addr(WT_BLOCK *block,\r\n    const uint8_t **pp, wt_off_t *offsetp, uint32_t *sizep, uint32_t *cksump)\r\n{\r\n        uint64_t o, s, c;\r\n\r\n        WT_RET(__wt_vunpack_uint(pp, 0, &o));\r\n        WT_RET(__wt_vunpack_uint(pp, 0, &s));\r\n        WT_RET(__wt_vunpack_uint(pp, 0, &c));\r\n```\r\nwould become:\r\n```\r\n__block_buffer_to_addr(WT_BLOCK *block,\r\n    const uint8_t **pp, wt_off_t *offsetp, uint32_t *sizep, uint32_t *cksump)\r\n{\r\n        WT_DECL_RET;\r\n        uint64_t o, s, c;\r\n\r\n        WT_TRET(__wt_vunpack_uint(pp, 0, &o));\r\n        WT_TRET(__wt_vunpack_uint(pp, 0, &s));\r\n        WT_TRET(__wt_vunpack_uint(pp, 0, &c));\r\n        if (ret != 0)\r\n                return (block->verify ? WT_VERIFICATION_FAILURE : ret);\r\n```\r\nWe won't ever know those changes are complete, but it won't be difficult to be confident we'll return `WT_VERIFICATION_FAILED` in almost all cases, as there simply aren't that many system calls or that much block manager processing in the verification code paths.\r\n\r\n@michaelcahill, @sueloverso, @agorrod: thoughts?\r\n"}}