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

[Julien Grall <julien@xxxxxxx>]

Hi Jan,

On 14/08/2023 14:31, Jan Beulich wrote:
> 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.
I agree they are latent bugs. But the question is whether the time 
looking up the histoy is well spent?
In this situation, we have a lot of of similar patches where we need to 
add Fixes. So there is quite a bit of time sinked into finding the 
original commit message. Is it valuable? I don't believe so.
A least for Arm, I would not require the tag to be added for any missing 
prototypes.
Cheers,

--
Julien Grall