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

refactor: improve path handling in iroh dir #1345

Merged
merged 3 commits into from
Aug 12, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 2 additions & 3 deletions iroh/src/commands/doctor.rs
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ use std::{
time::{Duration, Instant},
};

use crate::config::{iroh_config_path, iroh_data_root, Config, CONFIG_FILE_NAME, ENV_PREFIX};
use crate::config::{iroh_config_path, Config, IrohPaths, CONFIG_FILE_NAME, ENV_PREFIX};

use anyhow::Context;
use clap::Subcommand;
Expand Down Expand Up @@ -778,8 +778,7 @@ fn create_secret_key(private_key: PrivateKey) -> anyhow::Result<SecretKey> {
SecretKey::from(bytes)
}
PrivateKey::Local => {
let iroh_data_root = iroh_data_root()?;
let path = iroh_data_root.join("keypair");
let path = IrohPaths::Keypair.with_env()?;
if path.exists() {
let bytes = std::fs::read(&path)?;
let keypair = Keypair::try_from_openssh(bytes)?;
Expand Down
22 changes: 8 additions & 14 deletions iroh/src/commands/provide.rs
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ use quic_rpc::{transport::quinn::QuinnServerEndpoint, ServiceEndpoint};
use tokio::io::AsyncWriteExt;
use tracing::{info_span, Instrument};

use crate::config::iroh_data_root;
use crate::config::IrohPaths;

use super::{
add::{aggregate_add_response, print_add_response},
Expand Down Expand Up @@ -49,20 +49,14 @@ pub async fn run(
);
}

let mut iroh_data_root = iroh_data_root()?;
if !iroh_data_root.is_absolute() {
iroh_data_root = std::env::current_dir()?.join(iroh_data_root);
}
tokio::fs::create_dir_all(&iroh_data_root).await?;
let db = flat::Store::load(&iroh_data_root, &iroh_data_root, rt)
let blob_dir = IrohPaths::BaoFlatStoreComplete.with_env()?;
let partial_blob_dir = IrohPaths::BaoFlatStorePartial.with_env()?;
tokio::fs::create_dir_all(&blob_dir).await?;
tokio::fs::create_dir_all(&partial_blob_dir).await?;
let db = flat::Store::load(&blob_dir, &partial_blob_dir, rt)
.await
.with_context(|| {
format!(
"Failed to load iroh database from {}",
iroh_data_root.display()
)
})?;
let key = Some(iroh_data_root.join("keypair"));
.with_context(|| format!("Failed to load iroh database from {}", blob_dir.display()))?;
let key = Some(IrohPaths::Keypair.with_env()?);
let token = opts.request_token.clone();
let provider = provide(db.clone(), rt, key, opts).await?;
let controller = provider.controller();
Expand Down
80 changes: 78 additions & 2 deletions iroh/src/config.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,11 +2,12 @@

use std::{
collections::HashMap,
env,
env, fmt,
path::{Path, PathBuf},
str::FromStr,
};

use anyhow::{anyhow, Result};
use anyhow::{anyhow, bail, Result};
use config::{Environment, File, Value};
use iroh_net::{
defaults::{default_eu_derp_region, default_na_derp_region},
Expand All @@ -22,6 +23,65 @@ pub const CONFIG_FILE_NAME: &str = "iroh.config.toml";
/// For example, `IROH_PATH=/path/to/config` would set the value of the `Config.path` field
pub const ENV_PREFIX: &str = "IROH";

/// Paths to files or directory within the [`iroh_data_root`] used by Iroh.
#[derive(Debug, Clone, Eq, PartialEq)]
pub enum IrohPaths {
/// Path to the node's private key for the [`iroh_net::PeerId`].
Keypair,
/// Path to the node's [flat-file store](iroh::baomap::flat) for complete blobs.
BaoFlatStoreComplete,
/// Path to the node's [flat-file store](iroh::baomap::flat) for partial blobs.
BaoFlatStorePartial,
}
impl From<&IrohPaths> for &'static str {
fn from(value: &IrohPaths) -> Self {
match value {
IrohPaths::Keypair => "keypair",
IrohPaths::BaoFlatStoreComplete => "blobs.v0",
IrohPaths::BaoFlatStorePartial => "blobs-partial.v0",
}
}
}
impl FromStr for IrohPaths {
type Err = anyhow::Error;
fn from_str(s: &str) -> Result<Self> {
Ok(match s {
"keypair" => Self::Keypair,
"blobs.v0" => Self::BaoFlatStoreComplete,
"blobs-partial.v0" => Self::BaoFlatStorePartial,
_ => bail!("unknown file or directory"),
Copy link
Contributor

Choose a reason for hiding this comment

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

would be good to have a basic test for this

Copy link
Member Author

Choose a reason for hiding this comment

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

added a encode-parse roundtrip test

})
}
}
impl fmt::Display for IrohPaths {
fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
let s: &str = self.into();
write!(f, "{s}")
}
}
impl AsRef<Path> for IrohPaths {
fn as_ref(&self) -> &Path {
let s: &str = self.into();
Path::new(s)
}
}
impl IrohPaths {
/// Get the path for this [`IrohPath`] by joining the name to `IROH_DATA_DIR` environment variable.
pub fn with_env(self) -> Result<PathBuf> {
let mut root = iroh_data_root()?;
if !root.is_absolute() {
root = std::env::current_dir()?.join(root);
Copy link
Contributor

Choose a reason for hiding this comment

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

shouldn't this use https://doc.rust-lang.org/std/path/struct.Path.html#method.canonicalize instead?

Copy link
Member Author

Choose a reason for hiding this comment

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

This comes from current main, I only copied it from src/commands/provide.rs: /~https://github.com/n0-computer/iroh/pull/1345/files#diff-7de268e3454ececdfb93202f8f3bb012d38673ddcdca12fd5f7c5e2a6778dc5bL53
so dunno for sure. Does canonicalize join to env::current_dir() for relative paths? And - is that actually what we want?

Copy link
Contributor

Choose a reason for hiding this comment

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

hmm, okay lets leave it as is for now

}
Ok(self.with_root(root))
}

/// Get the path for this [`IrohPath`] by joining the name to a root directory.
pub fn with_root(self, root: impl AsRef<Path>) -> PathBuf {
let path = root.as_ref().join(self);
path
}
}

/// The configuration for the iroh cli.
#[derive(PartialEq, Eq, Debug, Deserialize, Serialize, Clone)]
#[serde(default)]
Expand Down Expand Up @@ -195,4 +255,20 @@ mod tests {

assert_eq!(config.derp_regions.len(), 2);
}

#[test]
fn test_iroh_paths_parse_roundtrip() {
let kinds = [
IrohPaths::BaoFlatStoreComplete,
IrohPaths::BaoFlatStorePartial,
IrohPaths::Keypair,
];
for iroh_path in &kinds {
let root = PathBuf::from("/tmp");
let path = root.join(iroh_path);
let fname = path.file_name().unwrap().to_str().unwrap();
let parsed = IrohPaths::from_str(fname).unwrap();
assert_eq!(*iroh_path, parsed);
}
}
}
13 changes: 8 additions & 5 deletions iroh/tests/cli.rs
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ use walkdir::WalkDir;

