Re: [PATCH 4/8] xen/arm: Add support for read-only foreign mappings

[Julien Grall <julien.grall@arm.com>]

Hi,

On 11/15/18 7:48 PM, Stefano Stabellini wrote:
> On Thu, 15 Nov 2018, Julien Grall wrote:
>> Hi Stefano,
>>
>> On 11/15/18 6:44 PM, Stefano Stabellini wrote:
>>> On Thu, 15 Nov 2018, Julien Grall wrote:
>>>> Hi,
>>>>
>>>> On 11/15/18 1:19 PM, Andrii Anisov wrote:
>>>>>> So I would prefer to stick with _t which is quite common within the
>>>>>> p2m
>>>>>> code base so far.
>>>>>
>>>>> I've found a similar code only in one place - p2m_get_entry()
>>>>> function. And it is, at least, somehow commented there:
>>>>> ...
>>>>>        /* Allow t to be NULL */
>>>>>        t = t ?: &_t;
>>>>>
>>>>>        *t = p2m_invalid;
>>>>> ...
>>>>
>>>> And in x86 code...
>>>>
>>>>>
>>>>> But IMO, it is really confusing to write a code to calculate and store
>>>>> a value which clearly is not needed by a caller.
>>>>
>>>> I disagree, you directly know that t can be NULL. Although, I agree that a
>>>> comment would help to understand.
>>>>
>>>>>    From another hand, I'm not sure if a compiler would be intelligent
>>>>> enough to factor out the odd code from execution pass on the incoming
>>>>> null pointer.
>>>>
>>>> You can't assume the compiler will inline even with the inline keyword.
>>>>
>>>>>
>>>>> I'm sorry, but I can't pass my RB for `_t`.
>>>>
>>>> I think this request is unreasonable because this is a matter of taste.
>>>>
>>>> While this is a nice feature to have in Xen 4.12 and address a long
>>>> standing
>>>> open issue on Arm. I don't require it and I am not inclined to address
>>>> bikeshedding.
>>>>
>>>> So I will keep aside for now until someone cares about it.
>>>
>>> Let's try to be reasonable and have constructive conversations. If we do
>>> have to disagree, let's disagree on meaningful design decisions, not
>>> silly code style issues :-)
>>>
>>> Andrii, when something can be done equally well in two different ways,
>>> especially when it is a matter of style, I think it is best to express
>>> our preference, but not to force a contributor in a particular
>>> direction, unless specifically stated in CODING_STYLE or equivalent
>>> docs. We don't have written rules about code reviews, but if we had,
>>> I think this should be one of them. (I would love to have written rulesAs
>>> about code reviews.)
>>>
>>> Julien, usually in situations like this, it is quicker to change the
>>> code than to argue. I personally don't care and wouldn't ask you to
>>> change it, but if a member of the community reads the code and has an
>>> adverse reaction, it is always worth reconsidering the approach, because
>>> others might find it antagonizing too. Given the choice, it is best to
>>> go for something obvious to most, so if a new contributor sends a patch
>>> to change, it is more likely to be correct from the start.
>>
>> I agree with your point here but this is also valid in the other way. If the
>> suggested alternative provokes an adverse reaction to the contributor, then
>> why should he use it?
>>
>> As I wrote earlier one trying to use ternary condition split over 2 lines is
>> not making the code more obvious. So I am not entirely sure why I should be
>> forced to write such code?
> 
> I don't think you should be. You can find another way. For instance, the
> whole thing could be reduce to one more "if" condition:
> 
>    if ( t != NULL )
>    {
>        *t = p2m_invalid;
>        if ( page->u.inuse.type_info & PGT_writable_page )
>            *t = p2m_ram_rw;
>        else
>            *t = p2m_ram_ro;
>    }
> 
> be creative :-)

Well the code suggested is different from what I intended :). t should 
be set to invalid before the check mfn_valid/get_page. So this needs at 
least 2 checks. 2 places were t != NULL needs to be explained.

As you said this is a matter of taste. If someone disagree on the coding 
style, then he should suggest an alternative way fitting the requirement.

Cheers,

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel