Re: [PATCH 1/2] report: safely update the result.xml file

["Darrick J. Wong" <djwong@kernel.org>]

On Thu, Jul 06, 2023 at 04:42:31PM -0400, Theodore Ts'o wrote:
> After every single test, we rewrite result.xml from scratch.  This
> ensures that the XML file is always in a valid, parseable state, even
> if the check script is killed or the machine crashes in the middle of
> a test.
> 
> If the test is being run in a Cloud VM as a "spot" (Amazon, Azure, or
> GCE) or "preemptible" (Oracle) instance, it is possible that the VM
> can be halted whenever the Cloud provider needs the capacity for
> customers who are willing to pay full price.  ("Spot" instances can be
> 60% to 90% cheaper --- allowing the frugal kernel developer to get up
> to 10 times more testing for the same amount of money.  :-)
> 
> Since a "spot" VM can get terminated at any time, it is possible for
> the VM to be terminated immediately after a test has completed and
> while the result.xml file is in the middle of being written out.  In
> that case, the result.xml file could partially written, resulting in
> an invalid result.xml file and lost information about the tests run
> before the VM was terminated.
> 
> To address this race, write the new result.xml file as result.xml.new,
> and only rename it to result.xml after the XML file is fully written
> out.
> 
> Signed-off-by: Theodore Ts'o <tytso@mit.edu>
> ---
>  common/report | 19 +++++++++++--------
>  1 file changed, 11 insertions(+), 8 deletions(-)
> 
> diff --git a/common/report b/common/report
> index 9bfa09ecc..3ad14f94e 100644
> --- a/common/report
> +++ b/common/report
> @@ -109,13 +109,15 @@ _xunit_make_section_report()
>  	local notrun_count="$4"
>  	local sect_time="$5"
>  	local timestamp
> +	local tmp_fn="$REPORT_DIR/result.xml.new"
> +	local out_fn="$REPORT_DIR/result.xml"
>  
>  	if [ $sect_name == '-no-sections-' ]; then
>  		sect_name='global'
>  	fi
>  	local report=$tmp.report.xunit.$sect_name.xml
>  	# Header
> -	echo "<?xml version=\"1.0\" encoding=\"UTF-8\"?>" > $REPORT_DIR/result.xml
> +	echo "<?xml version=\"1.0\" encoding=\"UTF-8\"?>" > "$tmp_fn"

Nit: You might want to rm -f $tmp_fn first to reduce the nastiness if
someone plants a named pipe at that path.

>  	if [ -n "$test_start_time" ]; then
>  		timestamp="$(date -Iseconds --date="$test_start_time")"
>  	else
> @@ -123,7 +125,7 @@ _xunit_make_section_report()
>  	fi
>  
>  	local fstests_ns="https://git.kernel.org/pub/scm/fs/xfs/xfstests-dev.git"
> -	cat >> $REPORT_DIR/result.xml << ENDL
> +	cat >> "$tmp_fn" << ENDL
>  <testsuite
>   xmlns="$fstests_ns"
>   xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
> @@ -142,19 +144,20 @@ ENDL
>  	__generate_report_vars
>  
>  	# Properties
> -	echo -e "\t<properties>" >> $REPORT_DIR/result.xml
> +	echo -e "\t<properties>" >> "$tmp_fn"
>  	(for key in "${!REPORT_VARS[@]}"; do
>  		_xunit_add_property "$key" "${REPORT_VARS["$key"]}"
>  	done;
>  	for p in "${REPORT_ENV_LIST[@]}"; do
>  		_xunit_add_property "$p" "${!p}"
> -	done) | sort >> $REPORT_DIR/result.xml
> -	echo -e "\t</properties>" >> $REPORT_DIR/result.xml
> +	done) | sort >> "$tmp_fn"
> +	echo -e "\t</properties>" >> "$tmp_fn"
>  	if [ -f $report ]; then
> -		cat $report >> $REPORT_DIR/result.xml
> +		cat $report >> "$tmp_fn"
>  	fi
> -	echo "</testsuite>" >> $REPORT_DIR/result.xml
> -	echo "Xunit report: $REPORT_DIR/result.xml"
> +	echo "</testsuite>" >> "$tmp_fn"
> +	mv "$tmp_fn" "$out_fn"

Second nit: Make sure we actually wrote tmp_fn before blowing away the
old report.

sync "$tmp_fn" && mv "$tmp_fn" "$out_fn"

With that fixed,
Reviewed-by: Darrick J. Wong <djwong@kernel.org>

--D

> +	echo "Xunit report: $out_fn"
>  }
>  
>  _xunit_make_testcase_report()
> -- 
> 2.31.0
>