Re: [pbs-devel] [PATCH v3 proxmox-backup 00/10] fix catalog dump and shell for split pxar archives

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

On 10/21/24 16:07, Thomas Lamprecht wrote:
> Am 12/08/2024 um 12:31 schrieb Christian Ebner:
>> Christian Ebner (10):
>>    client: tools: make tools module public
>>    client: pxar: move catalog lookup helper to pxar tools
>>    client: tools: move pxar root entry helper to pxar submodule
> 
> no hard feelings but maybe do a s/sub// to avoid making it sound
> like this is git submodule related (but I might just have to much
> exposure to the git ones to be "triggered" by just reading submodule
> ;-)

Okay, will adapt this according to your suggestion.

>>    client: make helper to get remote pxar reader reusable
>>    client: tools: factor out entry path prefix helper
>>    client: tools: factor out pxar entry to dir entry mapping
>>    client: add helper to dump catalog from metadata archive
>>    client: catalog: fallback to metadata archives for catalog dump
>>    client: helper to mimic catalog find using metadata archive
>>    client: catalog shell: fallback to accessor for navigation
>>
>>   pbs-client/src/catalog_shell.rs      | 291 +++++++++++++++++++++------
>>   pbs-client/src/pxar/extract.rs       |   2 +-
>>   pbs-client/src/pxar/mod.rs           |   4 +-
>>   pbs-client/src/pxar/tools.rs         | 256 ++++++++++++++++++++++-
>>   pbs-client/src/tools/mod.rs          | 120 -----------
>>   pbs-datastore/src/catalog.rs         |  40 ++++
>>   proxmox-backup-client/src/catalog.rs |  65 +++++-
>>   proxmox-file-restore/src/main.rs     |  38 +---
>>   pxar-bin/src/main.rs                 |   4 +-
>>   src/api2/admin/datastore.rs          |   2 +-
>>   src/api2/tape/restore.rs             |   2 +-
>>   11 files changed, 599 insertions(+), 225 deletions(-)
>>
> 
> Looks OK to me in general code wise, albeit I did not evaluate every
> patch very closely, but considering that a spot check did not find
> anything odd and this is all mostly internal and got already reviewed
> by Fabian I'd be fine with applying it.
> 
> That said, your S-o-b trailer seems to be missing from all patches.

Oh, you are right, but only on path 2 and 9? At least in the mails for 
the other patches I do see them. But this will be fixed in the next 
version, thanks for noticing!

> For the refactoring ones it might be also nice to have a short sentence
> for why this is done, e.g. referencing the future use like "this will
> be used to do XYZ in a future commit".

Okay, will amend the commit messages accordingly.

Thanks for looking at these patches!



_______________________________________________
pbs-devel mailing list
pbs-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel