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

[PATCH] 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: d7cba74 ("xfsrestore: fix rootdir due to xfsdump bulkstat misuse")
Signed-off-by: Donald Douwsma <ddouwsma@redhat.com>
---
 restore/tree.c | 13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)

diff --git a/restore/tree.c b/restore/tree.c
index bfa07fe..0b65d0f 100644
--- a/restore/tree.c
+++ b/restore/tree.c
@@ -783,8 +783,17 @@ 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__, __FUNCTION__);
+		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] xfsrestore: suggest -x rather than assert for false roots

[Pavel Reichl]

On 6/23/23 08:29, 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: d7cba74 ("xfsrestore: fix rootdir due to xfsdump bulkstat misuse")
> Signed-off-by: Donald Douwsma <ddouwsma@redhat.com>
> ---
>   restore/tree.c | 13 +++++++++++--
>   1 file changed, 11 insertions(+), 2 deletions(-)
>
> diff --git a/restore/tree.c b/restore/tree.c
> index bfa07fe..0b65d0f 100644
> --- a/restore/tree.c
> +++ b/restore/tree.c
> @@ -783,8 +783,17 @@ 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,
Trailing white space
> +			"%s:%d: %s: Assertion "
> +			"`ino != persp->p_rootino || hardh == persp->p_rooth` failed.\n",
> +			__FILE__, __LINE__, __FUNCTION__);
> +		mlog(MLOG_ERROR | MLOG_TREE, _(
> +			"False root detected. "
> +			"Recovery may be possible using the `-x` option\n"));
> +		return NH_NULL;
> +	};
Extra semicolon
>   	/* already present
>   	 */
Otherwise looks good to me, applies and builds.

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

[Darrick J. Wong]

On Fri, Jun 23, 2023 at 04:29:18PM +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: d7cba74 ("xfsrestore: fix rootdir due to xfsdump bulkstat misuse")
> Signed-off-by: Donald Douwsma <ddouwsma@redhat.com>
> ---
>  restore/tree.c | 13 +++++++++++--
>  1 file changed, 11 insertions(+), 2 deletions(-)
> 
> diff --git a/restore/tree.c b/restore/tree.c
> index bfa07fe..0b65d0f 100644
> --- a/restore/tree.c
> +++ b/restore/tree.c
> @@ -783,8 +783,17 @@ 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)) {

Usually we put the && operator on the line above.  But ignore that
comment if that's actually the predominant style in xfsrestore.

	if (need_fixrootdir == BOOL_FALSE &&
	    !(ino != persp->p_rootino || hardh == persp->p_rooth)) {

> +		mlog(MLOG_ERROR | MLOG_TREE, 

Trailing space.

> +			"%s:%d: %s: Assertion "
> +			"`ino != persp->p_rootino || hardh == persp->p_rooth` failed.\n",

Might be nice to put the entire string on one line for greppability?

		mlog(MLOG_ERROR | MLOG_TREE,
 "%s:%d: %s: Assertion `ino != persp->p_rootino || hardh == persp->p_rooth` failed.\n",

> +			__FILE__, __LINE__, __FUNCTION__);
> +		mlog(MLOG_ERROR | MLOG_TREE, _(
> +			"False root detected. "
> +			"Recovery may be possible using the `-x` option\n"));

Same here.

Otherwise this looks like a good improvement.

--D

> +		return NH_NULL;
> +	};
>  
>  	/* already present
>  	 */
> -- 
> 2.39.3
>