[pbs-devel] [PATCH proxmox-backup] client: catalog shell: avoid navigating below archive root

[Christian Ebner]

On 8/7/24 10:25, Fabian Grünbichler wrote:
> Quoting Christian Ebner (2024-07-22 12:07:05)
>> Avoid to underflow the catalogs shell position stack by navigating
>> below the archives root directory into the catalog root. Otherwise
>> the shell will panic, as the root entry is always expected to be
>> present.
>>
>> Signed-off-by: Christian Ebner <c.ebner at proxmox.com>
>> ---
>> Encountered while implementing the catalog shell for the split pxar
>> archive case.
>>
>> Without this additional check, a `cd ..` in the pxar archive root of
>> the catalog shell will panic.
>>
>>   pbs-client/src/catalog_shell.rs | 6 ++++++
>>   1 file changed, 6 insertions(+)
>>
>> diff --git a/pbs-client/src/catalog_shell.rs b/pbs-client/src/catalog_shell.rs
>> index 349bb7cbc..88045db5d 100644
>> --- a/pbs-client/src/catalog_shell.rs
>> +++ b/pbs-client/src/catalog_shell.rs
>> @@ -720,6 +720,12 @@ impl Shell {
>>                       &mut None,
>>                   )
>>                   .await?;
>> +
>> +                if new_position.len() < 1 {
>> +                    // Avoid moving below archive root into catalog root
>> +                    return Ok(());
>> +                }
> 
> since new_position is a Vec, wouldn't `new_position.is_empty()` be the right
> fit?

Yes, this is even a clippy warning I overlooked, will send a new version 
for this.

> 
> should this print a warning? I guess since regular shells pretend `/` is the
> parent of `/`, we can do the same here?

I'm not sure a warning would bring much benefit here, unless we plan to 
allow to navigate the whole catalog at some point, not just the archive 
given as parameter when invoking the shell.

Also, could you clarify what do you mean exactly with the latter 
question. With the patch applied the shell should treat '/' as parent of 
itself, or am I missing something.