[PATCH v1 proxmox-backup 0/3] improve fstatat error handling

[PATCH v1 proxmox-backup 0/3] improve fstatat error handling

[Robert Obkircher]

Improve error handling around fstatat when creating pxar archives.

Robert Obkircher (3):
  client: pxar: improve variable names
  client: pxar: consistently ignore vanished files and warn about them
  client: pxar: skip files on fstatat permission errors

 pbs-client/src/pxar/create.rs | 45 ++++++++++++++++-------------------
 1 file changed, 21 insertions(+), 24 deletions(-)

-- 
2.47.3

[PATCH v1 proxmox-backup 1/3] client: pxar: improve variable names

[Robert Obkircher]

The Option can hold at most one result, and the closure computes more
than just the mode.

Signed-off-by: Robert Obkircher <r.obkircher@proxmox.com>
---
 pbs-client/src/pxar/create.rs | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/pbs-client/src/pxar/create.rs b/pbs-client/src/pxar/create.rs
index d18021c4c..08c5c9b44 100644
--- a/pbs-client/src/pxar/create.rs
+++ b/pbs-client/src/pxar/create.rs
@@ -676,9 +676,9 @@ impl Archiver {
 
             let match_path = PathBuf::from("/").join(full_path.clone());
 
-            let mut stat_results: Option<FileStat> = None;
+            let mut stat_result: Option<FileStat> = None;
 
-            let get_file_mode = || {
+            let do_stat = || {
                 nix::sys::stat::fstatat(
                     Some(dir_fd),
                     file_name,
@@ -689,9 +689,9 @@ impl Archiver {
             let match_result = self
                 .patterns
                 .matches(match_path.as_os_str().as_bytes(), || {
-                    Ok::<_, Errno>(match &stat_results {
+                    Ok::<_, Errno>(match &stat_result {
                         Some(result) => result.st_mode,
-                        None => stat_results.insert(get_file_mode()?).st_mode,
+                        None => stat_result.insert(do_stat()?).st_mode,
                     })
                 });
 
@@ -711,10 +711,10 @@ impl Archiver {
                 }
             }
 
-            let stat = match stat_results {
-                Some(mode) => mode,
-                None => match get_file_mode() {
-                    Ok(mode) => mode,
+            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;
-- 
2.47.3

Re: [PATCH v1 proxmox-backup 1/3] client: pxar: improve variable names

[Christian Ebner]

On 5/6/26 1:49 PM, Robert Obkircher wrote:
> The Option can hold at most one result, and the closure computes more
> than just the mode.
> 
> Signed-off-by: Robert Obkircher <r.obkircher@proxmox.com>
> ---

Reviewed-by: Christian Ebner <c.ebner@proxmox.com>

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

[Robert Obkircher]

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:?}");
+                    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:?}")))
                 }
-            }
-
-            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;
-- 
2.47.3

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

[Christian Ebner]

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.

> +                    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

>                   }
> -            }
> -
> -            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;

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

[Robert Obkircher]

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.

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.
>
>>                   }
>> -            }
>> -
>> -            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;
>

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

[Christian Ebner]

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;
>>

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

[Robert Obkircher]

On 08.05.26 11:36, Christian Ebner wrote:
> 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? 

If we inline <Result as Context>::with_context then your version it
results in

match Err(Error::from(err)) {
    Ok(ok) => Ok(ok),
    Err(error) => Err(error.ext_context(context())),
}

which is not lazy. The original code had both versions and I chose the
more explicit one.

>
>>>
>>>>                    }
>>>> -            }
>>>> -
>>>> -            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;
>>>
>

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

[Christian Ebner]

On 5/8/26 1:38 PM, Robert Obkircher wrote:
> 
> On 08.05.26 11:36, Christian Ebner wrote:
>> On 5/7/26 5:48 PM, Robert Obkircher wrote:

[..]

>>>>
>>> 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?
> 
> If we inline <Result as Context>::with_context then your version it
> results in
> 
> match Err(Error::from(err)) {
>      Ok(ok) => Ok(ok),
>      Err(error) => Err(error.ext_context(context())),
> }
> 
> which is not lazy. The original code had both versions and I chose the
> more explicit one.

Right (should have checked the implementation details first).

In that case could you include a patch to cleanup the (one) other 
implicit context creation?

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

[Robert Obkircher]

On 08.05.26 15:24, Christian Ebner wrote:
> On 5/8/26 1:38 PM, Robert Obkircher wrote:
>>
>> On 08.05.26 11:36, Christian Ebner wrote:
>>> On 5/7/26 5:48 PM, Robert Obkircher wrote:
>
> [..]
>
>>>>>
>>>> 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?
>>
>> If we inline <Result as Context>::with_context then your version it
>> results in
>>
>> match Err(Error::from(err)) {
>>      Ok(ok) => Ok(ok),
>>      Err(error) => Err(error.ext_context(context())),
>> }
>>
>> which is not lazy. The original code had both versions and I chose the
>> more explicit one.
>
> Right (should have checked the implementation details first).
>
> In that case could you include a patch to cleanup the (one) other
> implicit context creation? 

