[PATCH v3 1/3] tst: Extend cross-test-ssh.sh to specify if target date can be altered

[PATCH v3 1/3] tst: Extend cross-test-ssh.sh to specify if target date can be altered

[Lukasz Majewski]

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.
---
 manual/install.texi       | 17 +++++++++++++++++
 scripts/cross-test-ssh.sh | 22 +++++++++++++++++++++-
 2 files changed, 38 insertions(+), 1 deletion(-)

diff --git a/manual/install.texi b/manual/install.texi
index 419576f49c..13a8058616 100644
--- a/manual/install.texi
+++ b/manual/install.texi
@@ -380,6 +380,23 @@ 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}.
 
+It is also possible to execute tests, which require setting date on
+the target machine. Following use cases are supported:
+@itemize @bullet
+@item
+The @code{GLIBC_TEST_ALLOW_TIME_SETTING} is set in the environment in
+which eligible tests are executed and have priviledges to run
+@code{clock_settime}. In this case there is no prevention from running
+those tests in parallel, so the caller shall assure that those tests
+are serialized or provide proper wrapper script for it.
+
+@item
+The @code{cross-test-ssh.sh} script is used and one passes
+@option{--allow-time-setting} flag. In this case both - setting the
+@code{GLIBC_TEST_ALLOW_TIME_SETTING} and serialization of tests 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.
 This program must preserve the arguments to the binary being run, its
diff --git a/scripts/cross-test-ssh.sh b/scripts/cross-test-ssh.sh
index 6d8fbcdfd2..c4b112aa1d 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,11 @@ 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 inform that
+time can be safely adjusted when e.g. tests are run in 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 +86,10 @@ while [ $# -gt 0 ]; do
       timeoutfactor="$1"
       ;;
 
+    "--allow-time-setting")
+      settimeallowed="1"
+      ;;
+
     "--help")
       echo "$usage"
       echo "$help"
@@ -127,6 +136,17 @@ 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 host to prevent from unintended
+# change of target time.
+FLOCK_PATH="/var/lock/clock_settime"
+if [ "$settimeallowed" ]; then
+  exec 99<>${FLOCK_PATH}
+  flock 99 || { echo "Cannot lock ${FLOCK_PATH}"; exit 1; }
+  command="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
-- 
2.20.1

[PATCH v3 2/3] support: Provide xclock_settime test helper function

[Lukasz Majewski]

The xclock_settime is a wrapper function on the clock_settime syscall
to be used in the test code.

It checks if the GLIBC_TEST_ALLOW_TIME_SETTING env variable is defined
in the environment in which test is executed. If it is not - the test
ends as unsupported. Otherwise, the clock-settime is executed and return
value is assessed.
---
 support/Makefile         |  1 +
 support/xclock_settime.c | 35 +++++++++++++++++++++++++++++++++++
 support/xtime.h          |  5 +++++
 3 files changed, 41 insertions(+)
 create mode 100644 support/xclock_settime.c

diff --git a/support/Makefile b/support/Makefile
index bb9889efb4..8d63fbd5da 100644
--- a/support/Makefile
+++ b/support/Makefile
@@ -90,6 +90,7 @@ libsupport-routines = \
   xchdir \
   xchroot \
   xclock_gettime \
+  xclock_settime \
   xclose \
   xchmod \
   xconnect \
diff --git a/support/xclock_settime.c b/support/xclock_settime.c
new file mode 100644
index 0000000000..12a83b9d79
--- /dev/null
+++ b/support/xclock_settime.c
@@ -0,0 +1,35 @@
+/* clock_settime with error checking.
+   Copyright (C) 2021 Free Software Foundation, Inc.
+   This file is part of the GNU C Library.
+
+   The GNU C Library is free software; you can redistribute it and/or
+   modify it under the terms of the GNU Lesser General Public
+   License as published by the Free Software Foundation; either
+   version 2.1 of the License, or (at your option) any later version.
+
+   The GNU C Library is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+   Lesser General Public License for more details.
+
+   You should have received a copy of the GNU Lesser General Public
+   License along with the GNU C Library; if not, see
+   <https://www.gnu.org/licenses/>.  */
+
+#include <stdlib.h>
+#include <support/check.h>
+#include <support/xtime.h>
+#include <support/xthread.h>
+
+void
+xclock_settime (clockid_t clockid,
+                const struct timespec *ts)
+{
+  if (getenv (SETTIME_ENV_NAME) == NULL)
+    FAIL_UNSUPPORTED ("clock_settime is executed only when "\
+                      SETTIME_ENV_NAME" is set\n");
+
+  const int ret = clock_settime (clockid, ts);
+  if (ret < 0)
+    FAIL_EXIT1 ("clock_settime (%d): %m", clockid);
+}
diff --git a/support/xtime.h b/support/xtime.h
index 2482837dee..b4ac3b59e2 100644
--- a/support/xtime.h
+++ b/support/xtime.h
@@ -23,10 +23,15 @@
 
 __BEGIN_DECLS
 
+/* Name of the env variable, which indicates if it is possible to
+   adjust time on target machine.  */
+#define SETTIME_ENV_NAME "GLIBC_TEST_ALLOW_TIME_SETTING"
+
 /* The following functions call the corresponding libc functions and
    terminate the process on error.  */
 
 void xclock_gettime (clockid_t clock, struct timespec *ts);
+void xclock_settime (clockid_t clock, const struct timespec *ts);
 
 /* This helper can often simplify tests by avoiding an explicit
    variable declaration or allowing that declaration to be const. */
-- 
2.20.1

Re: [PATCH v3 2/3] support: Provide xclock_settime test helper function

[DJ Delorie]

Lukasz Majewski <lukma@denx.de> writes:

> +#include <support/xthread.h>

Why is this included?

> +void
> +xclock_settime (clockid_t clockid,
> +                const struct timespec *ts)
> +{
> +  if (getenv (SETTIME_ENV_NAME) == NULL)
> +    FAIL_UNSUPPORTED ("clock_settime is executed only when "\
> +                      SETTIME_ENV_NAME" is set\n");
> +
> +  const int ret = clock_settime (clockid, ts);
> +  if (ret < 0)
> +    FAIL_EXIT1 ("clock_settime (%d): %m", clockid);
> +}

Ok.

> diff --git a/support/xtime.h b/support/xtime.h
> index 2482837dee..b4ac3b59e2 100644
> --- a/support/xtime.h
> +++ b/support/xtime.h
> @@ -23,10 +23,15 @@
>  
>  __BEGIN_DECLS
>  
> +/* Name of the env variable, which indicates if it is possible to
> +   adjust time on target machine.  */
> +#define SETTIME_ENV_NAME "GLIBC_TEST_ALLOW_TIME_SETTING"
> +
>  /* The following functions call the corresponding libc functions and
>     terminate the process on error.  */
>  
>  void xclock_gettime (clockid_t clock, struct timespec *ts);
> +void xclock_settime (clockid_t clock, const struct timespec *ts);
>  
>  /* This helper can often simplify tests by avoiding an explicit
>     variable declaration or allowing that declaration to be const. */

Ok.

Re: [PATCH v3 2/3] support: Provide xclock_settime test helper function

[Lukasz Majewski]

