Re: [PATCH v3] xen/arm: domain: Fix MISRA C 2012 Rule 8.7 violation

[Julien Grall <julien@xxxxxxx>]

Hi,

On 29/07/2022 16:47, Xenia Ragiadakou wrote:
> On 7/29/22 18:15, Julien Grall wrote:
> > Hi Xenia,
> >
> > On 29/07/2022 15:03, Xenia Ragiadakou wrote:
> > > On 7/29/22 16:41, Bertrand Marquis wrote:
> > > > Hi Xenia,
> > > >
> > > > > On 29 Jul 2022, at 07:31, Xenia Ragiadakou <burzalodowa@xxxxxxxxx> 
> > > > > wrote:
> > > > > Hi Jan,
> > > > >
> > > > > On 7/29/22 09:26, Jan Beulich wrote:
> > > > > > On 28.07.2022 18:21, Xenia Ragiadakou wrote:
> > > > > > > --- a/xen/arch/arm/domain.c
> > > > > > > +++ b/xen/arch/arm/domain.c
> > > > > > > @@ -63,7 +63,7 @@ static void do_idle(void)
> > > > > > >       rcu_idle_exit(cpu);
> > > > > > >   }
> > > > > > >   -void idle_loop(void)
> > > > > > > +static void idle_loop(void)
> > > > > > While you're adding "noreturn" below, shouldn't this one be marked so
> > > > > > as well? Preferably with the addition:
> > > > > > Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>
> > > > > Yes, but I was not sure if this should go in this patch or in a 
> > > > > separate one.
> > > > As you modify the function to make it static, I think it is ok to 
> > > > also add the noreturn in the same patch.
> > > > With that done, you can add my:
> > > > Reviewed-by: Bertrand Marquis <bertrand.marquis@xxxxxxx>
> > > >
> > > > Cheers
> > > > Bertrand
> > > I consider this change unrelated to the patch. I think it is a bad 
> > > practice to squash unrelated changes in a single patch. Also, I do 
> > > not think it is unfair to be obliged to make it in order for the 
> > > patch to be accepted.
> > > I could have taken the opportunity to fix this in the same patch but 
> > > I decided to not take it.
> > In general, I don't like having multiple changes within a patch. 
> > However, here this is a consistency problem. You are modifying the 3 
> > prototypes (well one is technically a declaration) and it reads odd 
> > that 2 are using noreturn but not the other one.
> The patch adds the 2 function declarations, it does not modify them.
Fair enough, you are adding 2 declarations and modifying one. This 
doesn't change the inconsistency problem though.
> Since they do not return, they are declared noreturn.
> If the function idle_loop was not declared noreturn, although it should 
> have been, for me is a completely different issue.
[...]

> I do not agree with you saying that the patch introduced this 
> inconsistency. The function idle_loop should have been declared noreturn 
> in the first place.
I think everyone involved in the discussion agrees that idle_loop() 
should have been marked as noreturn from the start.
And we all agree that this needs to be fixed. So I don't think this is 
particularly useful to spend time arguing on whether this needs to be 
handled within or separately. 3 of the reviewers agree that this should 
be preferably added here. So...
> If you would like to fix this in the current patch, 
> it is up to you.
... I will commit it with the change on the next swipe.

Cheers,

--
Julien Grall