[pbs-devel] [PATCH proxmox-backup 1/2] tape: correct mam format for some attributes

[pbs-devel] [PATCH proxmox-backup 1/2] tape: correct mam format for some attributes

[Dominik Csapak]

Some MAM attributes are of type 'TEXT' that is not only ascii, but
controlled by an addition field that specifies various 8bit text
formats.

For now, simply assume utf8 as the default is ascii, and we don't expect
any data that is not ASCII anyway.

This will be needed when we'll want to write those attributes.

Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
---
 pbs-tape/src/sg_tape/mam.rs | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/pbs-tape/src/sg_tape/mam.rs b/pbs-tape/src/sg_tape/mam.rs
index 61368d287..74e09c6d6 100644
--- a/pbs-tape/src/sg_tape/mam.rs
+++ b/pbs-tape/src/sg_tape/mam.rs
@@ -28,6 +28,7 @@ enum MamFormat {
     BINARY,
     ASCII,
     DEC,
+    TEXT,
 }
 
 struct MamType {
@@ -55,6 +56,9 @@ impl MamType {
     const fn dec(id: u16, len: u16, description: &'static str) -> Self {
         Self::new(id, len, MamFormat::DEC, description)
     }
+    const fn text(id: u16, len: u16, description: &'static str) -> Self {
+        Self::new(id, len, MamFormat::TEXT, description)
+    }
 }
 
 static MAM_ATTRIBUTES: &[MamType] = &[
@@ -95,12 +99,12 @@ static MAM_ATTRIBUTES: &[MamType] = &[
     MamType::ascii(0x08_00, 8, "Application Vendor"),
     MamType::ascii(0x08_01, 32, "Application Name"),
     MamType::ascii(0x08_02, 8, "Application Version"),
-    MamType::ascii(0x08_03, 160, "User Medium Text Label"),
+    MamType::text(0x08_03, 160, "User Medium Text Label"),
     MamType::ascii(0x08_04, 12, "Date And Time Last Written"),
     MamType::bin(0x08_05, 1, "Text Localization Identifier"),
     MamType::ascii(0x08_06, 32, "Barcode"),
     MamType::ascii(0x08_07, 80, "Owning Host Textual Name"),
-    MamType::ascii(0x08_08, 160, "Media Pool"),
+    MamType::text(0x08_08, 160, "Media Pool"),
     MamType::ascii(0x08_0B, 16, "Application Format Version"),
     // length for vol. coherency is not specified for IBM, and HP says 23-n
     MamType::bin(0x08_0C, 0, "Volume Coherency Information"),
@@ -188,7 +192,7 @@ fn decode_mam_attributes(data: &[u8]) -> Result<Vec<MamAttribute>, Error> {
         };
         if info.len == 0 || info.len == head.len {
             let value = match info.format {
-                MamFormat::ASCII => String::from_utf8_lossy(&data).to_string(),
+                MamFormat::ASCII | MamFormat::TEXT => String::from_utf8_lossy(&data).to_string(),
                 MamFormat::DEC => {
                     if info.len == 2 {
                         format!("{}", u16::from_be_bytes(data[0..2].try_into()?))
-- 
2.39.2



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

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

[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"),
+            (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] {
+            // ignore error
+            if let Err(err) = write_mam_attribute(&mut self.file, attr, b"") {
+                log::warn!("could not clear MAM attribute {attr:x}: {err}");
+            }
+        }
+    }
 }
 
 pub struct SgTapeReader<'a> {
diff --git a/pbs-tape/src/sg_tape/mam.rs b/pbs-tape/src/sg_tape/mam.rs
index 74e09c6d6..4e995d0b9 100644
--- a/pbs-tape/src/sg_tape/mam.rs
+++ b/pbs-tape/src/sg_tape/mam.rs
@@ -8,7 +8,7 @@ use proxmox_io::ReadExt;
 
 use pbs_api_types::MamAttribute;
 
-use crate::sgutils2::SgRaw;
+use crate::sgutils2::{alloc_page_aligned_buffer, SgRaw};
 
 use super::TapeAlertFlags;
 
@@ -143,6 +143,65 @@ fn read_tape_mam<F: AsRawFd>(file: &mut F) -> Result<Vec<u8>, Error> {
         .map(|v| v.to_vec())
 }
 
+/// Write attribute to MAM
+pub fn write_mam_attribute<F: AsRawFd>(
+    file: &mut F,
+    attribute_id: u16,
+    data: &[u8],
+) -> Result<(), Error> {
+    let mut sg_raw = SgRaw::new(file, 0)?;
+
+    let mut parameters = Vec::new();
+
+    let attribute = MAM_ATTRIBUTE_NAMES
+        .get(&attribute_id)
+        .ok_or_else(|| format_err!("MAM attribute '{attribute_id:x}' unknown"))?;
+
+    let mut attr_data = Vec::new();
+    attr_data.extend(attribute_id.to_be_bytes());
+    attr_data.push(match attribute.format {
+        MamFormat::BINARY | MamFormat::DEC => 0x00,
+        MamFormat::ASCII => 0x01,
+        MamFormat::TEXT => 0x02,
+    });
+    let len = if data.is_empty() { 0 } else { attribute.len };
+    attr_data.extend(len.to_be_bytes());
+    attr_data.extend(data);
+    if !data.is_empty() && data.len() < attribute.len as usize {
+        attr_data.resize(attr_data.len() - data.len() + attribute.len as usize, 0);
+    } else if data.len() > u16::MAX as usize {
+        bail!("data to long");
+    }
+
+    parameters.extend(attr_data);
+
+    let mut data_out = alloc_page_aligned_buffer(parameters.len() + 4)?;
+    data_out[..4].copy_from_slice(&(parameters.len() as u32).to_be_bytes());
+    data_out[4..].copy_from_slice(&parameters);
+
+    let mut cmd = vec![
+        0x8d, // WRITE ATTRIBUTE CDB (8Dh)
+        0x01, // WTC=1
+        0x00, // reserved
+        0x00, // reserved
+        0x00, // reserved
+        0x00, // Volume Number
+        0x00, // reserved
+        0x00, // Partition Number
+        0x00, // reserved
+        0x00, // reserved
+    ];
+    cmd.extend((data_out.len() as u32).to_be_bytes());
+    cmd.extend([
+        0x00, // reserved
+        0x00, // reserved
+    ]);
+
+    sg_raw.do_out_command(&cmd, &data_out)?;
+
+    Ok(())
+}
+
 /// Read Medium auxiliary memory attributes (cartridge memory) using raw SCSI command.
 pub fn read_mam_attributes<F: AsRawFd>(file: &mut F) -> Result<Vec<MamAttribute>, Error> {
     let data = read_tape_mam(file)?;
diff --git a/src/api2/tape/drive.rs b/src/api2/tape/drive.rs
index 7a791e09b..c6fc9f9c2 100644
--- a/src/api2/tape/drive.rs
+++ b/src/api2/tape/drive.rs
@@ -606,6 +606,8 @@ fn write_media_label(
 
     drive.rewind()?;
 
+    drive.write_additional_attributes(Some(media_id.label.label_text), pool);
+
     Ok(())
 }
 
diff --git a/src/tape/drive/lto/mod.rs b/src/tape/drive/lto/mod.rs
index f3143c907..23e043ce6 100644
--- a/src/tape/drive/lto/mod.rs
+++ b/src/tape/drive/lto/mod.rs
@@ -225,6 +225,8 @@ impl TapeDriver for LtoTapeHandle {
 
         self.set_encryption(encrypt_fingerprint)?;
 
+        self.write_additional_attributes(None, Some(media_set_label.pool.clone()));
+
         Ok(())
     }
 
@@ -272,6 +274,10 @@ impl TapeDriver for LtoTapeHandle {
     fn get_volume_statistics(&mut self) -> Result<pbs_api_types::Lp17VolumeStatistics, Error> {
         self.volume_statistics()
     }
+
+    fn write_additional_attributes(&mut self, label: Option<String>, pool: Option<String>) {
+        self.sg_tape.write_mam_attributes(label, pool)
+    }
 }
 
 fn run_sg_tape_cmd(subcmd: &str, args: &[&str], fd: RawFd) -> Result<String, Error> {
diff --git a/src/tape/drive/mod.rs b/src/tape/drive/mod.rs
index b21a62d20..b912b234d 100644
--- a/src/tape/drive/mod.rs
+++ b/src/tape/drive/mod.rs
@@ -245,6 +245,11 @@ pub trait TapeDriver {
 
     /// Returns volume statistics from a loaded tape
     fn get_volume_statistics(&mut self) -> Result<pbs_api_types::Lp17VolumeStatistics, Error>;
+
+    /// Writes additional attributes on the drive, like the vendor/application/etc. (e.g. on MAM)
+    ///
+    /// Since it's not fatal when it does not work, it only logs warnings in that case
+    fn write_additional_attributes(&mut self, label: Option<String>, pool: Option<String>);
 }
 
 /// A boxed implementor of [`MediaChange`].
diff --git a/src/tape/drive/virtual_tape.rs b/src/tape/drive/virtual_tape.rs
index c183e2681..866e4d323 100644
--- a/src/tape/drive/virtual_tape.rs
+++ b/src/tape/drive/virtual_tape.rs
@@ -465,6 +465,10 @@ impl TapeDriver for VirtualTapeHandle {
     fn get_volume_statistics(&mut self) -> Result<pbs_api_types::Lp17VolumeStatistics, Error> {
         Ok(Default::default())
     }
+
+    fn write_additional_attributes(&mut self, _label: Option<String>, _pool: Option<String>) {
+        // not implemented
+    }
 }
 
 impl MediaChange for VirtualTapeHandle {
-- 
2.39.2



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

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

[Thomas Lamprecht]

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

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

[Dominik Csapak]

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

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

[Thomas Lamprecht]

Am 23/05/2024 um 08:09 schrieb Dominik Csapak:
> On 5/22/24 19:24, Thomas Lamprecht wrote:
>> 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?)

Ok, that is a good point, and yeah a shame that the spec didn't make this
field 17 bytes wide, allowing to encode a [+-]ZZZZ UTC-timezone difference.

While for small setups in just one location, or set to the same timezone
in the whole organisation, independent of where the servers are located,
in can be even nice to use the local timezone, that still is a loss of
information.. Using UTC and documenting that is the only way to allow
being sure of what the actual time the tape was written to is in any
timezone.

So, I'd go for UTC here and document that. If we ever show this in the
UI or CLI we can render it correctly as ISO 8601 indicating that this is
UTC time.

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

This is mostly enforced indirectly currently or? As the label depends on
the pool name and that one is enforced to match the "safe" regex?

In that case it might be good to either future-proof or alternatively, IMO
not really better, to at least enforce/check that the saved text is ascii
directly here, as the data is a String, which is utf-8 in rust, so coupling
the assumption here to the rather distanced API schema format seems not
ideal to me.

btw. enforcing the length might be nice too, what would actually happen if
one writes more data than reserved by the spec, does it spill into the
next field, does something catches this and errors out?


> - APPLICATION FORMAT VERSION (always good to have)
> 
> isn't that implicated by the application version ?

Not necessarily, there can be a new way to write tapes from PBS in the
future and the version to use might be selectable, or the newer one
backported to an older stable version (at least as option).

IME, tracking format and program versions as separate things makes life
only easier in the long run.

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

You could combine those atoms that make out the whole tape format into a
full version by concatenating them with a separator like semicolon or a
plus or the like.

As this field has 16 characters you could even prefix each version with a
letter to make it slightly simpler to read, e.g.:

A1.2;C1.1;L1.0

Or use the letter as separator, making a bit more space for future version
extension:

A1.2C1.1L1.0

We could even use that now to define a global tape version or a, well,
versioning-version:

T1.0A1.2C1.1L1.0

A bit crowded but any (future) command of ours that outputs this information
could format that nicely and documenting it should cover third party tools.

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

At least in LTO-9 there would be "MEDIUM GLOBALLY UNIQUE IDENTIFIER" and "MEDIA
POOL GLOBALLY UNIQUE IDENTIFIER", differing per LTO version shouldn't (hopefully)
be an issue, but probably not _that_ important, at least if we do not have
existing information that can be mapped 1:1 to those two fields already.

btw. there's also BARCODE, as we support barcode labeling, it might be good
to write that out too I guess?


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

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

[Dominik Csapak]

On 5/23/24 10:10, Thomas Lamprecht wrote:
> Am 23/05/2024 um 08:09 schrieb Dominik Csapak:
>> On 5/22/24 19:24, Thomas Lamprecht wrote:
>>> 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?)
> 
> Ok, that is a good point, and yeah a shame that the spec didn't make this
> field 17 bytes wide, allowing to encode a [+-]ZZZZ UTC-timezone difference.
> 
> While for small setups in just one location, or set to the same timezone
> in the whole organisation, independent of where the servers are located,
> in can be even nice to use the local timezone, that still is a loss of
> information.. Using UTC and documenting that is the only way to allow
> being sure of what the actual time the tape was written to is in any
> timezone.
> 
> So, I'd go for UTC here and document that. If we ever show this in the
> UI or CLI we can render it correctly as ISO 8601 indicating that this is
> UTC time.

sounds reasonable

> 
>>
>>> - 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
> 
> This is mostly enforced indirectly currently or? As the label depends on
> the pool name and that one is enforced to match the "safe" regex?

yes it's only enforced by our regexes (though i don't expect them to
get less restrictive over time)

> 
> In that case it might be good to either future-proof or alternatively, IMO
> not really better, to at least enforce/check that the saved text is ascii
> directly here, as the data is a String, which is utf-8 in rust, so coupling
> the assumption here to the rather distanced API schema format seems not
> ideal to me.

i sent a patch to also include that field with the utf-8 value

> 
> btw. enforcing the length might be nice too, what would actually happen if
> one writes more data than reserved by the spec, does it spill into the
> next field, does something catches this and errors out?

we do enforce this already and fail with an error on writing
(before sending at all)


but my (educated) guess would be that if we'd try, the drive
would answer with a scsi error since they are relatively strict
in what they accept before doing anything
e.g. using the wrong type (ASCII vs TEXT) in the type field also does not work
even if it makes no difference in the actual data
so sending an invalid size wouldn't probably work either


> 
> 
>> - APPLICATION FORMAT VERSION (always good to have)
>>
>> isn't that implicated by the application version ?
> 
> Not necessarily, there can be a new way to write tapes from PBS in the
> future and the version to use might be selectable, or the newer one
> backported to an older stable version (at least as option).
> 
> IME, tracking format and program versions as separate things makes life
> only easier in the long run.
> 

makes sense

>>
>> 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
> 
> You could combine those atoms that make out the whole tape format into a
> full version by concatenating them with a separator like semicolon or a
> plus or the like.
> 
> As this field has 16 characters you could even prefix each version with a
> letter to make it slightly simpler to read, e.g.:
> 
> A1.2;C1.1;L1.0
> 
> Or use the letter as separator, making a bit more space for future version
> extension:
> 
> A1.2C1.1L1.0
> 
> We could even use that now to define a global tape version or a, well,
> versioning-version:
> 
> T1.0A1.2C1.1L1.0
> 
> A bit crowded but any (future) command of ours that outputs this information
> could format that nicely and documenting it should cover third party tools.

ok, i'll look into that. makes sense to combine the versions
(hopefully we don't get too man new archive types and our versions don't
get too long ;) )

> 
>>>
>>> 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
> 
> At least in LTO-9 there would be "MEDIUM GLOBALLY UNIQUE IDENTIFIER" and "MEDIA
> POOL GLOBALLY UNIQUE IDENTIFIER", differing per LTO version shouldn't (hopefully)
> be an issue, but probably not _that_ important, at least if we do not have
> existing information that can be mapped 1:1 to those two fields already.

for medium we'd actually have already a uuid, but i don't really want
to deviate for different lto versions, this would make it a bit more cumbersome
since we'd first have to query and parse the version of the tape each time

so i'd omit that for now until we decide we actually need it if that's
ok with you ?

> 
> btw. there's also BARCODE, as we support barcode labeling, it might be good
> to write that out too I guess?

well at least some vtls already set that automatically, and that might be used by
tape libs themselves, so I'm a bit hesitant to touch that field.
Though it could work out fine...



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

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

[Dominik Csapak]

On 5/23/24 10:22, Dominik Csapak wrote:
> On 5/23/24 10:10, Thomas Lamprecht wrote:
>> btw. enforcing the length might be nice too, what would actually happen if
>> one writes more data than reserved by the spec, does it spill into the
>> next field, does something catches this and errors out?
> 
> we do enforce this already and fail with an error on writing
> (before sending at all)

actually reading the code we don't, but i'll send a patch for that



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

[pbs-devel] applied: [PATCH proxmox-backup 1/2] tape: correct mam format for some attributes

[Dietmar Maurer]

applied both patches


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