remove dc_get_fine_* method and validate_filename

src/context.rs

@@ -10,7 +10,7 @@ use libc::uintptr_t;
 use crate::chat::*;
 use crate::constants::*;
 use crate::contact::*;
-use crate::dc_tools::dc_derive_safe_stem_ext;
+use crate::dc_tools::{dc_copy_file, dc_derive_safe_stem_ext};
 use crate::error::*;
 use crate::events::Event;
 use crate::imap::*;
@@ -162,6 +162,22 @@ impl Context {
         self.blobdir.as_path()
     }
 
+    pub fn copy_to_blobdir(&self, orig_filename: impl AsRef<str>) -> Result<String> {
+        // return a $BLOBDIR/<filename> with the content of orig_filename
+        // copied into it. The <filename> will be safely derived from
+        // orig_filename, and will not clash with existing filenames.
+        let dest = self.new_blob_file(&orig_filename, b"")?;
+        if dc_copy_file(
+            &self,
+            PathBuf::from(orig_filename.as_ref()),
+            PathBuf::from(&dest),
+        ) {
+            Ok(dest)
+        } else {
+            bail!("could not copy {} to {}", orig_filename.as_ref(), dest);
+        }
+    }
+
     pub fn new_blob_file(&self, orig_filename: impl AsRef<str>, data: &[u8]) -> Result<String> {
         // return a $BLOBDIR/<FILENAME> string which corresponds to the
         // respective file in the blobdir, and which contains the data.
@@ -526,16 +542,15 @@ mod tests {
 
         let y = &context.new_blob_file("hello", b"data").unwrap();
         assert!(dc_file_exist(&context, y));
-        assert!(y.starts_with("$BLOBDIR/data-"));
+        assert!(y.starts_with("$BLOBDIR/hello-"));
 
-        let x = &context.new_blob_file("hello", b"data.png").unwrap();
+        let x = &context.new_blob_file("xyz/hello.png", b"data").unwrap();
         assert!(dc_file_exist(&context, x));
-        assert!(x.starts_with("$BLOBDIR"));
+        assert_eq!(x, "$BLOBDIR/hello.png");
 
-        let y = &context.new_blob_file("hello", b"data.png").unwrap();
+        let y = &context.new_blob_file("hello\\world.png", b"data").unwrap();
         assert!(dc_file_exist(&context, y));
-        assert!(y.starts_with("$BLOBDIR/data-"));
+        assert_eq!(y, "$BLOBDIR/world.png");
-        assert!(y.ends_with(".png"));
     }
 
     #[test]

src/dc_tools.rs

@@ -3,7 +3,7 @@
 
 use core::cmp::max;
 use std::borrow::Cow;
-use std::ffi::{CStr, CString, OsString};
+use std::ffi::{CStr, CString};
 use std::path::{Path, PathBuf};
 use std::str::FromStr;
 use std::time::SystemTime;
@@ -596,57 +596,24 @@ pub fn dc_read_file<P: AsRef<std::path::Path>>(
     }
 }
 
