|
On 29/07/16 15:21, 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.
To offer an alternative opinion.
Looking at this patch, I personally would have a hard time
justifying breaking it down any further. Each of the changes are
closely related.
However, the commit message could be a lot clearer about which steps
are taken. If I were writing the commit message, I would go with
something a bit more like this:
----8<----
The two functions monitor_traps and mem_access_send_req duplicate
some of the same functionality. The mem_access_send_req however
leaves a lot of the standard vm_event fields to be filled by other
functions.
Remove mem_access_send_req() completely, making use of
monitor_traps() to put requests into the monitor ring. This in turn
causes some cleanup around the old callsites of
mem_access_send_req(), and on ARM, the introduction of the
__p2m_mem_access_send_req() helper to fill in common mem_access
information.
Finally, this change identifies that errors from
mem_access_send_req() were never checked. As errors constitute a
problem with the monitor ring, crashing the domain is the most
appropriate action to take.
----8<----
If in doubt, always spell out each of the changes, and their logical
relationships. If nothing else, it helps people trying to review
the patch.
~Andrew
|
