Re: [PATCH v3 4/6] xen: Switch to byteswap

[Bertrand Marquis <Bertrand.Marquis@arm.com>]

Hi,

> On 11 May 2022, at 04:12, Stefano Stabellini <sstabellini@kernel.org> wrote:
> 
> On Tue, 10 May 2022, Julien Grall wrote:
>>> It is not reasonable to say "this unrelated thing is broken, and you
>>> need to fix it first to get your series in".  Requests like that are,
>>> I'm sure, part of what Bertrand raised in the community call as
>>> unnecessary fiction getting work submitted.
>> 
>> To be honest, you put the contributor in this situation. I would have been
>> perfectly happy if we keep *cpup* around as there would be only a place to
>> fix.
>> 
>> With this approach, you are effectively going to increase the work later one
>> because now we would have to chase all the open-coded version of *cpup* and
>> check which one is not safe.
> 
> 
> Without disagreeing with Julien or Andrew, I am actually happy to see an
> effort to make the review process faster. We have lot of room for
> improvement and spotting opportunities to do so is the first step toward
> improving the process. I have actually been thinking about how to make
> things faster in cases like this and I have a suggesion below.

Definitely with you here, it is good to see that my message on review process
and effort is actually in people’s minds :-)

> 
> In this case all of the options are OK. Whether we fix the alignment
> problem as part of this patch or soon after it doesn't make much of a
> difference. It is more important that we don't get bogged down in a long
> discussion. Coding is faster and more fun.

I would just make a small exception here (which corresponds to something
Julien kind of suggested during the discussion): unless we introduce a
temporary bug between the patches.
But this could actually be solved here by making a patch upfront and merging
it before the one in discussion (which might require some rebasing).

> 
> It would take less time for Julien (or Andrew) to write the code than to
> explain to the contributor how to do it. English is a good language for
> an architectural discussion, but simply replying with the example code
> in C would be much faster in cases like this one.
> 
> So my suggestion is that it would be best if the reviewer (Julien in
> this case) replied directly with the code snipper he wants added. Just
> an example without looking too closely:
> 
> ---
> Please do this instead so that alignment also gets fixed:
> 
> return be16_to_cpu(*(const __packed uint16_t *)p);
> ---
> 
> 
> Alternatively, I also think that taking this patch as is without
> alignment fix (either using be16_to_cpu or be16_to_cpup) is fine. The
> alignment could be fixed afterwards. The key is that I think it should
> be one of the maintainers to write the follow-up fix. Both of you are
> very fast coders and would certainly finish the patch before finishing
> the explanation on what needs to be done, which then would need to be
> understood and implemented by the contributor (opportunity for
> misunderstandings), and verified again by the reviewer on v2. That would
> take an order of magnitude more of collective time and effort.

Agree with the exception I mentioned.

> 
> Of course this doesn't apply to all cases, but it should apply to quite
> a few.
> 
> In short, less English, more C  ;-)

Same goes for things like “please add a comment” or “please say
something in the commit message”, it would be most of the time easier
for everyone to do: Could you add the comment “xxx” on top of this or
the sentence “yyy” in your commit (or even better ask the contributor if
he is ok with it and do it on commit when it not modifying the code).

Cheers
Bertrand