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

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

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.

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