[PATCH] xfs_restore: Return on error when restoring file or symlink

[PATCH] xfs_restore: Return on error when restoring file or symlink

[Frank Sorenson]

If an error occurs while opening or truncating a regular
file, or while creating a symlink, during a restore, no error
is currently propagated back to the caller, so xfsrestore can
return SUCCESS on a failed restore.

Make restore_reg and restore_symlink return an error code
indicating the restore was incomplete.

Signed-off-by: Frank Sorenson <sorenson@redhat.com>
---
 restore/content.c | 17 +++++++++++------
 1 file changed, 11 insertions(+), 6 deletions(-)

diff --git a/restore/content.c b/restore/content.c
index c267234..5e30f08 100644
--- a/restore/content.c
+++ b/restore/content.c
@@ -7429,7 +7429,7 @@ done:
 
 /* called to begin a regular file. if no path given, or if just toc,
  * don't actually write, just read. also get into that situation if
- * cannot prepare destination. fd == -1 signifies no write. *statp
+ * cannot prepare destination. fd == -1 signifies no write. *rvp
  * is set to indicate drive errors. returns FALSE if should abort
  * this iteration.
  */
@@ -7486,12 +7486,13 @@ restore_reg(drive_t *drivep,
 
 	*fdp = open(path, oflags, S_IRUSR | S_IWUSR);
 	if (*fdp < 0) {
-		mlog(MLOG_NORMAL | MLOG_WARNING,
+		mlog(MLOG_NORMAL | MLOG_ERROR,
 		      _("open of %s failed: %s: discarding ino %llu\n"),
 		      path,
 		      strerror(errno),
 		      bstatp->bs_ino);
-		return BOOL_TRUE;
+		*rvp = RV_INCOMPLETE;
+		return BOOL_FALSE;
 	}
 
 	rval = fstat64(*fdp, &stat);
@@ -7510,10 +7511,12 @@ restore_reg(drive_t *drivep,
 
 			rval = ftruncate64(*fdp, bstatp->bs_size);
 			if (rval != 0) {
-				mlog(MLOG_VERBOSE | MLOG_WARNING,
+				mlog(MLOG_VERBOSE | MLOG_ERROR,
 				      _("attempt to truncate %s failed: %s\n"),
 				      path,
 				      strerror(errno));
+				*rvp = RV_INCOMPLETE;
+				return BOOL_FALSE;
 			}
 		}
 	}
@@ -8021,7 +8024,8 @@ restore_symlink(drive_t *drivep,
 			      bstatp->bs_ino,
 			      path);
 		}
-		return BOOL_TRUE;
+		*rvp = RV_INCOMPLETE;
+		return BOOL_FALSE;
 	}
 	scratchpath[nread] = 0;
 	if (!tranp->t_toconlypr && path) {
@@ -8045,7 +8049,8 @@ restore_symlink(drive_t *drivep,
 			      bstatp->bs_ino,
 			      path,
 			      strerror(errno));
-			return BOOL_TRUE;
+			*rvp = RV_INCOMPLETE;
+			return BOOL_FALSE;
 		}
 
 		/* set the owner and group (if enabled)
-- 
2.20.1

Re: [PATCH] xfs_restore: Return on error when restoring file or symlink

[Eric Sandeen]

On 12/12/19 5:32 PM, Frank Sorenson wrote:
> If an error occurs while opening or truncating a regular
> file, or while creating a symlink, during a restore, no error
> is currently propagated back to the caller, so xfsrestore can
> return SUCCESS on a failed restore.
> 
> Make restore_reg and restore_symlink return an error code
> indicating the restore was incomplete.

Thanks for looking at this, some stuff below

> Signed-off-by: Frank Sorenson <sorenson@redhat.com>
> ---
>  restore/content.c | 17 +++++++++++------
>  1 file changed, 11 insertions(+), 6 deletions(-)
> 
> diff --git a/restore/content.c b/restore/content.c
> index c267234..5e30f08 100644
> --- a/restore/content.c
> +++ b/restore/content.c
> @@ -7429,7 +7429,7 @@ done:
>  
>  /* called to begin a regular file. if no path given, or if just toc,
>   * don't actually write, just read. also get into that situation if
> - * cannot prepare destination. fd == -1 signifies no write. *statp
> + * cannot prepare destination. fd == -1 signifies no write. *rvp
>   * is set to indicate drive errors. returns FALSE if should abort
>   * this iteration.
>   */
> @@ -7486,12 +7486,13 @@ restore_reg(drive_t *drivep,
>  
>  	*fdp = open(path, oflags, S_IRUSR | S_IWUSR);
>  	if (*fdp < 0) {
> -		mlog(MLOG_NORMAL | MLOG_WARNING,
> +		mlog(MLOG_NORMAL | MLOG_ERROR,
>  		      _("open of %s failed: %s: discarding ino %llu\n"),
>  		      path,
>  		      strerror(errno),
>  		      bstatp->bs_ino);
> -		return BOOL_TRUE;
> +		*rvp = RV_INCOMPLETE;
> +		return BOOL_FALSE;
>  	}

I'm really not sure about original intent of the return values here and
when "this iteration should abort"

Before this patch, the function always returned BOOL_TRUE so there's
not much guidance.  Presumably all the "log something and return true"
considered these to be transient errors ...
  
>  	rval = fstat64(*fdp, &stat);
> @@ -7510,10 +7511,12 @@ restore_reg(drive_t *drivep,
>  
>  			rval = ftruncate64(*fdp, bstatp->bs_size);
>  			if (rval != 0) {
> -				mlog(MLOG_VERBOSE | MLOG_WARNING,
> +				mlog(MLOG_VERBOSE | MLOG_ERROR,
>  				      _("attempt to truncate %s failed: %s\n"),
>  				      path,
>  				      strerror(errno));
> +				*rvp = RV_INCOMPLETE;
> +				return BOOL_FALSE;
>  			}
>  		}
>  	}

so this aborts on an open or frtruncate failure, but continues on
an fstat failure or a set_file_owner failure or an XFS_IOC_FSSETXATTR
failure ... 

Was this motivated by ENOSPC, i.e. maybe the open couldn't even create
a new inode?  I could see that possibly being a hard error to stop on,
but given the prior behavior of trying to coninue as much as possible
I'm unsure about the BOOL_FALSE's here.  Setting RV_INCOMPLETE would
still make sense to me though, I think, for anything that caused the restore
to actually be incomplete.

(And this is all a little speculative as nobody really understands
this code anymore....)

-Eric

> @@ -8021,7 +8024,8 @@ restore_symlink(drive_t *drivep,
>  			      bstatp->bs_ino,
>  			      path);
>  		}
> -		return BOOL_TRUE;
> +		*rvp = RV_INCOMPLETE;
> +		return BOOL_FALSE;
>  	}
>  	scratchpath[nread] = 0;
>  	if (!tranp->t_toconlypr && path) {
> @@ -8045,7 +8049,8 @@ restore_symlink(drive_t *drivep,
>  			      bstatp->bs_ino,
>  			      path,
>  			      strerror(errno));
> -			return BOOL_TRUE;
> +			*rvp = RV_INCOMPLETE;
> +			return BOOL_FALSE;
>  		}
>  
>  		/* set the owner and group (if enabled)
>