Re: [PATCH v7] tst: Extend cross-test-ssh.sh to specify if target date can be altered

[Adhemerval Zanella <adhemerval.zanella@linaro.org>]

On 07/03/2021 09:02, Lukasz Majewski wrote:
> This code adds new flag - '--allow-time-setting' to cross-test-ssh.sh
> script to indicate if it is allowed to alter the date on the system
> on which tests are executed. This change is supposed to be used with
> test systems, which use virtual machines for testing.
> 
> The GLIBC_TEST_ALLOW_TIME_SETTING env variable is exported to the
> remote environment on which the eligible test is run and brings no
> functional change when it is not.
> 
> Changes for v2:
> - Utilize flock to provide serialization of cross-test-ssh.sh script
>   execution.
> - Add entry to manual/install.texi about --allow-time-setting flag
>   usage.
> 
> Changes for v3:
> - The install.texi manual has been augmented to explain two distinct
>   use cases for setting the time on target system.
> 
> Changes for v4:
> - Add -w switch to flock (specify timeout)
> - Check return value of flock - exit ssh shell script
> - Allow user override of FLOCK_* variables
> - Rewrite the script's information/help text
> - Rewrite the install.texi manual entry for this
>   script usage
> 
> Changes for v5:
> - Add two spaces after '.'
> - Add information to INSTALL file that 'flock' from util-linux is
>   required to provide serialization
> - Add information to INSTALL about flock need for test wrapper
> 
> Changes for v6:
> - Move the information about 'flock' requirement from INSTALL to
>   install.texi
> 
> Changes for v7:
> - Update INSTALL after regenerating it

Ok with above wording changes.

Reviewed-by: Adhemerval Zanella  <adhemerval.zanella@linaro.org>

> ---
>  INSTALL                   | 15 +++++++++++++++
>  manual/install.texi       | 20 ++++++++++++++++++++
>  scripts/cross-test-ssh.sh | 25 ++++++++++++++++++++++++-
>  3 files changed, 59 insertions(+), 1 deletion(-)
> 
> diff --git a/INSTALL b/INSTALL
> index 9a50e3ee06..c74ddd1d9f 100644
> --- a/INSTALL
> +++ b/INSTALL
> @@ -347,6 +347,21 @@ is the absolute directory name for the main source directory and
>  HOSTNAME is the host name of a system that can run the newly built
>  binaries of the GNU C Library.  The source and build directories must be
>  visible at the same locations on both the build system and HOSTNAME.
> +The 'cross-test-ssh.sh' script requres 'flock' from 'util-linux' to work
> +when GLIBC_TEST_ALLOW_TIME_SETTING environment variable is set.
> +
> +   It is also possible to execute tests, which require setting date on
> +the target machine.  Following use cases are supported:
> +   * 'GLIBC_TEST_ALLOW_TIME_SETTING' is set in the environment in which
> +     eligible tests are executed and have privilege to run
> +     'clock_settime'.  In this case nothing prevents those tests from
> +     running in parallel, so the caller shall assure that those tests
> +     are serialized or provide a proper wrapper script for it.
> +
> +   * The 'cross-test-ssh.sh' script is used and one passes the
> +     '--allow-time-setting' flag.  In this case both setting
> +     'GLIBC_TEST_ALLOW_TIME_SETTING' and serialization of tests
> +     execution are assured automatically.
>  
>     In general, when testing the GNU C Library, 'test-wrapper' may be set
>  to the name and arguments of any program to run newly built binaries.
> diff --git a/manual/install.texi b/manual/install.texi
> index 419576f49c..828c78ee1f 100644
> --- a/manual/install.texi
> +++ b/manual/install.texi
> @@ -379,6 +379,26 @@ directory and @var{hostname} is the host name of a system that can run
>  the newly built binaries of @theglibc{}.  The source and build
>  directories must be visible at the same locations on both the build
>  system and @var{hostname}.
> +The @samp{cross-test-ssh.sh} script requres @samp{flock} from

s/requres/requires

