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.

Sign up for GitHub

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

Jump to bottom

Resolves #53 #111

Merged
merged 11 commits into from Jun 7, 2022
Merged

Resolves #53 #111

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
522bfe2
Resolves #53
thedanvail Jun 3, 2022
9be1bff
Refactored for comments
thedanvail Jun 3, 2022
90a4747
removed unwraps, added Result to main return time
thedanvail Jun 5, 2022
9f07ce2
Clarified CHANGELOG.md, reworded extract_library's file creation cont…
thedanvail Jun 5, 2022
1278d00
Merge pull request #1 from metalbear-co/main
thedanvail Jun 5, 2022
14eace0
Adding missed changelog
thedanvail Jun 5, 2022
89e247f
Merge branch 'main' into 53-graceful-exit
thedanvail Jun 5, 2022
de3bed2
Merge pull request #2 from metalbear-co/main
thedanvail Jun 6, 2022
5423d27
Merge pull request #3 from thedanvail/main
thedanvail Jun 6, 2022
6a11a88
Ending all arms with '?'
thedanvail Jun 6, 2022
ca66ffb
Formatting
thedanvail Jun 6, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 2 additions & 0 deletions CHANGELOG.md
View file
Open in desktop
Expand Up @@ -6,6 +6,8 @@ Previous versions had CHANGELOG per component, we decided to combine all reposit
Check [Keep a Changelog](http://keepachangelog.com/) for recommendations on how to structure this file.

## [Unreleased]
### Changed
- Added a graceful exit for system paths that do not exist.
aviramha marked this conversation as resolved.
Show resolved Hide resolved
Copy link
Member

Choose a reason for hiding this comment

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

Reword - "Added graceful exit for library extraction logic in case of error"


## 2.1.0
### Added
Expand Down
15 changes: 9 additions & 6 deletions mirrord-cli/src/main.rs
View file
Open in desktop
@@ -1,6 +1,6 @@
use std::{env::temp_dir, fs::File, io::Write, time::Duration};

use anyhow::{anyhow, Result};
use anyhow::{anyhow, Result, Context};
use clap::{Args, Parser, Subcommand};
use exec::execvp;
use semver::Version;
Expand Down Expand Up @@ -60,19 +60,22 @@ const INJECTION_ENV_VAR: &str = "LD_PRELOAD";
#[cfg(target_os = "macos")]
const INJECTION_ENV_VAR: &str = "DYLD_INSERT_LIBRARIES";

fn extract_library(dest_dir: Option<String>) -> String {
fn extract_library(dest_dir: Option<String>) -> Result<String> {
let library_file = env!("CARGO_CDYLIB_FILE_MIRRORD_LAYER");
let library_path = std::path::Path::new(library_file);
let file_name = library_path.components().last().unwrap();
let file_path = match dest_dir {
Some(dest_dir) => std::path::Path::new(&dest_dir).join(file_name),
None => temp_dir().as_path().join(file_name),
};
let mut file = File::create(file_path.clone()).unwrap();
let mut file = File::create(&file_path)
Copy link
Member

Choose a reason for hiding this comment

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

We can't know for sure the reason it failed is due to path not existing, so I'd just add using context the file path ("Path .. creation failed")

.with_context(|| format!(
"Path \"{}\" does not exist", file_path.display()
))?;
let bytes = include_bytes!(env!("CARGO_CDYLIB_FILE_MIRRORD_LAYER"));
file.write_all(bytes).unwrap();
debug!("Extracted library file to {:?}", &file_path);
file_path.to_str().unwrap().to_string()
Ok(file_path.to_str().unwrap().to_string())
}

fn add_to_preload(path: &str) -> Result<()> {
Expand Down Expand Up @@ -117,7 +120,7 @@ fn exec(args: &ExecArgs) -> Result<()> {
if args.accept_invalid_certificates {
std::env::set_var("MIRRORD_ACCEPT_INVALID_CERTIFICATES", "true");
}
let library_path = extract_library(None);
let library_path = extract_library(None).unwrap();
Copy link
Member

Choose a reason for hiding this comment

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

you can use ? here instead of unwrap

add_to_preload(&library_path).unwrap();
let mut binary_args = args.binary_args.clone();
binary_args.insert(0, args.binary.clone());
Expand All @@ -138,7 +141,7 @@ fn main() {
match cli.commands {
Commands::Exec(args) => exec(&args).unwrap(),
Commands::Extract { path } => {
extract_library(Some(path));
extract_library(Some(path)).unwrap();
Copy link
Member

Choose a reason for hiding this comment

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

Change fn main to return Result<()> then you can use ? here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If I change the main function signature to return Result<()>, then we'd have to remove the unwrap()s/? to return the Result - or we can leave in the unwrap()s/? and leave out Result from the return type - which behavior would be preferred?

Copy link
Member

Choose a reason for hiding this comment

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

? is a syntatic sugar that returns when a Result/Option has Err/None else it returns the inner value.
So the change would to add ? and change the return type to Result
https://doc.rust-lang.org/rust-by-example/std/result/question_mark.html

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I understand that, but the issue is, if I run cargo check with the following code

fn main() -> Result<()> {
    registry()
        .with(fmt::layer())
        .with(EnvFilter::from_default_env())
        .init();
    prompt_outdated_version();

    let cli = Cli::parse();
    match cli.commands {
        Commands::Exec(args) => exec(&args)?,
        Commands::Extract { path } => {
            extract_library(Some(path))?;
        }
    }
}

I receive the following error:

Screen-Shot-2022-06-03-at-2-44-03-PM

Copy link
Member

Choose a reason for hiding this comment

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

Sorry but the image isn't available.. you can upload directly to GitHub by pasting the image here.
Considering the snippet, I think I know what the issue is. You can just drop the ? in both arms and it'd work because both exec and extract_library return Result, so no need to unwrap it and you can just return their return value.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added in your changes - since extract_library returns a Result<String> instead of Result<()>, I just return a normal Ok(()). I think that should cover everything!

}
}
}
Expand Down