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

[Fabian Grünbichler]

> Christian Ebner <c.ebner at proxmox.com> hat am 07.08.2024 12:54 CEST geschrieben:
> 
>  
> 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.

sorry, yes, that was not phrased clearly. 

I just wanted to make sure that this is intentional, and "shell" was referring to a regular shell not our catalog shell. as in - is it intentional that this is silent (like it is in regular shells when doing "cd .." while already being in "/"), or do we want to have a warning there?