Re: [PATCH v8 1/2] ima-evm-utils: Add some tests for evmctl

[Vitaly Chikunov <vt@altlinux.org>]

Mimi,

On Tue, Mar 31, 2020 at 10:25:24AM -0400, Mimi Zohar wrote:
> > +# For hard errors
> > +red_always() {
> > +  echo $@ $RED
> 
> A few functions - "red_always", "red_if_failure", "color_restore" -
>  use "$@", but none of the function callers pass any parameters.  Is
> there a reason for the "$@" or just something left over from
> debugging?

It was to pass `-n` I think, but it is never needed in the end.

> > +  if [ "$chash" ] && [ "$chash" != "$hash" ]; then
> > +    red_always
> 
> Only when "ima_hash.test" is invoked directly, the output is colored
> red.  Really confusing.

Non-TTY output is never colored to not clutter log files.
But logic is to color the errors in red.

So it thought as 'always red', _when_ there is colored output (TTY).

And it's "always" in contract to "red if failure" - which make text
red only when the test is expected to pass (thus, this is real error
condition), when the test is expected to fail there is no point to
color it red, because it's not real error (to not confuse user).

Because it is unconditional (in that sense) is it named "red always".

I can rename it to something like `color_red'. And rename
`red_if_failure' to `color_red_on_failure'.

> Nice!  The code is very concisely written.
> 
> Reviewing this patch would be a lot easier, if it was broken up into
> smaller pieces.  For example, and this is only an example, the initial
> patch could define the base ima_hash.test, a subsequent patch could
> add coloring for the base ima_hash.test, another patch could introduce
> "make check" and add its coloring.  There's all sorts of ways to break
> up this patch to simplify review.

This would make following commits to change code which is already
committed in previous commits. This would make editing code extra hard.
Especially, when tests was reworked a lot.

Also, I don't think splitting coloring into separate patch improves
review. Instead, we can just remember the rule that (real) errors are 
going to be printed in red.

For example, if we prefix every error message with word "ERROR:" - why
it would be easier to review if we split adding this prefix to every
message in a separate commit?

Red color, similarly to uppercase "ERROR", just improves visibility of
errors. (Which is useful, because there is really a lot of tests).

Thanks,

> 
> Mimi