[PATCH] xfsdump: call mlog_exit in content_stream_restore

[PATCH] xfsdump: call mlog_exit in content_stream_restore

[Bill Kendall]

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@sgi.com>
---
 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
@@ -1951,7 +1951,7 @@ content_stream_restore( ix_t thrdix )
 		      _("malloc of stream context failed (%d bytes): %s\n"),
 		      sizeof(stream_context_t),
 		      strerror( errno ));
-		return EXIT_ERROR;
+		return mlog_exit(EXIT_ERROR, RV_ERROR);
 	}
 	strctxp->sc_fd = -1;
 	Mediap->M_drivep->d_strmcontextp = (void *)strctxp;
@@ -1972,7 +1972,7 @@ content_stream_restore( ix_t thrdix )
 			unlock( );
 			sleep( 1 );
 			if ( cldmgr_stop_requested( )) {
-				return EXIT_NORMAL;
+				return mlog_exit(EXIT_NORMAL, RV_INTR);
 			}
 			continue;
 		}
@@ -1989,7 +1989,7 @@ content_stream_restore( ix_t thrdix )
 		     * into pi, and makes persp->s.dumpid valid.
 		     */
 		if ( ok == BOOL_ERROR ) {
-			return EXIT_ERROR;
+			return mlog_exit(EXIT_ERROR, RV_OPT);
 		}
 		tranp->t_dumpidknwnpr = ok;
 		tranp->t_sync1 = SYNC_DONE;
@@ -2036,11 +2036,11 @@ content_stream_restore( ix_t thrdix )
 		case RV_QUIT:
 		case RV_DRIVE:
 			Media_end( Mediap );
-			return EXIT_NORMAL;
+			return mlog_exit(EXIT_NORMAL, rv);
 		case RV_CORE:
 		default:
 			Media_end( Mediap );
-			return EXIT_FAULT;
+			return mlog_exit(EXIT_FAULT, rv);
 		}
 		dcaps = drivep->d_capabilities;
 
@@ -2050,7 +2050,7 @@ content_stream_restore( ix_t thrdix )
 			sleep( 1 );
 			if ( cldmgr_stop_requested( )) {
 				Media_end( Mediap );
-				return EXIT_NORMAL;
+				return mlog_exit(EXIT_NORMAL, RV_INTR);
 			}
 			lock( );
 		}
@@ -2134,7 +2134,7 @@ content_stream_restore( ix_t thrdix )
 		}
 		if ( cldmgr_stop_requested( )) {
 			Media_end( Mediap );
-			return EXIT_NORMAL;
+			return mlog_exit(EXIT_NORMAL, RV_INTR);
 		}
 		if ( ! matchpr ) {
 			Media_end( Mediap );
@@ -2145,13 +2145,13 @@ content_stream_restore( ix_t thrdix )
 			     ( ! ( dcaps & DRIVE_CAP_FILES )
 			       &&
 			       ! ( dcaps & DRIVE_CAP_REMOVABLE ))) {
-				return EXIT_NORMAL;
+				return mlog_exit(EXIT_NORMAL, RV_UNKNOWN);
 			}
 			continue;
 		}
 		if ( ! dumpcompat( resumepr, level, *baseidp, BOOL_TRUE )) {
 			Media_end( Mediap );
-			return EXIT_ERROR;
+			return mlog_exit(EXIT_ERROR, RV_COMPAT);
 		}
 		strncpyterm( persp->s.dumplab,
 			     grhdrp->gh_dumplabel,
@@ -2186,7 +2186,7 @@ content_stream_restore( ix_t thrdix )
 			 * if this is a match
 			 */
 		if ( fileh == DH_NULL ) {
-			return EXIT_FAULT;
+			return mlog_exit(EXIT_FAULT, RV_ERROR);
 		}
 		uuid_copy(persp->s.dumpid,grhdrp->gh_dumpid);
 		persp->s.begintime = time( 0 );
@@ -2226,11 +2226,11 @@ content_stream_restore( ix_t thrdix )
 		case RV_QUIT:
 		case RV_DRIVE:
 			Media_end( Mediap );
-			return EXIT_NORMAL;
+			return mlog_exit(EXIT_NORMAL, rv);
 		case RV_CORE:
 		default:
 			Media_end( Mediap );
-			return EXIT_FAULT;
+			return mlog_exit(EXIT_FAULT, rv);
 		}
 		dcaps = drivep->d_capabilities;
 		ASSERT( fileh != DH_NULL );
@@ -2257,7 +2257,7 @@ content_stream_restore( ix_t thrdix )
 			sleep( 1 );
 			if ( cldmgr_stop_requested( )) {
 				Media_end( Mediap );
-				return EXIT_NORMAL;
+				return mlog_exit(EXIT_NORMAL, RV_INTR);
 			}
 			lock( );
 		}
@@ -2278,7 +2278,7 @@ content_stream_restore( ix_t thrdix )
 					   scrhdrp->cih_inomap_dircnt );
 			if ( ! ok ) {
 				Media_end( Mediap );
-				return EXIT_ERROR;
+				return mlog_exit(EXIT_ERROR, RV_ERROR);
 			}
 			tranp->t_dirattrinitdonepr = BOOL_TRUE;
 		}
@@ -2293,7 +2293,7 @@ content_stream_restore( ix_t thrdix )
 					  scrhdrp->cih_inomap_nondircnt );
 			if ( ! ok ) {
 				Media_end( Mediap );
-				return EXIT_ERROR;
+				return mlog_exit(EXIT_ERROR, RV_ERROR);
 			}
 			tranp->t_namreginitdonepr = BOOL_TRUE;
 		}
