Repository: pkolaczk/fclones (language: rust)

Issue:
Optionally skip locking (Deduplicating Google Drive mounted on Mac as smbfs fails)
Dear Piotr,

Thank you very much for your excellent software. 

After having used it extensively on my Mac, I tried to used it on my Google Drive repository, which is mounted as a smbfs device.
`
//DRIVE@localhost:58874/Google%20Drive on /Volumes/GoogleDrive (smbfs, nodev, nosuid, nobrowse, mounted by bouge)
`
Unfortunately, `fclones remove` fails with message `fclones:  warn: Failed to lock file`. I assume it is because smbfs does not implement the locking system call.

Fortunately, `fclones move` succeeds, but only if moving to a directory which is within the same file system.

Would it be possible to imagine a way to let `fclones remove` succeed? Maybe adding an option to use a more basic (but somewhat unsafe) locking strategy?

Best regards, Luc.


Discussion / hints:
Sure. I can add option `--no-lock` to skip locking. Thank you for reporting the issue. 

Fix patch:
diff --git a/src/config.rs b/src/config.rs
index 3d09ad9..1261ebe 100644
--- a/src/config.rs
+++ b/src/config.rs
@@ -590,6 +590,10 @@ pub struct DedupeConfig {
     /// `fclones group` command, if `--isolate` option was present.
     #[structopt(long = "isolate", value_name = "path", parse(from_os_str))]
     pub isolated_roots: Vec<Path>,
+
+    /// Skips locking files before performing an action on them.
+    #[structopt(long)]
+    pub no_lock: bool,
 }
 
 #[derive(Debug, StructOpt)]
diff --git a/src/dedupe.rs b/src/dedupe.rs
index 5be0daa..0c1d66e 100644
--- a/src/dedupe.rs
+++ b/src/dedupe.rs
@@ -97,8 +97,20 @@ pub enum FsCommand {
 }
 
 impl FsCommand {
+    /// Obtains a lock to the file if lock == true.
+    fn maybe_lock(path: &Path, lock: bool) -> io::Result<Option<FileLock>> {
+        if lock {
+            match FileLock::new(path) {
+                Ok(lock) => Ok(Some(lock)),
+                Err(e) if e.kind() == ErrorKind::Unsupported => Ok(None),
+                Err(e) => Err(e),
+            }
+        } else {
+            Ok(None)
+        }
+    }
+
     pub fn remove(path: &Path) -> io::Result<()> {
-        let _ = FileLock::new(path)?;
         fs::remove_file(path.to_path_buf()).map_err(|e| {
             io::Error::new(
                 e.kind(),
@@ -248,7 +260,6 @@ impl FsCommand {
         f: impl Fn(&Path) -> io::Result<R>,
         log: &Log,
     ) -> io::Result<R> {
-        let _ = FileLock::new(path)?; // don't remove a locked file
         let tmp = Self::temp_file(path);
         Self::unsafe_rename(path, &tmp)?;
         let result = match f(path) {
@@ -278,21 +289,25 @@ impl FsCommand {
     }
 
     /// Executes the command and returns the number of bytes reclaimed
-    pub fn execute(&self, log: &Log) -> io::Result<FileLen> {
+    pub fn execute(&self, should_lock: bool, log: &Log) -> io::Result<FileLen> {
         match self {
             FsCommand::Remove { file } => {
+                let _ = Self::maybe_lock(&file.path, should_lock)?;
                 Self::remove(&file.path)?;
                 Ok(file.metadata.len())
             }
             FsCommand::SoftLink { target, link } => {
+                let _ = Self::maybe_lock(&link.path, should_lock)?;
                 Self::safe_remove(&link.path, |link| Self::symlink(&target.path, link), log)?;
                 Ok(link.metadata.len())
             }
             FsCommand::HardLink { target, link } => {
+                let _ = Self::maybe_lock(&link.path, should_lock)?;
                 Self::safe_remove(&link.path, |link| Self::hardlink(&target.path, link), log)?;
                 Ok(link.metadata.len())
             }
             FsCommand::RefLink { target, link } => {
+                let _ = Self::maybe_lock(&link.path, should_lock)?;
                 crate::reflink::reflink(target, link, log)?;
                 Ok(link.metadata.len())
             }
@@ -301,6 +316,7 @@ impl FsCommand {
                 target,
                 use_rename,
             } => {
+                let _ = Self::maybe_lock(&source.path, should_lock);
                 let len = source.metadata.len();
                 if *use_rename && Self::move_rename(&source.path, target).is_ok() {
                     return Ok(len);
@@ -867,10 +883,14 @@ where
 /// On command execution failure, a warning is logged and the execution of remaining commands
 /// continues.
 /// Returns the number of files processed and the amount of disk space reclaimed.
-pub fn run_script(script: impl IntoParallelIterator<Item = FsCommand>, log: &Log) -> DedupeResult {
+pub fn run_script(
+    script: impl IntoParallelIterator<Item = FsCommand>,
+    should_lock: bool,
+    log: &Log,
+) -> DedupeResult {
     script
         .into_par_iter()
-        .map(|cmd| cmd.execute(log))
+        .map(|cmd| cmd.execute(should_lock, log))
         .inspect(|res| {
             if let Err(e) = res {
                 log.warn(e);
@@ -954,7 +974,7 @@ mod test {
             create_file(&file_path);
             let file = PathAndMetadata::new(Path::from(&file_path)).unwrap();
             let cmd = FsCommand::Remove { file };
-            cmd.execute(&log).unwrap();
+            cmd.execute(true, &log).unwrap();
             assert!(!file_path.exists())
         })
     }
@@ -972,7 +992,7 @@ mod test {
                 target: target.clone(),
                 use_rename: true,
             };
-            cmd.execute(&log).unwrap();
+            cmd.execute(true, &log).unwrap();
             assert!(!file_path.exists());
             assert!(target.to_path_buf().exists());
         })
@@ -991,7 +1011,7 @@ mod test {
                 target: target.clone(),
                 use_rename: false,
             };
-            cmd.execute(&log).unwrap();
+            cmd.execute(true, &log).unwrap();
             assert!(!file_path.exists());
             assert!(target.to_path_buf().exists());
         })
@@ -1011,7 +1031,7 @@ mod test {
                 target: Path::from(&target),
                 use_rename: false,
             };
-            assert!(cmd.execute(&log).is_err());
+            assert!(cmd.execute(true, &log).is_err());
         })
     }
 
@@ -1030,7 +1050,7 @@ mod test {
                 target: Arc::new(file_1),
                 link: file_2,
             };
-            cmd.execute(&log).unwrap();
+            cmd.execute(true, &log).unwrap();
 
             assert!(file_path_1.exists());
             assert!(file_path_2.exists());
@@ -1058,7 +1078,7 @@ mod test {
                 target: Arc::new(file_1),
                 link: file_2,
             };
-            cmd.execute(&log).unwrap();
+            cmd.execute(true, &log).unwrap();
 
             assert!(file_path_1.exists());
             assert!(file_path_2.exists());
@@ -1268,7 +1288,7 @@ mod test {
             config.priority = vec![Priority::LeastRecentlyModified];
             let log = Log::new();
             let script = dedupe(vec![group], DedupeOp::Remove, &config, &log);
-            let dedupe_result = run_script(script, &log);
+            let dedupe_result = run_script(script, !config.no_lock, &log);
             assert_eq!(dedupe_result.processed_count, 2);
             assert!(!root.join("file_1").exists());
             assert!(!root.join("file_2").exists());
diff --git a/src/error.rs b/src/error.rs
index d8ab9a0..321f00e 100644
--- a/src/error.rs
+++ b/src/error.rs
@@ -1,5 +1,5 @@
-use std::fmt;
 use std::fmt::{Display, Formatter};
+use std::{fmt, io};
 
 /// Error reported by top-level fclones functions
 #[derive(Debug)]
@@ -32,3 +32,13 @@ impl From<&str> for Error {
         Error::new(s.to_owned())
     }
 }
+
+/// Returns error kind.
+/// Maps `libc::EOPNOTSUPP` error to `ErrorKind::Unsupported` on Unix.
+pub fn error_kind(error: &io::Error) -> io::ErrorKind {
+    #[cfg(unix)]
+    if error.raw_os_error() == Some(libc::EOPNOTSUPP) {
+        return io::ErrorKind::Unsupported;
+    }
+    error.kind()
+}
diff --git a/src/lock.rs b/src/lock.rs
index 8366143..da58432 100644
--- a/src/lock.rs
+++ b/src/lock.rs
@@ -1,3 +1,4 @@
+use crate::error::error_kind;
 use std::fs::File;
 use std::{fs, io};
 
@@ -57,21 +58,21 @@ impl FileLock {
     pub fn new(path: &Path) -> io::Result<FileLock> {
         let path_buf = path.to_path_buf();
         let file = fs::OpenOptions::new()
-            .read(true)
+            .read(false)
             .write(true)
             .create(false)
             .open(&path_buf)
             .map_err(|e| {
                 io::Error::new(
-                    e.kind(),
-                    format!("Failed to open file {}: {}", path.display(), e),
+                    error_kind(&e),
+                    format!("Failed to open file {} for write: {}", path.display(), e),
                 )
             })?;
 
         #[cfg(unix)]
         if let Err(e) = Self::fcntl_lock(&file) {
             return Err(io::Error::new(
-                e.kind(),
+                error_kind(&e),
                 format!("Failed to lock file {}: {}", path.display(), e),
             ));
         };
diff --git a/src/main.rs b/src/main.rs
index eff5002..f4baa31 100644
--- a/src/main.rs
+++ b/src/main.rs
@@ -230,7 +230,7 @@ pub fn run_dedupe(op: DedupeOp, config: DedupeConfig, log: &mut Log) -> Result<(
             result.processed_count, upto, result.reclaimed_space
         ));
     } else {
-        let result = run_script(script, log);
+        let result = run_script(script, !dedupe_config.no_lock, log);
         log.info(format!(
             "Processed {} files and reclaimed {}{} space",
             result.processed_count, upto, result.reclaimed_space
diff --git a/src/reflink.rs b/src/reflink.rs
index 67103d6..b204c3c 100644
--- a/src/reflink.rs
+++ b/src/reflink.rs
@@ -5,15 +5,12 @@ use std::io;
 use filetime::FileTime;
 
 use crate::dedupe::{FsCommand, PathAndMetadata};
-use crate::lock::FileLock;
 use crate::log::Log;
 
 /// Calls OS-specific reflink implementations with an option to call the more generic
 /// one during testing one on Linux ("crosstesting").
 /// The destination file is allowed to exist.
 pub fn reflink(src: &PathAndMetadata, dest: &PathAndMetadata, log: &Log) -> io::Result<()> {
-    let _ = FileLock::new(&dest.path)?; // don't touch a locked file
-
     // Remember the original metadata of the parent directory of the destination file:
     let dest_parent = dest.path.parent();
     let dest_parent_metadata = dest_parent.map(|p| p.to_path_buf().metadata());
@@ -281,7 +278,7 @@ pub mod test {
             };
 
             assert!(
-                cmd.execute(&log)
+                cmd.execute(true, &log)
                     .unwrap_err()
                     .to_string()
                     .starts_with("Failed to deduplicate"),
@@ -328,7 +325,7 @@ pub mod test {
 
             if via_ioctl {
                 assert!(cmd
-                    .execute(&log)
+                    .execute(true, &log)
                     .unwrap_err()
                     .to_string()
                     .starts_with("Failed to deduplicate"));
@@ -338,7 +335,7 @@ pub mod test {
                 assert_eq!(read_file(&file_path_1), "foo");
                 assert_eq!(read_file(&file_path_2), "too large");
             } else {
-                cmd.execute(&log).unwrap();
+                cmd.execute(true, &log).unwrap();
 
                 assert!(file_path_2.exists());
                 assert_eq!(read_file(&file_path_2), "foo");
@@ -378,7 +375,7 @@ pub mod test {
                 target: Arc::new(file_1),
                 link: file_2,
             };
-            cmd.execute(&log).unwrap();
+            cmd.execute(true, &log).unwrap();
 
             assert!(file_path_1.exists());
             assert!(file_path_2.exists());


Test patch: