Re: [Xen-devel] [PATCH] mem_access: Use monitor_traps instead of mem_access_send_req

[Tamas K Lengyel <tamas.lengyel@xxxxxxxxxxxx>]

On Fri, Jul 29, 2016 at 3:38 PM, Julien Grall <julien.grall@xxxxxxx> wrote:
>
>
> On 29/07/2016 22:02, Tamas K Lengyel wrote:
>>
>> On Fri, Jul 29, 2016 at 11:38 AM, Stefano Stabellini
>> <sstabellini@xxxxxxxxxx> wrote:
>>>
>>> On Fri, 29 Jul 2016, Tamas K Lengyel wrote:
>>>>
>>>> On Jul 29, 2016 02:50, "Julien Grall" <julien.grall@xxxxxxx> wrote:
>>>>>
>>>>>
>>>>>
>>>>>
>>>>> On 28/07/16 23:54, Tamas K Lengyel wrote:
>>>>>>
>>>>>>
>>>>>> On Thu, Jul 28, 2016 at 2:38 PM, Julien Grall <julien.grall@xxxxxxx>
>>>>>> wrote:
>>>>>>>
>>>>>>>
>>>>>>> On 28/07/2016 20:35, Tamas K Lengyel wrote:
>>>>>>> This patch is doing more than it is claimed in the commit message.
>>>>>>>
>>>>>>> In general, moving the code and introducing changes within the same
>>>>>>> patch
>>>>>>> should really be avoided. So please split it in 2 patches.
>>>>>>
>>>>>>
>>>>>>
>>>>>> Well, the changes are largely cosmetic so doing a whole separate patch
>>>>>> IMHO is an overkill. How about adjusting the commit message to
>>>>>> something like "sanitize code surrounding sending mem_access
>>>>>> vm_events" to better describe the adjustments made in this patch?
>>>>>
>>>>>
>>>>>
>>>>> I think the wiki page "Submitting Xen Project patches" [1] should
>>>>> answer to your question.
>>>>>
>>>>> If not, trivial patches are easy to review, merging multiple trivial
>>>>> patches in a single patch is not. Moving
>>>>
>>>> code and at the same time as changing the behavior is fairly difficult
>>>> to review because it hides the real
>>>> modifications.
>>>>>
>>>>>
>>>>> Regards,
>>>>>
>>>>> [1]
>>>>> http://wiki.xenproject.org/wiki/Submitting_Xen_Project_Patches#Break_down_your_patches
>>>>>
>>>>
>>>> The behavior didn't change at all, this whole patch is code
>>>> sanitization. It's not worth doing a separate patch
>>>> for each minor change. The few change on the arm side is the vm_event
>>>> request allocation going from xzalloc to
>>>> stack based and using monitor_traps now in a split-out function. It
>>>> really should be no problem reviewing it. Even
>>>> Andrew requested minor adjustments to be included in this patch. Anyway,
>>>> I'm not looking to change this into a
>>>> series. If it's a no go from your side I'm just going to cut down the
>>>> ARM side sanitization to the bare minimum of
>>>> using monitor_traps as the rest just does not worth the effort.
>>>
>>>
>>> Hi Tamas,
>>> let me premise that, like you wrote, the patch is just code
>>> sanitization, certainly not worth turning it into an argument.
>>>
>>> I think different maintainers have different styles. I for one always
>>> dislike to have code movement, renaming or code style fixes together
>>> with any other more meaningful changes. In fact when people suggest
>>> "could you please also rename this variable" or "could you please also
>>> move the function to common code", I usually reply: "I can, but it will
>>> be in another patch".
>>>
>>> So I agree with Julien that I would prefer to see two patches. In fact
>>> if I were you, I would prefer to write two separate patches because it
>>> would be less troubles for me as a developer. But clearly not everybody
>>> agrees with this style as I get requests for cosmetic changes together
>>> with other changes by many other seasoned maintainers. And that's OK, it
>>> just means that different people think differently, which is a good
>>> thing for the project as a whole.
>>>
>>> You are a valued contributor and maintainer of this project -- if you
>>> strongly feel this way, I am sure we can find a way to make this work
>>> anyway.
>>
>>
>> I would highly appreciate that as I said it's just not worth the time
>> it takes to break this down into smaller patches. It really doubles
>> the effort it takes to test it.
>
>
> I find this paragraph really offensive for reviewers. This requires more
> efforts for reviewers to review a such patch. My time is as valuable as
> yours. I hope you will reconsider what you've written.
>

I don't know which part you find offensive and I certainly didn't mean
to offend anyone. I do value your time. I understand this may require
a bit more to review but honestly, I don't think the difference from
your side should be significant. From my side it is and it's not worth
the extra effort to go rounds about this and do a whole series so I'll
just drop the portions you are not happy about. It is your right as
maintainer to reject it as this code right now lives in a generic part
of the ARM code-base. However, as this patch really only touches
things that belong to mem_access/monitor components maybe we should
split these from the generic ARM code altogether to avoid such
conflicts in the future.

Tamas

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel