fix: Support creating file with open_file_nonblock

Updates the `OpenOptions` with `.create(true)` to support creating a
file when if it doesn't exist. This also brings the functionality
inline with the documentation which notes
`Create and opens a File for writing to it.`. This improves usability
simplifying setting up the logger.

Co-authored-by: Jonathan Woollett-Light <jcawl@amazon.co.uk>
Signed-off-by: Jonathan Woollett-Light <jcawl@amazon.co.uk>
Signed-off-by: Riccardo Mancini <mancio@amazon.com>

Author: Manciukic

docs/getting-started.md

@@ -239,9 +239,6 @@ sudo iptables -t nat -A POSTROUTING -o "$HOST_IFACE" -j MASQUERADE
 API_SOCKET="/tmp/firecracker.socket"
 LOGFILE="./firecracker.log"
 
-# Create log file
-touch $LOGFILE
-
 # Set log file
 sudo curl -X PUT --unix-socket "${API_SOCKET}" \
     --data "{

src/vmm/src/device_manager/mod.rs

@@ -7,7 +7,6 @@
 
 use std::convert::Infallible;
 use std::fmt::Debug;
-use std::os::unix::prelude::OpenOptionsExt;
 use std::path::PathBuf;
 use std::sync::{Arc, Mutex};
 
@@ -36,6 +35,7 @@ use crate::devices::virtio::device::VirtioDevice;
 use crate::devices::virtio::transport::mmio::{IrqTrigger, MmioTransport};
 use crate::resources::VmResources;
 use crate::snapshot::Persist;
+use crate::utils::open_file_write_nonblock;
 use crate::vstate::bus::BusError;
 use crate::vstate::memory::GuestMemoryMmap;
 use crate::{EmulateSerialInitError, EventManager, Vm};
