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

[Alex Bennée]

Paolo Bonzini <pbonzini@redhat.com> writes:

> 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 think you are attaching a little too much weight to a r-b tag here as
no one was suggesting the patch go in via a different tree. Ultimately
the maintainer can always NACK a reviewed patch. 

> 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.

Maybe more useful would be re-wording the comment:

  /* Merge multiple uses of option into a single list? */

to be explicit about its behaviour. 

> 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.

I think that's a fairly subjective requirement for something that
generally we can always use more of. I encourage people to review all
around the code base to get familiar with new sub-systems. I don't think
we should be dissuading people from exploring outside their silos. That
simple one liners can trip people up says more about the code than the
reviewer.

I sympathise with Philippe here who's current brief takes him around our
large and interconnected code base more than most.

>
> 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

-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro