Re: [LTP] [PATCH v2 0/3] lib: LTP_DEBUG cleanup

[Petr Vorel <pvorel@suse.cz>]

> > Hi Petr,

> > Patches 1/3 and 2/3 look good. Patch 3/3 has a regression
> > and a couple of nits.

> > > [PATCH 3/3]
> > > --- a/lib/tst_test.c
> > > +++ b/lib/tst_test.c
> > > +	{"D::", "-D[1,2]  Prints debug information (can be overwriten by LTP_DEBUG"},

> > Typo: "overwriten" -> "overwritten". Also missing closing ')'.

> > [...]

> > > 	case 'D':
> > > +		if (!getenv("LTP_DEBUG")) {

> > This undoes the empty-string fix from patch 1/3. When
> > LTP_DEBUG="" is set, getenv() returns "" (non-NULL), so -D is
> > skipped here. But in do_setup() the tdebug_env[0] != '\0'
> > check also skips it. Net result: LTP_DEBUG= ./test -D2
> > silently produces no debug output.

> Good point, thanks!

Actually, IMHO you're wrong. 1st commit treat empty LTP_DEBUG
as if it were not set (FYI LTP_DEBUG= and LTP_DEBUG="" are the same in shell -
variable is defined but empty).

And this functionality is kept here deliberately. LTP_DEBUG is always stronger,
that's why it ignores -D.

And why should be variable stronger? Can be set by user of any software. That's
actually the goal of environment variables. So we can run test via kirk/openQA
with LTP_DEBUG=2 to have different behavior without modifying software. But we
can also disable -D settings that way (imagine somebody puts -D1 into their
runner, user of that runner should be able to disable it). I'll describe it in
the commit message to be obvious.

Kind regards,
Petr

> nit: better to discuss in patch itself, discussion on cover letter is somehow
> hidden in patchwork.

> Kind regards,
> Petr

> > Should be something like:

> > 	char *env = getenv("LTP_DEBUG");
> > 	if (!env || !env[0]) {

> > Regards,

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