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 tpmdata file io functions for keylime to read/write tpm data #16

Merged
merged 1 commit into from
Dec 13, 2018

Conversation

leonjia0112
Copy link
Contributor

@leonjia0112 leonjia0112 commented Nov 26, 2018

Added four functions for read/white/get/set tpmdata.json file where tpm data is stored in tpm.rs. Also added a tpmdata_test.json for testing purpose.

  • Changed .travis.yml to single thread testing, because parallel testing causes the file changed by other tests and fail.
  • Modified tpmdata read function with error handling without panic.

Compare to Lincoln lab version:
Original python version used a global variable tpmdata to store tpmdata in global scope. This implementation eliminated global variable is to store tpm data since it is not a frequent access variable.

Test passed. Ready for a review. Thanks.

Copy link
Contributor

@frozencemetery frozencemetery left a comment

Choose a reason for hiding this comment

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

Please review and cleanup your diff.

.travis.yml Outdated Show resolved Hide resolved
@leonjia0112 leonjia0112 force-pushed the tpmdata-io branch 3 times, most recently from 08c42e5 to 1b63af4 Compare November 29, 2018 21:42
@leonjia0112
Copy link
Contributor Author

Test passed. Ready for another review. I changed Travis back to what it was, and now all the tests should pass without fail. Thanks.

Copy link
Contributor

@frozencemetery frozencemetery left a comment

Choose a reason for hiding this comment

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

By "clean your diff", I intend "remove all changes which do not belong as part of this PR". Changes which are part of this PR should be only the ones required for fulfilling the goals stated in the commit message. (Incidentally, please fix spelling there.)

src/tpm.rs Outdated Show resolved Hide resolved
src/tpm.rs Outdated Show resolved Hide resolved
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

Changed based on review:

  1. Changed error from -1 to message that explains the detail of the error.
  2. Changed Option to String
  3. Used is_ok() in testing
  4. Remove .unwrap() that panic the system with error handling and early return.

Test Passed. Ready for a review.
Thanks.

Copy link
Contributor

@frozencemetery frozencemetery left a comment

Choose a reason for hiding this comment

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

Diff still not clean; see previous review.

@leonjia0112 leonjia0112 force-pushed the tpmdata-io branch 3 times, most recently from d4f7c6f to de135dd Compare December 6, 2018 22:29
@leonjia0112
Copy link
Contributor Author

Now there are only tpm.rs and tpmdata_test.json files are changed in this PR.

Note:
tpmdata struct was removed because it is no longer used by the read_tpm_data() function.
File changed of main.rs and common.rs is because travis is complaining about the formatting, which it wasn't complaining before.

Test passed, ready for a review
Thanks

src/tpm.rs Outdated Show resolved Hide resolved
src/tpm.rs Outdated Show resolved Hide resolved
@leonjia0112 leonjia0112 force-pushed the tpmdata-io branch 3 times, most recently from 6a9a3ba to 8fe00d7 Compare December 10, 2018 17:24
@leonjia0112
Copy link
Contributor Author

leonjia0112 commented Dec 10, 2018

Add a output_error_message() function for Err(Box::new(foramt!(...))) that can take a error message and a error and return a error message string.

.unwrap()s are removed with error handling Result<>.

Rebased onto the new PR.

Test passed. Ready for a review.
Thanks!

1. Add an error message intergration helper function
2. All io support function has error handling result that must be handled
   by caller.
3. For set the tpm data function, it read the file first and write after
   content is set. Use local variable to store the tpm data content instead
   of global in python-keylime.

[[email protected]: renamed function to emsg, shortened code patterns]
@frozencemetery
Copy link
Contributor

@leonjia0112 I've made some changes. Please take a look and if you're satisfied they're okay, I'll merge.

@leonjia0112
Copy link
Contributor Author

Those changes are decent. I am ok with those changes. Thanks :)

@frozencemetery frozencemetery merged commit c2820d0 into keylime:master Dec 13, 2018
@leonjia0112 leonjia0112 mentioned this pull request Dec 13, 2018
@leonjia0112 leonjia0112 deleted the tpmdata-io branch March 13, 2019 01:39
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