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@xxxxxxxxxx>
> ---
> 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)
>
