tor_persist::Error: impl HasKind and adjust comments

And change the comments to slightly reinterpret these errors, to
relate to the circumstances rather than error generation site.

crates/tor-error/src/lib.rs

@@ -52,6 +52,31 @@ pub use internal::*;
 #[derive(Debug, Clone, Copy, PartialEq, Eq, Display)]
 #[non_exhaustive]
 pub enum ErrorKind {
+    /// IO error accessing local persistent state
+    ///
+    /// Eg, disk full or permissions problem.
+    /// Usually the source will be [`std::io::Error`].
+    #[display(fmt = "could not read/write persistent state")]
+    PersistentStateAccessFailed,
+
+    /// Tor client's persistent state has been corrupted
+    ///
+    /// This could be because of a bug in the Tor code, or because something else has been messing
+    /// with the data.
+    ///
+    /// This might also occur if the Tor code was upgraded and the new Tor is not compatible.
+    #[display(fmt = "corrupted data in persistent state")]
+    PersistentStateCorrupted,
+
+    /// Tried to write to read-only persistent state.
+    ///
+    /// Usually, errors of this kind should be handled before the user sees
+    /// them: the state manager's locking code is supposed to prevent
+    /// higher level crates from accidentally trying to do this.  This
+    /// error kind can indicate a bug.
+    #[display(fmt = "could not write to read-only persistent state")]
+    PersistentStateReadOnly,
+
     /// Internal error (bug)
     ///
     /// A supposedly impossible problem has arisen.  This indicates a bug in Arti.

crates/tor-guardmgr/src/lib.rs

@@ -1020,9 +1020,10 @@ pub enum GuardRestriction {
 #[derive(Clone, Debug, thiserror::Error)]
 #[non_exhaustive]
 pub enum GuardMgrError {
-    /// An error derived from the state manager.
+    /// An error manipulating persistent state
     #[error("Problem accessing persistent state")]
     State(#[from] tor_persist::Error),
+
     /// An error that occurred while trying to spawn a daemon task.
     #[error("Unable to spawn task")]
     Spawn(#[source] Arc<SpawnError>),

crates/tor-persist/src/lib.rs

@@ -59,6 +59,8 @@ pub use serde_json::Value as JsonValue;
 #[cfg(feature = "testing")]
 pub use testing::TestingStateMgr;
 
+use tor_error::ErrorKind;
+
 /// An object that can manage persistent state.
 ///
 /// State is implemented as a simple key-value store, where the values
@@ -127,7 +129,10 @@ impl LockStatus {
     }
 }
 
-/// An error type returned from a persistent state manager.
+/// An error manipulating persistent state.
+//
+// Such errors are "global" in the sense that it doesn't relate to any guard or any circuit
+// or anything, so callers may use `#[from]` when they include it in their own error.
 #[derive(thiserror::Error, Debug, Clone)]
 #[non_exhaustive]
 pub enum Error {
@@ -136,6 +141,10 @@ pub enum Error {
     IoError(#[source] Arc<std::io::Error>),
 
     /// Tried to save without holding an exclusive lock.
+    //
+    // TODO This error seems to actually be sometimes used to make store a no-op.
+    //      We should consider whether this is best handled as an error, but for now
+    //      this seems adequate.
     #[error("Storage not locked")]
     NoLock,
 
@@ -148,6 +157,20 @@ pub enum Error {
     Deserialize(#[source] Arc<serde_json::Error>),
 }
 
+impl tor_error::HasKind for Error {
+    #[rustfmt::skip] // the tabular layout of the `match` makes this a lot clearer
+    fn kind(&self) -> ErrorKind {
+        use Error as E;
+        use tor_error::ErrorKind as K;
+        match self {
+            E::IoError(..)     => K::PersistentStateAccessFailed,
+            E::NoLock          => K::PersistentStateReadOnly,
+            E::Serialize(..)   => K::InternalError,
+            E::Deserialize(..) => K::PersistentStateCorrupted,
+        }
+    }
+}
+
 impl From<std::io::Error> for Error {
     fn from(e: std::io::Error) -> Error {
         Error::IoError(Arc::new(e))