Re: [PATCH v4 1/9] KVM: selftest: Create KVM selftest runner

[Vipin Sharma <vipinsh@google.com>]

On Wed, Jun 10, 2026 at 06:17:26PM -0700, Ackerley Tng wrote:
> Vipin Sharma <vipinsh@google.com> writes:
> 
> >
> > [...snip...]
> >
> > +def setup_logging():
> > +    class TerminalColorFormatter(logging.Formatter):
> > +        reset = "\033[0m"
> > +        red_bold = "\033[31;1m"
> > +        green = "\033[32m"
> > +        yellow = "\033[33m"
> > +        blue = "\033[34m"
> > +
> > +        COLORS = {
> > +            SelftestStatus.PASSED: green,
> > +            SelftestStatus.NO_RUN: blue,
> > +            SelftestStatus.SKIPPED: yellow,
> > +            SelftestStatus.FAILED: red_bold
> > +        }
> > +
> > +        def __init__(self, fmt=None, datefmt=None):
> > +            super().__init__(fmt, datefmt)
> > +
> > +        def format(self, record):
> > +            return (self.COLORS.get(record.levelno, "") +
> > +                    super().format(record) + self.reset)
> > +
> 
> The commit message above says the printing will be in colors when the
> terminal supports it, but if I'm reading this correctly, the colors will
> always be printed.

I meant if a terminal doesn't have support for printing color output
then it won't be printed in color. Almost all modern terminal supports
these ANSI escape sequence, so it will always be printed. Its kind of
a dumb statement I wrote in the commit log.
> 
> I was expecting something like if the output is piped to some file or
> like "not TTY" then don't print colors.
> 
> Would you consider not printing in color at all? Or adding some kind of
> "turn off all display magic" flag?

Good point about color on/off. I can make following
changes in following priority, starting with the highest:

1. If NO_COLOR enivornment variable is set then don't print color at
   all.
2. If FORCE_COLOR environment variable is set then print color
   (default).
3. If output is not TTY then don't print color.
4. Print color.

> 
> I also saw code for a sticky footer in another patch in this series,
> that's probably not nice for other stuff wrapping this runner.
> 
For sticky update, I can hide it if there is no TTY.

> > +if __name__ == "__main__":
> > +    PYTHON_VERSION = (3, 6)
> > +    if sys.version_info < PYTHON_VERSION:
> > +        print(f"Minimum required python version {PYTHON_VERSION}, found {sys.version}")
> > +        sys.exit(1)
> > +
> > +    sys.exit(main())
> 
> This shouldn't block merge: why not align with the kernel's official
> required python version?
> 

When I first proposed official version was 3.5, it got upgraded to 3.9
in commit 5e25b972a22b ("docs: changes: update Python minimal version")

I was using some APIs which were not present in 3.5 and 3.6 was the
minimum version I was able to run all the features I needed.

It just stayed that version and I never checked to keep it in sync with
the latest python version.

> > +
> > +        run_args = {
> > +            "universal_newlines": True,
> > +            "shell": True,
> > +            "stdout": subprocess.PIPE,
> > +            "stderr": subprocess.PIPE
> > +        }
> > +        proc = subprocess.run(self.command, **run_args)
> > +
> > +        out, err = proc.stdout, proc.stderr
> > +        self.stdout = out.decode("utf-8", "replace") if isinstance(out, bytes) else (out or "")
> > +        self.stderr = err.decode("utf-8", "replace") if isinstance(err, bytes) else (err or "")
> 
> I think it would be useful to capture stdout and stderr in order that it
> was output, so that the output being saved shows everything
> interleaved. I think the order could be useful in debugging.
> 
> I can see benefits in knowing which was on stdout and which was on
> stderr too, so is there some way of having both?
> 

Both approaches are useful, I am not sure how to achieve what you are
asking reliably.

However, I think separate stdout and stderr is more useful in automation/CI
tools. Considering, runner is for the human use I am more inclined on
having combined output of stdout and stderr as you are suggsting.

I will change it to combined output in next version, new filename will
be "out" consisting of both stdout and stderr. If there is ever
need for separate dumps we can revisit it.

One thing we will lose is in current approach, non-zero stderr is signal
of finding errored out runs. Not a big loss as same information can be
grepped using master log file.


> > +
> > +        if proc.returncode == 0:
> > +            self.status = SelftestStatus.PASSED
> > +        elif proc.returncode == 4:
> > +            self.status = SelftestStatus.SKIPPED
> > +        else:
> > +            self.status = SelftestStatus.FAILED
> 
> Using this class-based pattern requires us to do
> Selftest(test_path).run(). Would you consider using a function-based
> pattern, like run_selftest(test_path) instead?
> 

I find current class model provides an easy way to group data in
hierarchy. For example, test_runner has list of selftests which it needs to
execute. Selftests have their own test command, output data, result
status, etc.

Both are suitable to implement runner. But I am little hesitant to
change it now considering it is already written. If you can provide
benefits of using functions approach here then I am open to
rewrite.

> >
> > [...snip...]
> >

-- 
kvm-riscv mailing list
kvm-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kvm-riscv