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