tape: cleanup/simplify media_change code

src/api2/tape/backup.rs

@@ -139,7 +139,7 @@ fn update_media_online_status(drive: &str) -> Result<(), Error> {
 
     let (config, _digest) = config::drive::config()?;
 
-    if let Ok((changer, changer_name)) = media_changer(&config, drive, false) {
+    if let Ok(Some((changer, changer_name))) = media_changer(&config, drive) {
 
         let changer_id_list = changer.list_media_changer_ids()?;
 

src/api2/tape/drive.rs

@@ -55,6 +55,7 @@ use crate::{
         linux_tape_device_list,
         open_drive,
         media_changer,
+        required_media_changer,
         update_changer_online_status,
         linux_tape::{
             LinuxTapeHandle,
@@ -122,7 +123,7 @@ pub async fn load_media(drive: String, changer_id: String) -> Result<(), Error>
     let (config, _digest) = config::drive::config()?;
 
     tokio::task::spawn_blocking(move || {
-        let (mut changer, _) = media_changer(&config, &drive, false)?;
+        let (mut changer, _) = required_media_changer(&config, &drive)?;
         changer.load_media(&changer_id)
     }).await?
 }
@@ -288,14 +289,17 @@ pub async fn eject_media(drive: String) -> Result<(), Error> {
     let (config, _digest) = config::drive::config()?;
 
     tokio::task::spawn_blocking(move || {
-        let (mut changer, _) = media_changer(&config, &drive, false)?;
+        if let Some((mut changer, _)) = media_changer(&config, &drive)? {
-
             if !changer.eject_on_unload() {
                 let mut drive = open_drive(&config, &drive)?;
                 drive.eject_media()?;
             }
-
+            changer.unload_media()?;
-        changer.unload_media()
+        } else {
+            let mut drive = open_drive(&config, &drive)?;
+            drive.eject_media()?;
+        }
+        Ok(())
     }).await?
 }
 
@@ -524,7 +528,7 @@ pub async fn inventory(
     let (config, _digest) = config::drive::config()?;
 
     tokio::task::spawn_blocking(move || {
-        let (changer, changer_name) = media_changer(&config, &drive, false)?;
+        let (changer, changer_name) = required_media_changer(&config, &drive)?;
 
         let changer_id_list = changer.list_media_changer_ids()?;
 
@@ -607,7 +611,7 @@ pub fn update_inventory(
         true,
         move |worker| {
 
-            let (mut changer, changer_name) = media_changer(&config, &drive, false)?;
+            let (mut changer, changer_name) = required_media_changer(&config, &drive)?;
 
             let changer_id_list = changer.list_media_changer_ids()?;
             if changer_id_list.is_empty() {
@@ -721,7 +725,7 @@ fn barcode_label_media_worker(
 
     let (config, _digest) = config::drive::config()?;
 
-    let (mut changer, changer_name) = media_changer(&config, &drive, false)?;
+    let (mut changer, changer_name) = required_media_changer(&config, &drive)?;
 
     let changer_id_list = changer.list_media_changer_ids()?;
 

src/tape/changer/email.rs

@@ -2,39 +2,24 @@ use anyhow::Error;
 
 use proxmox::tools::email::sendmail;
 
-use super::MediaChange;
-
 /// Send email to a person to request a manual media change
-pub struct ChangeMediaEmail {
+pub fn send_load_media_email(
-    drive: String,
+    drive: &str,
-    to: String,
+    changer_id: &str,
-}
+    to: &str,
+) -> Result<(), Error> {
 
-impl ChangeMediaEmail {
+    let subject = format!("Load Media '{}' request for drive '{}'", changer_id, drive);
-
-    pub fn new(drive: &str, to: &str) -> Self {
-        Self {
-            drive: String::from(drive),
-            to: String::from(to),
-        }
-    }
-}
-
-impl MediaChange for ChangeMediaEmail {
-
-    fn load_media(&mut self, changer_id: &str) -> Result<(), Error> {
-
-        let subject = format!("Load Media '{}' request for drive '{}'", changer_id, self.drive);
 
     let mut text = String::new();
 
     text.push_str("Please insert the requested media into the backup drive.\n\n");
 
-        text.push_str(&format!("Drive: {}\n", self.drive));
+    text.push_str(&format!("Drive: {}\n", drive));
     text.push_str(&format!("Media: {}\n", changer_id));
 
     sendmail(
-            &[&self.to],
+        &[to],
         &subject,
         Some(&text),
         None,
@@ -44,14 +29,3 @@ impl MediaChange for ChangeMediaEmail {
 
     Ok(())
 }
-
-    fn unload_media(&mut self) -> Result<(), Error> {
-        /* ignore ? */
-        Ok(())
-    }
-
-    fn list_media_changer_ids(&self) -> Result<Vec<String>, Error> {
-        Ok(Vec::new())
-    }
-
-}

src/tape/drive/mod.rs

@@ -33,7 +33,7 @@ use crate::{
         },
         changer::{
             MediaChange,
-            ChangeMediaEmail,
+            send_load_media_email,
         },
     },
 };
@@ -146,41 +146,32 @@ pub trait TapeDriver {
     fn eject_media(&mut self) -> Result<(), Error>;
 }
 
-/// Get the media changer (name + MediaChange) associated with a tape drie.
+/// Get the media changer (MediaChange + name) associated with a tape drive.
 ///
-/// If allow_email is set, returns an ChangeMediaEmail instance for
+/// Returns Ok(None) if the drive has no associated changer device.
-/// standalone tape drives (changer name set to "").
 pub fn media_changer(
     config: &SectionConfigData,
     drive: &str,
-    allow_email: bool,
+) -> Result<Option<(Box<dyn MediaChange>, String)>, Error> {
-) -> Result<(Box<dyn MediaChange>, String), Error> {
 
     match config.sections.get(drive) {
         Some((section_type_name, config)) => {
             match section_type_name.as_ref() {
                 "virtual" => {
                     let tape = VirtualTapeDrive::deserialize(config)?;
-                    Ok((Box::new(tape), drive.to_string()))
+                    Ok(Some((Box::new(tape), drive.to_string())))
                 }
                 "linux" => {
                     let tape = LinuxTapeDrive::deserialize(config)?;
                     match tape.changer {
                         Some(ref changer_name) => {
                             let changer_name = changer_name.to_string();
-                            Ok((Box::new(tape), changer_name))
+                            Ok(Some((Box::new(tape), changer_name)))
                         }
-                        None =>  {
+                        None => Ok(None),
-                            if !allow_email {
-                                bail!("drive '{}' has no changer device", drive);
-                            }
-                            let to = "root@localhost"; // fixme
-                            let changer = ChangeMediaEmail::new(drive, to);
-                            Ok((Box::new(changer), String::new()))
-                        },
                     }
                 }
-                _ => bail!("drive type '{}' not implemented!"),
+                _ => bail!("unknown drive type '{}' - internal error"),
             }
         }
         None => {
@@ -189,6 +180,26 @@ pub fn media_changer(
     }
 }
 
+/// Get the media changer (MediaChange + name) associated with a tape drive.
+///
+/// This fail if the drive has no associated changer device.
+pub fn required_media_changer(
+    config: &SectionConfigData,
+    drive: &str,
+) -> Result<(Box<dyn MediaChange>, String), Error> {
+    match media_changer(config, drive) {
+        Ok(Some(result)) => {
+            return Ok(result);
+        }
+        Ok(None) => {
+            bail!("drive '{}' has no associated changer device", drive);
+        },
+        Err(err) => {
+            return Err(err);
+        }
+    }
+}
+
 pub fn open_drive(
     config: &SectionConfigData,
     drive: &str,
@@ -209,7 +220,7 @@ pub fn open_drive(
                         .map_err(|err| format_err!("open drive '{}' ({}) failed - {}", drive, tape.path, err))?;
                     Ok(Box::new(handle))
                 }
-                _ => bail!("drive type '{}' not implemented!"),
+                _ => bail!("unknown drive type '{}' - internal error"),
             }
         }
         None => {
@@ -235,7 +246,11 @@ pub fn request_and_load_media(
 
     let check_label = |handle: &mut dyn TapeDriver, uuid: &proxmox::tools::Uuid| {
         if let Ok(Some(media_id)) = handle.read_label() {
-            println!("found media label {} ({})", media_id.label.changer_id, media_id.label.uuid.to_string());
+            worker.log(format!(
+                "found media label {} ({})",
+                media_id.label.changer_id,
+                media_id.label.uuid.to_string(),
+            ));
             if media_id.label.uuid == *uuid {
                 return Ok(media_id);
             }
@@ -278,9 +293,8 @@ pub fn request_and_load_media(
                     worker.log(format!("Please insert media '{}' into drive '{}'", changer_id, drive));
 
                     let to = "root@localhost"; // fixme
-                    let mut changer = ChangeMediaEmail::new(drive, to);
 
-                    changer.load_media(&changer_id)?; // semd email
+                    send_load_media_email(drive, &changer_id, to)?;
 
                     let mut last_media_uuid = None;