Which one? I don't see another one

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

[Christian Ebner]

On 5/8/26 4:01 PM, Robert Obkircher wrote:
> 
> On 08.05.26 15:24, Christian Ebner wrote:
>> On 5/8/26 1:38 PM, Robert Obkircher wrote:
>>>
>>> On 08.05.26 11:36, Christian Ebner wrote:
>>>> On 5/7/26 5:48 PM, Robert Obkircher wrote:
>>
>> [..]
>>
>>>>>>
>>>>> 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?
>>>
>>> If we inline <Result as Context>::with_context then your version it
>>> results in
>>>
>>> match Err(Error::from(err)) {
>>>       Ok(ok) => Ok(ok),
>>>       Err(error) => Err(error.ext_context(context())),
>>> }
>>>
>>> which is not lazy. The original code had both versions and I chose the
>>> more explicit one.
>>
>> Right (should have checked the implementation details first).
>>
>> In that case could you include a patch to cleanup the (one) other
>> implicit context creation?
> 
> Which one? I don't see another one

I was referring to:

https://git.proxmox.com/?p=proxmox-backup.git;a=blob;f=pbs-client/src/pxar/create.rs;h=d18021c4c57620897c8a1122f521365f913573e9;hb=HEAD#l709

Should be

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

[PATCH v1 proxmox-backup 3/3] client: pxar: skip files on fstatat permission errors

[Robert Obkircher]

Log a warning instead of aborting the entire backup if fstatat returns
a permission error. This is preferable when running backups without
full knowledge or control over the contents [1]. It is also more
consistent with how we treat EACCES when opening files.

Note that it was already possible to explicitly exclude such files[2].

[1] https://forum.proxmox.com/threads/bug-with-fuse-mounts.182315/
[2] https://bugzilla.proxmox.com/show_bug.cgi?id=4380

Signed-off-by: Robert Obkircher <r.obkircher@proxmox.com>
---
 pbs-client/src/pxar/create.rs | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/pbs-client/src/pxar/create.rs b/pbs-client/src/pxar/create.rs
index 47cbf50a8..c101415c1 100644
--- a/pbs-client/src/pxar/create.rs
+++ b/pbs-client/src/pxar/create.rs
@@ -710,6 +710,11 @@ impl Archiver {
                     warn!("warning: file vanished while reading directory: {full_path:?}");
                     continue;
                 }
+                Err(Errno::EACCES) => {
+                    // may happen for fuse mountpoints without allow_other or allow_root
+                    warn!("failed to stat file: {full_path:?}: access denied");
+                    continue;
+                }
                 Err(Errno::ESTALE) => {
                     self.report_stale_file_handle(Some(&full_path));
                     continue;
-- 
2.47.3

Re: [PATCH v1 proxmox-backup 3/3] client: pxar: skip files on fstatat permission errors

[Christian Ebner]

On 5/6/26 1:49 PM, Robert Obkircher wrote:
> Log a warning instead of aborting the entire backup if fstatat returns
> a permission error. This is preferable when running backups without
> full knowledge or control over the contents [1]. It is also more
> consistent with how we treat EACCES when opening files.
> 
> Note that it was already possible to explicitly exclude such files[2].
> 
> [1] https://forum.proxmox.com/threads/bug-with-fuse-mounts.182315/

nit: can be more concise by using thread id only

https://forum.proxmox.com/threads/182315/

> [2] https://bugzilla.proxmox.com/show_bug.cgi?id=4380
> 
> Signed-off-by: Robert Obkircher <r.obkircher@proxmox.com>
> ---
>   pbs-client/src/pxar/create.rs | 5 +++++
>   1 file changed, 5 insertions(+)
> 
> diff --git a/pbs-client/src/pxar/create.rs b/pbs-client/src/pxar/create.rs
> index 47cbf50a8..c101415c1 100644
> --- a/pbs-client/src/pxar/create.rs
> +++ b/pbs-client/src/pxar/create.rs
> @@ -710,6 +710,11 @@ impl Archiver {
>                       warn!("warning: file vanished while reading directory: {full_path:?}");
>                       continue;
>                   }
> +                Err(Errno::EACCES) => {
> +                    // may happen for fuse mountpoints without allow_other or allow_root
> +                    warn!("failed to stat file: {full_path:?}: access denied");

nit: similar to comment on previous patch, this should combine the 
warning into a report_file_access_denied() or similar.

> +                    continue;
> +                }
>                   Err(Errno::ESTALE) => {
>                       self.report_stale_file_handle(Some(&full_path));
>                       continue;