Message-Id: <20170422072641.e1805c90b0dc13b9de3db276@kernel.org>
Date:   Sat, 22 Apr 2017 07:26:41 +0900
From:   Masami Hiramatsu <mhiramat@...nel.org>
To:     Steven Rostedt <rostedt@...dmis.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 09:53:34 -0400
Steven Rostedt <rostedt@...dmis.org> wrote:

> 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.

BTW, would you need "testcases only for instance" or
"testcase for top-level and instance" ? I thought latter...

> > > > 
> > > > 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.

OK, that's better.

> 
>  # 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.

My idea is using INSTANCE_DIR not only for instance name, but also
for a flag which indicates "instance test is running, need to be removed".


> > > >    [ $? -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.

I see.

> 
> > 
> > > >    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?

Yes, that's good to me :)

> 
> > 
> > > > +    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.

As I said above, INSTANCE_DIR can be a test flag too, and that
ensures we've actually made it. (Just for make sure there is)
BTW, maybe we can use mktemp if you'd like avoid name conflict.

Thank you,

> 
> > 
> > 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>  
> > 
> > 
> 


-- 
Masami Hiramatsu <mhiramat@...nel.org>

Powered by blists - more mailing lists