Re: [Qemu-devel] [PATCH v2] target/i386: Fixes to the check missing features routine

[Wainer dos Santos Moschetta <wainersm@redhat.com>]

On 12/11/2018 03:15 PM, Eric Blake wrote:
> On 12/11/18 10:28 AM, Wainer dos Santos Moschetta wrote:
>> The x86_cpu_class_check_missing_features() returns a list
>> of unavailable features compared to the host CPU. Currently it may
>> return empty strings for unnamed features as well as duplicated
>> names.
>>
>> For example, the qmp "query-cpu-definitions" below shows one empty
>> string and repeated "mpx" entries:
>>
>
>> Signed-off-by: Wainer dos Santos Moschetta <wainersm@redhat.com>
>> Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>
>> Reviewed-by: Eric Blake <eblake@redhat.com>
>
> Careful. While I spotted typos in v1,...
>
>> Reviewed-by: Caio Carrara <ccarrara@redhat.com>
>> ---
>> v2:
>>   * Fixed typos. [eblake]
>
> ...and you indeed addressed them, me pointing out typos does not imply 
> that I reviewed the patch for correctness.  In fact, I specifically 
> did NOT give my R-by: tag to v1, because I'm not (yet?) familiar 
> enough with the tests/acceptance/ framework to state that I have fully 
> reviewed the patch for correctness; instead, I'm comfortable relying 
> on the reviews of others (and I am again intentionally not giving R-by 
> to v2).
>
> Also, when posting a v2, you should include the R-by actually given to 
> v1 only if the patch is roughly the same as the original.  Fixing 
> minor issues that a reviewer pointed out, or doing obvious rebasing to 
> changes applied earlier in the series or on upstream git, but where 
> the algorithm of the patch itself did not change, is okay for keeping 
> R-b (so if I _had_ given R-b, and your spelling changes were the only 
> difference, then keeping my R-b would make sense); but where the patch 
> is fundamentally different, such as:
>
>>  * Removed unwanted manual test case. [ccarrara, ehabkost]
>>  * Not passing 'accel=kvm' on test's VM. [ehabkost]
>
> then omitting ALL R-by tags, in order to ensure that reviewers check 
> that the new patch is still correct, is a wiser course of action.  
> Yes, this is more of a rule of thumb, and there are cases where 
> keeping or dropping R-b is more of an art form than an exact science; 
> but hopefully this helps you understand how the tag can be useful for 
> iterative reviews.
>

Hi Eric,

Yes, it helped a lot, thanks. And I apologize for my mistake, I'm gonna 
send a v3 fixing it.

Another doubt that I have: is it advisable to CC everyone that reviewed 
(with or without R-by) the previous version of my patch?

Thanks!

- Wainer