> +@samp{util-linux} to work when @var{glibc_test_allow_time_setting}
> +environment variable is set.
> +
> +It is also possible to execute tests, which require setting date on

I think it misses a determiner: 'require setting *the* date'.

> +the target machine.  Following use cases are supported:
> +@itemize @bullet
> +@item
> +@code{GLIBC_TEST_ALLOW_TIME_SETTING} is set in the environment in
> +which eligible tests are executed and have privilege to run

Similar as before, I think misses an article: "and have the privilege'

> +@code{clock_settime}.  In this case nothing prevents those tests from

I think it misses an comma here: "In this case, nothing"

> +running in parallel, so the caller shall assure that those tests
> +are serialized or provide a proper wrapper script for it.

s/it/them (the tests).

> +
> +@item
> +The @code{cross-test-ssh.sh} script is used and one passes the
> +@option{--allow-time-setting} flag.  In this case both setting

Missing comma here as well: "In this case, ..." and it should be
'both sets'.

> +@code{GLIBC_TEST_ALLOW_TIME_SETTING} and serialization of tests

s/tests/test

> +execution are assured automatically.
> +@end itemize
>  
>  In general, when testing @theglibc{}, @samp{test-wrapper} may be set
>  to the name and arguments of any program to run newly built binaries.
> diff --git a/scripts/cross-test-ssh.sh b/scripts/cross-test-ssh.sh
> index 6d8fbcdfd2..a839e22dd0 100755
> --- a/scripts/cross-test-ssh.sh
> +++ b/scripts/cross-test-ssh.sh
> @@ -22,7 +22,7 @@
>  
>  progname="$(basename $0)"
>  
> -usage="usage: ${progname} [--ssh SSH] HOST COMMAND ..."
> +usage="usage: ${progname} [--ssh SSH] [--allow-time-setting] HOST COMMAND ..."
>  help="Run a glibc test COMMAND on the remote machine HOST, via ssh,
>  preserving the current working directory, and respecting quoting.
>  
> @@ -32,6 +32,10 @@ instead of ordinary 'ssh'.
>  If the '--timeoutfactor FACTOR' flag is present, set TIMEOUTFACTOR on
>  the remote machine to the specified FACTOR.
>  
> +If the '--allow-time-setting' flag is present, set
> +GLIBC_TEST_ALLOW_TIME_SETTING on the remote machine to indicate that
> +time can be safely adjusted (e.g. on a virtual machine).
> +
>  To use this to run glibc tests, invoke the tests as follows:
>  
>    $ make test-wrapper='ABSPATH/cross-test-ssh.sh HOST' tests
> @@ -81,6 +85,10 @@ while [ $# -gt 0 ]; do
>        timeoutfactor="$1"
>        ;;
>  
> +    "--allow-time-setting")
> +      settimeallowed="1"
> +      ;;
> +
>      "--help")
>        echo "$usage"
>        echo "$help"
> @@ -127,6 +135,21 @@ if [ "$timeoutfactor" ]; then
>  ${command}"
>  fi
>  
> +# Add command to set the info that time on target can be adjusted,
> +# if required.
> +# Serialize execution of this script on target to prevent from unintended
> +# change of target time.
> +FLOCK_PATH="${FLOCK_PATH:-/var/lock/clock_settime}"
> +FLOCK_TIMEOUT="${FLOCK_TIMEOUT:-20}"
> +FLOCK_FD="${FLOCK_FD:-99}"
> +if [ "$settimeallowed" ]; then
> +  command="exec ${FLOCK_FD}<>${FLOCK_PATH}
> +flock -w ${FLOCK_TIMEOUT} ${FLOCK_FD}
> +if [ $? -ne 0 ]; then exit 1; fi
> +export GLIBC_TEST_ALLOW_TIME_SETTING=1
> +${command}"
> +fi
> +
>  # HOST's sshd simply concatenates its arguments with spaces and
>  # passes them to some shell.  We want to force the use of /bin/sh,
>  # so we need to re-quote the whole command to ensure it appears as
>