Tasleson read bda signature #1440
Conversation
| /// Return None if the static header's magic does not match for *both* copies. | ||
| /// Return None if it's not a Stratis device. | ||
| /// Return an Err if the metadata seems to indicate that the device is | ||
| /// a Stratis device, but no well-formed signature block could be read. |
There was a problem hiding this comment.
This returns an error in two situations:
- If the metadata seems to indicate that the device is a Stratis deivce, but no well-formed signature could be found.
- If a well-formed signature was found but an error occurred during the repair of a second, ill-formed, old, or unreadable signature.
I think it is right to return an error in the first case, but I think the second should possibly be handled differently. I think a good option is to enrich the return type to
StratisResult<Option<StaticHeader>, Option<some error thing if repair fails>>.
Alternatively, it is fine to open an issue to address this question in a separate PR.
| Ok(Some(loc_2)) | ||
| (Err(_), Err(_)) => { | ||
| let err_str = | ||
| "Appeared to be a Stratis device, but no valid sigblock found"; |
There was a problem hiding this comment.
Might as well enhance this error message. The contents of both buffers are available, so they might as well be included. Actually, better to include both errors, and improve the error messages in the errors returned from sigblock_from_buf. I'ld say that it makes sense to file an issue for improving sigblock_from_buf error messages rather than address that in this PR.
mulkieran
force-pushed
the
tasleson-read_bda_signature
branch
from
5232491 to
719a2c3
Compare
|
Jenkins error is just the udevadm error. |
Change signature to return (io::Result<[u8; SECTOR_SIZE]>, io::Result<[u8; SECTOR_SIZE]>) so that we can distinguish the difference from an IO error and a device that simply has nothing but zeros. Addresses: stratis-storage#1016 Signed-off-by: Tony Asleson <[email protected]> Signed-off-by: mulhern <[email protected]>
mulkieran
force-pushed
the
tasleson-read_bda_signature
branch
2 times, most recently
from
8c09d1d to
19d8431
Compare
If the sigblocks differ but the initialization times agree return an error There is no good explanation for why this should happen, and no way to know how to repair. Return an error if unable to read data at either sigblock location. This preserves previous behavior and seems more correct to me. Signed-off-by: mulhern <[email protected]>
mulkieran
force-pushed
the
tasleson-read_bda_signature
branch
from
19d8431 to
700ac15
Compare
| } else if loc_1.initialization_time == loc_2.initialization_time { | ||
| // Inexplicable disagreement among static headers | ||
| let err_str = "Appeared to be a Stratis device, but signature blocks disagree."; | ||
| Err(StratisError::Engine(ErrorEnum::Invalid, err_str.into())) |
There was a problem hiding this comment.
IMHO this would be extremely unlikely, but if it did happen I would like to have the contents of each super blocks dumped to the logs.
| @@ -373,7 +383,8 @@ impl StaticHeader { | |||
| } | |||
| (Err(_), Err(_)) => { | |||
| // Unable to read the device at all. | |||
| Ok(None) | |||
| let err_str = "Unable to read data at sigblock locations."; | |||
| Err(StratisError::Engine(ErrorEnum::Invalid, err_str.into())) | |||
There was a problem hiding this comment.
Returning None was the previous behavior. scratch that, if we got a read error from both we would have bubbled up the read error from the first one.
|
Merged into #1439 . |
The first three commits just get rid of stray formatting changes.
The next three are straightforward tidies.
The last just adds more precision to
setup()function specification.There are also two review comments.