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

[Julien Grall <julien@xen.org>]

Hi Bertrand,

On 15/11/2021 11:23, Bertrand Marquis wrote:
>> On 15 Nov 2021, at 10:55, Jan Beulich <jbeulich@suse.com> wrote:
>>
>> On 15.11.2021 11:23, Bertrand Marquis wrote:
>>> Hi Jan,
>>>
>>>> On 15 Nov 2021, at 10:20, Jan Beulich <jbeulich@suse.com> wrote:
>>>>
>>>> On 15.11.2021 11:13, Bertrand Marquis wrote:
>>>>>> On 11 Nov 2021, at 17:57, Andrew Cooper <andrew.cooper3@citrix.com> 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