feat: Use Option<PathBuf> && write log to stdout by default

Signed-off-by: GFX9 <[email protected]>

crates/curp/src/server/curp_node.rs

@@ -28,8 +28,10 @@ use utils::{
 use super::{
     cmd_board::{CmdBoardRef, CommandBoard},
     cmd_worker::{conflict_checked_mpmc, start_cmd_workers},
-    conflict::spec_pool_new::{SpObject, SpeculativePool},
-    conflict::uncommitted_pool::{UcpObject, UncommittedPool},
+    conflict::{
+        spec_pool_new::{SpObject, SpeculativePool},
+        uncommitted_pool::{UcpObject, UncommittedPool},
+    },
     gc::gc_cmd_board,
     lease_manager::LeaseManager,
     raw_curp::{AppendEntries, RawCurp, Vote},

crates/curp/src/server/mod.rs

@@ -10,8 +10,10 @@ use utils::ClientTlsConfig;
 use utils::{config::CurpConfig, task_manager::TaskManager, tracing::Extract};
 
 use self::curp_node::CurpNode;
-pub use self::raw_curp::RawCurp;
-pub use self::{conflict::spec_pool_new::SpObject, conflict::uncommitted_pool::UcpObject};
+pub use self::{
+    conflict::{spec_pool_new::SpObject, uncommitted_pool::UcpObject},
+    raw_curp::RawCurp,
+};
 use crate::{
     cmd::{Command, CommandExecutor},
     members::{ClusterInfo, ServerId},

crates/curp/src/server/raw_curp/mod.rs

@@ -47,9 +47,11 @@ use self::{
     state::{CandidateState, LeaderState, State},
 };
 use super::{
-    cmd_worker::CEEventTxApi, conflict::spec_pool_new::SpeculativePool,
-    conflict::uncommitted_pool::UncommittedPool, lease_manager::LeaseManagerRef,
-    storage::StorageApi, DB,
+    cmd_worker::CEEventTxApi,
+    conflict::{spec_pool_new::SpeculativePool, uncommitted_pool::UncommittedPool},
+    lease_manager::LeaseManagerRef,
+    storage::StorageApi,
+    DB,
 };
 use crate::{
     cmd::Command,

crates/utils/Cargo.toml

@@ -29,6 +29,7 @@ parking_lot = { version = "0.12.2", optional = true }
 pbkdf2 = { version = "0.12.2", features = ["simple"] }
 petgraph = "0.6.4"
 rand = "0.8.5"
+regex = "1.10.4"
 serde = { version = "1.0.199", features = ["derive"] }
 thiserror = "1.0.61"
 tokio = { version = "0.2.25", package = "madsim-tokio", features = [

crates/utils/src/config.rs

@@ -801,9 +801,9 @@ impl LogConfig {
     /// Generate a new `LogConfig` object
     #[must_use]
     #[inline]
-    pub fn new(path: PathBuf, rotation: RotationConfig, level: LevelConfig) -> Self {
+    pub fn new(path: Option<PathBuf>, rotation: RotationConfig, level: LevelConfig) -> Self {
         Self {
-            path: Some(path),
+            path,
             rotation,
             level,
         }
@@ -1326,7 +1326,7 @@ mod tests {
         assert_eq!(
             config.log,
             LogConfig::new(
-                PathBuf::from("/var/log/xline"),
+                Some(PathBuf::from("/var/log/xline")),
                 RotationConfig::Daily,
                 LevelConfig::INFO
             )
@@ -1452,7 +1452,7 @@ mod tests {
         assert_eq!(
             config.log,
             LogConfig::new(
-                PathBuf::from("/var/log/xline"),
+                Some(PathBuf::from("/var/log/xline")),
                 RotationConfig::Daily,
                 LevelConfig::INFO
             )

crates/utils/src/parser.rs

@@ -1,6 +1,7 @@
-use std::{collections::HashMap, time::Duration};
+use std::{collections::HashMap, path::PathBuf, time::Duration};
 
 use clippy_utilities::OverflowArithmetic;
+use regex::Regex;
 use thiserror::Error;
 
 use crate::config::{
@@ -162,6 +163,37 @@ pub fn parse_state(s: &str) -> Result<InitialClusterState, ConfigParseError> {
     }
 }
 
+/// Parse `LOG_PATH` from string
+/// # Errors
+/// Return error when parsing the given string to `PathBuf` failed
+#[inline]
+pub fn parse_log_file(s: &str) -> Result<PathBuf, ConfigParseError> {
+    if s.is_empty() {
+        return Err(ConfigParseError::InvalidValue(
+            "the log path should not be empty".to_owned(),
+        ));
+    }
+    // This regular expression matches file paths in a specific format.
+    // Explanation of the regex pattern:
+    // ^(\.|\.\.|~)?            - Matches an optional prefix consisting of a dot (.), two dots (..), or a tilde (~).
+    // (/[a-zA-Z0-9._-]+)+/?    - Matches one or more occurrences of a forward slash (/) followed by one or more alphanumeric characters, dots (.), underscores (_), or hyphens (-).
+    // /?$                      - Matches an optional trailing forward slash (/) at the end of the string. // Overall, this regex pattern is used to validate file paths that follow
+    //                            a specific format commonly used in systems like Cargo in Rust.
+    let re = Regex::new(r"^(\.|\.\.|~)?(/[a-zA-Z0-9._-]+)+/?$").unwrap_or_else(|err| {
+        unreachable!(
+            r"regex expression (^(\.|\.\.|~)?(/[a-zA-Z0-9._-]+)+/?$) should not return Error: {}",
+            err.to_string()
+        )
+    });
+    if !re.is_match(s) {
+        Err(ConfigParseError::InvalidValue(format!(
+            "Invalid log path {s}"
+        )))
+    } else {
+        Ok(PathBuf::from(s))
+    }
+}
+
 /// Parse `LevelConfig` from string
 /// # Errors
 /// Return error when parsing the given string to `LevelConfig` failed
@@ -366,4 +398,30 @@ mod test {
         );
         assert!(parse_metrics_push_protocol("thrift").is_err());
     }
+
+    #[test]
+    fn test_parse_log_file() {
+        // Test case 1: Valid log file path
+        assert!(parse_log_file("/path/to/log/file.log").is_ok());
+
+        // Test case 2: Empty log file path
+        assert!(parse_log_file("").is_err());
+
+        // Test case 4: Log file path with spaces
+        assert!(parse_log_file("/path/with spaces/log file.log").is_err());
+
+        // Test case 5: Log file path with special character
+        assert!(parse_log_file("/path/with@spaces/log?!file.log").is_err());
+
+        // Test case 6: Log file path with special character
+        assert!(parse_log_file("/path/with-spaces/log_file.log-123.456-789").is_ok());
+
+        // Test case 7: Log file path starts with ., .. or ~
+        assert!(parse_log_file("./path/with-spaces/log_file.log-123.456-789").is_ok());
+        assert!(parse_log_file("../path/with-spaces/log_file.log-123.456-789").is_ok());
+        assert!(parse_log_file("~/path/with-spaces/log_file.log-123.456-789").is_ok());
+
+        assert!(parse_log_file(".../path/with-spaces/log_file.log-123.456-789").is_err());
+        assert!(parse_log_file("~~/path/with-spaces/log_file.log-123.456-789").is_err());
+    }
 }

crates/xline/src/utils/args.rs

@@ -20,8 +20,8 @@ use utils::{
         MetricsPushProtocol, RotationConfig, ServerTimeout, StorageConfig, TlsConfig, TraceConfig,
         XlineServerConfig,
     },
-    parse_batch_bytes, parse_duration, parse_log_level, parse_members, parse_metrics_push_protocol,
-    parse_rotation, parse_state, ConfigFileError,
+    parse_batch_bytes, parse_duration, parse_log_file, parse_log_level, parse_members,
+    parse_metrics_push_protocol, parse_rotation, parse_state, ConfigFileError,
 };
 
 /// Xline server config path env name
@@ -92,8 +92,8 @@ pub struct ServerArgs {
     #[clap(long, value_parser = parse_metrics_push_protocol, default_value_t = default_metrics_push_protocol())]
     metrics_push_protocol: MetricsPushProtocol,
     /// Log file path
-    #[clap(long, default_value = "/stdout")]
-    log_file: PathBuf,
+    #[clap(long, value_parser = parse_log_file, default_value = None)]
+    log_file: Option<PathBuf>,
     /// Log rotate strategy, eg: never, hourly, daily
     #[clap(long, value_parser = parse_rotation, default_value_t = default_rotation())]
     log_rotate: RotationConfig,

crates/xline/src/utils/trace.rs

@@ -8,15 +8,13 @@ use utils::config::{default_rotation, file_appender, LogConfig, TraceConfig};
 
 /// Return a Box trait from the config
 fn generate_writer(name: &str, log_config: &LogConfig) -> Box<dyn std::io::Write + Send> {
-    match *log_config.path() {
-        Some(ref file_path) if file_path.to_string_lossy() == "/stdout" => {
-            if *log_config.rotation() != default_rotation() {
-                warn!("The log is output to the terminal, so the rotation parameter is ignored.");
-            }
-            Box::new(std::io::stdout())
+    if let Some(ref file_path) = *log_config.path() {
+        Box::new(file_appender(*log_config.rotation(), file_path, name))
+    } else {
+        if *log_config.rotation() != default_rotation() {
+            warn!("The log is output to the terminal, so the rotation parameter is ignored.");
         }
-        Some(ref file_path) => Box::new(file_appender(*log_config.rotation(), file_path, name)),
-        None => unreachable!("the path of log cannot be empty in config"),
+        Box::new(std::io::stdout())
     }
 }
 

scripts/common.sh

@@ -37,10 +37,10 @@ function common::run_xline() {
     ith=${1}
     mount_point="-v ${DIR}:/mnt"
     if [ -n "$LOG_PATH" ]; then
-        mkdir -p ${LOG_PATH}/node${ith}
-        mount_point="${mount_point} -v ${LOG_PATH}/node${ith}:/var/log/xline"
+        mkdir -p ${DIR}/logs/node${ith}
+        mount_point="${mount_point} -v ${DIR}/logs/node${ith}:${LOG_PATH}"
     fi
-    
+
     log::info starting container node${ith} ...
     cmd="/usr/local/bin/xline \
     --name node${1} \
@@ -56,7 +56,7 @@ function common::run_xline() {
     --initial-cluster-state=${3}"
 
     if [ -n "${LOG_PATH}" ]; then
-        cmd="${cmd} --log-file /var/log/xline"
+        cmd="${cmd} --log-file ${LOG_PATH}"
     fi
 
     if [ -n "$LOG_LEVEL" ]; then
@@ -68,18 +68,19 @@ function common::run_xline() {
     fi
 
     if [ -n "$RUST_LOG" ]; then
-        docker run -e RUST_LOG=${RUST_LOG} -d -it --rm --name=node${ith} --net=xline_net \
+        docker_cmd="docker run -e RUST_LOG=${RUST_LOG} -d -it --rm --name=node${ith} --net=xline_net \
             --ip=${SERVERS[$ith]} --cap-add=NET_ADMIN --cpu-shares=1024 \
-            -m=512M ${mount_point} ${XLINE_IMAGE} ${cmd}
-    else 
-        docker run -d -it --rm --name=node${ith} --net=xline_net \
+            -m=512M ${mount_point} ${XLINE_IMAGE} ${cmd}"
+    else
+        docker_cmd="docker run -d -it --rm --name=node${ith} --net=xline_net \
             --ip=${SERVERS[$ith]} --cap-add=NET_ADMIN --cpu-shares=1024 \
-            -m=512M ${mount_point} ${XLINE_IMAGE} ${cmd}
+            -m=512M ${mount_point} ${XLINE_IMAGE} ${cmd}"
     fi
-
-
-
+
+    eval ${docker_cmd}
     wait $!
 
-    log::info "container node${ith} started: ${cmd}"
+    log::info "${docker_cmd}"
+
+
 }

scripts/quick_start.sh

@@ -11,11 +11,7 @@ source $DIR/log.sh
 stop_all() {
     log::info stopping
     for name in "node1" "node2" "node3" "client"; do
-        docker_id=$(docker ps -qf "name=${name}")
-        if [ -n "$docker_id" ]; then
-            docker exec $docker_id rm -rf $LOG_PATH/$name
-            docker stop $docker_id -t 1
-        fi
+        common::stop_container ${name}
     done
     docker network rm xline_net >/dev/null 2>&1
     docker stop "prometheus" > /dev/null 2>&1

scripts/validation_test.sh

@@ -6,7 +6,7 @@ source $DIR/log.sh
 QUICK_START="${DIR}/quick_start.sh"
 ETCDCTL="docker exec -i client etcdctl --endpoints=http://172.20.0.3:2379,http://172.20.0.4:2379"
 LOCK_CLIENT="docker exec -i client /mnt/validation_lock_client --endpoints=http://172.20.0.3:2379,http://172.20.0.4:2379,http://172.20.0.5:2379"
-export LOG_PATH=/mnt/logs
+export LOG_PATH=/var/log/xline
 export LOG_LEVEL=debug
 
 bash ${QUICK_START}
@@ -30,7 +30,6 @@ function run_new_member() {
     if [ -n "$docker_id" ]; then
         docker stop $docker_id
     fi
-    common::run_container 4
     common::run_xline 4 ${NEWMEMBERS} existing
 }