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

["Philippe Mathieu-Daudé" <philmd@linaro.org>]

On 1/7/24 18:43, Paolo Bonzini wrote:
> 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.

I admit I haven't reviewed what .merge_lists does but went to look
at its use cases (I see 'git grep -wW merge_lists' in my history)
and mis-read:

util/keyval.c-370- * - lists are concatenated
util/keyval.c-371- *
util/keyval.c-372- * - dictionaries are merged recursively
util/keyval.c-373- *
util/keyval.c-374- * - for scalar values, @merged wins
util/keyval.c-375- *
util/keyval.c-376- * In case an error is reported, @dest may already 
have been modified.
util/keyval.c-377- *
util/keyval.c-378- * This function can be used to implement semantics 
analogous to QemuOpts's
util/keyval.c:379: * .merge_lists = true case, or to implement -set for 
options backed by QDicts.

which made me confident enough with the patch description:

  > and now it's safe to activate 'merge_lists' for 'qemu_accel_opts'.
  > This will merge all accel options in the same list

OTOH I wasn't clear about the first patch and knew this one depends
on it, so if the first is wrong, this one is automatically discarded.

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

Don't blame the review but the reviewer :) Reviews aim to be
perfect, unfortunately the human beings sending them aren't
(at least I am not, as I just proved).

Thankfully maintainers are gatekeepers on their areas and can
catch issues like that. I duly noted "my Reviewed-by tag carries
some weigh" and could confuse other maintainers, and will think
it twice before posting it on topics I'm unsure. Thanks for
taking the time to warn me.

Regards,

Phil.

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