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

[Dominik Csapak <d.csapak@proxmox.com>]

On 5/22/24 19:24, Thomas Lamprecht wrote:
> 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"...

true..

> 
> 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

looks good, thanks

> 
> 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

i hesitated with this one as there is no timezone included and it does not
specify one either, but i guess we could just use UTC (although that might
be confusing for some people?)

> - TEXT LOCALIZATION IDENTIFIER (Strings are UTF-8 in rust, and we do not
>    explicitly keep them in ASCII or the like FWICT)

that one i explicitly left out because we (currently) only write ascii,
but yes, we could simply set that to utf-8 for "future-proof"ness


> - OWNING HOST TEXTUAL NAME (nodename/FQDN might be interesting to see)

yes that one make sense

> - APPLICATION FORMAT VERSION (always good to have)

isn't that implicated by the application version ?

we don't really have a 'format version' for the tape format, but each
archive on it has it's own version e.g. the snapshot archives
have version 1.2 while the chunk archive and catalog archive have 1.1
and the labels have only 1.0

> 
> 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.

not sure which field you mean here? in  LTO-5 there is only one standardized
field left and that is the VOLUME COHERENCY INFORMATION
and i don't think we'll need that

> 
>> +            (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..


true, i should know better (and not introduce this in new code)
i'll send a cleanup for this shortly that fixes that.

if i have time i'll try to cleanup the rest of the tape code
(i'll open a bug for that so i don't forget ;) )

thanks for looking at this again :)




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