Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Relax the error type in handle #21

Merged
merged 4 commits into from
Mar 15, 2024
Merged

Relax the error type in handle #21

merged 4 commits into from
Mar 15, 2024

Conversation

wiktor-k
Copy link
Owner

This allows clients to directly use ? with their error types instead of mapping everything to AgentError which obscures the error reason.

Signed-off-by: Wiktor Kwapisiewicz <[email protected]>
Without this change the client informs about error in communication:

```
$ ssh-add -s test
Enter passphrase for PKCS#11:
Could not add card "test": communication with agent failed
```

After this change the error informs about agent refusing the operation:

```
$ ssh-add -s test
Enter passphrase for PKCS#11:
Could not add card "test": agent refused operation
```

Signed-off-by: Wiktor Kwapisiewicz <[email protected]>
@wiktor-k wiktor-k force-pushed the more-useful-user-error branch from 9632941 to c08cb6b Compare March 12, 2024 11:23
Signed-off-by: Wiktor Kwapisiewicz <[email protected]>
@wiktor-k wiktor-k force-pushed the more-useful-user-error branch from 358aa9b to 022a4ef Compare March 12, 2024 11:31
@wiktor-k
Copy link
Owner Author

@baloo If you'd like to take a look at this I'd much appreciate.

I've made this PR after working a bit with a downstream crate that uses it and the annoying fact that everything need to be mapped to AgentError and that variant erases what actually went wrong (nothing useful in logs).

I've also made it return Failure instead of abruptly closing.

@baloo
Copy link
Collaborator

baloo commented Mar 12, 2024

That is a problem with the downstream crate that should have an error like:

enum Error {
   Agent(ssh_agent_lib::error::AgentError)
}

impl From<ssh_agent_lib::error::AgentError> for Error {
   fn from(e:ssh_agent_lib::error::AgentError) -> Error {
       Error::Agent(e)
   }
}

src/agent.rs Show resolved Hide resolved
@wiktor-k
Copy link
Owner Author

That is a problem with the downstream crate that should have an error like:

enum Error {
   Agent(ssh_agent_lib::error::AgentError)
}

impl From<ssh_agent_lib::error::AgentError> for Error {
   fn from(e:ssh_agent_lib::error::AgentError) -> Error {
       Error::Agent(e)
   }
}

I think it's quite unlikely that they will ever want to convert from AgentError since if AgentError is raised in the handle function then it's bubbled up up to tokio::spawn and that's it - it's at the end of the call stack.

The only reason where the client can get a AgentError is via listen but this is also a function that doesn't really return if all goes well so it may be quite an edge case 🤔

@wiktor-k
Copy link
Owner Author

wiktor-k commented Mar 13, 2024 via email

@baloo
Copy link
Collaborator

baloo commented Mar 13, 2024

Oh yeah, you're right. But we can remove the AgentError::User altogether then right?

@wiktor-k
Copy link
Owner Author

wiktor-k commented Mar 13, 2024 via email

Signed-off-by: Wiktor Kwapisiewicz <[email protected]>
@wiktor-k wiktor-k force-pushed the more-useful-user-error branch from b91f2fc to 678d32c Compare March 14, 2024 08:58
@wiktor-k wiktor-k merged commit 2edc1fb into main Mar 15, 2024
6 checks passed
@wiktor-k wiktor-k deleted the more-useful-user-error branch March 15, 2024 07:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants