[pbs-devel] [PATCH 0/7] fuse_loop cleanups and named mappings

[pbs-devel] [PATCH 0/7] fuse_loop cleanups and named mappings

[Stefan Reiter]

First three patches are independent cleanups/documentation.

Patch 4 is the meat of the matter, switching over to using names generated from
the mapped image to identify mappings. This allows easier unmap by having a
specific tab-completion handler. Also allows listing all current mappings and
where they are mapped to with the 'unmap' command without parameters.

The last 3 patches are for better cleanup handling. With all three in place I
could not come up with a realistic situation in which a user would have to rely
on external tools to cancel a connection or clean up.


proxmox-backup: Stefan Reiter (7):
  format: fix typo in function name
  fuse_loop: add documentation
  loopdev: add module doc
  mount/map: use names for map/unmap for easier use
  fuse_loop: add automatic cleanup of run files and dangling instances
  fuse_loop: wait for instance to close after killing
  fuse_loop: handle unmap on crashed instance

 src/bin/proxmox-backup-client.rs       |   6 +-
 src/bin/proxmox_backup_client/mount.rs |  58 +++++-
 src/client/backup_writer.rs            |   2 +-
 src/tools/format.rs                    |   4 +-
 src/tools/fuse_loop.rs                 | 257 ++++++++++++++++++++-----
 src/tools/loopdev.rs                   |   2 +
 6 files changed, 268 insertions(+), 61 deletions(-)

-- 
2.20.1

[pbs-devel] [PATCH proxmox-backup 1/7] format: fix typo in function name

[Stefan Reiter]

Signed-off-by: Stefan Reiter <s.reiter@proxmox.com>
---
 src/bin/proxmox-backup-client.rs | 6 +++---
 src/client/backup_writer.rs      | 2 +-
 src/tools/format.rs              | 4 ++--
 3 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/src/bin/proxmox-backup-client.rs b/src/bin/proxmox-backup-client.rs
index 5a6ed4fe..97398f49 100644
--- a/src/bin/proxmox-backup-client.rs
+++ b/src/bin/proxmox-backup-client.rs
@@ -1817,7 +1817,7 @@ async fn complete_server_file_name_do(param: &HashMap<String, String>) -> Vec<St
 fn complete_archive_name(arg: &str, param: &HashMap<String, String>) -> Vec<String> {
     complete_server_file_name(arg, param)
         .iter()
-        .map(|v| tools::format::strip_server_file_expenstion(&v))
+        .map(|v| tools::format::strip_server_file_extension(&v))
         .collect()
 }
 
@@ -1826,7 +1826,7 @@ pub fn complete_pxar_archive_name(arg: &str, param: &HashMap<String, String>) ->
         .iter()
         .filter_map(|name| {
             if name.ends_with(".pxar.didx") {
-                Some(tools::format::strip_server_file_expenstion(name))
+                Some(tools::format::strip_server_file_extension(name))
             } else {
                 None
             }
@@ -1839,7 +1839,7 @@ pub fn complete_img_archive_name(arg: &str, param: &HashMap<String, String>) ->
         .iter()
         .filter_map(|name| {
             if name.ends_with(".img.fidx") {
-                Some(tools::format::strip_server_file_expenstion(name))
+                Some(tools::format::strip_server_file_extension(name))
             } else {
                 None
             }
diff --git a/src/client/backup_writer.rs b/src/client/backup_writer.rs
index b3fd3703..fc0eccfc 100644
--- a/src/client/backup_writer.rs
+++ b/src/client/backup_writer.rs
@@ -262,7 +262,7 @@ impl BackupWriter {
         let archive = if self.verbose {
             archive_name.to_string()
         } else {
-            crate::tools::format::strip_server_file_expenstion(archive_name.clone())
+            crate::tools::format::strip_server_file_extension(archive_name.clone())
         };
         if archive_name != CATALOG_NAME {
             let speed: HumanByte = ((uploaded * 1_000_000) / (duration.as_micros() as usize)).into();
diff --git a/src/tools/format.rs b/src/tools/format.rs
index 98946689..c3aa791b 100644
--- a/src/tools/format.rs
+++ b/src/tools/format.rs
@@ -1,7 +1,7 @@
 use anyhow::{Error};
 use serde_json::Value;
 
-pub fn strip_server_file_expenstion(name: &str) -> String {
+pub fn strip_server_file_extension(name: &str) -> String {
 
     if name.ends_with(".didx") || name.ends_with(".fidx") || name.ends_with(".blob") {
         name[..name.len()-5].to_owned()
@@ -12,7 +12,7 @@ pub fn strip_server_file_expenstion(name: &str) -> String {
 
 pub fn render_backup_file_list(files: &[String]) -> String {
     let mut files: Vec<String> = files.iter()
-        .map(|v| strip_server_file_expenstion(&v))
+        .map(|v| strip_server_file_extension(&v))
         .collect();
 
     files.sort();
-- 
2.20.1

[pbs-devel] [PATCH proxmox-backup 2/7] fuse_loop: add documentation

[Stefan Reiter]

Signed-off-by: Stefan Reiter <s.reiter@proxmox.com>
---
 src/tools/fuse_loop.rs | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/src/tools/fuse_loop.rs b/src/tools/fuse_loop.rs
index 8c1e04aa..cdad0230 100644
--- a/src/tools/fuse_loop.rs
+++ b/src/tools/fuse_loop.rs
@@ -1,3 +1,5 @@
+//! Map a raw data reader as a loop device via FUSE
+
 use anyhow::{Error, format_err, bail};
 use std::ffi::OsStr;
 use std::path::{Path, PathBuf};
@@ -18,6 +20,10 @@ use super::loopdev;
 
 const RUN_DIR: &'static str = "/run/pbs-loopdev";
 
+/// Represents an ongoing FUSE-session that has been mapped onto a loop device.
+/// Create with map_loop, then call 'main' and poll until startup_chan reports
+/// success. Then, daemonize or otherwise finish setup, and continue polling
+/// main's future until completion.
 pub struct FuseLoopSession<R: AsyncRead + AsyncSeek + Unpin> {
     session: Option<Fuse>,
     stat: libc::stat,
-- 
2.20.1

[pbs-devel] [PATCH proxmox-backup 3/7] loopdev: add module doc

[Stefan Reiter]

Signed-off-by: Stefan Reiter <s.reiter@proxmox.com>
---
 src/tools/loopdev.rs | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/src/tools/loopdev.rs b/src/tools/loopdev.rs
index 9f1d6fd1..68918dfd 100644
--- a/src/tools/loopdev.rs
+++ b/src/tools/loopdev.rs
@@ -1,3 +1,5 @@
+//! Helpers to work with /dev/loop* devices
+
 use anyhow::Error;
 use std::fs::{File, OpenOptions};
 use std::path::Path;
-- 
2.20.1

[pbs-devel] [PATCH proxmox-backup 4/7] mount/map: use names for map/unmap for easier use

[Stefan Reiter]

So user doesn't need to remember which loop devices he has mapped to
what.

systemd unit encoding is used to transform a unique identifier for the
mapped image into a suitable name. The files created in /run/pbs-loopdev
will be named accordingly.

The encoding all happens outside fuse_loop.rs, so the fuse_loop module
does not need to care about encodings - it can always assume a name is a
valid filename.

'unmap' without parameter displays all current mappings. It's
autocompletion handler will list the names of all currently mapped
images for easy selection. Unmap by /dev/loopX or loopdev number is
maintained, as those can be distinguished from mapping names.

Signed-off-by: Stefan Reiter <s.reiter@proxmox.com>
---
 src/bin/proxmox_backup_client/mount.rs |  56 ++++++++++---
 src/tools/fuse_loop.rs                 | 106 ++++++++++++++++++++++---
 2 files changed, 140 insertions(+), 22 deletions(-)

diff --git a/src/bin/proxmox_backup_client/mount.rs b/src/bin/proxmox_backup_client/mount.rs
index 4cadec55..ad06cba4 100644
--- a/src/bin/proxmox_backup_client/mount.rs
+++ b/src/bin/proxmox_backup_client/mount.rs
@@ -4,6 +4,7 @@ use std::os::unix::io::RawFd;
 use std::path::Path;
 use std::ffi::OsStr;
 use std::collections::HashMap;
+use std::hash::BuildHasher;
 
 use anyhow::{bail, format_err, Error};
 use serde_json::Value;
@@ -81,7 +82,9 @@ const API_METHOD_UNMAP: ApiMethod = ApiMethod::new(
     &ObjectSchema::new(
         "Unmap a loop device mapped with 'map' and release all resources.",
         &sorted!([
-            ("loopdev", false, &StringSchema::new("Path to loopdev (/dev/loopX) or loop device number.").schema()),
+            ("name", true, &StringSchema::new(
+                "Archive name, path to loopdev (/dev/loopX) or loop device number. Omit to list all current mappings."
+            ).schema()),
         ]),
     )
 );
@@ -108,8 +111,20 @@ pub fn map_cmd_def() -> CliCommand {
 pub fn unmap_cmd_def() -> CliCommand {
 
     CliCommand::new(&API_METHOD_UNMAP)
-        .arg_param(&["loopdev"])
-        .completion_cb("loopdev", tools::complete_file_name)
+        .arg_param(&["name"])
+        .completion_cb("name", complete_mapping_names)
+}
+
+fn complete_mapping_names<S: BuildHasher>(_arg: &str, _param: &HashMap<String, String, S>)
+    -> Vec<String>
+{
+    match tools::fuse_loop::find_all_mappings() {
+        Ok(mappings) => mappings
+            .filter_map(|(name, _)| {
+                tools::systemd::unescape_unit(&name).ok()
+            }).collect(),
+        Err(_) => Vec::new()
+    }
 }
 
 fn mount(
@@ -262,7 +277,10 @@ async fn mount_do(param: Value, pipe: Option<RawFd>) -> Result<Value, Error> {
         let chunk_reader = RemoteChunkReader::new(client.clone(), crypt_config, file_info.chunk_crypt_mode(), HashMap::new());
         let reader = AsyncIndexReader::new(index, chunk_reader);
 
-        let mut session = tools::fuse_loop::FuseLoopSession::map_loop(size, reader, options).await?;
+        let name = &format!("{}:{}/{}", repo.to_string(), path, archive_name);
+        let name_escaped = tools::systemd::escape_unit(name, false);
+
+        let mut session = tools::fuse_loop::FuseLoopSession::map_loop(size, reader, &name_escaped, options).await?;
         let loopdev = session.loopdev_path.clone();
 
         let (st_send, st_recv) = futures::channel::mpsc::channel(1);
@@ -288,7 +306,7 @@ async fn mount_do(param: Value, pipe: Option<RawFd>) -> Result<Value, Error> {
         }
 
         // daemonize only now to be able to print mapped loopdev or startup errors
-        println!("Image mapped as {}", loopdev);
+        println!("Image '{}' mapped on {}", name, loopdev);
         daemonize()?;
 
         // continue polling until complete or interrupted (which also happens on unmap)
@@ -316,13 +334,33 @@ fn unmap(
     _rpcenv: &mut dyn RpcEnvironment,
 ) -> Result<Value, Error> {
 
-    let mut path = tools::required_string_param(&param, "loopdev")?.to_owned();
+    let mut name = match param["name"].as_str() {
+        Some(name) => name.to_owned(),
+        None => {
+            let mut any = false;
+            for (backing, loopdev) in tools::fuse_loop::find_all_mappings()? {
+                let name = tools::systemd::unescape_unit(&backing)?;
+                println!("{}:\t{}", loopdev.unwrap_or("(unmapped)".to_owned()), name);
+                any = true;
+            }
+            if !any {
+                println!("Nothing mapped.");
+            }
+            return Ok(Value::Null);
+        },
+    };
 
-    if let Ok(num) = path.parse::<u8>() {
-        path = format!("/dev/loop{}", num);
+    // allow loop device number alone
+    if let Ok(num) = name.parse::<u8>() {
+        name = format!("/dev/loop{}", num);
     }
 
-    tools::fuse_loop::unmap(path)?;
+    if name.starts_with("/dev/loop") {
+        tools::fuse_loop::unmap_loopdev(name)?;
+    } else {
+        let name = tools::systemd::escape_unit(&name, false);
+        tools::fuse_loop::unmap_name(name)?;
+    }
 
     Ok(Value::Null)
 }
diff --git a/src/tools/fuse_loop.rs b/src/tools/fuse_loop.rs
index cdad0230..f0d19acc 100644
--- a/src/tools/fuse_loop.rs
+++ b/src/tools/fuse_loop.rs
@@ -3,23 +3,29 @@
 use anyhow::{Error, format_err, bail};
 use std::ffi::OsStr;
 use std::path::{Path, PathBuf};
-use std::fs::{File, remove_file, read_to_string};
+use std::fs::{File, remove_file, read_to_string, OpenOptions};
 use std::io::SeekFrom;
 use std::io::prelude::*;
+use std::collections::HashMap;
 
-use nix::unistd::{Pid, mkstemp};
+use nix::unistd::Pid;
 use nix::sys::signal::{self, Signal};
 
 use tokio::io::{AsyncRead, AsyncSeek, AsyncReadExt, AsyncSeekExt};
 use futures::stream::{StreamExt, TryStreamExt};
 use futures::channel::mpsc::{Sender, Receiver};
 
-use proxmox::try_block;
+use proxmox::{try_block, const_regex};
 use proxmox_fuse::{*, requests::FuseRequest};
 use super::loopdev;
+use super::fs;
 
 const RUN_DIR: &'static str = "/run/pbs-loopdev";
 
+const_regex! {
+    pub LOOPDEV_REGEX = r"^loop\d+$";
+}
+
 /// Represents an ongoing FUSE-session that has been mapped onto a loop device.
 /// Create with map_loop, then call 'main' and poll until startup_chan reports
 /// success. Then, daemonize or otherwise finish setup, and continue polling
@@ -37,19 +43,29 @@ impl<R: AsyncRead + AsyncSeek + Unpin> FuseLoopSession<R> {
 
     /// Prepare for mapping the given reader as a block device node at
     /// /dev/loopN. Creates a temporary file for FUSE and a PID file for unmap.
-    pub async fn map_loop(size: u64, mut reader: R, options: &OsStr)
+    pub async fn map_loop<P: AsRef<str>>(size: u64, mut reader: R, name: P, options: &OsStr)
         -> Result<Self, Error>
     {
         // attempt a single read to check if the reader is configured correctly
         let _ = reader.read_u8().await?;
 
         std::fs::create_dir_all(RUN_DIR)?;
-        let mut base_path = PathBuf::from(RUN_DIR);
-        base_path.push("XXXXXX"); // template for mkstemp
-        let (_, path) = mkstemp(&base_path)?;
+        let mut path = PathBuf::from(RUN_DIR);
+        path.push(name.as_ref());
         let mut pid_path = path.clone();
         pid_path.set_extension("pid");
 
+        match OpenOptions::new().write(true).create_new(true).open(&path) {
+            Ok(_) => { /* file created, continue on */ },
+            Err(e) => {
+                if e.kind() == std::io::ErrorKind::AlreadyExists {
+                    bail!("the given archive is already mapped, cannot map twice");
+                } else {
+                    bail!("error while creating backing file ({:?}) - {}", &path, e);
+                }
+            },
+        }
+
         let res: Result<(Fuse, String), Error> = try_block!{
             let session = Fuse::builder("pbs-block-dev")?
                 .options_os(options)?
@@ -213,12 +229,7 @@ impl<R: AsyncRead + AsyncSeek + Unpin> FuseLoopSession<R> {
     }
 }
 
-/// Try and unmap a running proxmox-backup-client instance from the given
-/// /dev/loopN device
-pub fn unmap(loopdev: String) -> Result<(), Error> {
-    if loopdev.len() < 10 || !loopdev.starts_with("/dev/loop") {
-        bail!("malformed loopdev path, must be in format '/dev/loopX'");
-    }
+fn get_backing_file(loopdev: &str) -> Result<String, Error> {
     let num = loopdev.split_at(9).1.parse::<u8>().map_err(|err|
         format_err!("malformed loopdev path, does not end with valid number - {}", err))?;
 
@@ -232,6 +243,7 @@ pub fn unmap(loopdev: String) -> Result<(), Error> {
     })?;
 
     let backing_file = backing_file.trim();
+
     if !backing_file.starts_with(RUN_DIR) {
         bail!(
             "loopdev {} is in use, but not by proxmox-backup-client (mapped to '{}')",
@@ -240,6 +252,10 @@ pub fn unmap(loopdev: String) -> Result<(), Error> {
         );
     }
 
+    Ok(backing_file.to_owned())
+}
+
+fn unmap_from_backing(backing_file: &Path) -> Result<(), Error> {
     let mut pid_path = PathBuf::from(backing_file);
     pid_path.set_extension("pid");
 
@@ -254,6 +270,70 @@ pub fn unmap(loopdev: String) -> Result<(), Error> {
     Ok(())
 }
 
+/// Returns an Iterator over a set of currently active mappings, i.e.
+/// FuseLoopSession instances. Returns ("backing-file-name", Some("/dev/loopX"))
+/// where .1 is None when a user has manually called 'losetup -d' or similar but
+/// the FUSE instance is still running.
+pub fn find_all_mappings() -> Result<impl Iterator<Item = (String, Option<String>)>, Error> {
+    // get map of all /dev/loop mappings belonging to us
+    let mut loopmap = HashMap::new();
+    for ent in fs::scan_subdir(libc::AT_FDCWD, Path::new("/dev/"), &LOOPDEV_REGEX)? {
+        match ent {
+            Ok(ent) => {
+                let loopdev = format!("/dev/{}", ent.file_name().to_string_lossy());
+                match get_backing_file(&loopdev) {
+                    Ok(file) => {
+                        // insert filename only, strip RUN_DIR/
+                        loopmap.insert(file[RUN_DIR.len()+1..].to_owned(), loopdev);
+                    },
+                    Err(_) => {},
+                }
+            },
+            Err(_) => {},
+        }
+    }
+
+    Ok(fs::read_subdir(libc::AT_FDCWD, Path::new(RUN_DIR))?
+        .filter_map(move |ent| {
+            match ent {
+                Ok(ent) => {
+                    let file = ent.file_name().to_string_lossy();
+                    if file == "." || file == ".." || file.ends_with(".pid") {
+                        None
+                    } else {
+                        let loopdev = loopmap.get(file.as_ref()).map(String::to_owned);
+                        Some((file.into_owned(), loopdev))
+                    }
+                },
+                Err(_) => None,
+            }
+        }))
+}
+
+/// Try and unmap a running proxmox-backup-client instance from the given
+/// /dev/loopN device
+pub fn unmap_loopdev<S: AsRef<str>>(loopdev: S) -> Result<(), Error> {
+    let loopdev = loopdev.as_ref();
+    if loopdev.len() < 10 || !loopdev.starts_with("/dev/loop") {
+        bail!("malformed loopdev path, must be in format '/dev/loopX'");
+    }
+
+    let backing_file = get_backing_file(loopdev)?;
+    unmap_from_backing(Path::new(&backing_file))
+}
+
+/// Try and unmap a running proxmox-backup-client instance from the given name
+pub fn unmap_name<S: AsRef<str>>(name: S) -> Result<(), Error> {
+    for (mapping, _) in find_all_mappings()? {
+        if mapping.ends_with(name.as_ref()) {
+            let mut path = PathBuf::from(RUN_DIR);
+            path.push(&mapping);
+            return unmap_from_backing(&path);
+        }
+    }
+    Err(format_err!("no mapping for name '{}' found", name.as_ref()))
+}
+
 fn minimal_stat(size: i64) -> libc::stat {
     let mut stat: libc::stat = unsafe { std::mem::zeroed() };
     stat.st_mode = libc::S_IFREG;
-- 
2.20.1

[pbs-devel] [PATCH proxmox-backup 5/7] fuse_loop: add automatic cleanup of run files and dangling instances

[Stefan Reiter]

A 'map' call will only clean up what it needs, that is only leftover
files or dangling instances of it's own name.

For a full cleanup the user can call 'unmap' without any arguments.

The 'cleanup on error' behaviour of map_loop is removed. It is no longer
needed (since the next call will clean up anyway), and in fact fixes a
bug where trying to map an image twice would result in an error, but
also cleanup the .pid file of the running instance, causing 'unmap' to
fail afterwards.

Signed-off-by: Stefan Reiter <s.reiter@proxmox.com>
---
 src/bin/proxmox_backup_client/mount.rs |  4 +-
 src/tools/fuse_loop.rs                 | 88 ++++++++++++++++----------
 2 files changed, 59 insertions(+), 33 deletions(-)

diff --git a/src/bin/proxmox_backup_client/mount.rs b/src/bin/proxmox_backup_client/mount.rs
index ad06cba4..8d4c7133 100644
--- a/src/bin/proxmox_backup_client/mount.rs
+++ b/src/bin/proxmox_backup_client/mount.rs
@@ -83,7 +83,8 @@ const API_METHOD_UNMAP: ApiMethod = ApiMethod::new(
         "Unmap a loop device mapped with 'map' and release all resources.",
         &sorted!([
             ("name", true, &StringSchema::new(
-                "Archive name, path to loopdev (/dev/loopX) or loop device number. Omit to list all current mappings."
+                concat!("Archive name, path to loopdev (/dev/loopX) or loop device number. ",
+                        "Omit to list all current mappings and force cleaning up leftover instances.")
             ).schema()),
         ]),
     )
@@ -337,6 +338,7 @@ fn unmap(
     let mut name = match param["name"].as_str() {
         Some(name) => name.to_owned(),
         None => {
+            tools::fuse_loop::cleanup_unused_run_files(None);
             let mut any = false;
             for (backing, loopdev) in tools::fuse_loop::find_all_mappings()? {
                 let name = tools::systemd::unescape_unit(&backing)?;
diff --git a/src/tools/fuse_loop.rs b/src/tools/fuse_loop.rs
index f0d19acc..da186b4c 100644
--- a/src/tools/fuse_loop.rs
+++ b/src/tools/fuse_loop.rs
@@ -15,7 +15,7 @@ use tokio::io::{AsyncRead, AsyncSeek, AsyncReadExt, AsyncSeekExt};
 use futures::stream::{StreamExt, TryStreamExt};
 use futures::channel::mpsc::{Sender, Receiver};
 
-use proxmox::{try_block, const_regex};
+use proxmox::const_regex;
 use proxmox_fuse::{*, requests::FuseRequest};
 use super::loopdev;
 use super::fs;
@@ -55,6 +55,11 @@ impl<R: AsyncRead + AsyncSeek + Unpin> FuseLoopSession<R> {
         let mut pid_path = path.clone();
         pid_path.set_extension("pid");
 
+        // cleanup previous instance with same name
+        // if loopdev is actually still mapped, this will do nothing and the
+        // create_new below will fail as intended
+        cleanup_unused_run_files(Some(name.as_ref().to_owned()));
+
         match OpenOptions::new().write(true).create_new(true).open(&path) {
             Ok(_) => { /* file created, continue on */ },
             Err(e) => {
@@ -66,40 +71,27 @@ impl<R: AsyncRead + AsyncSeek + Unpin> FuseLoopSession<R> {
             },
         }
 
-        let res: Result<(Fuse, String), Error> = try_block!{
-            let session = Fuse::builder("pbs-block-dev")?
-                .options_os(options)?
-                .enable_read()
-                .build()?
-                .mount(&path)?;
+        let session = Fuse::builder("pbs-block-dev")?
+            .options_os(options)?
+            .enable_read()
+            .build()?
+            .mount(&path)?;
 
-            let loopdev_path = loopdev::get_or_create_free_dev().map_err(|err| {
-                format_err!("loop-control GET_FREE failed - {}", err)
-            })?;
+        let loopdev_path = loopdev::get_or_create_free_dev().map_err(|err| {
+            format_err!("loop-control GET_FREE failed - {}", err)
+        })?;
 
-            // write pidfile so unmap can later send us a signal to exit
-            Self::write_pidfile(&pid_path)?;
+        // write pidfile so unmap can later send us a signal to exit
+        Self::write_pidfile(&pid_path)?;
 
-            Ok((session, loopdev_path))
-        };
-
-        match res {
-            Ok((session, loopdev_path)) =>
-                Ok(Self {
-                    session: Some(session),
-                    reader,
-                    stat: minimal_stat(size as i64),
-                    fuse_path: path.to_string_lossy().into_owned(),
-                    pid_path: pid_path.to_string_lossy().into_owned(),
-                    loopdev_path,
-                }),
-            Err(e) => {
-                // best-effort temp file cleanup in case of error
-                let _ = remove_file(&path);
-                let _ = remove_file(&pid_path);
-                Err(e)
-            }
-        }
+        Ok(Self {
+            session: Some(session),
+            reader,
+            stat: minimal_stat(size as i64),
+            fuse_path: path.to_string_lossy().into_owned(),
+            pid_path: pid_path.to_string_lossy().into_owned(),
+            loopdev_path,
+        })
     }
 
     fn write_pidfile(path: &Path) -> Result<(), Error> {
@@ -229,6 +221,38 @@ impl<R: AsyncRead + AsyncSeek + Unpin> FuseLoopSession<R> {
     }
 }
 
+/// Clean up leftover files as well as FUSE instances without a loop device
+/// connected. Best effort, never returns an error.
+/// If filter_name is Some("..."), only this name will be cleaned up.
+pub fn cleanup_unused_run_files(filter_name: Option<String>) {
+    if let Ok(maps) = find_all_mappings() {
+        for (name, loopdev) in maps {
+            if loopdev.is_none() &&
+                (filter_name.is_none() || &name == filter_name.as_ref().unwrap())
+            {
+                let mut path = PathBuf::from(RUN_DIR);
+                path.push(&name);
+
+                // clean leftover FUSE instances (e.g. user called 'losetup -d' or similar)
+                // does nothing if files are already stagnant (e.g. instance crashed etc...)
+                if let Ok(_) = unmap_from_backing(&path) {
+                    // we have reaped some leftover instance, tell the user
+                    eprintln!(
+                        "Cleaned up dangling mapping '{}': no loop device assigned",
+                        &name
+                    );
+                }
+
+                // remove remnant files
+                // these we're not doing anything, so no need to inform the user
+                let _ = remove_file(&path);
+                path.set_extension("pid");
+                let _ = remove_file(&path);
+            }
+        }
+    }
+}
+
 fn get_backing_file(loopdev: &str) -> Result<String, Error> {
     let num = loopdev.split_at(9).1.parse::<u8>().map_err(|err|
         format_err!("malformed loopdev path, does not end with valid number - {}", err))?;
-- 
2.20.1

[pbs-devel] [PATCH proxmox-backup 6/7] fuse_loop: wait for instance to close after killing

[Stefan Reiter]

On unmap, only report success if the instance we are killing actually
terminates. This is especially important so that cleanup routines can be
assured that /run files are actually cleaned up after calling
cleanup_unused_run_files.

Signed-off-by: Stefan Reiter <s.reiter@proxmox.com>
---
 src/tools/fuse_loop.rs | 23 ++++++++++++++++++++++-
 1 file changed, 22 insertions(+), 1 deletion(-)

diff --git a/src/tools/fuse_loop.rs b/src/tools/fuse_loop.rs
index da186b4c..05d92525 100644
--- a/src/tools/fuse_loop.rs
+++ b/src/tools/fuse_loop.rs
@@ -16,6 +16,7 @@ use futures::stream::{StreamExt, TryStreamExt};
 use futures::channel::mpsc::{Sender, Receiver};
 
 use proxmox::const_regex;
+use proxmox::tools::time;
 use proxmox_fuse::{*, requests::FuseRequest};
 use super::loopdev;
 use super::fs;
@@ -288,8 +289,28 @@ fn unmap_from_backing(backing_file: &Path) -> Result<(), Error> {
     let pid = pid_str.parse::<i32>().map_err(|err|
         format_err!("malformed PID ({}) in pidfile - {}", pid_str, err))?;
 
+    let pid = Pid::from_raw(pid);
+
     // send SIGINT to trigger cleanup and exit in target process
-    signal::kill(Pid::from_raw(pid), Signal::SIGINT)?;
+    signal::kill(pid, Signal::SIGINT)?;
+
+    // block until unmap is complete or timeout
+    let start = time::epoch_i64();
+    loop {
+        match signal::kill(pid, None) {
+            Ok(_) => {
+                // 10 second timeout, then assume failure
+                if (time::epoch_i64() - start) > 10 {
+                    return Err(format_err!("timed out waiting for PID '{}' to exit", &pid));
+                }
+                std::thread::sleep(std::time::Duration::from_millis(100));
+            },
+            Err(nix::Error::Sys(nix::errno::Errno::ESRCH)) => {
+                break;
+            },
+            Err(e) => return Err(e.into()),
+        }
+    }
 
     Ok(())
 }
-- 
2.20.1

[pbs-devel] [PATCH proxmox-backup 7/7] fuse_loop: handle unmap on crashed instance

[Stefan Reiter]

If a fuse_loop instance dies suddenly (e.g. SIGKILL), the FUSE mount and
loop device assignment are left behind. We can determine this scenario
on specific unmap, when the PID file is either missing or contains a PID
of a non-running process, but the backing file and potentially loop
device are still there.

If that's the case, do an "emergency cleanup", by unassigning the
loopdev, calling 'fusermount -u' and then cleaning any leftover files
manually.

With this in place, pretty much any situation is now recoverable via
only the 'proxmox-backup-client' binary, by either calling 'unmap' with
or without parameters.

Signed-off-by: Stefan Reiter <s.reiter@proxmox.com>
---
 src/tools/fuse_loop.rs | 50 +++++++++++++++++++++++++++++++++++-------
 1 file changed, 42 insertions(+), 8 deletions(-)

diff --git a/src/tools/fuse_loop.rs b/src/tools/fuse_loop.rs
index 05d92525..ab733f27 100644
--- a/src/tools/fuse_loop.rs
+++ b/src/tools/fuse_loop.rs
@@ -236,7 +236,7 @@ pub fn cleanup_unused_run_files(filter_name: Option<String>) {
 
                 // clean leftover FUSE instances (e.g. user called 'losetup -d' or similar)
                 // does nothing if files are already stagnant (e.g. instance crashed etc...)
-                if let Ok(_) = unmap_from_backing(&path) {
+                if let Ok(_) = unmap_from_backing(&path, None) {
                     // we have reaped some leftover instance, tell the user
                     eprintln!(
                         "Cleaned up dangling mapping '{}': no loop device assigned",
@@ -280,19 +280,53 @@ fn get_backing_file(loopdev: &str) -> Result<String, Error> {
     Ok(backing_file.to_owned())
 }
 
-fn unmap_from_backing(backing_file: &Path) -> Result<(), Error> {
+// call in broken state: we found the mapping, but the client is already dead,
+// only thing to do is clean up what we can
+fn emerg_cleanup (loopdev: Option<&str>, mut backing_file: PathBuf) {
+    eprintln!(
+        "warning: found mapping with dead process ({:?}), attempting cleanup",
+        &backing_file
+    );
+
+    if let Some(loopdev) = loopdev {
+        let _ = loopdev::unassign(loopdev);
+    }
+
+    // killing the backing process does not cancel the FUSE mount automatically
+    let mut command = std::process::Command::new("fusermount");
+    command.arg("-u");
+    command.arg(&backing_file);
+    let _ = crate::tools::run_command(command, None);
+
+    let _ = remove_file(&backing_file);
+    backing_file.set_extension("pid");
+    let _ = remove_file(&backing_file);
+}
+
+fn unmap_from_backing(backing_file: &Path, loopdev: Option<&str>) -> Result<(), Error> {
     let mut pid_path = PathBuf::from(backing_file);
     pid_path.set_extension("pid");
 
-    let pid_str = read_to_string(&pid_path).map_err(|err|
-        format_err!("error reading pidfile {:?}: {}", &pid_path, err))?;
+    let pid_str = read_to_string(&pid_path).map_err(|err| {
+        if err.kind() == std::io::ErrorKind::NotFound {
+            emerg_cleanup(loopdev, backing_file.to_owned());
+        }
+        format_err!("error reading pidfile {:?}: {}", &pid_path, err)
+    })?;
     let pid = pid_str.parse::<i32>().map_err(|err|
         format_err!("malformed PID ({}) in pidfile - {}", pid_str, err))?;
 
     let pid = Pid::from_raw(pid);
 
     // send SIGINT to trigger cleanup and exit in target process
-    signal::kill(pid, Signal::SIGINT)?;
+    match signal::kill(pid, Signal::SIGINT) {
+        Ok(()) => {},
+        Err(nix::Error::Sys(nix::errno::Errno::ESRCH)) => {
+            emerg_cleanup(loopdev, backing_file.to_owned());
+            return Ok(());
+        },
+        Err(e) => return Err(e.into()),
+    }
 
     // block until unmap is complete or timeout
     let start = time::epoch_i64();
@@ -364,16 +398,16 @@ pub fn unmap_loopdev<S: AsRef<str>>(loopdev: S) -> Result<(), Error> {
     }
 
     let backing_file = get_backing_file(loopdev)?;
-    unmap_from_backing(Path::new(&backing_file))
+    unmap_from_backing(Path::new(&backing_file), Some(loopdev))
 }
 
 /// Try and unmap a running proxmox-backup-client instance from the given name
 pub fn unmap_name<S: AsRef<str>>(name: S) -> Result<(), Error> {
-    for (mapping, _) in find_all_mappings()? {
+    for (mapping, loopdev) in find_all_mappings()? {
         if mapping.ends_with(name.as_ref()) {
             let mut path = PathBuf::from(RUN_DIR);
             path.push(&mapping);
-            return unmap_from_backing(&path);
+            return unmap_from_backing(&path, loopdev.as_deref());
         }
     }
     Err(format_err!("no mapping for name '{}' found", name.as_ref()))
-- 
2.20.1

[pbs-devel] applied: [PATCH 0/7] fuse_loop cleanups and named mappings

[Dietmar Maurer]

applied

> On 10/07/2020 1:53 PM Stefan Reiter <s.reiter@proxmox.com> wrote:
> 
>  
> First three patches are independent cleanups/documentation.
> 
> Patch 4 is the meat of the matter, switching over to using names generated from
> the mapped image to identify mappings. This allows easier unmap by having a
> specific tab-completion handler. Also allows listing all current mappings and
> where they are mapped to with the 'unmap' command without parameters.
> 
> The last 3 patches are for better cleanup handling. With all three in place I
> could not come up with a realistic situation in which a user would have to rely
> on external tools to cancel a connection or clean up.
> 
> 
> proxmox-backup: Stefan Reiter (7):
>   format: fix typo in function name
>   fuse_loop: add documentation
>   loopdev: add module doc
>   mount/map: use names for map/unmap for easier use
>   fuse_loop: add automatic cleanup of run files and dangling instances
>   fuse_loop: wait for instance to close after killing
>   fuse_loop: handle unmap on crashed instance
> 
>  src/bin/proxmox-backup-client.rs       |   6 +-
>  src/bin/proxmox_backup_client/mount.rs |  58 +++++-
>  src/client/backup_writer.rs            |   2 +-
>  src/tools/format.rs                    |   4 +-
>  src/tools/fuse_loop.rs                 | 257 ++++++++++++++++++++-----
>  src/tools/loopdev.rs                   |   2 +
>  6 files changed, 268 insertions(+), 61 deletions(-)
> 
> -- 
> 2.20.1
> 
> 
> _______________________________________________
> pbs-devel mailing list
> pbs-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel