[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Xen-devel] [PATCH 4/8] xen/arm: Add support for read-only foreign mappings
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@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel
|
