Removed all unwraps/panics for all code except testing #36

leonjia0112 · 2018-12-17T18:59:56Z

Removed all the unwraps for all the scripts under src/ directory except the testing suite.
Changes functions' return type to Result<> after removing the unwraps, which has better error handling.
Add a script to CI for catch a list of banned function that could possibly be panicking the thread and terminate the process. Both the shell script and the banned list are both .travis/ directory.

The main change in main.rs is a loop scope is used to wrap around the request, once an error is encountered early exit the loop scope with the response that has the corresponding content regarding the error.

Now there should be now unwraps/panics in the repo and prevent further use of these panic function in this repo.

frozencemetery · 2018-12-21T18:48:58Z

src/secure_mount.rs

    let secure_dir = format!("{}{}", common::WORK_DIR, "/secure");
+    match check_mount(&secure_dir) {
+        Ok(b) => {


Match on Ok(false) and Ok(true) rather than having an if inside the match.

frozencemetery · 2018-12-21T18:50:29Z

src/secure_mount.rs

+                // Create directory if the directory is not exist. The
+                // directory permission is set to 448.
+                if !secure_dir_path.exists() {
+                    match fs::create_dir(secure_dir_path) {


use an if let here

leonjia0112 · 2019-01-02T17:22:41Z

Requested change implemented.

Test passed. Ready for a review.
Thanks!

leonjia0112 · 2019-01-09T16:01:29Z

This is PR that remove all the unwrap in the code base, except the testing suite.

In main, I used loop scope to wrap around the whole function, the function will exit the loop if either finish executing all the code or encounter an error.

In common, the function json_response_content, which was generating a new response this function is called, is changed to set_response_content that a response is borrowed and edit its content based on the given parameters.

In TPM, I simply use a match or if let to remove the unwrap, but many functions' return types are not Result which doesn't support error returning. Therefore, those functions need to be updated with Result return type in the future PR. An issue has been opened regarding this change.

Test passed. Ready for a review.
Thanks!

leonjia0112 · 2019-01-22T19:14:20Z

Add a bash script for catching panic call in CI.

Test Passed. Ready for a review.
Thanks!

.travis/panic-test.sh

frozencemetery · 2019-01-22T19:55:57Z

src/secure_mount.rs

-                    secure_dir
-                );
+            if tokens[0] != "tmpfs" {
+                return emsg(format!("secure storage location {} already mounted on wrong file system type: {}.  Unmount to continue.", secure_dir, tokens[0]).as_str(), None::<String>);


Is there any way you can wrap this better?

Is the message string too long? How about using a shorter message instead?

src/secure_mount.rs

frozencemetery · 2019-01-22T19:58:52Z

src/common.rs

@@ -47,34 +48,35 @@ pub static MOUNT_SECURE: bool = true;
 * 2. There is no space in the json content, but python version response
 * content contains white space in between keys.
 */
-pub fn json_response_content(
+pub fn set_response_content(


Why is this being renamed?

Because I changed the function's implementation from creating a new response each time to modifying the response body. I think the new name is more appropriate for the function.

src/tpm.rs

src/main.rs

.travis/panic-test.sh

leonjia0112 · 2019-01-28T17:58:39Z

Reimplemented the GET/POST request handler in main.rs into two separate functions that are more adequate for error handling using Result.
Test Passed. Ready for a review.
Thanks.

src/secure_mount.rs

src/tpm.rs

frozencemetery · 2019-01-28T22:46:26Z

src/main.rs

@@ -44,7 +46,7 @@ fn main() {
    info!("Starting server...");

    /* Should be port 3000 eventually */
-    let addr = "127.0.0.1:1337".parse().unwrap();
+    let addr = ([127, 0, 0, 1], 3000).into();


Changing port number is not in scope for this PR and would need the comment above removed.

removed changed of port number

src/tpm.rs

frozencemetery · 2019-01-28T22:48:24Z

.travis/panic-test.sh

+
+# Output result
+if [ "$panic_line" -gt 0 ]
+then


This goes on the previous line

Just curious. Would it cause any difference or this is what people normally do?

You did it yourself earlier in the file. It's also pretty normal because you get a block structure similar to C (or Rust) - "then" corresponds to "{", and "fi" to "}".

I see, thanks.

frozencemetery · 2019-01-28T22:49:15Z

.travis/panic-test.sh

+done < "$banned_function"
+
+output=""
+panic_line=0


0 isn't a good sentinel. Consider what'll happen if a banned function appears on the first line of a file.

Changed to using boolean as a sentinel instead of using 0.

leonjia0112 · 2019-01-29T20:11:25Z

Test Passed. Ready for a review.
Thanks.

frozencemetery

Partial review. You still have outstanding comments from previous review.

.travis/panic-test.sh

src/tpm.rs

leonjia0112

Small change request

src/tpm.rs

This code needs cleanup. [rharwood@redhat.com: squash, commit message, some cleanup]

frozencemetery · 2019-02-13T23:01:27Z

As I am quite tired of looking at this PR; I have rewritten the checker in python and squished the results. I'm curious if you even ran the old code; $true and $false aren't built-ins in bash.

Some effort will be spent going forward on a cohesive codebase cleanup.

leonjia0112 · 2019-02-14T15:39:12Z

My apologies. Sorry for making you have this feeling. Will be more careful next time.

leonjia0112 force-pushed the update-secure-mount branch from c7a7d45 to 9936736 Compare December 17, 2018 20:48

frozencemetery suggested changes Dec 21, 2018

View reviewed changes

leonjia0112 force-pushed the update-secure-mount branch 4 times, most recently from 84fc754 to 70fea43 Compare January 2, 2019 17:18

leonjia0112 force-pushed the update-secure-mount branch from 70fea43 to 41ec48a Compare January 9, 2019 15:49

leonjia0112 force-pushed the update-secure-mount branch 2 times, most recently from 083ceee to 91d1d4d Compare January 9, 2019 22:31

leonjia0112 changed the title ~~Add error handling support for secure mount function~~ Removed all unwraps/panics for all code except testing Jan 17, 2019

leonjia0112 force-pushed the update-secure-mount branch from bafe874 to 0752ef2 Compare January 21, 2019 22:31

frozencemetery suggested changes Jan 22, 2019

View reviewed changes

leonjia0112 force-pushed the update-secure-mount branch 7 times, most recently from 37b0145 to a7fafa7 Compare January 28, 2019 17:52

frozencemetery suggested changes Jan 28, 2019

View reviewed changes

leonjia0112 force-pushed the update-secure-mount branch from a7fafa7 to 063d0d9 Compare January 29, 2019 19:48

frozencemetery suggested changes Feb 1, 2019

View reviewed changes

leonjia0112 force-pushed the update-secure-mount branch from 063d0d9 to 3d3d631 Compare February 3, 2019 18:23

leonjia0112 commented Feb 11, 2019

View reviewed changes

src/tpm.rs Outdated Show resolved Hide resolved

src/tpm.rs Outdated Show resolved Hide resolved

leonjia0112 force-pushed the update-secure-mount branch from 3d3d631 to eb3b28c Compare February 11, 2019 21:42

frozencemetery force-pushed the update-secure-mount branch from eb3b28c to 9e7d384 Compare February 13, 2019 21:29

Remove all calls that likely panic

c9ee5e7

This code needs cleanup. [rharwood@redhat.com: squash, commit message, some cleanup]

frozencemetery force-pushed the update-secure-mount branch from 9e7d384 to 6c4f3e1 Compare February 13, 2019 21:38

[travis] Start banning calls to functions that might panic

bc9a10a

frozencemetery force-pushed the update-secure-mount branch from 6c4f3e1 to bc9a10a Compare February 13, 2019 21:53

frozencemetery merged commit c087297 into keylime:master Feb 13, 2019

lukehinds self-requested a review March 8, 2019 08:08

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Removed all unwraps/panics for all code except testing #36

Removed all unwraps/panics for all code except testing #36

leonjia0112 commented Dec 17, 2018 •

edited

Loading

frozencemetery Dec 21, 2018

leonjia0112 Jan 2, 2019

frozencemetery Dec 21, 2018

leonjia0112 Jan 2, 2019

leonjia0112 commented Jan 2, 2019

leonjia0112 commented Jan 9, 2019

leonjia0112 commented Jan 22, 2019

frozencemetery Jan 22, 2019

leonjia0112 Feb 12, 2019

frozencemetery Jan 22, 2019

leonjia0112 Feb 12, 2019

leonjia0112 commented Jan 28, 2019

frozencemetery Jan 28, 2019

leonjia0112 Feb 12, 2019

frozencemetery Jan 28, 2019

leonjia0112 Jan 29, 2019

frozencemetery Jan 29, 2019

leonjia0112 Jan 29, 2019

frozencemetery Jan 28, 2019

leonjia0112 Feb 12, 2019

leonjia0112 commented Jan 29, 2019

frozencemetery left a comment

leonjia0112 left a comment

frozencemetery commented Feb 13, 2019

leonjia0112 commented Feb 14, 2019

Removed all unwraps/panics for all code except testing #36

Removed all unwraps/panics for all code except testing #36

Conversation

leonjia0112 commented Dec 17, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

leonjia0112 commented Jan 2, 2019

leonjia0112 commented Jan 9, 2019

leonjia0112 commented Jan 22, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

leonjia0112 commented Jan 28, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

leonjia0112 commented Jan 29, 2019

frozencemetery left a comment

Choose a reason for hiding this comment

leonjia0112 left a comment

Choose a reason for hiding this comment

frozencemetery commented Feb 13, 2019

leonjia0112 commented Feb 14, 2019

leonjia0112 commented Dec 17, 2018 •

edited

Loading