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

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

[Donald Douwsma]

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,
+"%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

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

[Darrick J. Wong]

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.

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
> 

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

[Donald Douwsma]

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
>>
>