Re: [PATCH v2] xfsrestore: suggest -x rather than assert for false roots

Donald Douwsma
2023-08-24T01:07:34Z

On 23/8/23 04:34, Darrick J. Wong wrote:
> On Mon, Aug 07, 2023 at 02:53:57PM +1000, Donald Douwsma wrote:
>> If we're going to have a fix for false root problems its a good idea to
>> let people know that there's a way to recover, error out with a useful
>> message that mentions the `-x` option rather than just assert.
>>
>> Before
>>
>> xfsrestore: searching media for directory dump
>> xfsrestore: reading directories
>> xfsrestore: tree.c:757: tree_begindir: Assertion `ino != persp->p_rootino || hardh == persp->p_rooth' failed.
>> Aborted
>>
>> After
>>
>> xfsrestore: ERROR: tree.c:791: tree_begindir: Assertion `ino != persp->p_rootino || hardh == persp->p_rooth` failed.
>> xfsrestore: ERROR: False root detected. Recovery may be possible using the `-x` option
>> Aborted
>>
>> Fixes: d7cba7410710 ("xfsrestore: fix rootdir due to xfsdump bulkstat misuse")
>> Signed-off-by: Donald Douwsma <ddouwsma@redhat.com>
>>
>> ---
>> Changes for v2
>> - Use xfsprogs style for conditional
>> - Remove trailing white-space
>> - Place printf format all on one line for grepability
>> - use __func__ instead of gcc specific __FUNCTION__
>> ---
>> restore/tree.c | 11 +++++++++--
>> 1 file changed, 9 insertions(+), 2 deletions(-)
>>
>> diff --git a/restore/tree.c b/restore/tree.c
>> index bfa07fe..e959aa1 100644
>> --- a/restore/tree.c
>> +++ b/restore/tree.c
>> @@ -783,8 +783,15 @@ tree_begindir(filehdr_t *fhdrp, dah_t *dahp)
>> /* lookup head of hardlink list
>> */
>> hardh = link_hardh(ino, gen);
>> - if (need_fixrootdir == BOOL_FALSE)
>> - assert(ino != persp->p_rootino || hardh == persp->p_rooth);
>> + if (need_fixrootdir == BOOL_FALSE &&
>> + !(ino != persp->p_rootino || hardh == persp->p_rooth)) {
>> + mlog(MLOG_ERROR | MLOG_TREE,
>
> Nit: Second line of the if test has the same level of indentation as the
> if body.

Urg, I cant unsee it now, will fix.

Thanks for the Review(s)
Don

>
> With that fixed,
> Reviewed-by: Darrick J. Wong <djwong@kernel.org>
>
> --D
>
>> +"%s:%d: %s: Assertion `ino != persp->p_rootino || hardh == persp->p_rooth` failed.\n",
>> + __FILE__, __LINE__, __func__);
>> + mlog(MLOG_ERROR | MLOG_TREE, _(
>> +"False root detected. Recovery may be possible using the `-x` option\n"));
>> + return NH_NULL;
>> + }
>>
>> /* already present
>> */
>> --
>> 2.39.3
>>
>