Re: [XEN PATCH 3/6] x86/vm_event: add missing include

[Jan Beulich <jbeulich@xxxxxxxx>]

On 14.08.2023 15:06, Julien Grall wrote:
> Hi Nicola,
> 
> On 14/08/2023 13:53, Nicola Vetrini wrote:
>> On 14/08/2023 13:01, Julien Grall wrote:
>>> Hi,
>>>
>>> On 14/08/2023 11:33, Nicola Vetrini wrote:
>>>> On 14/08/2023 09:39, Jan Beulich wrote:
>>>>> On 11.08.2023 09:19, Nicola Vetrini wrote:
>>>>>> The missing header included by this patch provides declarations for
>>>>>> the functions 
>>>>>> 'vm_event_{fill_regs,set_registers,monitor_next_interrupt}'
>>>>>> that are defined in the source file. This also resolves violations
>>>>>> of MISRA C:2012 Rule 8.4.
>>>>>>
>>>>>> Signed-off-by: Nicola Vetrini <nicola.vetrini@xxxxxxxxxxx>
>>>>>> Fixes: adc75eba8b15 ("x86/vm_event: consolidate hvm_event_fill_regs 
>>>>>> and p2m_vm_event_fill_regs")
>>>>>> Fixes: 975efd3baa8d ("introduce VM_EVENT_FLAG_SET_REGISTERS")
>>>>>> Fixes: 9864841914c2 ("x86/vm_event: add support for 
>>>>>> VM_EVENT_REASON_INTERRUPT")
>>>>>
>>>>> It's hard to see how it can be three commit here. The oldest one is at
>>>>> fault, I would say.
>>>>
>>>> Since the patch is concerned with more than one function then in a 
>>>> sense I agree
>>>> with you (the headers should have been included in the proper way the 
>>>> first time around), but
>>>> then more definitions have been added by adc75eba8b15 and 
>>>> 9864841914c2, and these should have
>>>> triggered a refactoring too. I can leave just 975efd3baa8d in the 
>>>> Fixes if the preferred way is to list just the first problematic 
>>>> commit (perhaps with a little explanation after --- ).
>>>
>>> To be honest, I don't exactly see the value of using Fixes tag for
>>> those patches. I agree they are technically issues, but they are
>>> unlikely going to be backported.
>>>
>>> So if it were me, I would just drop all the Fixes tags for missing
>>> includes unless there is an actual bug associated
>>> with them (e.g. a caller was miscalling the function because the
>>> prototype was incorrect).
>>>
>>> Cheers,
>>
>> Adding those tags for this kind of situation was requested on the 
>> previous discussion [1],
>> so in this series I kept the same strategy (though probably here I put 
>> too many of them).
> 
> I disagree with the suggestion made. They are just noise for this sort 
> of patch and require extra digging (I assume you spent 10-15min to 
> figure out the multiple fixes) for a limited benefits (I don't expect 
> anyone to backport the patches).

While I agree that Fixes: is primarily for marking of backporting
candidates, I don't think this is its exclusive purpose. As far as I'm
concerned, it also aids review in the specific cases here (this isn't
a commonly occurring aspect, though, I agree). Yet the primary reason
I asked for them to be added is because each of the omissions is an
at least latent bug.

Jan