From 03252bd83fd9f62c5de460abcddd48cb52cc384b Mon Sep 17 00:00:00 2001
From: Egor Ivkov <e.ivkov@picodata.io>
Date: Thu, 30 May 2024 21:26:10 +0300
Subject: [PATCH] fix: use Path and PathBuf by default in all configs and args
this fixes the following bug:
```
picodata run --data-dir tmp/i1/
C> invalid socket path: tmp/i1//admin.sock
[supervisor:214288] no ipc message from child
[supervisor:214288] subprocess 214289 exited with code 1
```
---
src/cli/args.rs | 17 ++++++++++-------
src/cli/run.rs | 4 +++-
src/config.rs | 45 ++++++++++++++++++++++++++-------------------
src/lib.rs | 18 ++++++++++++------
src/pico_service.rs | 11 +++++++----
src/tarantool.rs | 7 ++++---
src/util.rs | 28 +++++++++++++++++-----------
7 files changed, 79 insertions(+), 51 deletions(-)
diff --git a/src/cli/args.rs b/src/cli/args.rs
index 1b398fb5e5..44a5d8fde6 100644
--- a/src/cli/args.rs
+++ b/src/cli/args.rs
@@ -5,6 +5,7 @@ use crate::util::Uppercase;
use clap::Parser;
use std::borrow::Cow;
use std::ffi::{CStr, CString};
+use std::path::PathBuf;
use tarantool::auth::AuthMethod;
use tarantool::log::SayLevel;
use tarantool::tlua;
@@ -29,7 +30,7 @@ pub const CONFIG_PARAMETERS_ENV: &'static str = "PICODATA_CONFIG_PARAMETERS";
// Run
////////////////////////////////////////////////////////////////////////////////
-#[derive(Debug, Parser, tlua::Push, PartialEq)]
+#[derive(Debug, Parser, PartialEq)]
#[clap(about = "Run the picodata instance")]
pub struct Run {
#[clap(long, value_name = "NAME", env = "PICODATA_CLUSTER_ID")]
@@ -43,13 +44,13 @@ pub struct Run {
/// Here the instance persists all of its data.
///
/// By default this is the current working directory (".").
- pub data_dir: Option<String>,
+ pub data_dir: Option<PathBuf>,
#[clap(long, value_name = "PATH", env = "PICODATA_CONFIG_FILE")]
/// Path to configuration file in yaml format.
///
/// By default the "config.yaml" in the data directory is used.
- pub config: Option<String>,
+ pub config: Option<PathBuf>,
#[clap(
short = 'c',
@@ -153,7 +154,7 @@ pub struct Run {
/// At the moment the script is executed, the local storage is
/// already initialized and HTTP server is running (if specified).
/// But the raft node is uninitialized yet.
- pub script: Option<String>,
+ pub script: Option<PathBuf>,
#[clap(long, value_name = "[HOST][:PORT]", env = "PICODATA_HTTP_LISTEN")]
/// Address to start the HTTP server on. The routing API is exposed
@@ -173,11 +174,11 @@ pub struct Run {
///
/// By default the "admin.sock" in the data directory is used.
// TODO: rename to admin_socket
- pub admin_sock: Option<String>,
+ pub admin_sock: Option<PathBuf>,
#[clap(long, value_name = "PATH", env = "PICODATA_PLUGIN_DIR")]
/// Path to directory with plugin files
- pub plugin_dir: Option<String>,
+ pub plugin_dir: Option<PathBuf>,
#[clap(long = "tier", value_name = "TIER", env = "PICODATA_INSTANCE_TIER")]
/// Name of the tier to which the instance will belong.
@@ -196,6 +197,7 @@ pub struct Run {
pub init_cfg: Option<String>,
#[clap(long = "audit", value_name = "PATH", env = "PICODATA_AUDIT_LOG")]
+ // As it's not always a path the value type is left as `String`.
/// Configuration for the audit log.
/// Valid options:
///
@@ -219,6 +221,7 @@ pub struct Run {
pub shredding: bool,
#[clap(long = "log", value_name = "PATH", env = "PICODATA_LOG")]
+ // As it's not always a path the value type is left as `String`.
/// Configuration for the picodata diagnostic log.
/// Valid options:
///
@@ -250,7 +253,7 @@ pub struct Run {
/// Path to a plain-text file with a password for the system user "pico_service".
/// This password will be used for internal communication among instances of
/// picodata, so it must be the same on all instances.
- pub service_password_file: Option<String>,
+ pub service_password_file: Option<PathBuf>,
}
// Copy enum because clap:ArgEnum can't be derived for the foreign SayLevel.
diff --git a/src/cli/run.rs b/src/cli/run.rs
index 6891431fb6..e435bb23ec 100644
--- a/src/cli/run.rs
+++ b/src/cli/run.rs
@@ -1,3 +1,5 @@
+use std::path::Path;
+
use nix::sys::signal;
use nix::sys::termios::{tcgetattr, tcsetattr, SetArg::TCSADRAIN};
use nix::sys::wait::{waitpid, WaitStatus};
@@ -204,7 +206,7 @@ pub fn main(args: args::Run) -> ! {
}
}
-fn rm_tarantool_files(data_dir: &str) {
+fn rm_tarantool_files(data_dir: impl AsRef<Path>) {
std::fs::read_dir(data_dir)
.expect("[supervisor] failed reading data_dir")
.map(|entry| entry.expect("[supervisor] failed reading directory entry"))
diff --git a/src/config.rs b/src/config.rs
index 07d792bef4..7f49b83b5f 100644
--- a/src/config.rs
+++ b/src/config.rs
@@ -21,6 +21,7 @@ use serde_yaml::Value as YamlValue;
use std::collections::HashMap;
use std::io::Write;
use std::path::Path;
+use std::path::PathBuf;
use tarantool::log::SayLevel;
use tarantool::tlua;
@@ -77,13 +78,13 @@ impl PicodataConfig {
validate_args(&args)?;
let cwd = std::env::current_dir();
- let cwd = cwd.as_deref().unwrap_or_else(|_| Path::new(".")).display();
- let default_path = format!("{cwd}/{DEFAULT_CONFIG_FILE_NAME}");
+ let cwd = cwd.as_deref().unwrap_or_else(|_| Path::new("."));
+ let default_path = cwd.join(DEFAULT_CONFIG_FILE_NAME);
let args_path = args.config.clone().map(|path| {
- if Path::new(&path).is_absolute() {
+ if path.is_absolute() {
path
} else {
- format!("{cwd}/{path}")
+ cwd.join(path)
}
});
@@ -91,6 +92,8 @@ impl PicodataConfig {
match (&args_path, file_exists(&default_path)) {
(Some(args_path), true) => {
if args_path != &default_path {
+ let cwd = cwd.display();
+ let args_path = args_path.display();
#[rustfmt::skip]
tlog!(Warning, "A path to configuration file '{args_path}' was provided explicitly,
but a '{DEFAULT_CONFIG_FILE_NAME}' file in the current working directory '{cwd}' also exists.
@@ -104,7 +107,7 @@ Using configuration file '{args_path}'.");
}
(None, true) => {
#[rustfmt::skip]
- tlog!(Info, "Reading configuration file '{DEFAULT_CONFIG_FILE_NAME}' in the current working directory '{cwd}'.");
+ tlog!(Info, "Reading configuration file '{DEFAULT_CONFIG_FILE_NAME}' in the current working directory '{}'.", cwd.display());
config_from_file = Some((Self::read_yaml_file(&default_path)?, &default_path));
}
(None, false) => {}
@@ -185,9 +188,13 @@ Using configuration file '{args_path}'.");
}
#[inline]
- pub fn read_yaml_file(path: &str) -> Result<Box<Self>, Error> {
- let contents = std::fs::read_to_string(path)
- .map_err(|e| Error::other(format!("can't read from '{path}': {e}")))?;
+ pub fn read_yaml_file(path: impl AsRef<Path>) -> Result<Box<Self>, Error> {
+ let contents = std::fs::read_to_string(path.as_ref()).map_err(|e| {
+ Error::other(format!(
+ "can't read from '{}': {e}",
+ path.as_ref().display()
+ ))
+ })?;
Self::read_yaml_contents(&contents)
}
@@ -805,14 +812,14 @@ impl ClusterConfig {
#[derive(PartialEq, Default, Debug, Clone, serde::Deserialize, serde::Serialize, Introspection)]
pub struct InstanceConfig {
- #[introspection(config_default = ".")]
- pub data_dir: Option<String>,
- pub service_password_file: Option<String>,
+ #[introspection(config_default = PathBuf::from("."))]
+ pub data_dir: Option<PathBuf>,
+ pub service_password_file: Option<PathBuf>,
// Skip serializing, so that default config doesn't contain this option,
// which isn't allowed to be set from file.
#[serde(skip_serializing_if = "Option::is_none")]
- pub config_file: Option<String>,
+ pub config_file: Option<PathBuf>,
// Skip serializing, so that default config doesn't contain duplicate
// cluster_id fields.
@@ -851,17 +858,17 @@ pub struct InstanceConfig {
pub http_listen: Option<Address>,
- #[introspection(config_default = format!("{}/admin.sock", self.data_dir.as_ref().unwrap()))]
- pub admin_socket: Option<String>,
+ #[introspection(config_default = self.data_dir.as_ref().map(|dir| dir.join("admin.sock")))]
+ pub admin_socket: Option<PathBuf>,
// TODO:
// - sepparate config file for common parameters
- pub plugin_dir: Option<String>,
+ pub plugin_dir: Option<PathBuf>,
// Skip serializing, so that default config doesn't contain this option,
// because it's deprecated.
#[serde(skip_serializing_if = "Option::is_none")]
- pub deprecated_script: Option<String>,
+ pub deprecated_script: Option<PathBuf>,
pub audit: Option<String>,
@@ -904,7 +911,7 @@ pub struct InstanceConfig {
// TODO: remove all of the .clone() calls from these methods
impl InstanceConfig {
#[inline]
- pub fn data_dir(&self) -> String {
+ pub fn data_dir(&self) -> PathBuf {
self.data_dir
.clone()
.expect("is set in PicodataConfig::set_defaults_explicitly")
@@ -956,7 +963,7 @@ impl InstanceConfig {
}
#[inline]
- pub fn admin_socket(&self) -> String {
+ pub fn admin_socket(&self) -> PathBuf {
self.admin_socket
.clone()
.expect("is set in PicodataConfig::set_defaults_explicitly")
@@ -1706,7 +1713,7 @@ instance:
assert_eq!(config.instance.log.level.unwrap(), args::LogLevel::Debug);
assert_eq!(config.instance.memtx.memory.unwrap(), 0xdead_beef);
assert_eq!(config.instance.audit.unwrap(), "audit.txt");
- assert_eq!(config.instance.data_dir.unwrap(), ".");
+ assert_eq!(config.instance.data_dir.unwrap(), PathBuf::from("."));
//
// Errors
diff --git a/src/lib.rs b/src/lib.rs
index 816184d3fe..ff26f67408 100644
--- a/src/lib.rs
+++ b/src/lib.rs
@@ -7,7 +7,7 @@
#![allow(clippy::redundant_static_lifetimes)]
#![allow(clippy::vec_init_then_push)]
use serde::{Deserialize, Serialize};
-use std::path::{Path, PathBuf};
+use std::path::Path;
use ::raft::prelude as raft;
use ::tarantool::error::Error as TntError;
@@ -562,7 +562,7 @@ fn init_common(
std::fs::create_dir_all(config.instance.data_dir()).unwrap();
if let Some(log_config) = &cfg.log {
- tlog!(Info, "switching to log configuration: {log_config}");
+ tlog!(Info, "switching to log configuration: {}", log_config);
}
// See doc comments in tlog.rs for explanation.
tlog::set_core_logger_is_initialized(true);
@@ -894,8 +894,14 @@ fn postjoin(
// Execute postjoin script if present
if let Some(ref script) = config.instance.deprecated_script {
let l = ::tarantool::lua_state();
- l.exec_with("dofile(...)", script)
- .unwrap_or_else(|err| panic!("failed to execute postjoin script: {err}"))
+ l.exec_with(
+ "dofile(...)",
+ script.to_str().ok_or(Error::other(format!(
+ "postjoin script path {} is not encoded in UTF-8",
+ script.to_string_lossy()
+ )))?,
+ )
+ .unwrap_or_else(|err| panic!("failed to execute postjoin script: {err}"))
}
// Reset the quorum BEFORE initializing the raft node.
@@ -921,7 +927,7 @@ fn postjoin(
.expect("changing listen port shouldn't fail");
// Start admin console
- let socket_uri = util::validate_and_complete_unix_socket_path(&config.instance.admin_socket())?;
+ let socket_uri = util::validate_and_complete_unix_socket_path(config.instance.admin_socket())?;
let lua = ::tarantool::lua_state();
lua.exec_with(r#"require('console').listen(...)"#, &socket_uri)?;
@@ -1038,7 +1044,7 @@ fn postjoin(
let pg_config = &config.instance.pg;
if pg_config.enabled() {
- pgproto::start(pg_config, PathBuf::from(config.instance.data_dir()))?;
+ pgproto::start(pg_config, config.instance.data_dir())?;
}
Ok(())
diff --git a/src/pico_service.rs b/src/pico_service.rs
index c1a80b9537..512baccde1 100644
--- a/src/pico_service.rs
+++ b/src/pico_service.rs
@@ -4,6 +4,7 @@ use crate::unwrap_ok_or;
use std::fs::File;
use std::io::Read;
use std::os::unix::fs::PermissionsExt as _;
+use std::path::Path;
/// Password of the special system user "pico_service".
///
@@ -19,12 +20,14 @@ pub(crate) fn pico_service_password() -> &'static str {
unsafe { PICO_SERVICE_PASSWORD.as_deref() }.unwrap_or("")
}
-pub(crate) fn read_pico_service_password_from_file(filename: &str) -> Result<(), Error> {
- let res = read_file_contents_and_mode(filename);
+pub(crate) fn read_pico_service_password_from_file(
+ filename: impl AsRef<Path>,
+) -> Result<(), Error> {
+ let res = read_file_contents_and_mode(filename.as_ref());
let (data, mode) = unwrap_ok_or!(
res,
Err(e) => {
- return Err(Error::other(format!("failed to read password from file '{filename}': {e}")));
+ return Err(Error::other(format!("failed to read password from file '{}': {e}", filename.as_ref().display())));
}
);
@@ -50,7 +53,7 @@ pub(crate) fn read_pico_service_password_from_file(filename: &str) -> Result<(),
Ok(())
}
-fn read_file_contents_and_mode(filename: &str) -> std::io::Result<(Vec<u8>, u32)> {
+fn read_file_contents_and_mode(filename: impl AsRef<Path>) -> std::io::Result<(Vec<u8>, u32)> {
let mut file = File::open(filename)?;
let metadata = file.metadata()?;
let mode = metadata.permissions().mode();
diff --git a/src/tarantool.rs b/src/tarantool.rs
index 6f237c3cf1..71b65e68c9 100644
--- a/src/tarantool.rs
+++ b/src/tarantool.rs
@@ -15,6 +15,7 @@ use file_shred::*;
use std::collections::HashMap;
use std::ffi::CStr;
use std::os::unix::ffi::OsStrExt;
+use std::path::PathBuf;
use tlua::CallError;
#[macro_export]
@@ -100,9 +101,9 @@ pub struct Cfg {
pub log: Option<String>,
pub log_level: Option<u8>,
- pub wal_dir: String,
- pub memtx_dir: String,
- pub vinyl_dir: String,
+ pub wal_dir: PathBuf,
+ pub memtx_dir: PathBuf,
+ pub vinyl_dir: PathBuf,
#[serde(flatten)]
pub other_fields: HashMap<String, rmpv::Value>,
diff --git a/src/util.rs b/src/util.rs
index 6556162fcb..6d38ad0ef9 100644
--- a/src/util.rs
+++ b/src/util.rs
@@ -8,6 +8,7 @@ use std::io::Write as _;
use std::mem::replace;
use std::os::fd::AsRawFd;
use std::panic::Location;
+use std::path::Path;
use std::time::Duration;
use tarantool::session::{self, UserId};
pub use Either::{Left, Right};
@@ -503,24 +504,29 @@ pub fn prompt_password(prompt: &str) -> Result<String, std::io::Error> {
/// Non-absolute paths are prepended with `./`.
///
/// Returns and error in case validation using lua `uri` module fails.
-pub fn validate_and_complete_unix_socket_path(socket_path: &str) -> Result<String, Error> {
+pub fn validate_and_complete_unix_socket_path(
+ socket_path: impl AsRef<Path>,
+) -> Result<String, Error> {
let l = ::tarantool::lua_state();
- let path = std::path::Path::new(socket_path);
- let console_sock = match path.components().next() {
- Some(std::path::Component::Normal(_)) | Some(std::path::Component::ParentDir) => {
- format!("unix/:./{socket_path}")
- }
- _ => format!("unix/:{socket_path}"),
+ let path = socket_path.as_ref();
+ let path_str = path.to_str().ok_or(Error::other(format!(
+ "socket_path {} is not encoded in UTF-8",
+ socket_path.as_ref().to_string_lossy()
+ )))?;
+ let path_str = if path.is_absolute() {
+ format!("unix/:{path_str}")
+ } else {
+ format!("unix/:./{path_str}")
};
// Check that Lua can correctly parse the unix socket path
l.exec_with(
"local u = require('uri').parse(...); assert(u and u.unix)",
- &console_sock,
+ &path_str,
)
- .map_err(|_| Error::other(format!("invalid socket path: {socket_path}")))?;
+ .map_err(|_| Error::other(format!("invalid socket path: {}", path.display())))?;
- Ok(console_sock)
+ Ok(path_str)
}
////////////////////////////////////////////////////////////////////////////////
@@ -820,7 +826,7 @@ impl<'a> Lexer<'a> {
////////////////////////////////////////////////////////////////////////////////
#[inline(always)]
-pub fn file_exists(path: &str) -> bool {
+pub fn file_exists(path: impl AsRef<Path>) -> bool {
std::fs::metadata(path).is_ok()
}
--
GitLab