address most of wills review feedback, fix serialization and stringly error handling in DiagTask::start

Author: untitaker

daemon/src/diag.rs

@@ -40,7 +40,7 @@ const DISK_CHECK_BYTES_INTERVAL: usize = 256 * 1024;
 pub enum DiagDeviceCtrlMessage {
     StopRecording,
     StartRecording {
-        response_tx: Option<oneshot::Sender<Result<(), String>>>,
+        response_tx: Option<oneshot::Sender<Result<(), RecordingStoreError>>>,
     },
     DeleteEntry {
         name: String,
@@ -129,7 +129,7 @@ impl DiagTask {
     }
 
     /// Start recording, returning an error if disk space is too low.
-    async fn start(&mut self, qmdl_store: &mut RecordingStore) -> Result<(), String> {
+    async fn start(&mut self, qmdl_store: &mut RecordingStore) -> Result<(), RecordingStoreError> {
         self.max_type_seen = EventType::Informational;
         self.bytes_since_space_check = 0;
         self.low_space_warned = false;
@@ -140,66 +140,54 @@ impl DiagTask {
             self.min_space_to_continue_mb,
         ) {
             DiskSpaceCheck::Critical(mb) | DiskSpaceCheck::Warning(mb) => {
-                let msg = format!(
-                    "Insufficient disk space: {}MB available, {}MB required",
-                    mb, self.min_space_to_start_mb
-                );
-                error!("{msg}");
-                return Err(msg);
+                return Err(RecordingStoreError::InsufficientDiskSpace(
+                    mb,
+                    self.min_space_to_start_mb,
+                ));
             }
             DiskSpaceCheck::Ok(mb) => {
                 info!("Starting recording with {}MB disk space available", mb);
             }
             DiskSpaceCheck::Failed => {}
         }
 
-        let (qmdl_file, analysis_file) = match qmdl_store.new_entry(self.gps_mode).await {
-            Ok(files) => files,
-            Err(e) => {
-                let msg = format!("failed creating QMDL file entry: {e}");
-                error!("{msg}");
-                return Err(msg);
-            }
-        };
+        let (qmdl_file, analysis_file) = qmdl_store.new_entry(self.gps_mode).await?;
+
         // For fixed-mode sessions, write the configured coordinates to the sidecar
         // immediately so the per-session GPS is stored durably and isn't affected
         // by future config changes or GPS API calls.
         if self.gps_mode == GpsMode::Fixed
             && let Some((lat, lon)) = self.gps_fixed_coords
             && let Some((entry_idx, _)) = qmdl_store.get_current_entry()
         {
-            match qmdl_store.open_entry_gps_for_append(entry_idx).await {
-                Ok(Some(mut gps_file)) => {
-                    let record = GpsRecord {
-                        unix_ts: 0,
-                        lat,
-                        lon,
-                    };
-                    if let Ok(json) = serde_json::to_string(&record) {
-                        let _ = gps_file.write_all(format!("{json}\n").as_bytes()).await;
-                    }
-                }
-                Ok(None) => {
-                    error!("GPS sidecar directory not found, cannot write fixed-mode coordinates")
-                }
-                Err(e) => error!("failed to open GPS sidecar for fixed-mode entry: {e}"),
-            }
+            let mut gps_file = qmdl_store
+                .open_entry_gps_for_append(entry_idx)
+                .await?
+                .ok_or(RecordingStoreError::GpsSidecarNotFound)?;
+
+            let record = GpsRecord {
+                unix_ts: chrono::Utc::now().timestamp(),
+                lat,
+                lon,
+            };
+            let json = serde_json::to_string(&record)?;
+            gps_file
+                .write_all(format!("{json}\n").as_bytes())
+                .await
+                .map_err(RecordingStoreError::WriteFileError)?;
         }
+
         self.stop_current_recording().await;
         let qmdl_writer = QmdlWriter::new(qmdl_file);
-        let analysis_writer = match AnalysisWriter::new(analysis_file, &self.analyzer_config).await
-        {
-            Ok(writer) => Box::new(writer),
-            Err(e) => {
-                let msg = format!("failed to create analysis writer: {e}");
-                error!("{msg}");
-                return Err(msg);
-            }
-        };
+        let analysis_writer = AnalysisWriter::new(analysis_file, &self.analyzer_config)
+            .await
+            .map_err(RecordingStoreError::WriteFileError)?;
+
         self.state = DiagState::Recording {
             qmdl_writer,
-            analysis_writer,
+            analysis_writer: Box::new(analysis_writer),
         };
+
         if let Err(e) = self
             .ui_update_sender
             .send(display::DisplayState::Recording)
@@ -530,7 +518,7 @@ pub async fn start_recording(
 
     match response_rx.await {
         Ok(Ok(())) => Ok((StatusCode::ACCEPTED, "ok".to_string())),
-        Ok(Err(reason)) => Err((StatusCode::INSUFFICIENT_STORAGE, reason)),
+        Ok(Err(reason)) => Err((StatusCode::INSUFFICIENT_STORAGE, reason.to_string())),
         Err(e) => Err((
             StatusCode::INTERNAL_SERVER_ERROR,
             format!("failed to receive start recording response: {e}"),

daemon/src/main.rs

@@ -306,14 +306,12 @@ async fn run_with_config(
             config.webdav.clone().into(),
         );
     }
-    // For fixed configuration, we use timestamp 0 to not break other
-    // the GET request for GPS but user won't see the 0 in PCAPs
     let initial_gps = if config.gps_mode == GpsMode::Fixed {
         match (config.gps_fixed_latitude, config.gps_fixed_longitude) {
             (Some(lat), Some(lon)) => Some(gps::GpsData {
                 latitude: lat,
                 longitude: lon,
-                timestamp: 0,
+                timestamp: chrono::Utc::now().timestamp(),
             }),
             _ => {
                 warn!(

daemon/src/qmdl_store.rs

@@ -23,6 +23,8 @@ pub enum RecordingStoreError {
     CreateFileError(tokio::io::Error),
     #[error("Couldn't read file: {0}")]
     ReadFileError(tokio::io::Error),
+    #[error("Couldn't write file: {0}")]
+    WriteFileError(tokio::io::Error),
     #[error("Couldn't delete file: {0}")]
     DeleteFileError(tokio::io::Error),
     #[error("Couldn't open directory at path: {0}")]
@@ -33,6 +35,12 @@ pub enum RecordingStoreError {
     WriteManifestError(tokio::io::Error),
     #[error("Couldn't parse QMDL store manifest file: {0}")]
     ParseManifestError(toml::de::Error),
+    #[error("Insufficient disk space: {0}MB available, {1}MB required")]
+    InsufficientDiskSpace(u64, u64),
+    #[error("GPS sidecar directory not found")]
+    GpsSidecarNotFound,
+    #[error("Serialization error: {0}")]
+    SerializationError(#[from] serde_json::Error),
 }
 
 pub struct RecordingStore {

daemon/src/webdav.rs

@@ -245,6 +245,7 @@ pub fn run_webdav_upload_worker(
 #[cfg(test)]
 mod tests {
     use super::*;
+    use crate::config::GpsMode;
     use axum::{
         Router,
         body::Bytes,

lib/src/pcap.rs

@@ -10,6 +10,7 @@ use pcap_file_tokio::pcapng::blocks::enhanced_packet::{EnhancedPacketBlock, Enha
 use pcap_file_tokio::pcapng::blocks::interface_description::InterfaceDescriptionBlock;
 use pcap_file_tokio::pcapng::blocks::section_header::{SectionHeaderBlock, SectionHeaderOption};
 use pcap_file_tokio::{Endianness, PcapError};
+use serde::Serialize;
 use std::borrow::Cow;
 use thiserror::Error;
 use tokio::io::AsyncWrite;
@@ -26,11 +27,13 @@ pub enum GsmtapPcapError {
     Deku(#[from] DekuError),
 }
 
+#[derive(Serialize)]
 pub struct GpsPoint {
+    pub unix_ts: i64,
+    #[serde(rename = "lat")]
     pub latitude: f64,
+    #[serde(rename = "lon")]
     pub longitude: f64,
-    /// Unix timestamp of the GPS fix. 0 means fixed/synthetic (no real GPS time).
-    pub unix_ts: i64,
 }
 
 pub struct GsmtapPcapWriter<T>
@@ -148,17 +151,7 @@ where
 
         let mut options = vec![];
         if let Some(p) = gps {
-            let comment = if p.unix_ts == 0 {
-                format!(
-                    r#"{{"latitude":{:.7},"longitude":{:.7}}}"#,
-                    p.latitude, p.longitude
-                )
-            } else {
-                format!(
-                    r#"{{"latitude":{:.7},"longitude":{:.7},"timestamp":{}}}"#,
-                    p.latitude, p.longitude, p.unix_ts
-                )
-            };
+            let comment = serde_json::to_string(p).expect("GpsPoint serialization cannot fail");
             options.push(EnhancedPacketOption::Comment(Cow::Owned(comment)));
         }
         let packet = EnhancedPacketBlock {