linux-kernel - Re: [RFC][PATCH] selftests: ftrace: Allow some tests to be run in a tracing instance

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20170421095334.6b1c1c31@gandalf.local.home>
Date:   Fri, 21 Apr 2017 09:53:34 -0400
From:   Steven Rostedt <rostedt@...dmis.org>
To:     Masami Hiramatsu <mhiramat@...nel.org>
Cc:     LKML <linux-kernel@...r.kernel.org>, Shuah Khan <shuah@...nel.org>,
        Namhyung Kim <namhyung@...nel.org>
Subject: Re: [RFC][PATCH] selftests: ftrace: Allow some tests to be run in a
 tracing instance

On Fri, 21 Apr 2017 19:00:25 +0900
Masami Hiramatsu <mhiramat@...nel.org> wrote:

> On Fri, 21 Apr 2017 18:36:17 +0900
> Masami Hiramatsu <mhiramat@...nel.org> wrote:
> 
> > On Thu, 20 Apr 2017 13:30:34 -0400
> > Steven Rostedt <rostedt@...dmis.org> wrote:  
> > > 
> > > An tracing instance has several of the same capabilities as the top level
> > > instance, but may be implemented slightly different. Instead of just writing
> > > tests that duplicat the same test cases of the top level instance, allow a
> > > test to be written for both the top level as well as for an instance.
> > > 
> > > If a test case can be run in both the top level as well as in an tracing
> > > instance directory, then it should have a ".itc" extension instead of just
> > > the ".tc" extension.  
> > 
> > Hm, I'd rather like to have a tag for each script instead of
> > using the extension. E.g.

OK, I'll update. I just finished testing all this too :-/

> > 
> > #!/bin/sh
> > # description: Kprobe dynamic event - busy event check
> > # test: instance  

Hmm, what about "flags:" instead. As a way to denote what this test has.

 # flags: instance

> 
> BTW, I also have some comments, see below,
> 
> [...]
> > > +INSTANCE_DIR="."
> > >  __run_test() { # testfile
> > >    # setup PID and PPID, $$ is not updated.
> > > -  (cd $TRACING_DIR; read PID _ < /proc/self/stat; set -e; set -x; initialize_ftrace; . $1)
> > > +  (cd $TRACING_DIR; read PID _ < /proc/self/stat; set -e; set -x; initialize_ftrace; cd $INSTANCE_DIR; . $1)  
> 
> You can just change "cd $TRACING_DIR;" with "cd $TRACING_DIR/$INSTANCE_DIR;"
> Then, the default value of INSTANCE_DIR can be "".

I'll look into doing this. I originally had an option to run just
instance tests, and overriding TRACING_DIR wasn't an option at the time.

> 
> > >    [ $? -ne 0 ] && kill -s $SIG_FAIL $SIG_PID
> > >  }
> > >  
> > >  # Run one test case
> > > -run_test() { # testfile
> > > +run_test() { # [-i] testfile
> > > +  local makeinstance=0
> > > +  if [ $1 == "-i" ]; then
> > > +    makeinstance=1
> > > +    shift
> > > +  fi  
> 
> No, don't parse argument in the function. You can just define
> MAKEINSTANCE=1 global variable before calling this.

Again, this was due to the ftracetest option. I'll update.

> 
> > >    local testname=`basename $1`
> > >    local testlog=`mktemp $LOG_DIR/${testname}-log.XXXXXX`
> > >    export TMPDIR=`mktemp -d /tmp/ftracetest-dir.XXXXXX`
> > >    testcase $1  
> 
> Anyway, if we use a "tag" in the script header, we can get it
> in this testcase() and update TEST_TAGS global variable there.

TEST_TAGS? Or you mean something like the way to get the description.

> 
> > > +  if [ $makeinstance -eq 1 ]; then  
> 
> and check whether "instance" tag in $TEST_TAGS here.

TEST_FLAGS?

> 
> > > +    INSTANCE_DIR=$TRACING_DIR/instances/${testname}_test
> > > +    mkdir $INSTANCE_DIR
> > > +  fi  
> 
> 
> > >    echo "execute: "$1 > $testlog
> > >    SIG_RESULT=0
> > >    if [ $VERBOSE -ge 2 ]; then
> > > @@ -260,17 +274,36 @@ run_test() { # testfile
> > >      [ $VERBOSE -ge 1 ] && catlog $testlog
> > >      TOTAL_RESULT=1
> > >    fi
> > > +  if [ $makeinstance -eq 1 ]; then  
> 
> Here also we can just check the "$INSTANCE_DIR" is not empty.

Why? It never will be empty. It's a pseudo dir and files can not be
created or destroyed in it by the user. It should be created by the
test, and destroyed by the test. Hopefully it's a unique name. The name
is currently something like instances/func_event_triggers.tc_test
It's "instances/${testname}_test". I could even add a $$ just to be
more random here.

> 
> Thank you,

Thanks for you review! I'll have a new patch out soon.

-- Steve

> 
> > > +    rmdir $INSTANCE_DIR
> > > +    INSTANCE_DIR="."
> > > +  fi
> > >    rm -rf $TMPDIR
> > >  }
> > >  
> > >  # load in the helper functions
> > >  . $TEST_DIR/functions
> > >  
> > > +RUN_INSTANCES=0
> > > +
> > >  # Main loop
> > >  for t in $TEST_CASES; do
> > > +  if [ "${t/*./}" == "itc" ]; then
> > > +    RUN_INSTANCES=1
> > > +  fi
> > >    run_test $t
> > >  done
> > >  
> > > +if [ $RUN_INSTANCES -eq 1 ]; then
> > > +    echo "Running tests in an tracing instance:"
> > > +    for t in $TEST_CASES; do
> > > +	if [ "${t/*./}" != "itc" ]; then
> > > +	    continue
> > > +	fi
> > > +	run_test -i $t
> > > +    done
> > > +fi
> > > +
> > >  prlog ""
> > >  prlog "# of passed: " `echo $PASSED_CASES | wc -w`
> > >  prlog "# of failed: " `echo $FAILED_CASES | wc -w`
> > > -- 
> > > 2.9.3
> > >   
> > 
> > 
> > -- 
> > Masami Hiramatsu <mhiramat@...nel.org>  
> 
>