Re: [pbs-devel] [PATCH proxmox-backup 2/2] tape: write informational MAM attributes on tapes

[Thomas Lamprecht <t.lamprecht@proxmox.com>]

Am 14/05/2024 um 16:12 schrieb Dominik Csapak:
> namely:
> 
> Vendor: Proxmox
> Name: Backup Server
> Version: current running package version
> User Label Text: the label text
> Media Pool: the current media pool
> 
> write it on labeling and when writing a new media-set to a tape.
> 
> While we currently don't use this info for anything, this can help users
> to identify tapes, even with different backup software.
> 
> If we need it in the future, we can e.g. make decisions based on these
> fields (e.g. the version).
> 
> On format, delete them again.
> 
> Note that some VTLs don't correctly delete the attributes from the
> virtual tapes.
> 
> Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
> ---
>  pbs-tape/Cargo.toml            |  1 +
>  pbs-tape/src/sg_tape.rs        | 39 ++++++++++++++++++++++
>  pbs-tape/src/sg_tape/mam.rs    | 61 +++++++++++++++++++++++++++++++++-
>  src/api2/tape/drive.rs         |  2 ++
>  src/tape/drive/lto/mod.rs      |  6 ++++
>  src/tape/drive/mod.rs          |  5 +++
>  src/tape/drive/virtual_tape.rs |  4 +++
>  7 files changed, 117 insertions(+), 1 deletion(-)
> 
> diff --git a/pbs-tape/Cargo.toml b/pbs-tape/Cargo.toml
> index 970315b7a..f4110706b 100644
> --- a/pbs-tape/Cargo.toml
> +++ b/pbs-tape/Cargo.toml
> @@ -34,4 +34,5 @@ proxmox-schema = { workspace = true, features = [ "api-macro" ] }
>  proxmox-router = { workspace = true, features = ["cli", "server"] }
>  
>  pbs-api-types.workspace = true
> +pbs-buildcfg.workspace = true
>  pbs-config.workspace = true
> diff --git a/pbs-tape/src/sg_tape.rs b/pbs-tape/src/sg_tape.rs
> index f30481b31..058c14ae9 100644
> --- a/pbs-tape/src/sg_tape.rs
> +++ b/pbs-tape/src/sg_tape.rs
> @@ -295,6 +295,8 @@ impl SgTape {
>                  self.erase_media(fast)?
>              }
>  
> +            self.clear_mam_attributes();
> +
>              Ok(())
>          }
>      }
> @@ -1048,6 +1050,43 @@ impl SgTape {
>  
>          Ok(status)
>      }
> +
> +    /// Tries to write useful attributes to the MAM like Vendor/Application/Version
> +    pub fn write_mam_attributes(&mut self, label: Option<String>, pool: Option<String>) {
> +        let version = format!(
> +            "{}-{}",
> +            pbs_buildcfg::PROXMOX_PKG_VERSION,
> +            pbs_buildcfg::PROXMOX_PKG_RELEASE
> +        );
> +        let mut attribute_list: Vec<(u16, &[u8])> = vec![
> +            (0x08_00, b"Proxmox"),
> +            (0x08_01, b"Backup Server"),

This is not the product (or application) name though, that is "Proxmox Backup Server"...

I made a follow-up for that (and a few smaller style issues):
https://git.proxmox.com/?p=proxmox-backup.git;a=commit;h=e50448e4ecd0bd9e8d54d8024aaa60967bbf0c84

What your commit did not mention is why you skip setting a few others, like I
could imagine that the following would have some use:
- DATE AND TIME LAST WRITTEN
- TEXT LOCALIZATION IDENTIFIER (Strings are UTF-8 in rust, and we do not
  explicitly keep them in ASCII or the like FWICT)
- OWNING HOST TEXTUAL NAME (nodename/FQDN might be interesting to see)
- APPLICATION FORMAT VERSION (always good to have)

Not so sure from top of my head about the UIDS, i.e., if we even have something
that can be easily mapped to this.

> +            (0x08_02, version.as_bytes()),
> +        ];
> +        if let Some(ref label) = label {
> +            attribute_list.push((0x08_03, label.as_bytes()));
> +        }
> +
> +        if let Some(ref pool) = pool {
> +            attribute_list.push((0x08_08, pool.as_bytes()));
> +        }
> +
> +        for (id, data) in attribute_list {
> +            if let Err(err) = write_mam_attribute(&mut self.file, id, data) {
> +                log::warn!("could not set MAM Attribute {id:x}: {err}");
> +            }
> +        }
> +    }
> +
> +    // clear all custom set mam attributes
> +    fn clear_mam_attributes(&mut self) {
> +        for attr in [0x08_00, 0x08_01, 0x08_02, 0x08_03, 0x08_08] {

meh, gets easily out of sync with above and tape code is really a huge mess
with all those hex codes sprinkled uncommented all over the place instead
of using actual constants, but that part is pre-existing..


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