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

[Julien Grall <julien.grall@xxxxxxx>]

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.
Regards,
        
> Thanks,
> Tamas
--
Julien Grall

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