Repository: zargony/fuse-rs (language: rust)

Issue:
BackgroundSession does not stop filesystem thread on OS X
If you create a `BackgroundSession` object and destroy it, the background task running the real session won't actually terminate right away on OS X.  It seems that the `libc::read` call is not interrupted by the call to `fuse_unmount_compat22`, so it won't kill the background task until something else tries to use the filesystem.  Unmounting it externally (by running "umount" in another shell) does not appear to have this problem--it interrupts the read call right away.  The problem also doesn't seem to happen on Linux.



Discussion / hints:
Mmh, that's weird. Dropping a `BackgroundSession` actually does an unmount on the mount point which should be the same than using `umount` at the console. It actually does not kill the background task (which wouldn't unmount and therefore leave the system with an inaccessible path and stale inodes until some timeout in the filesystem driver unmounts it). I tried killing the background task before, but neither native blocking reads nor green blocking reads are interruptible in Rust.

Anyway, since it actually does an unmount, that should lead to the background task exiting. Probably that doesn't work reliable when the filesystem is still in use (or even because of #9). Maybe `channel::unmount` should try harder to unmount by checking if the unmount succeeds (without accessing the mount point, since it may still be mounted but stale - it would need to check the kernel's mount table to see if unmount succeeded). If unmount doesn't work after a few tries with a small delay, do a force unmount (`umount -f`) as a last resort. Just like process kill handlers try to send SIGTERM a few times before they finally force a process to quit with SIGKILL.

Ok, now I know what the underlying cause is here.  The problem only appears if the filesystem returns an error for `getattr` on its root.

In osxfuse, `fuse_unmount_compat22` is a very short function that looks like this:

``` c
void
fuse_unmount_compat22(const char *mountpoint)
{
    char resolved_path[PATH_MAX];
    char *rp = realpath(mountpoint, resolved_path);
    if (rp) {
        (void)unmount(resolved_path, 0);
    }

    return;
}
```

The problem is that if the call to `realpath` fails, the error is swallowed and the filesystem is not unmounted.  But the call to `realpath` actually calls `getattr` on the mounted FUSE file system.  If that returns an error, it will never unmount.

The call to `realpath` is there for a reason--I don't think it'll work with a non-canonical path.  I would suggest this as a fix: before mounting, call `realpath` _before_ mounting, and store the resolved path name instead of the path as passed in.  Then at unmount, for OS X only, call `unmount` directly instead of `fuse_unmount_compat22` using the stored real path.  (On Linux, `fuse_unmount_compat22` is more than just `realpath` and then `unmount`, and does not have the underlying problem, so we'll keep it as is.)

Of course, a decent filesystem would always be able to `getattr` its own root anyway, so this isn't that big of a problem in the first place.

Ah, I see. Could this be related to the ioctl we're still missing? For some reason, the C lib sends ioctl FUSEDEVIOCSETDAEMONDEAD to the kernel driver before closing the fd (on OS X only). This sounds like it could be related to that.

Getting the realpath of the mount point shouldn't be hard for us, it's already there in `mountpoint` inside a channel. To make sure it's absolute, we just need to use `os::make_absolute` in `Channel::new` and we could call `libc::umount` instead of `fuse_unmount` in the destructor.

I'm not sure if replacing `fuse_unmount` for certain environments is a good idea. FUSE is doing so much stuff when mounting/unmounting depending on wether it's on Linux, OS X or BSD, depending on which driver version is installed, sometimes it uses fusermount, sometimes not, and so on. I was always glad to leave that weird logic to libfuse instead of replacing it. Can't we just call `libc::umount` and keep doing the `fuse_unmount`? The kernel unmount would bring down the mountpoint so that the following FUSE unmount will work and FUSE has a chance to properly shut down in cases where it's doing more than just calling the kernel unmount (just like if the user unmounts the mountpoint from terminal or in the finder).

I don't think the ioctl has anything to do with this.  This problem doesn't arise when we're unmounting after the session has already been destroyed and we're closing the FD.  It's when we're trying to unmount from another task, outside of the session loop.  (I tried making it close the FD at that point in #12, mistakenly thinking that's what osxfuse did, but you correctly pointed out that this was mistaken and would lead to race conditions. Osxfuse only sends the FUSEDEVIOCSETDAEMONDEAD ioctl, and closes the FD, after the session ends.  We need to continue do the same.)

I agree that we need to keep the `fuse_unmount_compat22` on non-OSX platforms.  Sorry if this wasn't clear in the original comment.  If I understand you correctly, we intend the same thing--call libc's `unmount` on OS X, but continue to use `fuse_unmount_compat22` elsewhere.


Fix patch:
diff --git a/src/channel.rs b/src/channel.rs
index 8724dc3..aa5d921 100644
--- a/src/channel.rs
+++ b/src/channel.rs
@@ -4,13 +4,15 @@
 
 use std::libc::{c_int, c_void, c_char, size_t};
 use std::os;
-use fuse::{fuse_args, fuse_mount_compat25, fuse_unmount_compat22};
+use fuse::{fuse_args, fuse_mount_compat25};
+#[cfg(not(target_os = "macos"))]
+use fuse::fuse_unmount_compat22;
 
 // Libc provides iovec based I/O using readv and writev functions
 #[allow(dead_code)]
 #[allow(non_camel_case_types)]
 mod libc {
-	use std::libc::{c_int, c_void, size_t, ssize_t};
+	use std::libc::{c_char, c_int, c_void, size_t, ssize_t};
 
 	/// Iovec data structure for readv and writev calls.
 	pub struct iovec {
@@ -23,7 +25,33 @@ mod libc {
 		pub fn readv (fd: c_int, iov: *mut iovec, iovcnt: c_int) -> ssize_t;
 		/// Write data from multiple buffers to fd
 		pub fn writev (fd: c_int, iov: *iovec, iovcnt: c_int) -> ssize_t;
+
+		pub fn realpath (file_name:*c_char, resolved_name:*mut c_char) -> *c_char;
+
+		#[cfg(target_os = "macos")]
+		pub fn unmount(dir:*c_char, flags:c_int) -> c_int;
 	}
+
+	#[cfg(target_os = "macos")]
+	pub static PATH_MAX:int = 1024;
+
+	#[cfg(target_os = "linux")]
+	pub static PATH_MAX:int = 4096;
+}
+
+/// Wrapper around libc's realpath.  Returns the errno value if the real path cannot be obtained.
+fn real_path(path:&PosixPath) -> Result<PosixPath, c_int> {
+	path.with_c_str(|p| {
+		let mut resolved = [0 as c_char, ..libc::PATH_MAX];
+		unsafe {
+			let rp = libc::realpath(p, resolved.as_mut_ptr());
+			if rp.is_null() { 
+				Err(::std::os::errno() as c_int)
+			} else { 
+				Ok(PosixPath::new(::std::c_str::CString::new(rp,false)))
+			}
+		}
+	})
 }
 
 /// Helper function to provide options as a fuse_args struct
@@ -52,7 +80,7 @@ impl Channel {
 	/// the given path to the channel. If the channel is dropped, the path is
 	/// unmounted.
 	pub fn new (mountpoint: &Path, options: &[&[u8]]) -> Result<Channel, c_int> {
-		mountpoint.with_c_str(|mnt| {
+		real_path(mountpoint).and_then(|mountpoint| mountpoint.with_c_str(|mnt| {
 			with_fuse_args(options, |args| {
 				let fd = unsafe { fuse_mount_compat25(mnt, args) };
 				if fd < 0 {
@@ -61,7 +89,7 @@ impl Channel {
 					Ok(Channel { mountpoint: mountpoint.clone(), fd: fd })
 				}
 			})
-		})
+		}))
 	}
 
 	/// Receives data up to the capacity of the given buffer.
@@ -122,6 +150,19 @@ impl ChannelSender {
 	}
 }
 
+// On OS X, fuse_unmount_compat22 attempts to call realpath, which in turn calls into the filesystem.
+// If the filesystem returns an error, the unmount does not take place, with no indication of the error
+// available to the caller.  So we call unmount directly, which is what osxfuse does anyway, since
+// we already converted to the real path when we first mounted.
+#[cfg(target_os = "macos")]
+/// Unmount an arbitrary mount point
+pub fn unmount (mountpoint: &Path) {
+	mountpoint.with_c_str(|mnt| {
+		unsafe { libc::unmount(mnt, 0); }
+	})
+}
+
+#[cfg(not(target_os = "macos"))]
 /// Unmount an arbitrary mount point
 pub fn unmount (mountpoint: &Path) {
 	mountpoint.with_c_str(|mnt| {
diff --git a/src/fuse.rs b/src/fuse.rs
index a200999..4bd2080 100644
--- a/src/fuse.rs
+++ b/src/fuse.rs
@@ -33,6 +33,7 @@ pub struct fuse_args {
 
 extern "system" {
 	pub fn fuse_mount_compat25 (mountpoint: *c_char, args: *fuse_args) -> c_int;
+	#[cfg(not(target_os = "macos"))]
 	pub fn fuse_unmount_compat22 (mountpoint: *c_char);
 }
 


Test patch: