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

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

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.

Tamas

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