xfs
[Top] [All Lists]

Re: [PATCH] xfsdump: call mlog_exit in content_stream_restore

To: aelder@xxxxxxx
Subject: Re: [PATCH] xfsdump: call mlog_exit in content_stream_restore
From: Bill Kendall <wkendall@xxxxxxx>
Date: Mon, 29 Aug 2011 10:20:52 -0500
Cc: xfs@xxxxxxxxxxx
In-reply-to: <1314375344.2821.47.camel@doink>
References: <1313434763-22340-1-git-send-email-wkendall@xxxxxxx> <1314375344.2821.47.camel@doink>
User-agent: Thunderbird 1.5.0.14ubu (X11/20080502)
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

<Prev in Thread] Current Thread [Next in Thread>