[pbs-devel] [RFC proxmox-backup] backup/backup_info: ignore vanished backup dirs during listing

[Fabian Grünbichler]

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 at 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 at lists.proxmox.com
>>> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel
>>>
>>>
>>>
>> 
>> 
>> _______________________________________________
>> pbs-devel mailing list
>> pbs-devel at lists.proxmox.com
>> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel
>> 
>> 
> 
> 
> 
> _______________________________________________
> pbs-devel mailing list
> pbs-devel at lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel
>