Alex Elder wrote:
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 <wkendall@xxxxxxx>
It looks like content_stream_restore() *never* used it.
You are right. I was thinking of calls to mlog_exit_hint() from functions
called by content_stream_restore().
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.
Currently the stream exit status ("EXIT_*") is not printed on Linux, due
to only supporting a single stream. So we have some flexibility there.
i.e., exit_codestring() could return "ERROR" instead of "EXIT_ERROR".
More suggestions below. They apply to pretty much
the whole thing so I've put it all at the bottom.
...
@@ -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( );
EOMFIX is always defined, it really shouldn't be conditional code anymore.
I was planning to take care of that in a future patch, but since it may
cleanup this patch a bit I'll do the EOMFIX patch first and resubmit this
afterwards. There's also a number of other #defines that should go away,
I'll look at those as well.
. . .
@@ -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.
I'll take a look at doing this.
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...)
Still need to map the RVAL_* value from finalize() to
the right EXIT_*, so the switch cannot be removed
altogether but can be simplified.
Bill
_______________________________________________
xfs mailing list
xfs@xxxxxxxxxxx
http://oss.sgi.com/mailman/listinfo/xfs
