Re: [LTP] [PATCH v3 2/3] lib: Add support for TDEBUG tst_res() flag

[Petr Vorel <pvorel@suse.cz>]

> Hi!
> > diff --git a/lib/tst_res.c b/lib/tst_res.c
> > index e0896eb05..5125001f7 100644
> > --- a/lib/tst_res.c
> > +++ b/lib/tst_res.c
> > @@ -157,6 +157,7 @@ const char *strttype(int ttype)
> >  		PAIR(TCONF)
> >  		PAIR(TWARN)
> >  		PAIR(TINFO)
> > +		PAIR(TDEBUG)
> >  	};

> >  	PAIR_LOOKUP(ttype_pairs, TTYPE_RESULT(ttype));
> > @@ -174,8 +175,10 @@ static void tst_res__(const char *file, const int lineno, int ttype,
> >  	int len = 0;
> >  	int ttype_result = TTYPE_RESULT(ttype);

> > -	if (file && (ttype_result != TPASS && ttype_result != TINFO))
> > +	if (file && (ttype_result != TPASS && ttype_result != TINFO &&
> > +		     ttype_result != TDEBUG))
> >  		len = sprintf(tmesg, "%s:%d: ", file, lineno);
> > +
> >  	EXPAND_VAR_ARGS(tmesg + len, arg_fmt, USERMESG - len);

> >  	/*
> > @@ -193,7 +196,7 @@ static void tst_res__(const char *file, const int lineno, int ttype,
> >  	 * Set the test case number and print the results, depending on the
> >  	 * display type.
> >  	 */
> > -	if (ttype_result == TWARN || ttype_result == TINFO) {
> > +	if (ttype_result == TWARN || ttype_result == TINFO || ttype_result == TDEBUG) {
> >  		tst_print(TCID, 0, ttype, tmesg);
> >  	} else {
> >  		if (tst_count < 0)
> > @@ -411,7 +414,7 @@ void tst_exit(void)

> >  	tst_old_flush();

> > -	T_exitval &= ~TINFO;
> > +	T_exitval &= ~(TINFO | TDEBUG);

> >  	if (T_exitval == TCONF && passed_cnt)
> >  		T_exitval &= ~TCONF;

> I woudln't add it to the old library. None of the old tests uses it so
> there is no reason to export it there.

> I suppose that we just abort if we get this flag in old library:

> diff --git a/lib/tst_res.c b/lib/tst_res.c
> index e0896eb05..e87918ed1 100644
> --- a/lib/tst_res.c
> +++ b/lib/tst_res.c
> @@ -174,6 +174,11 @@ static void tst_res__(const char *file, const int lineno, int ttype,
>         int len = 0;
>         int ttype_result = TTYPE_RESULT(ttype);

> +       if (ttype_result == TDEBUG) {
> +               printf("%s: %i: TDEBUG is not supported\n", __func__, __LINE__);
> +               abort();
> +       }
> +

> What do you think?

Makes sense, thanks!

> > diff --git a/lib/tst_test.c b/lib/tst_test.c
> > index c2f8f503f..f5c87ed9e 100644
> > --- a/lib/tst_test.c
> > +++ b/lib/tst_test.c
> > @@ -60,6 +60,7 @@ static pid_t main_pid, lib_pid;
> >  static int mntpoint_mounted;
> >  static int ovl_mounted;
> >  static struct timespec tst_start_time; /* valid only for test pid */
> > +static int tdebug;

> >  struct results {
> >  	int passed;
> > @@ -224,6 +225,9 @@ static void print_result(const char *file, const int lineno, int ttype,
> >  	case TINFO:
> >  		res = "TINFO";
> >  	break;
> > +	case TDEBUG:
> > +		res = "TDEBUG";
> > +	break;
> >  	default:
> >  		tst_brk(TBROK, "Invalid ttype value %i", ttype);
> >  		abort();
> > @@ -352,6 +356,9 @@ void tst_res_(const char *file, const int lineno, int ttype,
> >  {
> >  	va_list va;

> > +	if (ttype == TDEBUG && !tdebug)
> > +		return;
> > +
> >  	va_start(va, fmt);
> >  	tst_vres_(file, lineno, ttype, fmt, va);
> >  	va_end(va);
> > @@ -511,6 +518,7 @@ static struct option {
> >  	{"h",  "-h       Prints this help"},
> >  	{"i:", "-i n     Execute test n times"},
> >  	{"I:", "-I x     Execute test for n seconds"},
> > +	{"v",  "-v       Prints debug information"},

> Maybe this should now be called -d since we call it TDEBUG and that will
> avoid having a bit confusing parameters with both -v and -V used.

Yes, I didn't like the flag name does not correspond to the option.
I previously wrote that more tests use -d for something else:

$ git grep '"d:"' $(git grep -l tst_option)
testcases/kernel/fs/read_all/read_all.c:                {"d:", &root_dir,
testcases/kernel/io/ltp-aiodio/aio-stress.c:            { "d:", &str_depth, "Number of pending aio requests for each file (default 64)" },
testcases/network/netstress/netstress.c:                {"d:", &rpath, "Path to file where result is saved"},
testcases/network/nfs/nfs_stress/nfs05_make_tree.c:     {"d:", &d_arg, "Number of subdirs to generate, default: 100"},
testcases/network/stress/route/route-change-netlink.c:          {"d:", &d_opt, "Interface to work on (mandatory)"},

Hopefully all could be changed to -D in separate commits.

$ git grep '"d"' $(git grep -l tst_option)
Also some other tests already use it as debug, this will be trivial to convert
to delete it and use TDEBUG (will have to be in the same commit which introduces
the flag or I would have to remove the debug and print always in commit before
introducing the flag):

testcases/kernel/mem/mmapstress/mmapstress01.c:         {"d", &debug, "Enable debug output"},
testcases/kernel/security/integrity/ima/src/ima_boot_aggregate.c:       {"d", &debug, "Enable debug"},
testcases/kernel/syscalls/mq_notify/mq_notify01.c:              {"d", &str_debug, "Print debug messages"},

Also, shell API uses -d

$ git grep "TST_OPTS=.*d"
testcases/network/virt/geneve01.sh:TST_OPTS="hi:d:"
testcases/network/virt/geneve02.sh:TST_OPTS="hi:d:"
testcases/network/virt/vxlan01.sh:TST_OPTS="hi:d:"
testcases/network/virt/vxlan02.sh:TST_OPTS="hi:d:"
testcases/network/virt/vxlan03.sh:TST_OPTS="hi:d:"
testcases/network/virt/vxlan04.sh:TST_OPTS="hi:d:"

=> all for VxLAN destination address in testcases/network/virt/virt_lib.sh
Again, it could be changed to -D in separate commits.

Alternative to all this work would be to have TVERBOSE. But I'm ok to do the job
if you like TDEBUG.

> >  	{"V",  "-V       Prints LTP version"},
> >  	{"C:", "-C ARG   Run child process with ARG arguments (used internally)"},
> >  };
> > @@ -692,6 +700,10 @@ static void parse_opts(int argc, char *argv[])
> >  			else
> >  				duration = SAFE_STRTOF(optarg, 0.1, HUGE_VALF);
> >  		break;
> > +		case 'v':
> > +			tdebug = 1;
> > +			tst_res(TINFO, "Run with -v, printing debug info");
>                                            ^
> 					   Maybe just "Enabling debug info"

+1

If none objects, I hopefully send patch this week.

Kind regards,
Petr

> > +		break;
> >  		case 'V':
> >  			fprintf(stderr, "LTP version: " LTP_VERSION "\n");
> >  			exit(0);
> > -- 
> > 2.43.0

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