From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from firstgate.proxmox.com (firstgate.proxmox.com [IPv6:2a01:7e0:0:424::9]) by lore.proxmox.com (Postfix) with ESMTPS id 3D3B21FF399 for ; Thu, 23 May 2024 08:10:00 +0200 (CEST) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id F006017FCE; Thu, 23 May 2024 08:10:17 +0200 (CEST) Message-ID: Date: Thu, 23 May 2024 08:09:44 +0200 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Beta To: Thomas Lamprecht , Proxmox Backup Server development discussion References: <20240514141248.1614306-1-d.csapak@proxmox.com> <20240514141248.1614306-2-d.csapak@proxmox.com> Content-Language: en-US From: Dominik Csapak In-Reply-To: X-SPAM-LEVEL: Spam detection results: 0 AWL 0.016 Adjusted score from AWL reputation of From: address BAYES_00 -1.9 Bayes spam probability is 0 to 1% DMARC_MISSING 0.1 Missing DMARC policy KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_PASS -0.001 SPF: sender matches SPF record URIBL_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to URIBL was blocked. See http://wiki.apache.org/spamassassin/DnsBlocklists#dnsbl-block for more information. [drive.rs, proxmox.com, mod.rs, mam.rs] Subject: Re: [pbs-devel] [PATCH proxmox-backup 2/2] tape: write informational MAM attributes on tapes X-BeenThere: pbs-devel@lists.proxmox.com X-Mailman-Version: 2.1.29 Precedence: list List-Id: Proxmox Backup Server development discussion List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Reply-To: Proxmox Backup Server development discussion Content-Transfer-Encoding: 7bit Content-Type: text/plain; charset="us-ascii"; Format="flowed" Errors-To: pbs-devel-bounces@lists.proxmox.com Sender: "pbs-devel" 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 >> --- >> 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, pool: Option) { >> + 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