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) server-digest SHA256) (No client certificate requested) by lists.proxmox.com (Postfix) with ESMTPS id D0A4A6C1B3 for ; Fri, 29 Jan 2021 10:02:29 +0100 (CET) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id BD4DECC10 for ; Fri, 29 Jan 2021 10:01:59 +0100 (CET) Received: from proxmox-new.maurer-it.com (proxmox-new.maurer-it.com [212.186.127.180]) (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 id B9C15CC03 for ; Fri, 29 Jan 2021 10:01:57 +0100 (CET) Received: from proxmox-new.maurer-it.com (localhost.localdomain [127.0.0.1]) by proxmox-new.maurer-it.com (Proxmox) with ESMTP id 7CE2F46167 for ; Fri, 29 Jan 2021 10:01:57 +0100 (CET) Date: Fri, 29 Jan 2021 10:01:49 +0100 From: Fabian =?iso-8859-1?q?Gr=FCnbichler?= To: Proxmox Backup Server development discussion References: <20210127134314.22530-1-d.csapak@proxmox.com> <1611818626.6jkzjorqup.astroid@nora.none> <0c7984b2-e88a-1a87-c388-95d539b85cf1@proxmox.com> In-Reply-To: <0c7984b2-e88a-1a87-c388-95d539b85cf1@proxmox.com> MIME-Version: 1.0 User-Agent: astroid/0.15.0 (https://github.com/astroidmail/astroid) Message-Id: <1611910414.lsy9syj5t1.astroid@nora.none> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable X-SPAM-LEVEL: Spam detection results: 0 AWL -0.375 Adjusted score from AWL reputation of From: address KAM_ASCII_DIVIDERS 0.8 Spam that uses ascii formatting tricks KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment RCVD_IN_DNSWL_MED -2.3 Sender listed at https://www.dnswl.org/, medium trust 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. [proxmox.com] Subject: Re: [pbs-devel] [RFC proxmox-backup] backup/backup_info: ignore vanished backup dirs during listing 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: Fri, 29 Jan 2021 09:02:29 -0000 On January 28, 2021 1:26 pm, Dominik Csapak wrote: > On 1/28/21 8:34 AM, Fabian Gr=C3=BCnbichler wrote: >> when looking at the call sites of list_backup_files, this seems wrong - >> e.g. when listing a BackupGroup's backups, such a no-longer-existing >> snapshot is now returned but has no files. when creating a new >> BackupInfo from a BackupDir, or listing a BackupInfo's files, it returns >> Ok but contains no files. >>=20 >> the decision whether to ignore such an error must be made at the call >> sites of the call sites above, since in most cases I'd want to ignore >> the snapshot and not just the file listing.. >>=20 >> list_snapshot_files already throws an error if it can't read the >> manifest, list_snapshots and list_groups seem to include it with no >> files? >>=20 >=20 > yeah you are right, we should probably catch this wider up in the call > chain, but do we really want to downcast the anyhow errors wider up > to nix errors? or do we want to have either an extra error type? > or do we maybe want to make the return type an > Result>, Error> to differentiate between > 'no files' and 'vanished' ? IMHO the correct thing would probably be a custom error or result type=20 that we can match on directly higher up the stack - but that is a can of=20 worms that we tried to avoid so far ;) Result>> is not really readable without consulting the=20 corresponding docs/code. we could have a=20 ListFilesResult { Files(Vec), Vanished, Err(...), } or (wrapped in a regular Result) ListFilesResult { Files(Vec), Vanished, } if we want to postpone thinking about custom error types even further ;)=20 the latter would be kind of like your proposal but with more readable=20 (and extensible) encoding of the "half-way" error state, and it could be=20 re-used for other list files operations as well? >=20 >> On January 27, 2021 2:43 pm, Dominik Csapak wrote: >>> this had the effect that that a snapshot listing errored out when calle= d >>> during a prune action >>> >>> we now ignore such errors and simply return no files >>> >>> also add some context for the error if one happens >>> >>> Signed-off-by: Dominik Csapak >>> --- >>> maybe we also want to filter such snapshots out one level higher? >>> so that we do not show 'empty' snapshots >>> >>> src/backup/backup_info.rs | 28 ++++++++++++++++++++++------ >>> 1 file changed, 22 insertions(+), 6 deletions(-) >>> >>> diff --git a/src/backup/backup_info.rs b/src/backup/backup_info.rs >>> index 5ff1a6f8..dba3fea5 100644 >>> --- a/src/backup/backup_info.rs >>> +++ b/src/backup/backup_info.rs >>> @@ -1,4 +1,7 @@ >>> -use crate::tools; >>> +use crate::tools::{ >>> + self, >>> + fs::FileIterOps, >>> +}; >>> =20 >>> use anyhow::{bail, format_err, Error}; >>> use std::os::unix::io::RawFd; >>> @@ -339,11 +342,24 @@ impl BackupInfo { >>> fn list_backup_files(dirfd: RawFd, path: &P= ) -> Result, Error> { >>> let mut files =3D vec![]; >>> =20 >>> - tools::scandir(dirfd, path, &BACKUP_FILE_REGEX, |_, filename, file= _type| { >>> - if file_type !=3D nix::dir::Type::File { return Ok(()); } >>> - files.push(filename.to_owned()); >>> - Ok(()) >>> - })?; >>> + match tools::fs::read_subdir(dirfd, path) { >>> + Ok(iter) =3D> { >>> + for entry in iter.filter_file_name_regex(&BACKUP_FILE_REGE= X) { >>> + let entry =3D entry?; >>> + let file_type =3D entry.file_type().ok_or_else(|| form= at_err!("unable to detect file type"))?; >>> + if file_type =3D=3D nix::dir::Type::File { >>> + // ok because we filtered with BACKUP_FILE_REGEX >>> + let filename =3D unsafe { entry.file_name_utf8_unc= hecked() }; >>> + files.push(filename.to_owned()); >>> + } >>> + } >>> + }, >>> + Err(nix::Error::Sys(errno)) if errno =3D=3D nix::errno::Errno:= :ENOENT =3D> { >>> + // ignore vanished backup dirs >>> + eprintln!("backup dir vanished during listing"); >>> + }, >>> + Err(err) =3D> bail!("error listing backup files: {}", err), >>> + } >>> =20 >>> Ok(files) >>> } >>> --=20 >>> 2.20.1 >>> >>> >>> >>> _______________________________________________ >>> pbs-devel mailing list >>> pbs-devel@lists.proxmox.com >>> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel >>> >>> >>> >>=20 >>=20 >> _______________________________________________ >> pbs-devel mailing list >> pbs-devel@lists.proxmox.com >> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel >>=20 >>=20 >=20 >=20 >=20 > _______________________________________________ > pbs-devel mailing list > pbs-devel@lists.proxmox.com > https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel >=20 =