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

Re: [Xen-devel] [V10 PATCH 11/23] PVH xen: support invalid op emulation for PVH



>>> On 10.08.13 at 04:13, Mukesh Rathor <mukesh.rathor@xxxxxxxxxx> wrote:
> On Thu, 8 Aug 2013 09:55:26 +0100
> George Dunlap <george.dunlap@xxxxxxxxxxxxx> wrote:
> 
>> On 08/08/13 02:49, Mukesh Rathor wrote:
>> > On Wed, 7 Aug 2013 12:29:13 +0100
>> > George Dunlap <George.Dunlap@xxxxxxxxxxxxx> wrote:
>> >
>> >> On Wed, Jul 24, 2013 at 2:59 AM, Mukesh Rathor
>> >> <mukesh.rathor@xxxxxxxxxx> wrote:
>> >>> This patch supports invalid op emulation for PVH by calling
>> >>> appropriate copy macros and and HVM function to inject PF.
>> >>>
>> >>> Signed-off-by: Mukesh Rathor <mukesh.rathor@xxxxxxxxxx>
>> >>> Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>
>> >>> ---
>> >>>   xen/arch/x86/traps.c        |   17 ++++++++++++++---
>> >>>   xen/include/asm-x86/traps.h |    1 +
>> >> Why make this non-static?  No one is using this in this patch.  If
>> >> a later patch needs it, you should make it non-static there, so we
>> >> can decide at that point if making it non-static is merited or not.
>> > Sigh! Originally, it was that way, but then to keep that patch from
>> > getting too big, it got moved here after few versions. We are making
>> > emulation available for outside the PV, ie, to PVH.
>> 
>> As far as I'm concerned, the size of the patch itself is immaterial;
>> the only important question, regarding how to break down patches
>> (just like in breaking down functions), is how easy or difficult it
>> is to understand the whole thing.
>> 
>> Now it's typically the case that long patches are hard to understand, 
>> and that breaking them down into smaller chunks makes them easier to 
>> read.  But a division like this, where you've moved some random hunk 
>> into a different patch with which it has no logical relation, makes
>> the series *harder* to understand, not easier.
>> 
>> Additionally, as the series evolves, it makes it difficult to keep
>> all of the dependencies straight.  Suppose you changed your approach
>> for that future patch so that you didn't need this public anymore.
>> You, and all the reviewers, could easily forget about the dependency,
>> since it's in a separate patch which may have already been classified
>> as "OK".
> 
> But that would happen even if the function was static. Say I make
> changes in function for PVH, dont' make it public. Now I forget to
> use it, the function has been changed already? 
> 
> We make it public to be used by future patch, I'll add which patch is 
> using it to make it easier to understand. Not making it public makes
> possible  another comment -- why the change if it can't be used by
> another PVH module anyways. Can't please all reviewers
> simultaneously!!! All my life so far, all reviews are done by one
> person per file, and that makes so much sense..... this is hell!!!

Yes, I see how this is not pleasant for you. But that's the way it is
with community projects. And I've seem numerous occasions
where there were multiple reviewers, and one has to find a way
to make all of them happy.

For what it's worth - I had pointed out the non-logical breakup of
the series as an issue quite early in the process, and merely gave
in realizing that your life with this is already difficult enough.

Jan


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


 


Rackspace

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