On Wed, Feb 05, 2014 at 09:12:15AM -0500, Brian Foster wrote:
> On 02/04/2014 08:48 PM, Dave Chinner wrote:
> > From: Dave Chinner <dchinner@xxxxxxxxxx>
> >
> > xfsrestore fails to restore file capabilities correctly because it
> > sets the owner on the file after it has restored the capability
> > attributes. This results in the kernel stripping the capabilities
> > when changing the owner of the file and hence the restored file is
> > not complete.
> >
> > Fix this by changing the owner of the file when it is created rather
> > than after it has been fully restored. This ensures we don't kill
> > the caps as they are restored after the owner it appropriately set.
> > This fixes the xfs/296 failure.
> >
> > Signed-off-by: Dave Chinner <dchinner@xxxxxxxxxx>
> > ---
> > restore/content.c | 116 ++++++++++++++++++++++++++++++++++++------------------
> > 1 file changed, 77 insertions(+), 39 deletions(-)
> >
> > diff --git a/restore/content.c b/restore/content.c
> > index 54d933c..b655ed1 100644
> > --- a/restore/content.c
> > +++ b/restore/content.c
> ...
> > @@ -7442,6 +7509,12 @@ restore_reg( drive_t *drivep,
> > }
> > }
> >
> > + if (strctxp->sc_ownerset == BOOL_FALSE && persp->a.ownerpr) {
> > + rval = set_file_owner(path, fdp, strctxp);
> > + if (rval)
> > + return BOOL_TRUE;
> > + }
> > +
>
> If this fails (rval == -1), we have unlinked the file and closed the fd
> in set_file_owner(). Why return BOOL_TRUE? I've not gone through and
> learned what dump/restore is doing, but this path looks like it's going
> to continue on with the restore in that scenario. Even if that is what
> we want to happen, it seems inconsistent with failure handling in
> set_file_owner() and the fact that we skip the rest of this function.
If any of the rest of the operations in this function fail, then it
keeps trying to do more operations and then finally it returns
BOOL_TRUE even after errors have occurred. Because the file has been
unlinked and the fd has been closed, we know all further operations
are going to fail, so we may as well just return immediately.
And if we follow the caller path back, we get back to
applynondirdump
for (;;) {
restore_file
treecb_link
restore_file_cb
restore_reg
if (not ok)
break;
}
So, returning BOOL_FALSE in the context above results in
tree_cb_link() returning RV_NOT_OK, which results in the stream
processing loop in applynondirdump() being aborted. i.e. returning
an error restoring a file causes the entire dump stream to be
aborted at that point.
IOWs, the dump image cannot be restored past that point, hence
returning an error of any kind is a bad idea. Mostly complete
restores missing just a file (that were reported on during the
restore) is much, much better than having restore stop half way
through and refusing to go any further.
The code is convoluted, obtuse and difficult to read. But we're not
going to rewrite it from scratch any time soon so we just have to
live with it's craziness.
> > - if ( mode != (mode_t)bstatp->bs_mode ) {
> > - rval = fchmod( fd, mode );
> > - if ( rval ) {
> > - mlog( MLOG_VERBOSE | MLOG_ERROR,
> > - _("unable to strip setuid/setgid "
> > - "on %s, unlinking file.\n"),
> > - path );
> > - unlink( path );
> > - }
> > - }
> > - }
> > + if (strcxtp->sc_ownerset == BOOL_FALSE && persp->a.ownerpr) {
> > + rval = set_file_owner(path, &fd, strcxtp);
> > + if (rval)
> > + return BOOL_TRUE;
> > }
>
> Ok, so we handle both set_file_owner() situations because the ordering
> here is really only important for regular files. IIUC, this particular
> block is still required for that non-ifreg case. Though, in the failure
> case, it looks like we're leaking the fd by returning now.
set_file_owner() closes the fd on error, immediately after unlinking
the file. Hence there is no fd leak in the error path.
And, again, we return BOOL_TRUE because that's what all the other
paths return even on error. And, in fact, none of the callers even
look at the error returned, so it's pretty much irrelevant in this
function.
Cheers,
Dave.
--
Dave Chinner
david@xxxxxxxxxxxxx
_______________________________________________
xfs mailing list
xfs@xxxxxxxxxxx
http://oss.sgi.com/mailman/listinfo/xfs