@@ -2326,7 +2326,7 @@ content_stream_restore( ix_t thrdix )
 					persp->a.dstdirisxfspr );
 			if ( ! ok ) {
 				Media_end( Mediap );
-				return EXIT_ERROR;
+				return mlog_exit(EXIT_ERROR, RV_ERROR);
 			}
 			tranp->t_treeinitdonepr = BOOL_TRUE;
 		}
@@ -2357,11 +2357,11 @@ content_stream_restore( ix_t thrdix )
 		case RV_INTR:
 		case RV_DRIVE:
 			Media_end( Mediap );
-			return EXIT_NORMAL;
+			return mlog_exit(EXIT_NORMAL, rv);
 		case RV_CORE:
 		default:
 			Media_end( Mediap );
-			return EXIT_FAULT;
+			return mlog_exit(EXIT_FAULT, rv);
 		}
 	}
 
@@ -2396,7 +2396,7 @@ content_stream_restore( ix_t thrdix )
 			sleep( 1 );
 			if ( cldmgr_stop_requested( )) {
 				Media_end( Mediap );
-				return EXIT_NORMAL;
+				return mlog_exit(EXIT_NORMAL, RV_INTR);
 			}
 			lock( );
 		}
@@ -2416,14 +2416,14 @@ content_stream_restore( ix_t thrdix )
 			break;
 		case RV_ERROR:
 			Media_end( Mediap );
-			return EXIT_ERROR;
+			return mlog_exit(EXIT_ERROR, rv);
 		case RV_INTR:
 			Media_end( Mediap );
-			return EXIT_INTERRUPT;
+			return mlog_exit(EXIT_INTERRUPT, rv);
 		case RV_CORE:
 		default:
 			Media_end( Mediap );
-			return EXIT_FAULT;
+			return mlog_exit(EXIT_FAULT, rv);
 		}
 
 		/* now that we have a tree and inomap, scan the
@@ -2476,11 +2476,11 @@ content_stream_restore( ix_t thrdix )
 		case RV_QUIT:
 		case RV_DRIVE:
 			Media_end( Mediap );
-			return EXIT_NORMAL;
+			return mlog_exit(EXIT_NORMAL, rv);
 		case RV_CORE:
 		default:
 			Media_end( Mediap );
-			return EXIT_FAULT;
+			return mlog_exit(EXIT_FAULT, rv);
 		}
 		dcaps = drivep->d_capabilities;
 		ASSERT( fileh > DH_NULL );
@@ -2521,11 +2521,11 @@ content_stream_restore( ix_t thrdix )
 		case RV_DRIVE:
 		case RV_INCOMPLETE:
 			Media_end( Mediap );
-			return EXIT_NORMAL;
+			return mlog_exit(EXIT_NORMAL, rv);
 		case RV_CORE:
 		default:
 			Media_end( Mediap );
-			return EXIT_FAULT;
+			return mlog_exit(EXIT_FAULT, rv);
 		}
 	}
 
@@ -2545,7 +2545,7 @@ content_stream_restore( ix_t thrdix )
 #ifndef EOMFIX
 		Media_end( Mediap );
 #endif /* ! EOMFIX */
-		return EXIT_NORMAL;
+		return mlog_exit(EXIT_NORMAL, RV_DONE);
 	}
 	tranp->t_sync5 = SYNC_BUSY;
 	unlock( );
@@ -2569,18 +2569,18 @@ content_stream_restore( ix_t thrdix )
 #ifndef EOMFIX
 		Media_end( Mediap );
 #endif /* ! EOMFIX */
-		return EXIT_ERROR;
+		return mlog_exit(EXIT_ERROR, rv);
 	case RV_INTR:
 #ifndef EOMFIX
 		Media_end( Mediap );
 #endif /* ! EOMFIX */
-		return EXIT_NORMAL;
+		return mlog_exit(EXIT_NORMAL, rv);
 	case RV_CORE:
 	default:
 #ifndef EOMFIX
 		Media_end( Mediap );
 #endif /* ! EOMFIX */
-		return EXIT_FAULT;
+		return mlog_exit(EXIT_FAULT, rv);
 	}
 
 	/* made it! I'm last, now exit
@@ -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);
 }
 
 /* called after all threads have exited. scans state to decide
-- 
1.7.0.4

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH] xfsdump: call mlog_exit in content_stream_restore

[Christoph Hellwig]

On Mon, Aug 15, 2011 at 01:59:23PM -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@sgi.com>

Looks okay, but once again I'd prefer to use goto labels so that
we have a logical grouping of that required exit.

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH] xfsdump: call mlog_exit in content_stream_restore

[Alex Elder]

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@sgi.com>

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.

> ---
>  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

. . .
 
> @@ -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( );

. . .

> @@ -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

Re: [PATCH] xfsdump: call mlog_exit in content_stream_restore

[Bill Kendall]

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@sgi.com>
> 
> 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@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH] xfsdump: call mlog_exit in content_stream_restore

[Bill Kendall]

On 08/26/2011 11:15 AM, 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.

...

> 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 looked at doing this, but it didn't result in any switch
statements being removed. Each switch statement is different
enough that we can't just have a single switch at the end
of the function that maps the RV_* value to the proper
EXIT_* value.

I made some other minor changes based on suggestions, and
will repost after the "xfsdump: remove unnecessary" patch
is reviewed. (This patch overlaps a bit with that one.)

Thanks,
Bill

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs