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 5D5BA96ED8 for ; Tue, 16 Apr 2024 16:17:31 +0200 (CEST) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id 45F5B1CE7B for ; Tue, 16 Apr 2024 16:17:31 +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)) (No client certificate requested) by firstgate.proxmox.com (Proxmox) with ESMTPS for ; Tue, 16 Apr 2024 16:17:25 +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 C0F1940657 for ; Tue, 16 Apr 2024 16:17:25 +0200 (CEST) Mime-Version: 1.0 Content-Transfer-Encoding: quoted-printable Content-Type: text/plain; charset=UTF-8 Date: Tue, 16 Apr 2024 16:17:24 +0200 Message-Id: From: "Hannes Laimer" To: "Christian Ebner" , "Proxmox Backup Server development discussion" X-Mailer: aerc 0.17.0-78-g4ffbaa6b3946 References: <20240409110012.166472-1-h.laimer@proxmox.com> <20240409110012.166472-6-h.laimer@proxmox.com> In-Reply-To: X-SPAM-LEVEL: Spam detection results: 0 AWL -0.008 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: Tue, 16 Apr 2024 14:17:31 -0000 On Mon Apr 15, 2024 at 5:09 PM CEST, Christian Ebner wrote: > 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(-) > >=20 > > diff --git a/pbs-datastore/src/datastore.rs b/pbs-datastore/src/datasto= re.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(()) > > } > > =20 > > +pub fn check_if_available(config: &DataStoreConfig) -> Result<(), Erro= r> { > > the function signature suggests that this checks if the datastore is=20 > available, so maybe call this `is_datastore_available` and let it return= =20 > a boolean instead, further distinguishing between error states because=20 > of runtime errors and the datastore simply not being available. > makes sense, will do in v4 > > + config.backing_device.as_ref().map_or(Ok(()), |uuid| { > > + let mut command =3D std::process::Command::new("findmnt"); > > + command.args(["-n", "-o", "TARGET", "--source", &format!("UUID= =3D{uuid}")]); > > + > > + match proxmox_sys::command::run_command(command, None) { > > + Ok(mount_point) if mount_point.trim_end() =3D=3D config.pa= th =3D> Ok(()), > > + _ =3D> Err(format_err!( > > this should distinguish between an error because of the `run_command`=20 > failing because of e.g. outdated/incompatible command line arguments=20 > passed to `findmnt` and the filesystem not being mounted. > I see however that `findmnt` only has 2 error codes, so maybe this could= =20 > be done by using `lsblk` and filtering the output based on the `uuid`=20 > instead, but there might be better solutions that I am not aware of. > yes, but I won't use an external tool in v4 > > + "device for datastore '{}' has to be mounted at '{}'", > > should actually be the filesystem on the device that needs to be=20 > 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 =3D config.name.clone(); > > =20 > > + check_if_available(&config)?; > > + > > let tuning: DatastoreTuning =3D serde_json::from_value( > > DatastoreTuning::API_SCHEMA > > .parse_property_string(config.tuning.as_deref().unwra= p_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; > > =20 > > mod datastore; > > -pub use datastore::{check_backup_owner, DataStore}; > > +pub use datastore::{check_backup_owner, check_if_available, DataStore}= ; > > =20 > > mod hierarchy; > > pub use hierarchy::{