Re: [pbs-devel] [PATCH proxmox-backup 1/1] Automatically select a drive (if part of a changer) when loading tapes

["Laurențiu Leahu-Vlăducu" <l.leahu-vladucu@proxmox.com>]

Thanks for your review! I already made almost all the requested changes. 
Some further comments inline:

On 28.01.25 13:25, Dominik Csapak wrote:
> Thanks for tackling this!
> 
> a few high level comments:
> 
> * please write the relevant text as commit message (as mentioned off-list)
>    basically the text in the (here not needed) cover-letter would fit
>    as the commit message
> 
> * rustfmt: please use it, there are some lines that are very long.
>    comments should probably also not exceed the line length limit

Sorry, I thought rustfmt is running, but it was my editor's default 
fallback formatter due to the fact that it could not find the 
rust-analyzer (I forgot to install it). My bad. I fixed this already and 
will be available in the next version of the patch.

> 
> * it would be good to explain the reason for the code a bit more, e.g. 
> you introduce
>    the product-config crate here and do an initialization,
>    but neither by looking at the code, nor by reading the commit
>    message i know exactly why (i know it because we talked off-list)
>    a short sentence like:
> 
>    added the product-config crate so we're able to reuse the
>    'replace_config' instead of copying it.
> 
>    can already be enough
> 
> * the patch does not currently apply cleanly because we did
>    some code moves (e.g. pbs-api-types is now an external crate)
> 
> 
> Some other comments inline:
> 
> On 1/16/25 12:51, Laurențiu Leahu-Vlăducu wrote:
>> Signed-off-by: Laurențiu Leahu-Vlăducu <l.leahu-vladucu@proxmox.com>
>> ---
>>   Cargo.toml                        |   2 +
>>   pbs-api-types/src/tape/changer.rs |  14 +++-
>>   src/api2/tape/changer.rs          | 117 +++++++++++++++++++++++++++++-
>>   src/bin/proxmox-backup-api.rs     |   2 +
>>   src/bin/proxmox-backup-proxy.rs   |   2 +
>>   src/bin/proxmox-tape.rs           |  50 +++++++++++--
>>   src/tape/changer/mod.rs           |  19 ++++-
>>   src/tape/drive/virtual_tape.rs    |   8 +-
>>   src/tape/drive_info.rs            |  51 +++++++++++++
>>   src/tape/mod.rs                   |   1 +
>>   src/tools/mod.rs                  |   7 ++
>>   www/tape/ChangerStatus.js         |  16 +++-
>>   12 files changed, 274 insertions(+), 15 deletions(-)
>>   create mode 100644 src/tape/drive_info.rs
>>
>> diff --git a/Cargo.toml b/Cargo.toml
>> index adeeb6ef..348d1cea 100644
>> --- a/Cargo.toml
>> +++ b/Cargo.toml
>> @@ -72,6 +72,7 @@ proxmox-ldap = "0.2.1"
>>   proxmox-metrics = "0.3.1"
>>   proxmox-notify = "0.5.1"
>>   proxmox-openid = "0.10.0"
>> +proxmox-product-config = "0.2.2"
>>   proxmox-rest-server = { version = "0.8.5", features = [ "templates" ] }
>>   # some use "cli", some use "cli" and "server", pbs-config uses nothing
>>   proxmox-router = { version = "3.0.0", default-features = false }
>> @@ -221,6 +222,7 @@ proxmox-ldap.workspace = true
>>   proxmox-metrics.workspace = true
>>   proxmox-notify = { workspace = true, features = [ "pbs-context" ] }
>>   proxmox-openid.workspace = true
>> +proxmox-product-config.workspace = true
>>   proxmox-rest-server = { workspace = true, features = [ "rate- 
>> limited-stream" ] }
>>   proxmox-router = { workspace = true, features = [ "cli", "server"] }
>>   proxmox-schema = { workspace = true, features = [ "api-macro" ] }
>> diff --git a/pbs-api-types/src/tape/changer.rs b/pbs-api-types/src/ 
>> tape/changer.rs
>> index df3823cf..52ee08db 100644
>> --- a/pbs-api-types/src/tape/changer.rs
>> +++ b/pbs-api-types/src/tape/changer.rs
>> @@ -8,10 +8,20 @@ use proxmox_schema::{
>>   use crate::{OptionalDeviceIdentification, PROXMOX_SAFE_ID_FORMAT};
>> +const TAPE_CHANGER_MIN_LENGTH: usize = 3;
>> +const TAPE_CHANGER_MAX_LENGTH: usize = 32;
>> +
>>   pub const CHANGER_NAME_SCHEMA: Schema = StringSchema::new("Tape 
>> Changer Identifier.")
>>       .format(&PROXMOX_SAFE_ID_FORMAT)
>> -    .min_length(3)
>> -    .max_length(32)
>> +    .min_length(TAPE_CHANGER_MIN_LENGTH)
>> +    .max_length(TAPE_CHANGER_MAX_LENGTH)
>> +    .schema();
>> +
>> +pub const CHANGER_NAME_SCHEMA_AUTOMATIC_DRIVE_ASIGNMENT: Schema =
> 
> typo: ASIGNMENT vs ASSIGNMENT
> 
>> +    StringSchema::new("Tape Changer Identifier to be used for 
>> automatic tape drive assignment.")
>> +    .format(&PROXMOX_SAFE_ID_FORMAT)
>> +    .min_length(TAPE_CHANGER_MIN_LENGTH)
>> +    .max_length(TAPE_CHANGER_MAX_LENGTH)
>>       .schema();
>>   pub const SCSI_CHANGER_PATH_SCHEMA: Schema =
>> diff --git a/src/api2/tape/changer.rs b/src/api2/tape/changer.rs
>> index 7ecf7bff..b73d6832 100644
>> --- a/src/api2/tape/changer.rs
>> +++ b/src/api2/tape/changer.rs
>> @@ -1,6 +1,6 @@
>>   use std::collections::HashMap;
>> -use anyhow::Error;
>> +use anyhow::{bail, Error};
>>   use serde_json::Value;
>>   use proxmox_router::{list_subdirs_api_method, Permission, Router, 
>> RpcEnvironment, SubdirMap};
>> @@ -8,7 +8,8 @@ use proxmox_schema::api;
>>   use pbs_api_types::{
>>       Authid, ChangerListEntry, LtoTapeDrive, MtxEntryKind, 
>> MtxStatusEntry, ScsiTapeChanger,
>> -    CHANGER_NAME_SCHEMA, PRIV_TAPE_AUDIT, PRIV_TAPE_READ,
>> +    CHANGER_NAME_SCHEMA, PRIV_TAPE_AUDIT, PRIV_TAPE_READ, 
>> MEDIA_LABEL_SCHEMA, UPID_SCHEMA,
>> +    DriveListEntry, DeviceActivity
>>   };
>>   use pbs_config::CachedUserInfo;
>>   use pbs_tape::{
>> @@ -199,7 +200,119 @@ pub fn list_changers(
>>       Ok(list)
>>   }
>> +#[api(
>> +    input: {
>> +        properties: {
>> +            name: {
>> +                schema: CHANGER_NAME_SCHEMA,
>> +            },
>> +            "label-text": {
>> +                schema: MEDIA_LABEL_SCHEMA,
>> +            },
>> +        },
>> +    },
>> +    returns: {
>> +        schema: UPID_SCHEMA,
>> +    },
>> +    access: {
>> +        permission: &Permission::Privilege(&["tape", "device", 
>> "{name}"], PRIV_TAPE_READ, false),
>> +    },
>> +)]
>> +/// Load media with specified label
>> +///
>> +/// Issue a media load request to the associated changer device.
>> +pub fn load_media(
>> +    name: String,
>> +    label_text: String,
>> +    rpcenv: &mut dyn RpcEnvironment,
>> +) -> Result<Value, Error> {
>> +    let drive = choose_drive(&name, rpcenv);
>> +    super::drive::load_media(drive?, label_text, rpcenv)
>> +}> +
>> +#[api(
>> +    input: {
>> +        properties: {
>> +            name: {
>> +                schema: CHANGER_NAME_SCHEMA,
>> +            },
>> +            "source-slot": {
>> +                description: "Source slot number.",
>> +                minimum: 1,
>> +            },
>> +        },
>> +    },
>> +    access: {
>> +        permission: &Permission::Privilege(&["tape", "device", 
>> "{name}"], PRIV_TAPE_READ, false),
>> +    },
>> +)]
>> +/// Load media from the specified slot
>> +///
>> +/// Issue a media load request to the associated changer device.
>> +pub async fn load_slot(
>> +    name: String,
>> +    source_slot: u64,
>> +    rpcenv: &mut dyn RpcEnvironment,) -> Result<(), Error> {
>> +    let drive = choose_drive(&name, rpcenv);
>> +    super::drive::load_slot(drive?, source_slot).await
>> +}
>> +
>> +fn choose_drive(
>> +    changer: &str,
>> +    rpcenv: &mut dyn RpcEnvironment) -> Result<String, Error> {
>> +    // We have to automatically select a drive from the specified 
>> changer. For that purpose, we take all drives that are part of the 
>> changer
>> +    // and search for the one that has not been used for the longest 
>> time (to ensure similar wear between the drives), or use one that has 
>> never been used.
> 
> Please keep the line length limit also for comments
> 
>> +    let drives = 
>> super::drive::list_drives(Option::from(changer.to_string()), true, 
>> Value::Null, rpcenv);
>> +
> 
> I'd use `Some(x)` instead of `Option::from(x)` because it's shorter, and 
> because it's
> clearer what it does
> 
>> +    match drives {
>> +        Ok(entries) => {
> 
> this pattern with a bail! in the Err case is imho much neater when 
> written like this:
> 
> let entries = list_drives(..).map_err(|err| format_err!("cannot query 
> drives: {err}"))?;
> 
> with that you eliminate a complete level of indentation for the 
> remaining code
> 
>> +            let idle_drives: Vec<DriveListEntry> = 
>> entries.into_iter().filter(|entry| matches!(entry.activity, None | 
>> Some(DeviceActivity::NoActivity))).collect();
>> +            let drives_info = 
>> crate::tape::drive_info::get_drives_info();
>> +
>> +            match drives_info {
>> +                Ok(drives_info) => {
>> +                    let mut index_oldest : Option<usize> = 
>> Option::default();
>> +                    let mut oldest_time: Option<i64> = 
>> Option::default();
>> +
> 
> I'd write
> 
> let mut foo: Option<T> = None;
> 
> here.
> 
> Yes, it's clear that None is the default for most rust developers, but
> seeing the `None` explicitely here makes the code much more readable IMO
> 
> 
>> +                    for index in 0..idle_drives.len() {
>> +                        let existing_drive = 
>> drives_info.drives.get(&idle_drives[index].config.name);
>> +
>> +                        match existing_drive {
>> +                            Some(existing_drive) => {
>> +                                if oldest_time.is_none() || 
>> oldest_time.is_some_and(|oldest_time| existing_drive.last_usage < 
>> oldest_time) {
>> +                                    oldest_time = 
>> Option::from(existing_drive.last_usage);
>> +                                    index_oldest = Option::from(index);
>> +                                }
>> +                            },
>> +                            None => {
>> +                                // Drive has never been used, so 
>> let's use this one!
>> +                                index_oldest = Option::from(index);
>> +                                break;
>> +                            }
>> +                        }
>> +                    }
>> +
>> +                    match index_oldest {
>> +                        Some(index_oldest) => 
>> Ok(idle_drives.get(index_oldest).unwrap().config.name.clone()),
>> +                        None => bail!("there are no idle drives to 
>> choose for automatic drive assignment")
>> +                    }
>> +                },
>> +                Err(_error) => {
>> +                    // Simply use the first drive, if possible.
>> +                    match idle_drives.first() {
>> +                        Some(idle_drive) => 
>> Ok(idle_drive.config.name.clone()),
>> +                        None => bail!("there are no idle drives to 
>> choose for automatic drive assignment")
>> +                    }
>> +                }
> 
> wouldn't the code here get much easier if the 'drives_info' would return 
> a list sorted by the last_usage?
> 
> the i think the code could boil down to something like this (just 
> pseudocode):
> 
> let idle_drives = get_idle_drives();
> let drives_in_order = get_sorted_drives_info(); // should include all 
> drives, also those that have no last usage recorded (sorted before ones 
> with usage)
> 
> let chosen = drives_in_order.filter(|drive| 
> idle_drives.contains(drive)).first();
> 
> match chosen {
>     Some(xx) => // do something with chosen drive and update list
>     None => // fallback mechanism or error
> }
> 

Sure, this would make it simpler for this use case - I agree. Taking a 
step back for a moment, we do not have any files storing any state for 
tape drives/changers except the ones made to be changed by the user (in 
/etc/proxmox-backup/tape.cfg).

My first thought was to create such a file for storing states related to 
tape drives/changers where we might want to store more than the time of 
the last usage (although only this is stored, at the moment). This is 
the reason why I named it "drives_info" or "tape_drive_info". In that 
context, sorting drives by the last time of usage can be added, but is 
pretty specific.

However, if a more general file for such purposes is not desired, I can 
gladly make it more use case-specific and rename the file and structs 
altogether to better reflect what the new code does. Returning a sorted 
list of drives would make more sense in that context.

>> +            }
>> +        }
>> +        Err(error) => bail!("cannot query drives: {}", error)
>> +    }
>> +}
>> +
>>   const SUBDIRS: SubdirMap = &[
>> +    ("load-media", &Router::new().post(&API_METHOD_LOAD_MEDIA)),
>> +    ("load-slot", &Router::new().post(&API_METHOD_LOAD_SLOT)),
>>       ("status", &Router::new().get(&API_METHOD_GET_STATUS)),
>>       ("transfer", &Router::new().post(&API_METHOD_TRANSFER)),
>>   ];
>> diff --git a/src/bin/proxmox-backup-api.rs b/src/bin/proxmox-backup- 
>> api.rs
>> index 7a72d49a..c6b2d532 100644
>> --- a/src/bin/proxmox-backup-api.rs
>> +++ b/src/bin/proxmox-backup-api.rs
>> @@ -43,6 +43,8 @@ fn get_index() -> Pin<Box<dyn Future<Output = 
>> Response<Body>> + Send>> {
>>   async fn run() -> Result<(), Error> {
>>       init_logger("PBS_LOG", LevelFilter::INFO)?;
>> +    let _ = proxmox_backup::tools::init_product_config();
>> +
> 
> you introduce this method here, but always ignore the error.
> what happens when the init fails? will the 'replace_config' panic later?
> (that would be very bad?)
> 
> i think it would be better to split this out into it's own patch
> with a good reasoning why the error is safe to be ignored (or not)
> 
> IMHO if the init fails, we cannot continue anyway, e.g.
> in the main function of the backup-proxy we fail if  `backup_user()` fails
> 
>>       config::create_configdir()?;
>>       config::update_self_signed_cert(false)?;
>> diff --git a/src/bin/proxmox-backup-proxy.rs b/src/bin/proxmox-backup- 
>> proxy.rs
>> index ce1be1c0..eb7da0e4 100644
>> --- a/src/bin/proxmox-backup-proxy.rs
>> +++ b/src/bin/proxmox-backup-proxy.rs
>> @@ -181,6 +181,8 @@ async fn get_index_future(env: RestEnvironment, 
>> parts: Parts) -> Response<Body>
>>   async fn run() -> Result<(), Error> {
>>       init_logger("PBS_LOG", LevelFilter::INFO)?;
>> +    let _ = proxmox_backup::tools::init_product_config();
>> +
>>       proxmox_backup::auth_helpers::setup_auth_context(false);
>>       proxmox_backup::server::notifications::init()?;
>>       metric_collection::init()?;
>> diff --git a/src/bin/proxmox-tape.rs b/src/bin/proxmox-tape.rs
>> index 8e8584b3..8de94af0 100644
>> --- a/src/bin/proxmox-tape.rs
>> +++ b/src/bin/proxmox-tape.rs
>> @@ -1,6 +1,8 @@
>>   use std::collections::HashMap;
>>   use anyhow::{bail, format_err, Error};
>> +use pbs_api_types::CHANGER_NAME_SCHEMA_AUTOMATIC_DRIVE_ASIGNMENT;
>> +use pbs_config::drive::complete_changer_name;
>>   use serde_json::{json, Value};
>>   use proxmox_human_byte::HumanByte;
>> @@ -214,6 +216,10 @@ async fn eject_media(mut param: Value) -> 
>> Result<(), Error> {
>>                   schema: DRIVE_NAME_SCHEMA,
>>                   optional: true,
>>               },
>> +            changer: {
>> +                schema: CHANGER_NAME_SCHEMA_AUTOMATIC_DRIVE_ASIGNMENT,
>> +                optional: true,
>> +            },
>>               "label-text": {
>>                   schema: MEDIA_LABEL_SCHEMA,
>>               },
>> @@ -230,11 +236,25 @@ async fn load_media(mut param: Value) -> 
>> Result<(), Error> {
>>       let (config, _digest) = pbs_config::drive::config()?;
>> -    let drive = extract_drive_name(&mut param, &config)?;
>> +    let drive = extract_drive_name(&mut param, &config);
>> +    let changer = param["changer"].as_str();
>> +
>> +    let path = match changer {
>> +        Some(changer) => {
>> +            format!("api2/json/tape/changer/{}/load-media", changer)
>> +        }
>> +        None => {
>> +            let drive = drive?;
>> +            format!("api2/json/tape/drive/{}/load-media", drive)
>> +        }
>> +    };
> 
> here you could instead do the following:
> 
> let path = match (changer, drive) {
>     (Some(changer), None) => // changer path
>     (None, Some(drive)) => // drive path
>     _ => // fail with error that exactly one of the options should be set
> };
> 
> in your code, i can give both, but it will autodetect the drive even if
> given explicitly when the changer is given, which might be confusing
> for users
> 
>> +
>> +    if let Some(param) = param.as_object_mut() {
>> +        param.remove("changer");
>> +    }
>>       let client = connect_to_localhost()?;
>> -    let path = format!("api2/json/tape/drive/{}/load-media", drive);
>>       let result = client.post(&path, Some(param)).await?;
>>       view_task_result(&client, result, &output_format).await?;
>> @@ -276,6 +296,10 @@ async fn export_media(mut param: Value) -> 
>> Result<(), Error> {
>>                   schema: DRIVE_NAME_SCHEMA,
>>                   optional: true,
>>               },
>> +            changer: {
>> +                schema: CHANGER_NAME_SCHEMA_AUTOMATIC_DRIVE_ASIGNMENT,
>> +                optional: true,
>> +            },
>>               "source-slot": {
>>                   description: "Source slot number.",
>>                   type: u64,
>> @@ -288,11 +312,25 @@ async fn export_media(mut param: Value) -> 
>> Result<(), Error> {
>>   async fn load_media_from_slot(mut param: Value) -> Result<(), Error> {
>>       let (config, _digest) = pbs_config::drive::config()?;
>> -    let drive = extract_drive_name(&mut param, &config)?;
>> +    let drive = extract_drive_name(&mut param, &config);
>> +    let changer = param["changer"].as_str();
>> +
>> +    let path = match changer {
>> +        Some(changer) => {
>> +            format!("api2/json/tape/changer/{}/load-slot", changer)
>> +        }
>> +        None => {
>> +            let drive = drive?;
>> +            format!("api2/json/tape/drive/{}/load-slot", drive)
>> +        }
>> +    };
> 
> the same here
> 
>> +
>> +    if let Some(param) = param.as_object_mut() {
>> +        param.remove("changer");
>> +    }
>>       let client = connect_to_localhost()?;
>> -    let path = format!("api2/json/tape/drive/{}/load-slot", drive);
>>       client.post(&path, Some(param)).await?;
>>       Ok(())
>> @@ -1091,13 +1129,15 @@ fn main() {
>>               CliCommand::new(&API_METHOD_LOAD_MEDIA)
>>                   .arg_param(&["label-text"])
>>                   .completion_cb("drive", complete_drive_name)
>> +                .completion_cb("changer", complete_changer_name)
>>                   .completion_cb("label-text", 
>> complete_media_label_text),
>>           )
>>           .insert(
>>               "load-media-from-slot",
>>               CliCommand::new(&API_METHOD_LOAD_MEDIA_FROM_SLOT)
>>                   .arg_param(&["source-slot"])
>> -                .completion_cb("drive", complete_drive_name),
>> +                .completion_cb("drive", complete_drive_name)
>> +                .completion_cb("changer", complete_changer_name),
>>           )
>>           .insert(
>>               "unload",
>> diff --git a/src/tape/changer/mod.rs b/src/tape/changer/mod.rs
>> index 18ea0f46..8e3ff0f7 100644
>> --- a/src/tape/changer/mod.rs
>> +++ b/src/tape/changer/mod.rs
>> @@ -273,6 +273,17 @@ pub trait MediaChange {
>>       }
>>   }
>> +/// Updates the drive's last usage time to now.
>> +pub(super) fn update_drive_usage(drive: &str) -> Result<(), Error> {
>> +    let _lock = crate::tape::drive_info::lock()?;
>> +
>> +    let mut drives_info = crate::tape::drive_info::get_drives_info()?;
>> +
>> +    let now = proxmox_time::epoch_i64();
>> +    drives_info.drives.entry(drive.into()).or_default().last_usage = 
>> now;
>> +    crate::tape::drive_info::save_config(&drives_info)
>> +}
>> +
>>   const USE_MTX: bool = false;
>>   impl ScsiMediaChange for ScsiTapeChanger {
>> @@ -423,7 +434,13 @@ impl MediaChange for MtxMediaChanger {
>>       }
>>       fn load_media_from_slot(&mut self, slot: u64) -> 
>> Result<MtxStatus, Error> {
>> -        self.config.load_slot(slot, self.drive_number())
>> +        let status = self.config.load_slot(slot, self.drive_number())?;
>> +
>> +        if let Err(err) = update_drive_usage(self.drive_name()) {
>> +            log::warn!("could not update drive usage: {err}");
>> +        }
>> +
>> +        Ok(status)
>>       }
>>       fn unload_media(&mut self, target_slot: Option<u64>) -> 
>> Result<MtxStatus, Error> {
>> diff --git a/src/tape/drive/virtual_tape.rs b/src/tape/drive/ 
>> virtual_tape.rs
>> index 866e4d32..213f17fe 100644
>> --- a/src/tape/drive/virtual_tape.rs
>> +++ b/src/tape/drive/virtual_tape.rs
>> @@ -567,7 +567,13 @@ impl MediaChange for VirtualTapeHandle {
>>           };
>>           self.store_status(&status)?;
>> -        self.status()
>> +        let status = self.status()?;
>> +
>> +        if let Err(err) = 
>> crate::tape::changer::update_drive_usage(self.drive_name()) {
>> +            log::warn!("could not update drive usage: {err}");
>> +        }
>> +
>> +        Ok(status)
>>       }
>>       fn unload_media(&mut self, _target_slot: Option<u64>) -> 
>> Result<MtxStatus, Error> {
>> diff --git a/src/tape/drive_info.rs b/src/tape/drive_info.rs
>> new file mode 100644
>> index 00000000..a0c2f836
>> --- /dev/null
>> +++ b/src/tape/drive_info.rs
>> @@ -0,0 +1,51 @@
>> +//! Serialize/deserialize tpae drive info (e.g. useful for statistics)
>> +//!
>> +//! This can be used to store a state over a longer period of time 
>> (e.g. last tape drive usage).
>> +
>> +use serde::{Serialize, Deserialize};
>> +use std::collections::HashMap;
>> +use anyhow::Error;
>> +use proxmox_product_config::ApiLockGuard;
>> +
>> +/// Drive info file name
>> +pub const DRIVE_INFO_FILENAME: &str = concat! 
>> (pbs_buildcfg::PROXMOX_BACKUP_STATE_DIR_M!(), "/tape_drive_info.json");
>> +/// Lock file name (used to prevent concurrent access)
>> +pub const DRIVE_INFO_LOCKFILE: &str = concat! 
>> (pbs_buildcfg::PROXMOX_BACKUP_STATE_DIR_M!(), 
>> "/.tape_drive_info.json.lock");
>> +
>> +#[derive(Serialize, Deserialize, Default)]
>> +pub struct SingleDriveInfo {
>> +    #[serde(with = "proxmox_serde::epoch_as_rfc3339")]
>> +    pub last_usage: i64,
>> +}
>> +
>> +#[derive(Serialize, Deserialize, Default)]
>> +pub struct DrivesInfo {
>> +    pub drives: HashMap<String, SingleDriveInfo>,
>> +}
> 
> as said above, this would probably more economic if it were just a list
> 
> maybe a `Vec<(String, i64)>` sorted by the i64
> 
> also not a super fan of the name `DrivesInfo` or `SingleDriveInfo` since 
> it does not really
> convey what it does. (E.g. I'd have expected it to have much more info 
> than just
> the last usage)
> 
> Maybe call it something like 'DrivesLastUsed' for now, and if we see we can
> add more information, we can still update the name.
> (Though I'm not the best at naming, so maybe someone else wants to chime 
> in on this)
> 

I thought I could keep the file and name rather general, as we might add 
more to it in the future - see my comment above. Of course, I can change 
it, if desired :)

>> +
>> +/// Get exclusive lock
>> +pub fn lock() -> Result<ApiLockGuard, Error> {
>> +    proxmox_product_config::open_api_lockfile(DRIVE_INFO_LOCKFILE, 
>> Option::None, true)
>> +}
>> +
>> +/// Read and parse the drive info file
>> +pub fn get_drives_info() -> Result<DrivesInfo, Error> {
>> +    let content =
>> +        
>> proxmox_sys::fs::file_read_optional_string(DRIVE_INFO_FILENAME)?;
>> +
>> +    match content {
>> +        Some(content) => {
>> +            let result = serde_json::from_str::<DrivesInfo>(&content)?;
>> +            Ok(result)
>> +        },
>> +        None => {
>> +            Ok(DrivesInfo::default())
>> +        }
>> +    }
>> +}
>> +
>> +/// Save the configuration file
>> +pub fn save_config(data: &DrivesInfo) -> Result<(), Error> {
>> +    let json = serde_json::to_string(data)?;
>> +    proxmox_product_config::replace_config(DRIVE_INFO_FILENAME, 
>> json.as_bytes())
>> +}
>> diff --git a/src/tape/mod.rs b/src/tape/mod.rs
>> index f276f948..8b87152d 100644
>> --- a/src/tape/mod.rs
>> +++ b/src/tape/mod.rs
>> @@ -20,6 +20,7 @@ pub use inventory::*;
>>   pub mod changer;
>>   pub mod drive;
>> +pub mod drive_info;
>>   pub mod encryption_keys;
>>   mod media_pool;
>> diff --git a/src/tools/mod.rs b/src/tools/mod.rs
>> index 322894dd..11fb2821 100644
>> --- a/src/tools/mod.rs
>> +++ b/src/tools/mod.rs
>> @@ -61,3 +61,10 @@ pub fn setup_safe_path_env() {
>>           std::env::remove_var(name);
>>       }
>>   }
>> +
>> +pub fn init_product_config() -> Result<(), Error> {
> 
> like most of the functions in this file, a short comment would be nice,
> even if it's just a reference to the `proxmox_product_config` crate
> 
>> +    let backup_user = pbs_config::backup_user()?;
>> +    let root_user = 
>> nix::unistd::User::from_uid(nix::unistd::ROOT)?.expect("could not find 
>> root user");
> 
> this seems inconsistent.
> 
> for two things we bubble the error up, but a missing root user panics
> 
> I'd probably make that into an error that bubbles up too
> 
>> +    proxmox_product_config::init(backup_user, root_user);
>> +    Ok(())
>> +}
> 
> 
>> diff --git a/www/tape/ChangerStatus.js b/www/tape/ChangerStatus.js
>> index e18af90e..0a875779 100644
>> --- a/www/tape/ChangerStatus.js
>> +++ b/www/tape/ChangerStatus.js
>> @@ -222,12 +222,17 @@ Ext.define('PBS.TapeManagement.ChangerStatus', {
>>               autoShow: true,
>>               submitText: gettext('OK'),
>>               title: gettext('Load Media into Drive'),
>> -            url: `/api2/extjs/tape/drive`,
>> +            url: `/api2/extjs/tape`,
>>               method: 'POST',
>>               submitUrl: function(url, values) {
>> -            let drive = values.drive;
>> -            delete values.drive;
>> -            return `${url}/${encodeURIComponent(drive)}/${apiCall}`;
>> +                let drive = values.drive;
>> +                delete values.drive;
>> +
>> +                if (drive) {
>> +                    return `${url}/drive/ 
>> ${encodeURIComponent(drive)}/${apiCall}`;
>> +                } else {
>> +                    return `${url}/changer/ 
>> ${encodeURIComponent(changer)}/${apiCall}`;
>> +                }
> 
> nit: just so we don't have too many places where we create such urls,
> I'd try to combine them into one:
> 
> let type = drive ? "drive" : "changer";
> let item = drive ? drive : changer;
> 
> return `${url}/${type}/${encodeURIComponent(item)}/${apiCall}`;
> 
> or something like that
> 
>>               },
>>               items: [
>>               label !== "" ? {
>> @@ -248,6 +253,9 @@ Ext.define('PBS.TapeManagement.ChangerStatus', {
>>                   fieldLabel: gettext('Drive'),
>>                   changer: changer,
>>                   name: 'drive',
>> +                emptyText: gettext('Choose Automatically'),
>> +                allowBlank: true,
>> +                autoSelect: false,
>>               },
>>               ],
>>               listeners: {
> 
> 



_______________________________________________
pbs-devel mailing list
pbs-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel