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

[Tamas K Lengyel <tamas.lengyel@xxxxxxxxxxxx>]

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. It's within your and Julien's right to
not accept such patches touching your code-base as Maintainer and
that's fine. If that's the case though we might want to look into
adjusting the layout of the code so that mem_access/monitor/vm_event
components are more isolated into separate files so that we can move
at a different phase and different style then the rest of the code
here without getting into arguments about that stuff.

Thanks,
Tamas

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