[-- Attachment #1: Type: text/plain, Size: 1698 bytes --]

Hi DJ,

> Lukasz Majewski <lukma@denx.de> writes:
> 
> > +#include <support/xthread.h>  
> 
> Why is this included?
> 

This is indeed not needed.

> > +void
> > +xclock_settime (clockid_t clockid,
> > +                const struct timespec *ts)
> > +{
> > +  if (getenv (SETTIME_ENV_NAME) == NULL)
> > +    FAIL_UNSUPPORTED ("clock_settime is executed only when "\
> > +                      SETTIME_ENV_NAME" is set\n");
> > +
> > +  const int ret = clock_settime (clockid, ts);
> > +  if (ret < 0)
> > +    FAIL_EXIT1 ("clock_settime (%d): %m", clockid);
> > +}  
> 
> Ok.
> 
> > diff --git a/support/xtime.h b/support/xtime.h
> > index 2482837dee..b4ac3b59e2 100644
> > --- a/support/xtime.h
> > +++ b/support/xtime.h
> > @@ -23,10 +23,15 @@
> >  
> >  __BEGIN_DECLS
> >  
> > +/* Name of the env variable, which indicates if it is possible to
> > +   adjust time on target machine.  */
> > +#define SETTIME_ENV_NAME "GLIBC_TEST_ALLOW_TIME_SETTING"
> > +
> >  /* The following functions call the corresponding libc functions
> > and terminate the process on error.  */
> >  
> >  void xclock_gettime (clockid_t clock, struct timespec *ts);
> > +void xclock_settime (clockid_t clock, const struct timespec *ts);
> >  
> >  /* This helper can often simplify tests by avoiding an explicit
> >     variable declaration or allowing that declaration to be const.
> > */  
> 
> Ok.
> 




Best regards,

Lukasz Majewski

--

DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-59 Fax: (+49)-8142-66989-80 Email: lukma@denx.de

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[PATCH v3 3/3] tst: Add test for clock_settime

[Lukasz Majewski]

This code brings test to check if time on target machine is properly set.
To avoid any issues with altering the time:

- The time, which was set before the test was executed is restored.

- The time is altered only when cross-test-ssh.sh is executed with
  --allow-time-setting flag
---
 time/Makefile            |  3 ++-
 time/tst-clock_settime.c | 45 ++++++++++++++++++++++++++++++++++++++++
 2 files changed, 47 insertions(+), 1 deletion(-)
 create mode 100644 time/tst-clock_settime.c

diff --git a/time/Makefile b/time/Makefile
index a8748480d6..b6f0969f3d 100644
--- a/time/Makefile
+++ b/time/Makefile
@@ -50,7 +50,8 @@ tests	:= test_time clocktest tst-posixtz tst-strptime tst_wcsftime \
 	   tst-tzname tst-y2039 bug-mktime4 tst-strftime2 tst-strftime3 \
 	   tst-clock tst-clock2 tst-clock_nanosleep tst-cpuclock1 \
 	   tst-adjtime tst-ctime tst-difftime tst-mktime4 tst-clock-y2038 \
-	   tst-clock2-y2038 tst-cpuclock1-y2038 tst-clock_nanosleep-y2038
+	   tst-clock2-y2038 tst-cpuclock1-y2038 tst-clock_nanosleep-y2038 \
+	   tst-clock_settime
 
 include ../Rules
 
diff --git a/time/tst-clock_settime.c b/time/tst-clock_settime.c
new file mode 100644
index 0000000000..2abf6b8eb8
--- /dev/null
+++ b/time/tst-clock_settime.c
@@ -0,0 +1,45 @@
+/* Test for clock_settime (in VM)
+   Copyright (C) 2021 Free Software Foundation, Inc.
+   This file is part of the GNU C Library.
+
+   The GNU C Library is free software; you can redistribute it and/or
+   modify it under the terms of the GNU Lesser General Public
+   License as published by the Free Software Foundation; either
+   version 2.1 of the License, or (at your option) any later version.
+
+   The GNU C Library is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+   Lesser General Public License for more details.
+
+   You should have received a copy of the GNU Lesser General Public
+   License along with the GNU C Library; if not, see
+   <https://www.gnu.org/licenses/>.  */
+
+#include <time.h>
+#include <support/check.h>
+#include <support/xtime.h>
+
+#define TIMESPEC_SEC_Y2038_OV 0x7FFFFFFF
+#define FUTURE_TIME (TIMESPEC_SEC_Y2038_OV - 10)
+
+static int
+do_test (void)
+{
+  const struct timespec tv = { FUTURE_TIME, 0};
+  struct timespec tv_future, tv_now;
+
+  tv_now = xclock_now(CLOCK_REALTIME);
+  xclock_settime(CLOCK_REALTIME, &tv);
+  tv_future = xclock_now(CLOCK_REALTIME);
+
+  /* Restore old time value on target machine.  */
+  xclock_settime(CLOCK_REALTIME, (const struct timespec*) &tv_now);
+
+  if (tv_future.tv_sec < tv.tv_sec)
+    FAIL_EXIT1 ("clock_settime set wrong time!\n");
+
+  return 0;
+}
+
+#include <support/test-driver.c>
-- 
2.20.1

Re: [PATCH v3 3/3] tst: Add test for clock_settime

[DJ Delorie]

Lukasz Majewski <lukma@denx.de> writes:
> -	   tst-clock2-y2038 tst-cpuclock1-y2038 tst-clock_nanosleep-y2038
> +	   tst-clock2-y2038 tst-cpuclock1-y2038 tst-clock_nanosleep-y2038 \
> +	   tst-clock_settime

Ok.

> +/* Test for clock_settime (in VM)

Perhaps "in VM" should be "if supported" as VMs aren't the only
supported case, and "in VM" doesn't really say *why* "in VM" is
important?

And we likely don't have to mention "if supported" at all; other tests
don't.  *Any* test could be unsupported, for a variety of reasons
unrelated to the test.

> +#include <time.h>
> +#include <support/check.h>
> +#include <support/xtime.h>
> +
> +#define TIMESPEC_SEC_Y2038_OV 0x7FFFFFFF

This test will, of course, fail after 2038.

> +#define FUTURE_TIME (TIMESPEC_SEC_Y2038_OV - 10)

Assuming the test takes less than 10 seconds, which is probably OK.

> +static int
> +do_test (void)
> +{
> +  const struct timespec tv = { FUTURE_TIME, 0};
> +  struct timespec tv_future, tv_now;
> +
> +  tv_now = xclock_now(CLOCK_REALTIME);

Current time (before 2038 we hope)...

> +  xclock_settime(CLOCK_REALTIME, &tv);

Set to 2038...

> +  tv_future = xclock_now(CLOCK_REALTIME);

Check "2038" time...

> +  /* Restore old time value on target machine.  */
> +  xclock_settime(CLOCK_REALTIME, (const struct timespec*) &tv_now);

The cast should not be needed.  Sometimes GCC complains about casting
structs to different types (although this shouldn't do that) so I tend
to avoid it when it's not needed.

> +  if (tv_future.tv_sec < tv.tv_sec)
> +    FAIL_EXIT1 ("clock_settime set wrong time!\n");

If the "2038" time is "before" the "now (hopefully before 2038)" time,
we failed to set the time to the future... ok.

> +  return 0;
> +}
> +
> +#include <support/test-driver.c>

LGTM aside from those nits.

Reviewed-by: DJ Delorie <dj@redhat.com>

Re: [PATCH v3 3/3] tst: Add test for clock_settime

[Lukasz Majewski]

[-- Attachment #1: Type: text/plain, Size: 2641 bytes --]

Hi DJ,

> Lukasz Majewski <lukma@denx.de> writes:
> > -	   tst-clock2-y2038 tst-cpuclock1-y2038
> > tst-clock_nanosleep-y2038
> > +	   tst-clock2-y2038 tst-cpuclock1-y2038
> > tst-clock_nanosleep-y2038 \
> > +	   tst-clock_settime  
> 
> Ok.
> 
> > +/* Test for clock_settime (in VM)  
> 
> Perhaps "in VM" should be "if supported" as VMs aren't the only
> supported case, and "in VM" doesn't really say *why* "in VM" is
> important?
> 
> And we likely don't have to mention "if supported" at all; other tests
> don't.  *Any* test could be unsupported, for a variety of reasons
> unrelated to the test.
> 
> > +#include <time.h>
> > +#include <support/check.h>
> > +#include <support/xtime.h>
> > +
> > +#define TIMESPEC_SEC_Y2038_OV 0x7FFFFFFF  
> 
> This test will, of course, fail after 2038.
> 
> > +#define FUTURE_TIME (TIMESPEC_SEC_Y2038_OV - 10)  
> 
> Assuming the test takes less than 10 seconds, which is probably OK.
> 
> > +static int
> > +do_test (void)
> > +{
> > +  const struct timespec tv = { FUTURE_TIME, 0};
> > +  struct timespec tv_future, tv_now;
> > +
> > +  tv_now = xclock_now(CLOCK_REALTIME);  
> 
> Current time (before 2038 we hope)...
> 
> > +  xclock_settime(CLOCK_REALTIME, &tv);  
> 
> Set to 2038...
> 
> > +  tv_future = xclock_now(CLOCK_REALTIME);  
> 
> Check "2038" time...
> 
> > +  /* Restore old time value on target machine.  */
> > +  xclock_settime(CLOCK_REALTIME, (const struct timespec*)
> > &tv_now);  
> 
> The cast should not be needed.  Sometimes GCC complains about casting
> structs to different types (although this shouldn't do that) so I tend
> to avoid it when it's not needed.

As fair as I remember, GCC was complaining loudly (and the test was
failed as we compile with -Werror) as clock_settime expects const struct
timespec* pointer argument.

The tv_now cannot be defined as const and hence the required cast.

> 
> > +  if (tv_future.tv_sec < tv.tv_sec)
> > +    FAIL_EXIT1 ("clock_settime set wrong time!\n");  
> 
> If the "2038" time is "before" the "now (hopefully before 2038)" time,
> we failed to set the time to the future... ok.

This is a very simple test indeed.

> 
> > +  return 0;
> > +}
> > +
> > +#include <support/test-driver.c>  
> 
> LGTM aside from those nits.
> 
> Reviewed-by: DJ Delorie <dj@redhat.com>
> 




Best regards,

Lukasz Majewski

--

DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-59 Fax: (+49)-8142-66989-80 Email: lukma@denx.de

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v3 3/3] tst: Add test for clock_settime

[DJ Delorie]

Lukasz Majewski <lukma@denx.de> writes:
> As fair as I remember, GCC was complaining loudly (and the test was
> failed as we compile with -Werror) as clock_settime expects const struct
> timespec* pointer argument.
>
> The tv_now cannot be defined as const and hence the required cast.

GCC should not warn if a non-const value is passed to a function
expecting a const, as long as the rest of the type is the same.  This
type of automatic conversion happens all the time.  Please try this
standalone version of the test case and report what your gcc does with
it (gcc -Wall ...):

#include <time.h>

static inline struct timespec clock_now (clockid_t clock)
{
  struct timespec ts;
  clock_gettime (clock, &ts);
  return ts;
}

#define TIMESPEC_SEC_Y2038_OV 0x7FFFFFFF
#define FUTURE_TIME (TIMESPEC_SEC_Y2038_OV - 10)

int
main (void)
{
  const struct timespec tv = { FUTURE_TIME, 0};
  struct timespec tv_future, tv_now;

  tv_now = clock_now(CLOCK_REALTIME);
  clock_settime(CLOCK_REALTIME, &tv);
  tv_future = clock_now(CLOCK_REALTIME);

  /* Restore old time value on target machine.  */
  clock_settime(CLOCK_REALTIME, &tv_now);

  if (tv_future.tv_sec < tv.tv_sec)
    return 1;

  return 0;
}

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

[DJ Delorie]

Lukasz Majewski <lukma@denx.de> writes:
> +The @code{GLIBC_TEST_ALLOW_TIME_SETTING} is set in the environment in

"The FOO is set" reads clumsy.  How about just "FOO is set" ?  It's OK
to start a sentence with a @code{}.

(In general, you should say "The FOO environment variable" or just "FOO"
but not "The FOO")

> In this case there is no prevention from running those tests in
> parallel,

"In this case nothing prevents those tests from running in parallel,"

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

"provide a proper" (add "a")

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

"One passes the @option{}" (add "the")

> In this case both - setting the

The "-" is not needed here.  Drop the "the"

> +usage="usage: ${progname} [--ssh SSH][--allow-time-setting] HOST COMMAND ..."

Space between "]["

> +If the '--allow-time-setting' flag is present, set
> +GLIBC_TEST_ALLOW_TIME_SETTING on the remote machine to inform that

s/inform/indicate/ perhaps ?

> +FLOCK_PATH="/var/lock/clock_settime"

Should we forcibly set this, or allow the user to override it in case
/var/lock is the wrong directory to use?  I.e.

FLOCK_PATH="${FLOCK_PATH:-/var/lock/clock_settime}"

> +if [ "$settimeallowed" ]; then

For paranoia, this should be something like

	if [ x"$settimeallowed" != x"" ]; then

> +  exec 99<>${FLOCK_PATH}

Check for error

> +  command="export GLIBC_TEST_ALLOW_TIME_SETTING=1 ${command}"

This needs the space before ${command} to be a newline, see the addition
of TIMEOUTFACTOR for an example.  This causes the exports and the
${command} to be separate command lines on the target.

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

[Joseph Myers]

On Tue, 16 Feb 2021, DJ Delorie via Libc-alpha wrote:

> > +if [ "$settimeallowed" ]; then
> 
> For paranoia, this should be something like
> 
> 	if [ x"$settimeallowed" != x"" ]; then

Those are exactly equivalent in any POSIX shell, by the definitions of how 
"test" operates with a single argument, or with three arguments of which 
the second is a binary operator.

-- 
Joseph S. Myers
joseph@codesourcery.com

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

[DJ Delorie]

Joseph Myers <joseph@codesourcery.com> writes:

> On Tue, 16 Feb 2021, DJ Delorie via Libc-alpha wrote:
>
>> > +if [ "$settimeallowed" ]; then
>> 
>> For paranoia, this should be something like
>> 
>> 	if [ x"$settimeallowed" != x"" ]; then
>
> Those are exactly equivalent in any POSIX shell, by the definitions of how 
> "test" operates with a single argument, or with three arguments of which 
> the second is a binary operator.

The paranoia accounts for $settimeallowed expanding to something
starting with "-" and being treated as an option to test.

Since $settimeallowed can be set in the parent's environment when the
clock option is *not* given... paranoia :-)

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