-pub(crate) fn dc_get_fine_path_filename(
+pub(crate) fn dc_get_next_backup_path(
-    context: &Context,
     folder: impl AsRef<Path>,
-    desired_filename_suffix: impl AsRef<str>,
+    backup_time: i64,
-) -> PathBuf {
+) -> Result<PathBuf, Error> {
-    let now = time();
-
     let folder = PathBuf::from(folder.as_ref());
-    // XXX sanitize desired_filename eg using
+    let stem = chrono::NaiveDateTime::from_timestamp(backup_time, 0)
-    // https://github.com/kardeiz/sanitize-filename/blob/master/src/lib.rs
+        .format("delta-chat-%Y-%m-%d")
-    let suffix = validate_filename(desired_filename_suffix.as_ref());
+        .to_string();
-    let file_name = PathBuf::from(suffix);
-    let extension = file_name.extension().map(|c| c.clone());
 
-    for i in 0..100_000 {
+    // 64 backup files per day should be enough for everyone
-        let ret = if i == 0 {
+    for i in 0..64 {
-            let mut folder = folder.clone();
+        let mut path = folder.clone();
-            folder.push(&file_name);
+        path.push(format!("{}-{}.bak", stem, i));
-            folder
+        if !path.exists() {
-        } else {
+            return Ok(path);
-            let idx = if i < 100 { i } else { now + i };
-            let file_name = if let Some(stem) = file_name.file_stem() {
-                let mut stem = stem.to_os_string();
-                stem.push(format!("-{}", idx));
-                stem
-            } else {
-                OsString::from(idx.to_string())
-            };
-            let mut folder = folder.clone();
-            folder.push(file_name);
-            if let Some(ext) = extension {
-                folder.set_extension(&ext);
-            }
-            folder
-        };
-
-        if !dc_file_exist(context, &ret) {
-            // fine filename found
-            return ret;
         }
     }
-
+    bail!("could not create backup file, disk full?");
-    panic!("Something is really wrong, you need to clean up your disk");
-}
-
-/// Function modifies the given buffer and replaces all characters not valid in filenames by a "-".
-fn validate_filename(filename: &str) -> String {
-    filename
-        .replace('/', "-")
-        .replace('\\', "-")
-        .replace(':', "-")
 }
 
 pub(crate) fn dc_is_blobdir_path(context: &Context, path: impl AsRef<str>) -> bool {
@@ -674,13 +641,10 @@ pub(crate) fn dc_make_rel_and_copy(context: &Context, path: &mut String) -> bool
         dc_make_rel_path(context, path);
         return true;
     }
-    let blobdir_path = dc_get_fine_path_filename(context, "$BLOBDIR", &path);
+    if let Ok(blobdir_path) = context.copy_to_blobdir(&path) {
-    if dc_copy_file(context, &path, &blobdir_path) {
+        *path = blobdir_path;
-        *path = blobdir_path.to_string_lossy().to_string();
-        dc_make_rel_path(context, path);
         return true;
     }
-
     false
 }
 
@@ -1522,6 +1486,7 @@ mod tests {
         assert!(dc_file_exist(context, &abs_path));
 
         assert!(dc_copy_file(context, "$BLOBDIR/foobar", "$BLOBDIR/dada",));
+
         assert_eq!(dc_get_filebytes(context, "$BLOBDIR/dada",), 7);
 
         let buf = dc_read_file(context, "$BLOBDIR/dada").unwrap();
@@ -1535,16 +1500,7 @@ mod tests {
         assert!(dc_file_exist(context, "$BLOBDIR/foobar-folder",));
         assert!(!dc_delete_file(context, "$BLOBDIR/foobar-folder"));
 
-        let fn0 = dc_get_fine_path_filename(context, "$BLOBDIR", "foobar.dadada");
+        let fn0 = "$BLOBDIR/data.data";
-        let fn0_s = fn0.to_string_lossy();
-        assert!(fn0_s.starts_with("$BLOBDIR/foobar-"));
-        assert!(fn0.extension().unwrap() == "dadada");
-
-        let fn0 = dc_get_fine_path_filename(context, "$BLOBDIR", "hello\\foobar.x");
-        let fn0_s = fn0.to_string_lossy();
-        assert!(fn0_s.starts_with("$BLOBDIR/foobar-"));
-        assert!(fn0_s.ends_with(".x"));
-
         assert!(dc_write_file(context, &fn0, b"content"));
 
         assert!(dc_delete_file(context, &fn0));

src/imex.rs

@@ -574,14 +574,10 @@ fn export_backup(context: &Context, dir: impl AsRef<Path>) -> Result<()> {
 
     let mut delete_dest_file: libc::c_int = 0;
     // get a fine backup file name (the name includes the date so that multiple backup instances are possible)
-    // FIXME: we should write to a temporary file first and rename it on success. this would guarantee the backup is complete. however, currently it is not clear it the import exists in the long run (may be replaced by a restore-from-imap)
+    // FIXME: we should write to a temporary file first and rename it on success. this would guarantee the backup is complete.
-    let now = time();
-    let res = chrono::NaiveDateTime::from_timestamp(now as i64, 0)
-        .format("delta-chat-%Y-%m-%d.bak")
-        .to_string();
-
     // let dest_path_filename = dc_get_next_backup_file(context, dir, res);
-    let dest_path_filename = dc_get_fine_path_filename(context, dir, res);
+    let now = time();
+    let dest_path_filename = dc_get_next_backup_path(dir, now)?;
 
     sql::housekeeping(context);