Re: [LTP] [PATCH 2/6] tst_env.sh: Backport common functions from tst_test.sh

[Cyril Hrubis <chrubis@suse.cz>]

Hi!
> > SAFE instead so that the name is closer to the SAFE_XXX macros in C.
> 
> Makes sense. I was even thinking on SAFE_CMD, but we use tst_rod.c, not
> tst_cmd.c. OTOH it was obvious that ROD uses tst_rod.c, should it be also
> renamed? What ROD means anyway? run or dye?

Run or die (we are not coloring anything).

And yes it would make sense to rename tst_rod.c to tst_safe_cmd.c or
similar.

> > > +_tst_expect_pass()
> > > +{
> > > +	local fnc="$1"
> > > +	shift
> > > +
> > > +	tst_rod "$@"
> 
> > If I remmeber correctly the whole reason why we introduced tst_rod.c was
> > that passing the $@ like this causes the $@ to be evaluated twice and
> > produces unexpected results.
> 
> FYI code is copy pasted from tst_test.sh. What do you want me to change?

I've spend a couple of minutes figuring out why we originaly implemented
ROD_BASE() in C.

The problem is that single quotes gets removed as soon as we pass the
parameter.

E.g.: ROD awk '{ print $1 }'

Once we pass that to a shell function the '' disappear, they managed to
keep the { print $1 } to be a single parameter in the list but they are
not there anymore.

And since we were manipulating the parameter list we got, we failed to
reconstruct it correctly in shell since in the next step the { print $1 }
breaks into four separate strings.

As long as we pass the $@ as a whole (even after shift), it does work
since the boundaries of parameters in $@ stay exactly as they were.

TL;DR; The code you copy&pasted is correct, I only misremembered what
was the problem back then.

> > I'm not sure that adding the PASS_BRK and FAIL_BRK is a good idea. I
> > would stick to simple EXPECT_PASS and EXPECT_FAIL. And maybe we can
> > export TST_PASS variable as we do in C to match the API. I think that
> > the closer the C and shell API are the better.
> 
> C API usually returns on if (!TST_PASS). I don't like it either, but it cannot
> be shortened much as we don't use functions but macros.
> 
> But changing the code in the shell API does not look to me an improvement:
> 
> -EXPECT_PASS_BRK ping$TST_IPV6 -c1 -I $lhost $rhost \>/dev/null
> +EXPECT_PASS_BRK ping$TST_IPV6 -c1 -I $lhost $rhost \>/dev/null
> +[ "$TST_PASS" = 1 ] || tst_brk "Quit due the above error"

Let's keep it then.

> But OTOH maybe route-change-*.sh tests even don't need to quit.
> And it's a question if isofs.sh does (and whether we should even keep isofs.sh).

For the isofs I would probably generate the iso filesystems in advance
and store them in git so that we don't have to re-generate them during
the test run. Other than that mounting an iso filesystem over loopback
and checking that the files are there (which the test does not do) is
valid kernel test. But that is a different problem.

-- 
Cyril Hrubis
chrubis@suse.cz

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp