[pbs-devel] [PATCH proxmox/proxmox-backup] fix #2915: stat when necessary

[pbs-devel] [PATCH proxmox/proxmox-backup] fix #2915: stat when necessary

[Dominik Csapak]

this series implements a fallback to stat'ing files when the
readdir entry does not contain the filetype (since only some
filesystems support that)

i opted for adding the 'FileStat' to the callback in 'scandir'
(even if we don't use it yet) so that we'll not stat again
sometimes when using that. If we want, we can of course omit
that part, then the interface stays as it currently is

we could also write our own 'ReadDir' that stats on demand, but given
how few cases we have where we actually use it, i am not so sure
if the benefits justify the work

proxmox:

Dominik Csapak (1):
  partially fix #2915: proxmox-sys: scandir: stat if file_type is
    unknown

 proxmox-sys/src/fs/read_dir.rs | 36 ++++++++++++++++++++++++++++++----
 1 file changed, 32 insertions(+), 4 deletions(-)

proxmox-backup:

Dominik Csapak (2):
  adapt to changed callback signature of 'scandir'
  partially fix #2915: 'stat' in case ReadDirEntry does not contain type

 pbs-datastore/src/backup_info.rs | 23 ++++++++-----
 pbs-datastore/src/chunk_store.rs | 23 ++++++-------
 pbs-datastore/src/hierarchy.rs   | 56 ++++++++++++++++++++++++++++++++
 src/tools/disks/zfs.rs           |  2 +-
 4 files changed, 81 insertions(+), 23 deletions(-)

-- 
2.30.2

[pbs-devel] [PATCH proxmox 1/1] partially fix #2915: proxmox-sys: scandir: stat if file_type is unknown

[Dominik Csapak]

when using readdir/getdents the file type might be 'DT_UNKNOWN'
(depending on the filesystem). Do a fstatat in that case to
get the file type. Since maybe the callback wants to do
a stat anyway, pass it there (if done)

Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
---
 proxmox-sys/src/fs/read_dir.rs | 36 ++++++++++++++++++++++++++++++----
 1 file changed, 32 insertions(+), 4 deletions(-)

diff --git a/proxmox-sys/src/fs/read_dir.rs b/proxmox-sys/src/fs/read_dir.rs
index f687748..785a412 100644
--- a/proxmox-sys/src/fs/read_dir.rs
+++ b/proxmox-sys/src/fs/read_dir.rs
@@ -141,20 +141,48 @@ pub fn scandir<P, F>(
     mut callback: F,
 ) -> Result<(), Error>
 where
-    F: FnMut(RawFd, &str, nix::dir::Type) -> Result<(), Error>,
+    F: FnMut(RawFd, &str, nix::dir::Type, Option<nix::sys::stat::FileStat>) -> Result<(), Error>,
     P: ?Sized + nix::NixPath,
 {
     for entry in scan_subdir(dirfd, path, regex)? {
         let entry = entry?;
-        let file_type = match entry.file_type() {
-            Some(file_type) => file_type,
-            None => bail!("unable to detect file type"),
+        let (file_type, stat) = match entry.file_type() {
+            Some(file_type) => (file_type, None),
+            None => {
+                use nix::sys::stat::SFlag;
+                let stat = nix::sys::stat::fstatat(
+                    entry.parent_fd(),
+                    entry.file_name(),
+                    nix::fcntl::AtFlags::AT_SYMLINK_NOFOLLOW,
+                )?;
+                let type_flag = SFlag::from_bits_truncate(stat.st_mode);
+                let file_type = if type_flag.contains(SFlag::S_IFDIR) {
+                    nix::dir::Type::Directory
+                } else if type_flag.contains(SFlag::S_IFIFO) {
+                    nix::dir::Type::Fifo
+                } else if type_flag.contains(SFlag::S_IFCHR) {
+                    nix::dir::Type::CharacterDevice
+                } else if type_flag.contains(SFlag::S_IFBLK) {
+                    nix::dir::Type::BlockDevice
+                } else if type_flag.contains(SFlag::S_IFREG) {
+                    nix::dir::Type::File
+                } else if type_flag.contains(SFlag::S_IFLNK) {
+                    nix::dir::Type::Symlink
+                } else if type_flag.contains(SFlag::S_IFSOCK) {
+                    nix::dir::Type::Socket
+                } else {
+                    bail!("unable to detect file type")
+                };
+
+                (file_type, Some(stat))
+            }
         };
 
         callback(
             entry.parent_fd(),
             unsafe { entry.file_name_utf8_unchecked() },
             file_type,
+            stat,
         )?;
     }
     Ok(())
-- 
2.30.2

Re: [pbs-devel] [PATCH proxmox 1/1] partially fix #2915: proxmox-sys: scandir: stat if file_type is unknown

[Wolfgang Bumiller]

On Tue, Jun 28, 2022 at 01:13:16PM +0200, Dominik Csapak wrote:
> when using readdir/getdents the file type might be 'DT_UNKNOWN'
> (depending on the filesystem). Do a fstatat in that case to
> get the file type. Since maybe the callback wants to do
> a stat anyway, pass it there (if done)
> 
> Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
> ---
>  proxmox-sys/src/fs/read_dir.rs | 36 ++++++++++++++++++++++++++++++----
>  1 file changed, 32 insertions(+), 4 deletions(-)
> 
> diff --git a/proxmox-sys/src/fs/read_dir.rs b/proxmox-sys/src/fs/read_dir.rs
> index f687748..785a412 100644
> --- a/proxmox-sys/src/fs/read_dir.rs
> +++ b/proxmox-sys/src/fs/read_dir.rs
> @@ -141,20 +141,48 @@ pub fn scandir<P, F>(
>      mut callback: F,
>  ) -> Result<(), Error>
>  where
> -    F: FnMut(RawFd, &str, nix::dir::Type) -> Result<(), Error>,
> +    F: FnMut(RawFd, &str, nix::dir::Type, Option<nix::sys::stat::FileStat>) -> Result<(), Error>,
>      P: ?Sized + nix::NixPath,
>  {
>      for entry in scan_subdir(dirfd, path, regex)? {
>          let entry = entry?;
> -        let file_type = match entry.file_type() {
> -            Some(file_type) => file_type,
> -            None => bail!("unable to detect file type"),
> +        let (file_type, stat) = match entry.file_type() {
> +            Some(file_type) => (file_type, None),
> +            None => {
> +                use nix::sys::stat::SFlag;
> +                let stat = nix::sys::stat::fstatat(
> +                    entry.parent_fd(),
> +                    entry.file_name(),
> +                    nix::fcntl::AtFlags::AT_SYMLINK_NOFOLLOW,
> +                )?;
> +                let type_flag = SFlag::from_bits_truncate(stat.st_mode);
> +                let file_type = if type_flag.contains(SFlag::S_IFDIR) {

Using `.contains()` here is wrong. The file type is not actually a
bitfield, but a number *within* a bitfield.

You mask out the type via `st_mode & S_IFMT` and can then use a `match`
statement to map it to `dir::Type`. (Which also makes for shorter code
;-) )

> +                    nix::dir::Type::Directory
> +                } else if type_flag.contains(SFlag::S_IFIFO) {
> +                    nix::dir::Type::Fifo
> +                } else if type_flag.contains(SFlag::S_IFCHR) {
> +                    nix::dir::Type::CharacterDevice
> +                } else if type_flag.contains(SFlag::S_IFBLK) {
> +                    nix::dir::Type::BlockDevice
> +                } else if type_flag.contains(SFlag::S_IFREG) {
> +                    nix::dir::Type::File
> +                } else if type_flag.contains(SFlag::S_IFLNK) {
> +                    nix::dir::Type::Symlink
> +                } else if type_flag.contains(SFlag::S_IFSOCK) {
> +                    nix::dir::Type::Socket
> +                } else {
> +                    bail!("unable to detect file type")
> +                };
> +
> +                (file_type, Some(stat))
> +            }
>          };
>  
>          callback(
>              entry.parent_fd(),
>              unsafe { entry.file_name_utf8_unchecked() },
>              file_type,
> +            stat,
>          )?;
>      }
>      Ok(())
> -- 
> 2.30.2

[pbs-devel] [PATCH proxmox-backup 1/2] adapt to changed callback signature of 'scandir'

[Dominik Csapak]

we now get an Option<FileStat> too

Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
---
 pbs-datastore/src/backup_info.rs | 23 ++++++++++++++---------
 src/tools/disks/zfs.rs           |  2 +-
 2 files changed, 15 insertions(+), 10 deletions(-)

diff --git a/pbs-datastore/src/backup_info.rs b/pbs-datastore/src/backup_info.rs
index 10320a35..21fe4602 100644
--- a/pbs-datastore/src/backup_info.rs
+++ b/pbs-datastore/src/backup_info.rs
@@ -87,7 +87,7 @@ impl BackupGroup {
             libc::AT_FDCWD,
             &path,
             &BACKUP_DATE_REGEX,
-            |l2_fd, backup_time, file_type| {
+            |l2_fd, backup_time, file_type, _stat| {
                 if file_type != nix::dir::Type::Directory {
                     return Ok(());
                 }
@@ -127,7 +127,7 @@ impl BackupGroup {
             libc::AT_FDCWD,
             &path,
             &BACKUP_DATE_REGEX,
-            |l2_fd, backup_time, file_type| {
+            |l2_fd, backup_time, file_type, _stat| {
                 if file_type != nix::dir::Type::Directory {
                     return Ok(());
                 }
@@ -635,13 +635,18 @@ fn list_backup_files<P: ?Sized + nix::NixPath>(
 ) -> Result<Vec<String>, Error> {
     let mut files = vec![];
 
-    proxmox_sys::fs::scandir(dirfd, path, &BACKUP_FILE_REGEX, |_, filename, file_type| {
-        if file_type != nix::dir::Type::File {
-            return Ok(());
-        }
-        files.push(filename.to_owned());
-        Ok(())
-    })?;
+    proxmox_sys::fs::scandir(
+        dirfd,
+        path,
+        &BACKUP_FILE_REGEX,
+        |_, filename, file_type, _stat| {
+            if file_type != nix::dir::Type::File {
+                return Ok(());
+            }
+            files.push(filename.to_owned());
+            Ok(())
+        },
+    )?;
 
     Ok(files)
 }
diff --git a/src/tools/disks/zfs.rs b/src/tools/disks/zfs.rs
index 30a6cc0c..5f09c2cf 100644
--- a/src/tools/disks/zfs.rs
+++ b/src/tools/disks/zfs.rs
@@ -178,7 +178,7 @@ pub(crate) fn update_zfs_objset_map(pool: &str) -> Result<(), Error> {
         libc::AT_FDCWD,
         &path,
         &OBJSET_REGEX,
-        |_l2_fd, filename, _type| {
+        |_l2_fd, filename, _type, _stat| {
             let (name, _) = parse_objset_stat(pool, filename)?;
             map.insert(name, (pool.to_string(), filename.to_string()));
             Ok(())
-- 
2.30.2

Re: [pbs-devel] [PATCH proxmox-backup 1/2] adapt to changed callback signature of 'scandir'

[Fabian Grünbichler]

On June 28, 2022 1:13 pm, Dominik Csapak wrote:
> we now get an Option<FileStat> too

but don't use it *anywhere* - so why introduce a breaking change in 
proxmox_sys for it? is there some other place where we use it?

> 
> Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
> ---
>  pbs-datastore/src/backup_info.rs | 23 ++++++++++++++---------
>  src/tools/disks/zfs.rs           |  2 +-
>  2 files changed, 15 insertions(+), 10 deletions(-)
> 
> diff --git a/pbs-datastore/src/backup_info.rs b/pbs-datastore/src/backup_info.rs
> index 10320a35..21fe4602 100644
> --- a/pbs-datastore/src/backup_info.rs
> +++ b/pbs-datastore/src/backup_info.rs
> @@ -87,7 +87,7 @@ impl BackupGroup {
>              libc::AT_FDCWD,
>              &path,
>              &BACKUP_DATE_REGEX,
> -            |l2_fd, backup_time, file_type| {
> +            |l2_fd, backup_time, file_type, _stat| {
>                  if file_type != nix::dir::Type::Directory {
>                      return Ok(());
>                  }
> @@ -127,7 +127,7 @@ impl BackupGroup {
>              libc::AT_FDCWD,
>              &path,
>              &BACKUP_DATE_REGEX,
> -            |l2_fd, backup_time, file_type| {
> +            |l2_fd, backup_time, file_type, _stat| {
>                  if file_type != nix::dir::Type::Directory {
>                      return Ok(());
>                  }
> @@ -635,13 +635,18 @@ fn list_backup_files<P: ?Sized + nix::NixPath>(
>  ) -> Result<Vec<String>, Error> {
>      let mut files = vec![];
>  
> -    proxmox_sys::fs::scandir(dirfd, path, &BACKUP_FILE_REGEX, |_, filename, file_type| {
> -        if file_type != nix::dir::Type::File {
> -            return Ok(());
> -        }
> -        files.push(filename.to_owned());
> -        Ok(())
> -    })?;
> +    proxmox_sys::fs::scandir(
> +        dirfd,
> +        path,
> +        &BACKUP_FILE_REGEX,
> +        |_, filename, file_type, _stat| {
> +            if file_type != nix::dir::Type::File {
> +                return Ok(());
> +            }
> +            files.push(filename.to_owned());
> +            Ok(())
> +        },
> +    )?;
>  
>      Ok(files)
>  }
> diff --git a/src/tools/disks/zfs.rs b/src/tools/disks/zfs.rs
> index 30a6cc0c..5f09c2cf 100644
> --- a/src/tools/disks/zfs.rs
> +++ b/src/tools/disks/zfs.rs
> @@ -178,7 +178,7 @@ pub(crate) fn update_zfs_objset_map(pool: &str) -> Result<(), Error> {
>          libc::AT_FDCWD,
>          &path,
>          &OBJSET_REGEX,
> -        |_l2_fd, filename, _type| {
> +        |_l2_fd, filename, _type, _stat| {
>              let (name, _) = parse_objset_stat(pool, filename)?;
>              map.insert(name, (pool.to_string(), filename.to_string()));
>              Ok(())
> -- 
> 2.30.2
> 
> 
> 
> _______________________________________________
> pbs-devel mailing list
> pbs-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel
> 
> 
> 

[pbs-devel] [PATCH proxmox-backup 2/2] partially fix #2915: 'stat' in case ReadDirEntry does not contain type

[Dominik Csapak]

readdir/getdents may return 'DT_UNKNOWN' for the file type
(which corresponds to 'None' in nix::dir::Entry), so stat the file and
check the type

Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
---
 pbs-datastore/src/chunk_store.rs | 23 ++++++-------
 pbs-datastore/src/hierarchy.rs   | 56 ++++++++++++++++++++++++++++++++
 2 files changed, 66 insertions(+), 13 deletions(-)

diff --git a/pbs-datastore/src/chunk_store.rs b/pbs-datastore/src/chunk_store.rs
index c9714c1e..2c281b0c 100644
--- a/pbs-datastore/src/chunk_store.rs
+++ b/pbs-datastore/src/chunk_store.rs
@@ -375,24 +375,21 @@ impl ChunkStore {
                 ),
             };
 
-            let file_type = match entry.file_type() {
-                Some(file_type) => file_type,
-                None => bail!(
-                    "unsupported file system type on chunk store '{}'",
-                    self.name
-                ),
-            };
-            if file_type != nix::dir::Type::File {
-                continue;
-            }
-
-            chunk_count += 1;
-
             let filename = entry.file_name();
 
             let lock = self.mutex.lock();
 
             if let Ok(stat) = fstatat(dirfd, filename, nix::fcntl::AtFlags::AT_SYMLINK_NOFOLLOW) {
+                if match entry.file_type() {
+                    Some(file_type) => file_type != nix::dir::Type::File,
+                    None => stat.st_mode & libc::S_IFREG == 0,
+                } {
+                    drop(lock);
+                    continue;
+                }
+
+                chunk_count += 1;
+
                 if stat.st_atime < min_atime {
                     //let age = now - stat.st_atime;
                     //println!("UNLINK {}  {:?}", age/(3600*24), filename);
diff --git a/pbs-datastore/src/hierarchy.rs b/pbs-datastore/src/hierarchy.rs
index d5007b07..a97a1d2a 100644
--- a/pbs-datastore/src/hierarchy.rs
+++ b/pbs-datastore/src/hierarchy.rs
@@ -9,6 +9,16 @@ use pbs_api_types::{BackupNamespace, BackupType, BACKUP_DATE_REGEX, BACKUP_ID_RE
 use crate::backup_info::{BackupDir, BackupGroup};
 use crate::DataStore;
 
+fn dir_entry_is_path(entry: &proxmox_sys::fs::ReadDirEntry) -> Result<bool, Error> {
+    let stat = nix::sys::stat::fstatat(
+        entry.parent_fd(),
+        entry.file_name(),
+        nix::fcntl::AtFlags::AT_SYMLINK_NOFOLLOW,
+    )?;
+    let is_dir = stat.st_mode & libc::S_IFDIR > 0;
+    Ok(is_dir)
+}
+
 /// A iterator for all BackupDir's (Snapshots) in a BackupGroup
 pub struct ListSnapshots {
     group: BackupGroup,
@@ -36,6 +46,17 @@ impl Iterator for ListSnapshots {
                 Ok(ref entry) => {
                     match entry.file_type() {
                         Some(nix::dir::Type::Directory) => entry, // OK
+                        None => match dir_entry_is_path(&entry) {
+                            Ok(true) => entry,
+                            Ok(false) => continue,
+                            Err(err) => {
+                                log::info!(
+                                    "error listing snapshots for {}: {err}",
+                                    self.group.group()
+                                );
+                                continue;
+                            }
+                        },
                         _ => continue,
                     }
                 }
@@ -93,6 +114,17 @@ impl Iterator for ListGroups {
                     Ok(ref entry) => {
                         match entry.file_type() {
                             Some(nix::dir::Type::Directory) => entry, // OK
+                            None => match dir_entry_is_path(&entry) {
+                                Ok(true) => entry,
+                                Ok(false) => continue,
+                                Err(err) => {
+                                    log::info!(
+                                        "error listing groups for {}: {err}",
+                                        self.store.name()
+                                    );
+                                    continue;
+                                }
+                            },
                             _ => continue,
                         }
                     }
@@ -114,6 +146,17 @@ impl Iterator for ListGroups {
                     Ok(ref entry) => {
                         match entry.file_type() {
                             Some(nix::dir::Type::Directory) => entry, // OK
+                            None => match dir_entry_is_path(&entry) {
+                                Ok(true) => entry,
+                                Ok(false) => continue,
+                                Err(err) => {
+                                    log::info!(
+                                        "error listing groups for {}: {err}",
+                                        self.store.name()
+                                    );
+                                    continue;
+                                }
+                            },
                             _ => continue,
                         }
                     }
@@ -176,6 +219,19 @@ impl Iterator for ListNamespaces {
                     Ok(ref entry) => {
                         match entry.file_type() {
                             Some(nix::dir::Type::Directory) => entry, // OK
+                            None => match dir_entry_is_path(&entry) {
+                                Ok(true) => entry,
+                                Ok(false) => continue,
+                                Err(err) => {
+                                    let mut base_path = self.base_path.to_owned();
+                                    if !self.ns.is_root() {
+                                        base_path.push(self.ns.path());
+                                    }
+                                    base_path.push("ns");
+                                    log::info!("error listing dirs in {:?}: {err}", base_path);
+                                    continue;
+                                }
+                            },
                             _ => continue,
                         }
                     }
-- 
2.30.2

Re: [pbs-devel] [PATCH proxmox-backup 2/2] partially fix #2915: 'stat' in case ReadDirEntry does not contain type

[Fabian Grünbichler]

On June 28, 2022 1:13 pm, Dominik Csapak wrote:
> readdir/getdents may return 'DT_UNKNOWN' for the file type
> (which corresponds to 'None' in nix::dir::Entry), so stat the file and
> check the type
> 
> Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
> ---
>  pbs-datastore/src/chunk_store.rs | 23 ++++++-------
>  pbs-datastore/src/hierarchy.rs   | 56 ++++++++++++++++++++++++++++++++
>  2 files changed, 66 insertions(+), 13 deletions(-)
> 
> diff --git a/pbs-datastore/src/chunk_store.rs b/pbs-datastore/src/chunk_store.rs
> index c9714c1e..2c281b0c 100644
> --- a/pbs-datastore/src/chunk_store.rs
> +++ b/pbs-datastore/src/chunk_store.rs
> @@ -375,24 +375,21 @@ impl ChunkStore {
>                  ),
>              };
>  
> -            let file_type = match entry.file_type() {
> -                Some(file_type) => file_type,
> -                None => bail!(
> -                    "unsupported file system type on chunk store '{}'",
> -                    self.name
> -                ),
> -            };
> -            if file_type != nix::dir::Type::File {
> -                continue;
> -            }
> -
> -            chunk_count += 1;
> -
>              let filename = entry.file_name();
>  
>              let lock = self.mutex.lock();
>  
>              if let Ok(stat) = fstatat(dirfd, filename, nix::fcntl::AtFlags::AT_SYMLINK_NOFOLLOW) {
> +                if match entry.file_type() {
> +                    Some(file_type) => file_type != nix::dir::Type::File,
> +                    None => stat.st_mode & libc::S_IFREG == 0,

nit: since we need the stat here anyway, this could just drop the 
entry.file_type() altogether?

> +                } {
> +                    drop(lock);
> +                    continue;
> +                }
> +
> +                chunk_count += 1;
> +
>                  if stat.st_atime < min_atime {
>                      //let age = now - stat.st_atime;
>                      //println!("UNLINK {}  {:?}", age/(3600*24), filename);
> diff --git a/pbs-datastore/src/hierarchy.rs b/pbs-datastore/src/hierarchy.rs
> index d5007b07..a97a1d2a 100644
> --- a/pbs-datastore/src/hierarchy.rs
> +++ b/pbs-datastore/src/hierarchy.rs
> @@ -9,6 +9,16 @@ use pbs_api_types::{BackupNamespace, BackupType, BACKUP_DATE_REGEX, BACKUP_ID_RE
>  use crate::backup_info::{BackupDir, BackupGroup};
>  use crate::DataStore;
>  
> +fn dir_entry_is_path(entry: &proxmox_sys::fs::ReadDirEntry) -> Result<bool, Error> {
> +    let stat = nix::sys::stat::fstatat(
> +        entry.parent_fd(),
> +        entry.file_name(),
> +        nix::fcntl::AtFlags::AT_SYMLINK_NOFOLLOW,
> +    )?;
> +    let is_dir = stat.st_mode & libc::S_IFDIR > 0;
> +    Ok(is_dir)
> +}

nit: why *is_path*? everything's a path ;) this should probably be called 
`dir_entry_is_dir` or `dir_entry_is_subdir`?

we could also re-use the matching from proxmox_sys -> have a helper that 
takes an entry, returns the file_type if possible, stats otherwise (the 
parent fd and name are in the entry after all), returns the stat result 
transposed to Option<FileType> (or change the call sites to expect 
Result<FileType>), if we want to support this across the board a single 
place for the stat implementation is probably better than multiple 
ones..

> +
>  /// A iterator for all BackupDir's (Snapshots) in a BackupGroup
>  pub struct ListSnapshots {
>      group: BackupGroup,
> @@ -36,6 +46,17 @@ impl Iterator for ListSnapshots {
>                  Ok(ref entry) => {
>                      match entry.file_type() {
>                          Some(nix::dir::Type::Directory) => entry, // OK
> +                        None => match dir_entry_is_path(&entry) {
> +                            Ok(true) => entry,
> +                            Ok(false) => continue,
> +                            Err(err) => {
> +                                log::info!(
> +                                    "error listing snapshots for {}: {err}",
> +                                    self.group.group()
> +                                );
> +                                continue;
> +                            }
> +                        },
>                          _ => continue,
>                      }
>                  }
> @@ -93,6 +114,17 @@ impl Iterator for ListGroups {
>                      Ok(ref entry) => {
>                          match entry.file_type() {
>                              Some(nix::dir::Type::Directory) => entry, // OK
> +                            None => match dir_entry_is_path(&entry) {
> +                                Ok(true) => entry,
> +                                Ok(false) => continue,
> +                                Err(err) => {
> +                                    log::info!(
> +                                        "error listing groups for {}: {err}",
> +                                        self.store.name()
> +                                    );
> +                                    continue;
> +                                }
> +                            },
>                              _ => continue,
>                          }
>                      }
> @@ -114,6 +146,17 @@ impl Iterator for ListGroups {
>                      Ok(ref entry) => {
>                          match entry.file_type() {
>                              Some(nix::dir::Type::Directory) => entry, // OK
> +                            None => match dir_entry_is_path(&entry) {
> +                                Ok(true) => entry,
> +                                Ok(false) => continue,
> +                                Err(err) => {
> +                                    log::info!(
> +                                        "error listing groups for {}: {err}",
> +                                        self.store.name()
> +                                    );
> +                                    continue;
> +                                }
> +                            },
>                              _ => continue,
>                          }
>                      }
> @@ -176,6 +219,19 @@ impl Iterator for ListNamespaces {
>                      Ok(ref entry) => {
>                          match entry.file_type() {
>                              Some(nix::dir::Type::Directory) => entry, // OK
> +                            None => match dir_entry_is_path(&entry) {
> +                                Ok(true) => entry,
> +                                Ok(false) => continue,
> +                                Err(err) => {
> +                                    let mut base_path = self.base_path.to_owned();
> +                                    if !self.ns.is_root() {
> +                                        base_path.push(self.ns.path());
> +                                    }
> +                                    base_path.push("ns");
> +                                    log::info!("error listing dirs in {:?}: {err}", base_path);
> +                                    continue;
> +                                }
> +                            },
>                              _ => continue,
>                          }
>                      }
> -- 
> 2.30.2
> 
> 
> 
> _______________________________________________
> pbs-devel mailing list
> pbs-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel
> 
> 
> 

Re: [pbs-devel] [PATCH proxmox-backup 2/2] partially fix #2915: 'stat' in case ReadDirEntry does not contain type

[Wolfgang Bumiller]

On Tue, Jun 28, 2022 at 01:13:18PM +0200, Dominik Csapak wrote:
> readdir/getdents may return 'DT_UNKNOWN' for the file type
> (which corresponds to 'None' in nix::dir::Entry), so stat the file and
> check the type
> 
> Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
> ---
>  pbs-datastore/src/chunk_store.rs | 23 ++++++-------
>  pbs-datastore/src/hierarchy.rs   | 56 ++++++++++++++++++++++++++++++++
>  2 files changed, 66 insertions(+), 13 deletions(-)
> 
> diff --git a/pbs-datastore/src/chunk_store.rs b/pbs-datastore/src/chunk_store.rs
> index c9714c1e..2c281b0c 100644
> --- a/pbs-datastore/src/chunk_store.rs
> +++ b/pbs-datastore/src/chunk_store.rs
> @@ -375,24 +375,21 @@ impl ChunkStore {
>                  ),
>              };
>  
> -            let file_type = match entry.file_type() {
> -                Some(file_type) => file_type,
> -                None => bail!(
> -                    "unsupported file system type on chunk store '{}'",
> -                    self.name
> -                ),
> -            };
> -            if file_type != nix::dir::Type::File {
> -                continue;
> -            }
> -
> -            chunk_count += 1;
> -
>              let filename = entry.file_name();
>  
>              let lock = self.mutex.lock();
>  
>              if let Ok(stat) = fstatat(dirfd, filename, nix::fcntl::AtFlags::AT_SYMLINK_NOFOLLOW) {
> +                if match entry.file_type() {
> +                    Some(file_type) => file_type != nix::dir::Type::File,
> +                    None => stat.st_mode & libc::S_IFREG == 0,

Should be `(stat.st_mode & libc::S_IFMT) != libc::S_IFREG`.

> +                } {
> +                    drop(lock);
> +                    continue;
> +                }
> +
> +                chunk_count += 1;
> +
>                  if stat.st_atime < min_atime {
>                      //let age = now - stat.st_atime;
>                      //println!("UNLINK {}  {:?}", age/(3600*24), filename);
> diff --git a/pbs-datastore/src/hierarchy.rs b/pbs-datastore/src/hierarchy.rs
> index d5007b07..a97a1d2a 100644
> --- a/pbs-datastore/src/hierarchy.rs
> +++ b/pbs-datastore/src/hierarchy.rs
> @@ -9,6 +9,16 @@ use pbs_api_types::{BackupNamespace, BackupType, BACKUP_DATE_REGEX, BACKUP_ID_RE
>  use crate::backup_info::{BackupDir, BackupGroup};
>  use crate::DataStore;
>  
> +fn dir_entry_is_path(entry: &proxmox_sys::fs::ReadDirEntry) -> Result<bool, Error> {
> +    let stat = nix::sys::stat::fstatat(
> +        entry.parent_fd(),
> +        entry.file_name(),
> +        nix::fcntl::AtFlags::AT_SYMLINK_NOFOLLOW,
> +    )?;
> +    let is_dir = stat.st_mode & libc::S_IFDIR > 0;

Should be `(stat.st_mode & libc::S_IFMT) != libc::S_IFDIR`.

Re: [pbs-devel] [PATCH proxmox-backup 2/2] partially fix #2915: 'stat' in case ReadDirEntry does not contain type

[Wolfgang Bumiller]

On Tue, Jun 28, 2022 at 01:49:23PM +0200, Wolfgang Bumiller wrote:
> On Tue, Jun 28, 2022 at 01:13:18PM +0200, Dominik Csapak wrote:
> > diff --git a/pbs-datastore/src/hierarchy.rs b/pbs-datastore/src/hierarchy.rs
> > index d5007b07..a97a1d2a 100644
> > --- a/pbs-datastore/src/hierarchy.rs
> > +++ b/pbs-datastore/src/hierarchy.rs
> > @@ -9,6 +9,16 @@ use pbs_api_types::{BackupNamespace, BackupType, BACKUP_DATE_REGEX, BACKUP_ID_RE
> >  use crate::backup_info::{BackupDir, BackupGroup};
> >  use crate::DataStore;
> >  
> > +fn dir_entry_is_path(entry: &proxmox_sys::fs::ReadDirEntry) -> Result<bool, Error> {
> > +    let stat = nix::sys::stat::fstatat(
> > +        entry.parent_fd(),
> > +        entry.file_name(),
> > +        nix::fcntl::AtFlags::AT_SYMLINK_NOFOLLOW,
> > +    )?;
> > +    let is_dir = stat.st_mode & libc::S_IFDIR > 0;
> 
> Should be `(stat.st_mode & libc::S_IFMT) != libc::S_IFDIR`.

I meant `==` here ;-)