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 3 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
18 changes: 11 additions & 7 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)?;
add_to_preload(&library_path).unwrap();
let mut binary_args = args.binary_args.clone();
binary_args.insert(0, args.binary.clone());
Expand All @@ -127,7 +130,7 @@ fn exec(args: &ExecArgs) -> Result<()> {
}

const CURRENT_VERSION: &str = env!("CARGO_PKG_VERSION");
fn main() {
fn main() -> Result<()> {
registry()
.with(fmt::layer())
.with(EnvFilter::from_default_env())
Expand All @@ -136,9 +139,10 @@ fn main() {

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

Choose a reason for hiding this comment

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

I think you would have warning here if you wont add ?

Copy link
Contributor Author

@thedanvail thedanvail Jun 5, 2022 •
edited

Choose a reason for hiding this comment

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

Nay - the function returns Result<()>, and because we added that to the main() -> Result<()> as well, this compiles as is. If I add the ? operator - Commands::Exec(args) => exec(&args)?, - cargo check returns the following:

error[E0308]: `?` operator has incompatible types
   --> mirrord-cli/src/main.rs:142:33
    |
142 |         Commands::Exec(args) => exec(&args)?,
    |                                 ^^^^^^^^^^^^ expected enum `Result`, found `()`
    |
    = note: `?` operator cannot convert from `()` to `Result<(), anyhow::Error>`
    = note:   expected enum `Result<(), anyhow::Error>`
            found unit type `()`
help: try removing this `?`
    |
142 -         Commands::Exec(args) => exec(&args)?,
142 +         Commands::Exec(args) => exec(&args),
    | 
help: try wrapping the expression in `Ok`
    |
142 |         Commands::Exec(args) => Ok(exec(&args)?),
    |                                 +++            +

For more information about this error, try `rustc --explain E0308`.
error: could not compile `mirrord` due to previous error

Copy link
Member

Choose a reason for hiding this comment

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

I think it'd be best to have ? in the end of all arm branches (on exec + Extract) then outside the match branch (in the end of function) just to have Ok(()) which means that if it got there, we're all good.

Ok(())
}
}
}
Expand Down