[Joseph Myers]

On Tue, 16 Feb 2021, DJ Delorie via Libc-alpha wrote:

> > Those are exactly equivalent in any POSIX shell, by the definitions of how 
> > "test" operates with a single argument, or with three arguments of which 
> > the second is a binary operator.
> 
> The paranoia accounts for $settimeallowed expanding to something
> starting with "-" and being treated as an option to test.

But "test" has specified semantics for when passed a single argument, 
which never involve treating that argument as an option, only testing 
whether it is nonempty.

-- 
Joseph S. Myers
joseph@codesourcery.com

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

[Zack Weinberg]

On Tue, Feb 16, 2021 at 3:33 PM Joseph Myers <joseph@codesourcery.com>
wrote:

> On Tue, 16 Feb 2021, DJ Delorie via Libc-alpha wrote:
> > The paranoia accounts for $settimeallowed expanding to something
> > starting with "-" and being treated as an option to test.
>
> But "test" has specified semantics for when passed a single argument,
> which never involve treating that argument as an option, only testing
> whether it is nonempty.


Sufficiently old shells get it wrong, though. I’m not sure precisely how
old, but I know Autoconf carefully avoids ever using test with a single
argument for this reason.

zw

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

[Joseph Myers]

On Tue, 16 Feb 2021, Zack Weinberg wrote:

