Re: [PATCH v1 proxmox-backup 2/3] client: pxar: consistently ignore vanished files and warn about them

[Christian Ebner <c.ebner@proxmox.com>]

On 5/7/26 5:48 PM, Robert Obkircher wrote:
> 
> On 07.05.26 10:36, Christian Ebner wrote:
>> Two nits inline, otherwise LGTM.
>>
>> Reviewed-by: Christian Ebner <c.ebner@proxmox.com>
>>
>> On 5/6/26 1:48 PM, Robert Obkircher wrote:
>>> Combine the error paths and treat deleted entries equally in both
>>> cases. Since this used to be a hard error in some cases, it also makes
>>> sense to emit a warning about it.
>>>
>>> Signed-off-by: Robert Obkircher <r.obkircher@proxmox.com>
>>> ---
>>>    pbs-client/src/pxar/create.rs | 32 ++++++++++++--------------------
>>>    1 file changed, 12 insertions(+), 20 deletions(-)
>>>
>>> diff --git a/pbs-client/src/pxar/create.rs
>>> b/pbs-client/src/pxar/create.rs
>>> index 08c5c9b44..47cbf50a8 100644
>>> --- a/pbs-client/src/pxar/create.rs
>>> +++ b/pbs-client/src/pxar/create.rs
>>> @@ -695,36 +695,28 @@ impl Archiver {
>>>                        })
>>>                    });
>>>    -            match match_result {
>>> +            let stat_result = match match_result {
>>>                    Ok(Some(MatchType::Exclude)) => {
>>>                        debug!("matched by exclude pattern
>>> '{full_path:?}'");
>>>                        continue;
>>>                    }
>>> -                Ok(_) => (),
>>> -                Err(err) if err.not_found() => continue,
>>> +                Ok(_) => stat_result.map_or_else(do_stat, Ok),
>>> +                Err(e) => Err(e),
>>> +            };
>>> +
>>> +            let stat = match stat_result {
>>> +                Ok(stat) => stat,
>>> +                Err(err) if err.not_found() => {
>>> +                    warn!("warning: file vanished while reading
>>> directory: {full_path:?}");
>>
>> nit: should use and slightly adapt the report_vanished_file() helper.
> Imo the messages should be different, so this would essentially turn into:
> 
> fn report_vanished_file(message: &str, path: &Path) {
>      warn!("warning: file vanished {message}: {path:?}")
> }
> 
> which seemed overly complicated for something that would only be
> called from two locations.

Fine by me, but in that case I would suggest to cleanup and inline the 
other call site as well. No need to keep it around then ...

> 
> I also noticed that `read_pxar_excludes` calls `open_file` without
> setting `self.path`, so the existing helpers would print the wrong
> message in that case.
> 
>>
>>> +                    continue;
>>> +                }
>>>                    Err(Errno::ESTALE) => {
>>>                        self.report_stale_file_handle(Some(&full_path));
>>>                        continue;
>>>                    }
>>>                    Err(err) => {
>>> -                    return Err(err).with_context(|| format!("stat
>>> failed on {full_path:?}"))
>>> +                    return
>>> Err(Error::from(err).context(format!("stat failed on {full_path:?}")))
>>
>> nit: please use with_context() over context() here, since the former
>> is evaluated lazily only once an error does occur [0].
>>
>> [0]
>> https://docs.rs/anyhow/latest/anyhow/trait.Context.html#required-methods
> How? This is the error path that converts the Errno to an anyhow
> error. It doesn't call the method from that trait.

diff --git a/pbs-client/src/pxar/create.rs b/pbs-client/src/pxar/create.rs
index c101415c1..e1e4eea3b 100644
--- a/pbs-client/src/pxar/create.rs
+++ b/pbs-client/src/pxar/create.rs
@@ -720,7 +720,8 @@ impl Archiver {
                      continue;
                  }
                  Err(err) => {
-                    return Err(Error::from(err).context(format!("stat 
failed on {full_path:?}")))
+                    return Err(Error::from(err))
+                        .with_context(|| format!("stat failed on 
{full_path:?}"))
                  }
              };

This should do, unless I'm overlooking something?

>>
>>>                    }
>>> -            }
>>> -
>>> -            let stat = match stat_result {
>>> -                Some(stat) => stat,
>>> -                None => match do_stat() {
>>> -                    Ok(stat) => stat,
>>> -                    Err(Errno::ESTALE) => {
>>> -
>>> self.report_stale_file_handle(Some(&full_path));
>>> -                        continue;
>>> -                    }
>>> -                    Err(err) => {
>>> -                        return Err(
>>> -                            Error::from(err).context(format!("stat
>>> failed on {full_path:?}"))
>>> -                        )
>>> -                    }
>>> -                },
>>>                };
>>>                  self.entry_counter += 1;
>>