Re: [PATCH 2/2] system/vl.c: parse all -accel options

[Paolo Bonzini <pbonzini@redhat.com>]

On Mon, Jul 1, 2024 at 4:34 PM Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

In principle, a Reviewed-by tag is just stating that you don't know of
any issues that would prevent the patch being included. However, as a
frequent participant to the project, your Reviewed-by tag carries some
weight and, to some extent, it is also a statement that you understand
the area being modified.  A Reviewed-by from an experienced
contributor may even imply that you could take the patch in one of
your pull requests. (*) That makes it even more important to
understand the area.

I would expect that anyone with an understanding of command line
parsing would know 1) what -accel kvm -accel tcg does, and 2) what
.merge_lists does; and this would be enough to flag an issue
preventing the patch from being included.

To be clear, I don't expect reviews to be perfect. But in this case
I'm speaking up because the patch is literally a one line declarative
change, and the only way to say "I've reviewed it" is by understanding
the deeper effects of that line.

Also, I think it's fair that the submitter didn't spot the problem;
it's okay to send out broken patches, that's part of the learning
experience. :)

Paolo

(*) as opposed to Acked-by, where your review probably has been more
conceptual than technical, and that you don't really want to take the
patch in a pull request.


Paolo