Re: [PATCH 05/17] xen/x86: Remove the non-typesafe version of pagetable_* helpers

[Jan Beulich <jbeulich@xxxxxxxx>]

On 18.04.2020 12:23, Julien Grall wrote:
> On 30/03/2020 08:52, Jan Beulich wrote:
>> On 28.03.2020 11:52, Julien Grall wrote:
>>> On 26/03/2020 15:39, Jan Beulich wrote:
>>>> On 22.03.2020 17:14, julien@xxxxxxx wrote:
>>>>> @@ -3116,24 +3116,24 @@ int vcpu_destroy_pagetables(struct vcpu *v)
>>>>>          /* Free that page if non-zero */
>>>>>        do {
>>>>> -        if ( mfn )
>>>>> +        if ( !mfn_eq(mfn, _mfn(0)) )
>>>>
>>>> I admit I'm not fully certain either, but at the first glance
>>>>
>>>>           if ( mfn_x(mfn) )
>>>>
>>>> would seem more in line with the original code to me (and then
>>>> also elsewhere).
>>>
>>> It is doing *exactly* the same things. The whole point of typesafe
>>> is to use typesafe helper not open-coding test everywhere.
>>>
>>> It is also easier to spot any use of MFN 0 within the code as you
>>> know could grep "_mfn(0)".
>>>
>>> Therefore I will insist to the code as-is.
>>
>> What I insit on is that readability of the result of such changes be
>> also kept in mind. The mfn_eq() construct is (I think) clearly less
>> easy to read and recognize than the simpler alternative suggested.
> 
> If mfn_eq() is less clear, then where do you draw the line when the
> macro should or not be used?

I'm afraid there may not be a clear line to draw until everything
got converted. I do seem to recall though that, perhaps in a
different context, Andrew recently agreed with my view here (Andrew,
please correct me if I'm wrong). It being a fuzzy thing, I guess
maintainers get to judge ...

Jan