const ADDR: &str = "127.0.0.1:0";
const RPC_PORT: &str = "4999";
const BAO_DIR: &str = "blobs.v0";

fn make_rand_file(size: usize, path: &Path) -> Result<Hash> {
let mut content = vec![0u8; size];
Expand Down Expand Up @@ -206,6 +207,7 @@ fn cli_provide_tree_resume() -> Result<()> {
make_rand_file(5000, &file3)?;
// leave the provider running for the entire test
let provider = make_provider_in(&src_iroh_data_dir, &src, Input::Path, None, None)?;
let src_db_dir = src_iroh_data_dir.join(BAO_DIR);
let count = count_input_files(&src);
let ticket = match_provide_output(&provider, count)?;
// first test - empty work dir
Expand All @@ -221,7 +223,7 @@ fn cli_provide_tree_resume() -> Result<()> {

// second test - full work dir
{
copy_dir_all(&src_iroh_data_dir, &tgt_work_dir)?;
copy_dir_all(&src_db_dir, &tgt_work_dir)?;
let get = make_get_cmd(&ticket, Some(tgt.clone()));
let get_output = get.unchecked().run()?;
assert!(get_output.status.success());
Expand All @@ -233,7 +235,7 @@ fn cli_provide_tree_resume() -> Result<()> {

// third test - partial work dir - remove some large files
{
copy_dir_all(&src_iroh_data_dir, &tgt_work_dir)?;
copy_dir_all(&src_db_dir, &tgt_work_dir)?;
make_partial(&tgt_work_dir, |_hash, size| {
if size == 100000 {
MakePartialResult::Remove
Expand All @@ -252,7 +254,7 @@ fn cli_provide_tree_resume() -> Result<()> {

// fourth test - partial work dir - truncate some large files
{
copy_dir_all(&src_iroh_data_dir, &tgt_work_dir)?;
copy_dir_all(&src_db_dir, &tgt_work_dir)?;
make_partial(tgt_work_dir, |_hash, size| {
if size == 100000 {
MakePartialResult::Truncate(1024 * 32)
Expand Down Expand Up @@ -344,13 +346,14 @@ fn cli_provide_persistence() -> anyhow::Result<()> {
tokio_util::task::LocalPoolHandle::new(1),
);
// should have some data now
let db = Store::load_blocking(&iroh_data_dir, &iroh_data_dir, &rt)?;
let db_path = iroh_data_dir.join(BAO_DIR);
let db = Store::load_blocking(&db_path, &db_path, &rt)?;
let blobs = db.blobs().collect::<Vec<_>>();
assert_eq!(blobs.len(), 2);

provide(&bar_path)?;
// should have more data now
let db = Store::load_blocking(&iroh_data_dir, &iroh_data_dir, &rt)?;
let db = Store::load_blocking(&db_path, &db_path, &rt)?;
let blobs = db.blobs().collect::<Vec<_>>();
assert_eq!(blobs.len(), 4);

Expand Down