Re: [PATCH 1/5] xen/domain: Remove function pointers from domain pause helpers

[Julien Grall <julien@xxxxxxx>]

Hi Bertrand,

On 15/11/2021 11:23, Bertrand Marquis wrote:
> > On 15 Nov 2021, at 10:55, Jan Beulich <jbeulich@xxxxxxxx> wrote:
> >
> > On 15.11.2021 11:23, Bertrand Marquis wrote:
> > > Hi Jan,
> > >
> > > > On 15 Nov 2021, at 10:20, Jan Beulich <jbeulich@xxxxxxxx> wrote:
> > > >
> > > > On 15.11.2021 11:13, Bertrand Marquis wrote:
> > > > > > On 11 Nov 2021, at 17:57, Andrew Cooper <andrew.cooper3@xxxxxxxxxx> wrote:
> > > > > > --- a/xen/common/domain.c
> > > > > > +++ b/xen/common/domain.c
> > > > > > @@ -1234,15 +1234,18 @@ int vcpu_unpause_by_systemcontroller(struct vcpu *v)
> > > > > >    return 0;
> > > > > > }
> > > > > >
> > > > > > -static void do_domain_pause(struct domain *d,
> > > > > > -                            void (*sleep_fn)(struct vcpu *v))
> > > > > > +static void _domain_pause(struct domain *d, bool sync /* or nosync */)
> > > > > Here you use comments inside the function definition.
> > > > > I think this is something that should be avoided and in this specific case a
> > > > > boolean sync is clear enough not to need to explain that false is nosing.
> > > > While I agree the comment here isn't overly useful, I think ...
> > > >
> > > > > > @@ -1250,12 +1253,12 @@ static void do_domain_pause(struct domain *d,
> > > > > > void domain_pause(struct domain *d)
> > > > > > {
> > > > > >    ASSERT(d != current->domain);
> > > > > > -    do_domain_pause(d, vcpu_sleep_sync);
> > > > > > +    _domain_pause(d, true /* sync */);
> > > > > Same here.
> > > > ... comments like this one are pretty useful to disambiguate the plain
> > > > "true" or "false" (without the reader needing to go look at the function
> > > > declaration or definition).
> > > I agree with that but the comment here is useful, it could be added before
> > > the call instead of inside it.
> > Except the form Andrew has used is the one we've been using elsewhere
> > for some time.
> I know I found some other examples and that why I say “should” and not must.
> If other consider that this is the right way to go and should not be changed 
> this
> is ok with me
Adding the comment after the parameter is a lot easier to read.

What is Misra/FuSA trying to solve by preventing to comment just after 
the parameters?
> but I wanted to make the comment as this could ease the work
> with FuSa and Misra compliance in the future.
This will need to be part of a larger discussion on how the community 
want to integrate FuSa/Misra rules. I do expect a few of the rules may 
be quite controversial to adopt (like the one above) and therefore we 
would need to discuss the pros/cons of them.
Cheers,

--
Julien Grall