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

[Bertrand Marquis <Bertrand.Marquis@xxxxxxx>]

Hi,

> On 15 Nov 2021, at 14:11, Julien Grall <julien@xxxxxxx> wrote:
> 
> 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?

I do not think Misra is always trying to prevent “bugs”, sometimes it is just 
trying to make
the code easier to read and review by making it always look the same. Here is 
to saying that
this should not be done but that comment should be written one or several lines 
using /* */ form.

> 
>> 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.

I definitely think this will require a discussion and maybe some more 
explanation from us on why.
Your comment asking “What is prevented” is interesting and I will try to keep 
that in mind when we start the discussion.


Cheers
Bertrand

> 
> Cheers,
> 
> -- 
> Julien Grall