On 29/07/16 15:21, Tamas K Lengyel wrote: > > On Jul 29, 2016 02:50, "Julien Grall" <julien.gr...@arm.com > <mailto:julien.gr...@arm.com>> wrote: > > > > > > > > On 28/07/16 23:54, Tamas K Lengyel wrote: > >> > >> On Thu, Jul 28, 2016 at 2:38 PM, Julien Grall <julien.gr...@arm.com > <mailto:julien.gr...@arm.com>> 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
_______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
