From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from firstgate.proxmox.com (firstgate.proxmox.com [212.224.123.68]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits)) (No client certificate requested) by lists.proxmox.com (Postfix) with ESMTPS id 5F99C9643B for ; Mon, 15 Apr 2024 17:10:25 +0200 (CEST) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id 3838FDBEE for ; Mon, 15 Apr 2024 17:09:55 +0200 (CEST) Received: from proxmox-new.maurer-it.com (proxmox-new.maurer-it.com [94.136.29.106]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by firstgate.proxmox.com (Proxmox) with ESMTPS for ; Mon, 15 Apr 2024 17:09:53 +0200 (CEST) Received: from proxmox-new.maurer-it.com (localhost.localdomain [127.0.0.1]) by proxmox-new.maurer-it.com (Proxmox) with ESMTP id AB75844ACC for ; Mon, 15 Apr 2024 17:09:53 +0200 (CEST) Message-ID: Date: Mon, 15 Apr 2024 17:09:52 +0200 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird To: Proxmox Backup Server development discussion , Hannes Laimer References: <20240409110012.166472-1-h.laimer@proxmox.com> <20240409110012.166472-6-h.laimer@proxmox.com> Content-Language: en-US, de-DE From: Christian Ebner In-Reply-To: <20240409110012.166472-6-h.laimer@proxmox.com> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit X-SPAM-LEVEL: Spam detection results: 0 AWL 0.030 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 Subject: Re: [pbs-devel] [PATCH proxmox-backup v3 05/24] add helper for checking if a removable datastore is available 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: , X-List-Received-Date: Mon, 15 Apr 2024 15:10:25 -0000 some comments inline On 4/9/24 12:59, Hannes Laimer wrote: > Signed-off-by: Hannes Laimer > --- > pbs-datastore/src/datastore.rs | 18 ++++++++++++++++++ > pbs-datastore/src/lib.rs | 2 +- > 2 files changed, 19 insertions(+), 1 deletion(-) > > diff --git a/pbs-datastore/src/datastore.rs b/pbs-datastore/src/datastore.rs > index 0685cc84..db47205c 100644 > --- a/pbs-datastore/src/datastore.rs > +++ b/pbs-datastore/src/datastore.rs > @@ -49,6 +49,22 @@ pub fn check_backup_owner(owner: &Authid, auth_id: &Authid) -> Result<(), Error> > Ok(()) > } > > +pub fn check_if_available(config: &DataStoreConfig) -> Result<(), Error> { the function signature suggests that this checks if the datastore is available, so maybe call this `is_datastore_available` and let it return a boolean instead, further distinguishing between error states because of runtime errors and the datastore simply not being available. > + config.backing_device.as_ref().map_or(Ok(()), |uuid| { > + let mut command = std::process::Command::new("findmnt"); > + command.args(["-n", "-o", "TARGET", "--source", &format!("UUID={uuid}")]); > + > + match proxmox_sys::command::run_command(command, None) { > + Ok(mount_point) if mount_point.trim_end() == config.path => Ok(()), > + _ => Err(format_err!( this should distinguish between an error because of the `run_command` failing because of e.g. outdated/incompatible command line arguments passed to `findmnt` and the filesystem not being mounted. I see however that `findmnt` only has 2 error codes, so maybe this could be done by using `lsblk` and filtering the output based on the `uuid` instead, but there might be better solutions that I am not aware of. > + "device for datastore '{}' has to be mounted at '{}'", should actually be the filesystem on the device that needs to be mounted, not the device? > + config.name, > + config.path > + )), > + } > + }) > +} > + > /// Datastore Management > /// > /// A Datastore can store severals backups, and provides the > @@ -261,6 +277,8 @@ impl DataStore { > ) -> Result, Error> { > let name = config.name.clone(); > > + check_if_available(&config)?; > + > let tuning: DatastoreTuning = serde_json::from_value( > DatastoreTuning::API_SCHEMA > .parse_property_string(config.tuning.as_deref().unwrap_or(""))?, > diff --git a/pbs-datastore/src/lib.rs b/pbs-datastore/src/lib.rs > index 43050162..f5e93e92 100644 > --- a/pbs-datastore/src/lib.rs > +++ b/pbs-datastore/src/lib.rs > @@ -206,7 +206,7 @@ pub use manifest::BackupManifest; > pub use store_progress::StoreProgress; > > mod datastore; > -pub use datastore::{check_backup_owner, DataStore}; > +pub use datastore::{check_backup_owner, check_if_available, DataStore}; > > mod hierarchy; > pub use hierarchy::{