Re: [Qemu-devel] [kvm-unit-tests PATCH v5 1/2] run_tests: put logs into per-test file

[Andrew Jones]

On Wed, Jan 11, 2017 at 06:12:44PM +0800, Peter Xu wrote:
> On Wed, Jan 11, 2017 at 10:51:12AM +0100, Andrew Jones wrote:
> > On Wed, Jan 11, 2017 at 10:06:07AM +0100, Andrew Jones wrote:
> > > On Wed, Jan 11, 2017 at 01:29:34PM +0800, Peter Xu wrote:
> > > > We were using test.log before to keep all the test logs. This patch
> > > > creates one log file per test case under logs/ directory with name
> > > > "TESTNAME.log". Meanwhile, we will keep the last time log into
> > > > logs.old/.
> > > > 
> > > > Renaming scripts/functions.bash into scripts/common.bash to store some
> > > > more global variables.
> > > > 
> > > > Signed-off-by: Peter Xu <address@hidden>
> > > > ---
> > > >  .gitignore                              |  3 ++-
> > > >  Makefile                                |  5 ++---
> > > >  run_tests.sh                            | 18 +++++++++++-------
> > > >  scripts/{functions.bash => common.bash} | 13 +++++++++++--
> > > >  scripts/mkstandalone.sh                 |  2 +-
> > > >  5 files changed, 27 insertions(+), 14 deletions(-)
> > > >  rename scripts/{functions.bash => common.bash} (75%)
> > > > 
> > > > diff --git a/.gitignore b/.gitignore
> > > > index 3155418..2213b9b 100644
> > > > --- a/.gitignore
> > > > +++ b/.gitignore
> > > > @@ -12,7 +12,8 @@ cscope.*
> > > >  /lib/asm
> > > >  /config.mak
> > > >  /*-run
> > > > -/test.log
> > > >  /msr.out
> > > >  /tests
> > > >  /build-head
> > > > +/logs/
> > > > +/logs.old/
> > > 
> > > We should send a cleanup patch fixing the other dirs too someday,
> > > patches, /lib/asm, and /tests
> > > 
> > > > diff --git a/Makefile b/Makefile
> > > > index a32333b..844bacc 100644
> > > > --- a/Makefile
> > > > +++ b/Makefile
> > > > @@ -94,9 +94,8 @@ libfdt_clean:
> > > >         $(LIBFDT_objdir)/.*.d
> > > >  
> > > >  distclean: clean libfdt_clean
> > > > -       $(RM) lib/asm config.mak $(TEST_DIR)-run test.log msr.out 
> > > > cscope.* \
> > > > -             build-head
> > > > -       $(RM) -r tests
> > > > +       $(RM) lib/asm config.mak $(TEST_DIR)-run msr.out cscope.* 
> > > > build-head
> > > > +       $(RM) -r tests logs logs.old
> > > >  
> > > >  cscope: cscope_dirs = lib lib/libfdt lib/linux $(TEST_DIR) 
> > > > $(ARCH_LIBDIRS) lib/asm-generic
> > > >  cscope:
> > > > diff --git a/run_tests.sh b/run_tests.sh
> > > > index 254129d..b6a1059 100755
> > > > --- a/run_tests.sh
> > > > +++ b/run_tests.sh
> > > > @@ -7,7 +7,7 @@ if [ ! -f config.mak ]; then
> > > >      exit 1
> > > >  fi
> > > >  source config.mak
> > > > -source scripts/functions.bash
> > > > +source scripts/common.bash
> > > >  
> > > >  function usage()
> > > >  {
> > > > @@ -46,17 +46,21 @@ while getopts "g:hv" opt; do
> > > >      esac
> > > >  done
> > > >  
> > > > -RUNTIME_log_stderr () { cat >> test.log; }
> > > > +# RUNTIME_log_file will be configured later
> > > > +RUNTIME_log_stderr () { cat >> $RUNTIME_log_file; }
> > > >  RUNTIME_log_stdout () {
> > > >      if [ "$PRETTY_PRINT_STACKS" = "yes" ]; then
> > > > -        ./scripts/pretty_print_stacks.py $1 >> test.log
> > > > +        ./scripts/pretty_print_stacks.py $1 >> $RUNTIME_log_file
> > > >      else
> > > > -        cat >> test.log
> > > > +        cat >> $RUNTIME_log_file
> > > >      fi
> > > >  }
> > > >  
> > > > -
> > > >  config=$TEST_DIR/unittests.cfg
> > > > -rm -f test.log
> > > > -printf "BUILD_HEAD=$(cat build-head)\n\n" > test.log
> > > > +
> > > > +rm -rf $unittest_log_dir.old
> > > > +mv $unittest_log_dir $unittest_log_dir.old
> > 
> > Actually this mv can fail, but we definitely wouldn't want to error-out
> > like v4 did, we want to ignore it. The error is that /logs doesn't exist,
> > as it won't the first time run_tests.sh is run. So, we need to redirect
> > stderr to /dev/null on this mv to avoid the message
> > "mv: cannot stat 'logs': No such file or directory"
> > on that first run.
> 
> Hmm, yes.
> 
> After a second thought, I think the mv can still fail even if this is
> not the first time we run the script - e.g. if the directory belongs
> to user1 but user2 runs run_tests.sh under it (while user2 don't have
> write permission on this directory). So I think we still need some way
> to stop the script if we detected that we might encounter issue.

I'm not really concerned about the user1/user2 scenario. The general use
of kvm-unit-tests is to build and run, meaning the user must have write
access to the dir in order to compile. But, ignoring the errors, as I
suggested, probably isn't a great idea either...

> 
> We can do explicit check before the mv, making sure that the directory
> is there.
> 
> So, how about this:
> 
>     rm -rf $unittest_log_dir.old || err "Failed remove old logs"
>     if [[ -d $unittest_log_dir ]]; then

Only [ ... ] for tests like these

>         mv $unittest_log_dir $unittest_log_dir.old ||
>             err "Failed backup logs"
>     fi
>     mkdir $unittest_log_dir || err "Failed to create log dir"
> 
> And define err() in common.bash:
> 
>     function err()
>     {
>         echo "$@"
>         exit 1
>     }

The above is mostly just translating rm/mv/mkdir stderr messages to
new messages. We can do it much more simply like

 rm -rf logs.old
 [ -d logs ] && mv logs logs.old
 mkdir logs || exit 2

Thanks,
drew