Re: [PATCH] xfsdump: call mlog_exit in content_stream_restore
From: Alex Elder <hidden>
Date: 2011-08-26 16:15:57
On Mon, 2011-08-15 at 13:59 -0500, Bill Kendall wrote:
This patch adds mlog_exit() calls to all the return paths in content_stream_restore(). mlog_exit() is supposed to be called before returning from content_stream_dump() and content_stream_restore(), but many paths in the latter did not do so, allowing for the stream exit status to be incorrect. Signed-off-by: Bill Kendall <redacted>
It looks like content_stream_restore() *never* used it. Looking at _mlog_exit(), it looks to me like it could benefit from using exit_codestring() rather than the exit_strings[] array (which could be deleted). This would report "EXIT_ERROR" instead of just "ERROR" though, and normal exit would be reported as "EXIT_NORMAL" rather than "SUCCESS". The function call is more resilient though because it handles any value it's passed (whereas one could conceivably index beyond the end of the exit_strings[] array). Just a thought--a suggestion for a future patch. More suggestions below. They apply to pretty much the whole thing so I've put it all at the bottom.
quoted hunk ↗ jump to hunk
--- restore/content.c | 64 ++++++++++++++++++++++++++-------------------------- 1 files changed, 32 insertions(+), 32 deletions(-)diff --git a/restore/content.c b/restore/content.c index e3e4994..a2f20e3 100644 --- a/restore/content.c +++ b/restore/content.c@@ -1930,7 +1930,7 @@ content_stream_restore( ix_t thrdix ) "chdir %s failed: %s\n"), persp->a.dstdir, strerror( errno )); - return EXIT_ERROR; + return mlog_exit(EXIT_ERROR, RV_ERROR); } /* set my file creation mask to zero, to avoid modifying the
. . .
quoted hunk ↗ jump to hunk
@@ -2545,7 +2545,7 @@ content_stream_restore( ix_t thrdix ) #ifndef EOMFIX Media_end( Mediap ); #endif /* ! EOMFIX */
These #ifdef's are pretty yucky. Another suggested future cleanup: Add a flag to Media_end() to indicate whether this is one of those #ifdef's calls, and within Media_end() use a single #ifdef to determine whether simply return or actually do it, based on that passed-in value. Then just call Media_end() in all spots without the #ifdef's.
- return EXIT_NORMAL; + return mlog_exit(EXIT_NORMAL, RV_DONE); } tranp->t_sync5 = SYNC_BUSY; unlock( );
. . .
quoted hunk ↗ jump to hunk
@@ -2588,7 +2588,7 @@ content_stream_restore( ix_t thrdix ) #ifndef EOMFIX Media_end( Mediap ); #endif /* ! EOMFIX */ - return EXIT_NORMAL; + return mlog_exit(EXIT_NORMAL, rv);
If you kept this line as-is, use RV_OK rather than rv, since it makes it more explicit we know that's what's getting returned. The same would hold in many spots above as well. But... The end of this function is a whole bunch of repetitive code. It would be cleaner to assign a "ret" variable (or whatever name you think fits the existing code) and then after this last switch statement call: return mlog_exit(ret, rv); (If Media_end() got a flag, you might not need the switch statement at all...) Christoph suggested a goto which would be similar but would affect the whole function. And in fact I think it might simplify a lot--possibly eliminating whole switch statements entirely--so I think that's an idea worth considering. I think you should re-post your patch after considering these suggestions. Thanks. -Alex
} /* called after all threads have exited. scans state to decide
_______________________________________________ xfs mailing list xfs@oss.sgi.com http://oss.sgi.com/mailman/listinfo/xfs