> On Tue, Feb 16, 2021 at 3:33 PM Joseph Myers <joseph@codesourcery.com>
> wrote:
> 
> > On Tue, 16 Feb 2021, DJ Delorie via Libc-alpha wrote:
> > > The paranoia accounts for $settimeallowed expanding to something
> > > starting with "-" and being treated as an option to test.
> >
> > But "test" has specified semantics for when passed a single argument,
> > which never involve treating that argument as an option, only testing
> > whether it is nonempty.
> 
> 
> Sufficiently old shells get it wrong, though. I’m not sure precisely how
> old, but I know Autoconf carefully avoids ever using test with a single
> argument for this reason.

This script uses #!/bin/bash, so there is definitely no concern about the 
shell getting it wrong.

(The rules based on the number of arguments are in the 1992 edition of 
POSIX.2.  Autoconf may well need to be concerned with proprietary systems 
(Solaris?) that had a non-POSIX /bin/sh long after then, but I don't think 
it's any problem in glibc to assume the shell and utilities, on any system 
used to build glibc, support features up to the 2008 edition of POSIX, 
which is still long before the versions of various build tools supported 
for building glibc.)

-- 
Joseph S. Myers
joseph@codesourcery.com

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

[DJ Delorie]

Joseph Myers <joseph@codesourcery.com> writes:
> But "test" has specified semantics for when passed a single argument, 
> which never involve treating that argument as an option, only testing 
> whether it is nonempty.

