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

Add KeylimeTpmError support and change return type in tpm.rs #53

Merged
merged 3 commits into from
Mar 21, 2019

Conversation

leonjia0112
Copy link
Contributor

Add support for tpm custom error type. This is aim on solving #40 for changing return type.
Implementation details:

  1. Implement KeylimeTpmError enum has two contribution
    • error generate from TPM command execution
    • error generate from rust part of TPM interface
  2. Implement From trait for Error:
    • serde_json::error::Error
    • std::num::ParseIntError
    • std::string::FromUtf8Error
    • std::io::Error
    • std::time::SystemTimeError
  3. Upate return type of run() function in a tuple and a custom error
    Result<(String, String), TpmExecError>
  4. Support return type of other function in tpm.rs with KeylimeTpmError, which has benefit in code efficient and early return efficient

1. Changed return type with error handling Result<> type
2. Removed rise_on_error parameter, since error is return by Result,
   There is no need to handle error within the run() function.
3. Converted the return outptu to String before returning
4. Changed function comment accordingly
5. Removed error logging within run(). Error information is return by
   returning Err() to Result<>
1. Implement From trait for KeylimeTpmError:
       std::string::FromUtf8Error
       std::io::Error
       std::time::SystemTimeError

2. Change return type of run() function in a tuple and a custom error
       Result<(String, String), KeylimeError>

3. Two enum type representing error cause by different part of tpm
   execution.
       - TpmError: error cause by TPM command execution
       - TpmRustError: error cause by rust function calls
@frozencemetery
Copy link
Contributor

@mbestavros PTAL

Copy link
Contributor

@mbestavros mbestavros left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@frozencemetery, @leonjia0112, things look good to me. Definitely makes the code much more readable, IMO -- a lot less boilerplate.

If I were to look for any improvement, it may help to make some of the more generic errors that KeylimeTpmError inherits a bit more verbose -- for example, it may be helpful for the FromUtf8 or serde_json errors to tell us where exactly it's going wrong, especially if they're being used in a lot of places.

Other than that, I've included a few minor typo fixes inline; nothing major.

src/tpm.rs Outdated Show resolved Hide resolved
src/tpm.rs Outdated Show resolved Hide resolved
src/tpm.rs Outdated Show resolved Hide resolved
@leonjia0112
Copy link
Contributor Author

@mbestavros thanks for catching those typos. Those are handled.

But for the improvement, I wasn't very sure what you mean. Do you suggest putting additional information instead of just using the description function?

@leonjia0112 leonjia0112 force-pushed the update-run branch 2 times, most recently from 025ad02 to 65ed7c5 Compare March 20, 2019 21:52
@frozencemetery frozencemetery merged commit fa01d11 into keylime:master Mar 21, 2019
lkatalin added a commit to lkatalin/rust-keylime that referenced this pull request Mar 8, 2022
It was updated to 2.1 based on Keylime enhancement keylime#53
(agent-local revocation), but this broke communication
with the Python components. We have decided that the
Rust agent should not have its own API version, but
rather follow API versioning from the Python components.

Signed-off-by: Lily Sturmann <[email protected]>
lkatalin added a commit that referenced this pull request Mar 9, 2022
It was updated to 2.1 based on Keylime enhancement #53
(agent-local revocation), but this broke communication
with the Python components. We have decided that the
Rust agent should not have its own API version, but
rather follow API versioning from the Python components.

Signed-off-by: Lily Sturmann <[email protected]>
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.

3 participants