| Message ID | 20230623062918.636014-1-ddouwsma@redhat.com (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
| Series | xfsrestore: suggest -x rather than assert for false roots | expand |
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.
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 >
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 */
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(-)