@@ -127,14 +127,7 @@ impl DeviceManager {
         output: Option<&PathBuf>,
     ) -> Result<Arc<Mutex<SerialDevice>>, std::io::Error> {
         let (serial_in, serial_out) = match output {
-            Some(ref path) => (
-                None,
-                std::fs::OpenOptions::new()
-                    .custom_flags(libc::O_NONBLOCK)
-                    .write(true)
-                    .open(path)
-                    .map(SerialOut::File)?,
-            ),
+            Some(path) => (None, open_file_write_nonblock(path).map(SerialOut::File)?),
             None => {
                 Self::set_stdout_nonblocking();
 

src/vmm/src/logger/logging.rs

@@ -3,7 +3,6 @@
 
 use std::fmt::Debug;
 use std::io::Write;
-use std::os::unix::fs::OpenOptionsExt;
 use std::path::PathBuf;
 use std::str::FromStr;
 use std::sync::{Mutex, OnceLock};
@@ -14,6 +13,7 @@ use serde::{Deserialize, Deserializer, Serialize};
 use utils::time::LocalTime;
 
 use super::metrics::{IncMetric, METRICS};
+use crate::utils::open_file_write_nonblock;
 
 /// Default level filter for logger matching the swagger specification
 /// (`src/firecracker/swagger/firecracker.yaml`).
@@ -62,11 +62,7 @@ impl Logger {
         );
 
         if let Some(log_path) = config.log_path {
-            let file = std::fs::OpenOptions::new()
-                .custom_flags(libc::O_NONBLOCK)
-                .write(true)
-                .open(log_path)
-                .map_err(LoggerUpdateError)?;
+            let file = open_file_write_nonblock(&log_path).map_err(LoggerUpdateError)?;
 
             guard.target = Some(file);
         };

src/vmm/src/utils/mod.rs

@@ -10,7 +10,13 @@ pub mod signal;
 /// Module with state machine
 pub mod sm;
 
+use std::fs::{File, OpenOptions};
 use std::num::Wrapping;
+use std::os::unix::fs::OpenOptionsExt;
+use std::path::Path;
+use std::result::Result;
+
+use libc::O_NONBLOCK;
 
 /// How many bits to left-shift by to convert MiB to bytes
 const MIB_TO_BYTES_SHIFT: usize = 20;
@@ -64,3 +70,15 @@ pub const fn align_down(addr: u64, align: u64) -> u64 {
     debug_assert!(align != 0);
     addr & !(align - 1)
 }
+
+/// Create and open a File for writing to it.
+/// In case we open a FIFO, in order to not block the instance if nobody is consuming the message
+/// that is flushed to it, we are opening it with `O_NONBLOCK` flag.
+/// In this case, writing to a pipe will start failing when reaching 64K of unconsumed content.
+pub fn open_file_write_nonblock(path: &Path) -> Result<File, std::io::Error> {
+    OpenOptions::new()
+        .custom_flags(O_NONBLOCK)
+        .create(true)
+        .write(true)
+        .open(path)
+}

src/vmm/src/vmm_config/metrics.rs

@@ -6,8 +6,8 @@ use std::path::PathBuf;
 
 use serde::{Deserialize, Serialize};
 
-use super::open_file_nonblock;
 use crate::logger::{FcLineWriter, METRICS};
+use crate::utils::open_file_write_nonblock;
 
 /// Strongly typed structure used to describe the metrics system.
 #[derive(Clone, Debug, PartialEq, Eq, Deserialize, Serialize)]
@@ -26,7 +26,7 @@ pub enum MetricsConfigError {
 /// Configures the metrics as described in `metrics_cfg`.
 pub fn init_metrics(metrics_cfg: MetricsConfig) -> Result<(), MetricsConfigError> {
     let writer = FcLineWriter::new(
-        open_file_nonblock(&metrics_cfg.metrics_path)
+        open_file_write_nonblock(&metrics_cfg.metrics_path)
             .map_err(|err| MetricsConfigError::InitializationFailure(err.to_string()))?,
     );
     METRICS
@@ -42,12 +42,6 @@ mod tests {
 
     #[test]
     fn test_init_metrics() {
-        // Error case: initializing metrics with invalid pipe returns error.
-        let desc = MetricsConfig {
-            metrics_path: PathBuf::from("not_found_file_metrics"),
-        };
-        init_metrics(desc).unwrap_err();
-
         // Initializing metrics with valid pipe is ok.
         let metrics_file = TempFile::new().unwrap();
         let desc = MetricsConfig {

src/vmm/src/vmm_config/mod.rs

@@ -2,12 +2,8 @@
 // SPDX-License-Identifier: Apache-2.0
 
 use std::convert::{From, TryInto};
-use std::fs::{File, OpenOptions};
 use std::io;
-use std::os::unix::fs::OpenOptionsExt;
-use std::path::Path;
 
-use libc::O_NONBLOCK;
 use serde::{Deserialize, Serialize};
 
 use crate::rate_limiter::{BucketUpdate, RateLimiter, TokenBucket};
@@ -167,18 +163,6 @@ impl RateLimiterConfig {
     }
 }
 
-/// Create and opens a File for writing to it.
-/// In case we open a FIFO, in order to not block the instance if nobody is consuming the message
-/// that is flushed to the two pipes, we are opening it with `O_NONBLOCK` flag.
-/// In this case, writing to a pipe will start failing when reaching 64K of unconsumed content.
-fn open_file_nonblock(path: &Path) -> Result<File, std::io::Error> {
-    OpenOptions::new()
-        .custom_flags(O_NONBLOCK)
-        .read(true)
-        .write(true)
-        .open(path)
-}
-
 #[cfg(test)]
 mod tests {
     use super::*;

tests/integration_tests/functional/test_logging.py

@@ -71,27 +71,6 @@ def check_log_message_format(log_str, instance_id, level, show_level, show_origi
         assert tag_level_no <= configured_level_no
 
 
-def test_log_config_failure(uvm_plain):
-    """
-    Check passing invalid FIFOs is detected and reported as an error.
-    """
-    microvm = uvm_plain
-    microvm.spawn(log_file=None)
-    microvm.basic_config()
-
-    # only works if log level is Debug
-    microvm.time_api_requests = False
-
-    expected_msg = re.escape("No such file or directory (os error 2)")
-    with pytest.raises(RuntimeError, match=expected_msg):
-        microvm.api.logger.put(
-            log_path="invalid log file",
-            level="Info",
-            show_level=True,
-            show_log_origin=True,
-        )
-
-
 def test_api_requests_logs(uvm_plain):
     """
     Test that API requests are logged.