Re: [pbs-devel] [PATCH proxmox-backup v1 1/2] fix #4001: datastore/catalog: add number of files to directory entry

[Stefan Sterz <s.sterz@proxmox.com>]

On 5/12/22 17:27, Thomas Lamprecht wrote:
> Am 5/5/22 um 15:52 schrieb Stefan Sterz:
>> when listing the content of a catalog, add the number of files
>> contained in the directory as its size. also removes redundant code,
>> the size of a file is already set when creating the archive entry.
>>
> 
> did you mean to write "the mtime of a file ..." as, we obviously need to
> set the size differently to get the number of children, but you dropped
> setting the mtime, which is OK as ArchiveEntry::new does that already,
> or did I overlooked something?
> 

sorry for the confusingly worded comment. the following is a bit more
verbose than it might need to be in order to be a bit clearer ^^' :

the original code would assign the `size` _and_ `mtime` again from the
`DirEntryAttribute` if it was of type `DirEntryAttribute::File`.
however, that is redundant as _both_ are already set by
`ArchiveEntry::new()` and `ArchiveEntry::new_with_size()` from the
same `DirEntryAttribute`. as you pointed out.

therefore, i removed those conditional assignments and added another
one for `DirEntryAttribute::Directory` because here
`ArchiveEntry::new()` wouldn't set any size.

hopefully this makes more sense.

>> Signed-off-by: Stefan Sterz <s.sterz@proxmox.com>
>> ---
>> this requires patch two from the widget toolkit part of this patch to
>> be applied already. otherwise the formatting and sorting of the folder
>> "size" will be wrong.
> 
> ok so no hard depends.
> 

no, it will build just fine and no errors will be logged in the
browser console either, but the amount of items will be displayed as
the size in bytes and sorting will work accordingly, which would be
confusing in most cases id assume.

>>
>>  pbs-datastore/src/catalog.rs | 10 ++++++----
>>  1 file changed, 6 insertions(+), 4 deletions(-)
>>
>> diff --git a/pbs-datastore/src/catalog.rs b/pbs-datastore/src/catalog.rs
>> index c07b71a6..6cb8aeb4 100644
>> --- a/pbs-datastore/src/catalog.rs
>> +++ b/pbs-datastore/src/catalog.rs
>> @@ -706,10 +706,11 @@ impl<R: Read + Seek> CatalogReader<R> {
>>              components.push(b'/');
>>              components.extend(&direntry.name);
>>              let mut entry = ArchiveEntry::new(&components, Some(&direntry.attr));
>> -            if let DirEntryAttribute::File { size, mtime } = direntry.attr {
>> -                entry.size = size.into();
>> -                entry.mtime = mtime.into();
>> +
>> +            if let DirEntryAttribute::Directory { start: _ } = direntry.attr {
>> +                entry.size = Some(u64::try_from(self.read_dir(&direntry)?.len())?);
>>              }
>> +
>>              res.push(entry);
>>          }
>>  
>> @@ -911,7 +912,8 @@ pub struct ArchiveEntry {
>>      pub entry_type: String,
>>      /// Is this entry a leaf node, or does it have children (i.e. a directory)?
>>      pub leaf: bool,
>> -    /// The file size, if entry_type is 'f' (file)
>> +    /// The file size, if entry_type is 'f' (file) or the amount of files in a
>> +    /// directory if entry_type is 'd' (directory)
>>      #[serde(skip_serializing_if = "Option::is_none")]
>>      pub size: Option<u64>,
>>      /// The file "last modified" time stamp, if entry_type is 'f' (file)
>