[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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





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

 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.