Perhaps I'm remembering a time when [ $foo ] was popular, and that was
problematic, since you can shell-inject that one.

But reading the test man page, it's not entirely obvious that this is
the case.  My old POSIX spec does explicitly say this, but it also says
that no implementation shall accept any options, so our implementation
is non-conforming anyway... but you REALLY have to "mean it" to get
those:

$ /usr/bin/[ --help
...gives help...

$ [ --help
...gives error...

Anyway, resolved, not important to me anyway.  Back to your regularly
scheduled argument ;-)

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

[Lukasz Majewski]

[-- Attachment #1: Type: text/plain, Size: 3021 bytes --]

Hi DJ,

> Lukasz Majewski <lukma@denx.de> writes:
> > +The @code{GLIBC_TEST_ALLOW_TIME_SETTING} is set in the environment
> > in  
> 
> "The FOO is set" reads clumsy.  How about just "FOO is set" ?  It's OK
> to start a sentence with a @code{}.
> 
> (In general, you should say "The FOO environment variable" or just
> "FOO" but not "The FOO")
> 
> > In this case there is no prevention from running those tests in
> > parallel,  
> 
> "In this case nothing prevents those tests from running in parallel,"
> 
> > so the caller shall assure that those tests
> > +are serialized or provide proper wrapper script for it.  
> 
> "provide a proper" (add "a")
> 
> > +The @code{cross-test-ssh.sh} script is used and one passes
> > +@option{--allow-time-setting} flag.  
> 
> "One passes the @option{}" (add "the")
> 
> > In this case both - setting the  
> 
> The "-" is not needed here.  Drop the "the"
> 
> > +usage="usage: ${progname} [--ssh SSH][--allow-time-setting] HOST
> > COMMAND ..."  
> 
> Space between "]["
> 
> > +If the '--allow-time-setting' flag is present, set
> > +GLIBC_TEST_ALLOW_TIME_SETTING on the remote machine to inform that
> >  
> 
> s/inform/indicate/ perhaps ?
> 

Thanks for above hints. I will fix them in next version of this patch.

> > +FLOCK_PATH="/var/lock/clock_settime"  
> 
> Should we forcibly set this, or allow the user to override it in case
> /var/lock is the wrong directory to use?  I.e.
> 
> FLOCK_PATH="${FLOCK_PATH:-/var/lock/clock_settime}"
> 

During tests it turned out that locking the script on host (according
to flock manual [1]) shall be done on the very beginning of the script.

In our case we need first to parse arguments and if setting time is
allowed, we serialize the execution of the rest of this script.

Considering the above, IMHO it would be better to use flock on target,
as the first command executed by ssh -c "...". 

It looks like a more robust way to ensure serialization when
--allow-time-setting is set.

> > +if [ "$settimeallowed" ]; then  
> 
> For paranoia, this should be something like
> 
> 	if [ x"$settimeallowed" != x"" ]; then
> 

I've followed the idiom used earlier in this script file - for example
if [ "$timeoutfactor" ]

> > +  exec 99<>${FLOCK_PATH}  
> 
> Check for error
> 
> > +  command="export GLIBC_TEST_ALLOW_TIME_SETTING=1 ${command}"  
> 
> This needs the space before ${command} to be a newline, see the
> addition of TIMEOUTFACTOR for an example.  This causes the exports
> and the ${command} to be separate command lines on the target.

Thanks for this hint - it was not obvious from the outset.

> 

Links:

[1] - https://man7.org/linux/man-pages/man1/flock.1.html


Best regards,

Lukasz Majewski

--

DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-59 Fax: (+49)-8142-66989-80 Email: lukma@denx.de

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

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

[DJ Delorie]

Lukasz Majewski <lukma@denx.de> writes:
>> Should we forcibly set this, or allow the user to override it in case
>> /var/lock is the wrong directory to use?  I.e.
>> 
>> FLOCK_PATH="${FLOCK_PATH:-/var/lock/clock_settime}"
>
> During tests it turned out that locking the script on host (according
> to flock manual [1]) shall be done on the very beginning of the script.

I'm not arguing for/against the locking, just saying the *path* to the
lock should be user-overridable.  Security may not allow user-level
locks in /var/lock.

>> > +if [ "$settimeallowed" ]; then  
>> 
>> For paranoia, this should be something like
>> 
>> 	if [ x"$settimeallowed" != x"" ]; then
>> 
>
> I've followed the idiom used earlier in this script file - for example
> if [ "$timeoutfactor" ]

Ok; I'm not strongly worried about this one