Re: [PATCH v2] xfs: test xfsdump subtree restores

[Zorro Lang <zlang@kernel.org>]

On Fri, Aug 07, 2026 at 05:48:57PM +1000, Donald Douwsma wrote:
> On 28/7/26 20:39, Zorro Lang wrote:
> > On Thu, Jul 23, 2026 at 04:26:17PM +1000, Donald Douwsma wrote:
> >> Regression test for cumulative restores where a directory has been
> >> renamed outside of the subtree being restored triggering the assert:
> >>
> >>   xfsrestore: tree.c:1421: noref_elim_recurse: Assertion 'isrealpr' failed
> >>
> >> Signed-off-by: Donald Douwsma <ddouwsma@redhat.com>
> >> ---
> >> Changes since v1
> >> - Fix use of _do, including label quoting
> >> - Update test output
> >> - Add tests for additional edge cases
> >> ---
> >>  tests/xfs/995     | 58 +++++++++++++++++++++++++++++++++++++++++++++++
> >>  tests/xfs/995.out |  8 +++++++
> >>  2 files changed, 66 insertions(+)
> >>  create mode 100755 tests/xfs/995
> >>  create mode 100644 tests/xfs/995.out
> >>
> >> diff --git a/tests/xfs/995 b/tests/xfs/995
> >> new file mode 100755
> >> index 000000000..bba0bcd52
> >> --- /dev/null
> >> +++ b/tests/xfs/995
> >> @@ -0,0 +1,58 @@
> >> +#! /bin/bash
> >> +# SPDX-License-Identifier: GPL-2.0
> >> +# Copyright (c) 2026 Red Hat.  All Rights Reserved.
> >> +#
> >> +# FS QA Test 995
> >> +#
> >> +# Regression test for cumulative restores where a directory has been
> >> +# renamed outside of the subtree being restored resulting in
> >> +#
> >> +#   xfsrestore: tree.c:1421: noref_elim_recurse: Assertion 'isrealpr' failed
> >> +#
> >> +. ./common/preamble
> >> +_begin_fstest auto dump
> >> +_do_die_on_error="always"
> > 
> > I think this line is useless now, right? I'll remove it.
> > 
> 
> Yes, thanks.
> 
> I did have a general question about how _do_die_on_error is used
> 
> $ git grep _do_die_on_error
> common/rc:# second argument. If the command fails and the variable _do_die_on_error
> common/rc:# is set to "always" or the two argument form is used and _do_die_on_error
> common/rc:      && [ "$_do_die_on_error" = "always" \
> common/rc:          -o \( $# -eq 2 -a "$_do_die_on_error" = "message_only" \) ]

Oh, my bad. I confused this parameter with the "enable_error" parameter you
tried to introduce in your previous patch.

Since this variable is used by the current _do helper, setting it to "always" or
"message_only" here is good. I will keep this line.

> tests/generic/017:_do_die_on_error=y
> tests/generic/053:_do_die_on_error=y
> tests/xfs/041:_do_die_on_error=message_only
> tests/xfs/042:_do_die_on_error=message_only
> tests/xfs/596:_do_die_on_error=message_only
> tests/xfs/635:_do_die_on_error=message_only
> 
> _do_die_on_error=y doesn't seem valid.

I think you're right, "y" isn't valid. The _do() helper is quite old and rarely used.
If you have any optimization suggestions, feel free to send a patch to improve it.
For this patch, I'll merge your current version directly.

Thanks,
Zorro

> 
> generic/017 doesn't use _do, and I think generic/053 wants _do_die_on_error=always

> 
> Thoughts?
> 
> Don
> 
> 
> 
>