* [pbs-devel] [RFC proxmox-backup] backup/backup_info: ignore vanished backup dirs during listing @ 2021-01-27 13:43 Dominik Csapak 2021-01-28 7:34 ` Fabian Grünbichler 0 siblings, 1 reply; 4+ messages in thread From: Dominik Csapak @ 2021-01-27 13:43 UTC (permalink / raw) To: pbs-devel this had the effect that that a snapshot listing errored out when called 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 <d.csapak@proxmox.com> --- 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, +}; use anyhow::{bail, format_err, Error}; use std::os::unix::io::RawFd; @@ -339,11 +342,24 @@ impl BackupInfo { fn list_backup_files<P: ?Sized + nix::NixPath>(dirfd: RawFd, path: &P) -> Result<Vec<String>, Error> { let mut files = vec![]; - tools::scandir(dirfd, path, &BACKUP_FILE_REGEX, |_, filename, file_type| { - if file_type != nix::dir::Type::File { return Ok(()); } - files.push(filename.to_owned()); - Ok(()) - })?; + match tools::fs::read_subdir(dirfd, path) { + Ok(iter) => { + for entry in iter.filter_file_name_regex(&BACKUP_FILE_REGEX) { + let entry = entry?; + let file_type = entry.file_type().ok_or_else(|| format_err!("unable to detect file type"))?; + if file_type == nix::dir::Type::File { + // ok because we filtered with BACKUP_FILE_REGEX + let filename = unsafe { entry.file_name_utf8_unchecked() }; + files.push(filename.to_owned()); + } + } + }, + Err(nix::Error::Sys(errno)) if errno == nix::errno::Errno::ENOENT => { + // ignore vanished backup dirs + eprintln!("backup dir vanished during listing"); + }, + Err(err) => bail!("error listing backup files: {}", err), + } Ok(files) } -- 2.20.1 ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [pbs-devel] [RFC proxmox-backup] backup/backup_info: ignore vanished backup dirs during listing 2021-01-27 13:43 [pbs-devel] [RFC proxmox-backup] backup/backup_info: ignore vanished backup dirs during listing Dominik Csapak @ 2021-01-28 7:34 ` Fabian Grünbichler 2021-01-28 12:26 ` Dominik Csapak 0 siblings, 1 reply; 4+ messages in thread From: Fabian Grünbichler @ 2021-01-28 7:34 UTC (permalink / raw) To: Proxmox Backup Server development discussion 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. 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.. 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? On January 27, 2021 2:43 pm, Dominik Csapak wrote: > this had the effect that that a snapshot listing errored out when called > 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 <d.csapak@proxmox.com> > --- > 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, > +}; > > use anyhow::{bail, format_err, Error}; > use std::os::unix::io::RawFd; > @@ -339,11 +342,24 @@ impl BackupInfo { > fn list_backup_files<P: ?Sized + nix::NixPath>(dirfd: RawFd, path: &P) -> Result<Vec<String>, Error> { > let mut files = vec![]; > > - tools::scandir(dirfd, path, &BACKUP_FILE_REGEX, |_, filename, file_type| { > - if file_type != nix::dir::Type::File { return Ok(()); } > - files.push(filename.to_owned()); > - Ok(()) > - })?; > + match tools::fs::read_subdir(dirfd, path) { > + Ok(iter) => { > + for entry in iter.filter_file_name_regex(&BACKUP_FILE_REGEX) { > + let entry = entry?; > + let file_type = entry.file_type().ok_or_else(|| format_err!("unable to detect file type"))?; > + if file_type == nix::dir::Type::File { > + // ok because we filtered with BACKUP_FILE_REGEX > + let filename = unsafe { entry.file_name_utf8_unchecked() }; > + files.push(filename.to_owned()); > + } > + } > + }, > + Err(nix::Error::Sys(errno)) if errno == nix::errno::Errno::ENOENT => { > + // ignore vanished backup dirs > + eprintln!("backup dir vanished during listing"); > + }, > + Err(err) => bail!("error listing backup files: {}", err), > + } > > Ok(files) > } > -- > 2.20.1 > > > > _______________________________________________ > pbs-devel mailing list > pbs-devel@lists.proxmox.com > https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel > > > ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [pbs-devel] [RFC proxmox-backup] backup/backup_info: ignore vanished backup dirs during listing 2021-01-28 7:34 ` Fabian Grünbichler @ 2021-01-28 12:26 ` Dominik Csapak 2021-01-29 9:01 ` Fabian Grünbichler 0 siblings, 1 reply; 4+ messages in thread From: Dominik Csapak @ 2021-01-28 12:26 UTC (permalink / raw) To: pbs-devel On 1/28/21 8:34 AM, Fabian Grünbichler 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. > > 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.. > > 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? > 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<Option<Vec<String>>, Error> to differentiate between 'no files' and 'vanished' ? > On January 27, 2021 2:43 pm, Dominik Csapak wrote: >> this had the effect that that a snapshot listing errored out when called >> 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 <d.csapak@proxmox.com> >> --- >> 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, >> +}; >> >> use anyhow::{bail, format_err, Error}; >> use std::os::unix::io::RawFd; >> @@ -339,11 +342,24 @@ impl BackupInfo { >> fn list_backup_files<P: ?Sized + nix::NixPath>(dirfd: RawFd, path: &P) -> Result<Vec<String>, Error> { >> let mut files = vec![]; >> >> - tools::scandir(dirfd, path, &BACKUP_FILE_REGEX, |_, filename, file_type| { >> - if file_type != nix::dir::Type::File { return Ok(()); } >> - files.push(filename.to_owned()); >> - Ok(()) >> - })?; >> + match tools::fs::read_subdir(dirfd, path) { >> + Ok(iter) => { >> + for entry in iter.filter_file_name_regex(&BACKUP_FILE_REGEX) { >> + let entry = entry?; >> + let file_type = entry.file_type().ok_or_else(|| format_err!("unable to detect file type"))?; >> + if file_type == nix::dir::Type::File { >> + // ok because we filtered with BACKUP_FILE_REGEX >> + let filename = unsafe { entry.file_name_utf8_unchecked() }; >> + files.push(filename.to_owned()); >> + } >> + } >> + }, >> + Err(nix::Error::Sys(errno)) if errno == nix::errno::Errno::ENOENT => { >> + // ignore vanished backup dirs >> + eprintln!("backup dir vanished during listing"); >> + }, >> + Err(err) => bail!("error listing backup files: {}", err), >> + } >> >> Ok(files) >> } >> -- >> 2.20.1 >> >> >> >> _______________________________________________ >> pbs-devel mailing list >> pbs-devel@lists.proxmox.com >> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel >> >> >> > > > _______________________________________________ > pbs-devel mailing list > pbs-devel@lists.proxmox.com > https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel > > ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [pbs-devel] [RFC proxmox-backup] backup/backup_info: ignore vanished backup dirs during listing 2021-01-28 12:26 ` Dominik Csapak @ 2021-01-29 9:01 ` Fabian Grünbichler 0 siblings, 0 replies; 4+ messages in thread From: Fabian Grünbichler @ 2021-01-29 9:01 UTC (permalink / raw) To: Proxmox Backup Server development discussion On January 28, 2021 1:26 pm, Dominik Csapak wrote: > On 1/28/21 8:34 AM, Fabian Grünbichler 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. >> >> 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.. >> >> 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? >> > > 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<Option<Vec<String>>, Error> to differentiate between > 'no files' and 'vanished' ? IMHO the correct thing would probably be a custom error or result type that we can match on directly higher up the stack - but that is a can of worms that we tried to avoid so far ;) Result<Option<Vec<..>>> is not really readable without consulting the corresponding docs/code. we could have a ListFilesResult { Files(Vec<String>), Vanished, Err(...), } or (wrapped in a regular Result) ListFilesResult { Files(Vec<String>), Vanished, } if we want to postpone thinking about custom error types even further ;) the latter would be kind of like your proposal but with more readable (and extensible) encoding of the "half-way" error state, and it could be re-used for other list files operations as well? > >> On January 27, 2021 2:43 pm, Dominik Csapak wrote: >>> this had the effect that that a snapshot listing errored out when called >>> 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 <d.csapak@proxmox.com> >>> --- >>> 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, >>> +}; >>> >>> use anyhow::{bail, format_err, Error}; >>> use std::os::unix::io::RawFd; >>> @@ -339,11 +342,24 @@ impl BackupInfo { >>> fn list_backup_files<P: ?Sized + nix::NixPath>(dirfd: RawFd, path: &P) -> Result<Vec<String>, Error> { >>> let mut files = vec![]; >>> >>> - tools::scandir(dirfd, path, &BACKUP_FILE_REGEX, |_, filename, file_type| { >>> - if file_type != nix::dir::Type::File { return Ok(()); } >>> - files.push(filename.to_owned()); >>> - Ok(()) >>> - })?; >>> + match tools::fs::read_subdir(dirfd, path) { >>> + Ok(iter) => { >>> + for entry in iter.filter_file_name_regex(&BACKUP_FILE_REGEX) { >>> + let entry = entry?; >>> + let file_type = entry.file_type().ok_or_else(|| format_err!("unable to detect file type"))?; >>> + if file_type == nix::dir::Type::File { >>> + // ok because we filtered with BACKUP_FILE_REGEX >>> + let filename = unsafe { entry.file_name_utf8_unchecked() }; >>> + files.push(filename.to_owned()); >>> + } >>> + } >>> + }, >>> + Err(nix::Error::Sys(errno)) if errno == nix::errno::Errno::ENOENT => { >>> + // ignore vanished backup dirs >>> + eprintln!("backup dir vanished during listing"); >>> + }, >>> + Err(err) => bail!("error listing backup files: {}", err), >>> + } >>> >>> Ok(files) >>> } >>> -- >>> 2.20.1 >>> >>> >>> >>> _______________________________________________ >>> pbs-devel mailing list >>> pbs-devel@lists.proxmox.com >>> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel >>> >>> >>> >> >> >> _______________________________________________ >> pbs-devel mailing list >> pbs-devel@lists.proxmox.com >> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel >> >> > > > > _______________________________________________ > pbs-devel mailing list > pbs-devel@lists.proxmox.com > https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel > ^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2021-01-29 9:02 UTC | newest] Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-01-27 13:43 [pbs-devel] [RFC proxmox-backup] backup/backup_info: ignore vanished backup dirs during listing Dominik Csapak 2021-01-28 7:34 ` Fabian Grünbichler 2021-01-28 12:26 ` Dominik Csapak 2021-01-29 9:01 ` Fabian Grünbichler
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.Service provided by Proxmox Server Solutions GmbH | Privacy | Legal