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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 20.04.2020 12:10, Julien Grall wrote:
> Hi,
> 
> On 20/04/2020 10:16, Jan Beulich wrote:
>> 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 am sorry but this doesn't add up. Here you say that we can't have
> a clear line to draw until everything is converted but...
> 
>> 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 ...
> 
> ... here you say the maintainers get to decide when to use mfn_eq()
> (or other typesafe construction). So basically, we would never be
> able to fully convert the code and therefore never draw a line.

Why? Eventually both sides of an mfn_eq() will be of type mfn_t. And
in the specific case hand even with my alternative suggestion no
further change would  be needed down the road. Type safety is for
things like function argument passing and assignments and alike. A
leaf expression like "if ( mfn_x() )" is not type-unsafe in any way.

Jan