Re: [Qemu-devel] [PATCH v2 4/6] msix: Split PBA into it's own MemoryRegion

[Alex Williamson]

On Thu, 2012-06-14 at 17:50 +0300, Michael S. Tsirkin wrote:
> On Thu, Jun 14, 2012 at 08:21:39AM -0600, Alex Williamson wrote:
> > On Thu, 2012-06-14 at 13:24 +0300, Michael S. Tsirkin wrote:
> > > On Wed, Jun 13, 2012 at 10:51:47PM -0600, Alex Williamson wrote:
> > > > These don't have to be contiguous.  Size them to only what
> > > > they need and use separate MemoryRegions for the vector
> > > > table and PBA.
> > > > 
> > > > Signed-off-by: Alex Williamson <address@hidden>
> > > 
> > > Why is this still using NATIVE?
> > 
> > Because the bug already exists,
> 
> We have lots of broken code. The way progress happens here is
> such code is in a kind of freeze until fixed. This way whoever needs new
> features gets to fix the bugs too.

In other words, you impose a toll and inhibit forward progress until
someone fixes it?  I have no place telling you how to be a maintainer,
but I personally find that this style makes attempting to contribute
code to anything pci/msi/msix related a huge pain.  There are far too
many of these land mines in the code and simple fixes easily explode
into tangentially related changes off your todo list.

> > this patch doesn't make it worse, so at best it's a tangentially related 
> > additional fix.
> > It may seem like a s/NATIVE/LITTLE/ to you, but to me it's asking to 
> > completely scrub
> > msix.c for endian correctness.  Is this going to be the carrot you hold
> > out to accept the rest of the series?
> > 
> > Alex
> 
> Unfortunately no promises yet, and that is because you basically decided
> to rewrite lots of code in your preferred style while also adding new 
> functionality.
> If changes were done in small steps, then I could apply things we can
> agree on and defer the ones we don't.  Sometimes it's hard, but clearly
> not in this case.

Patches can always be reduced into smaller changes, but at some point we
have to call it good enough.  I split one patch into 6 and thought that
did a pretty good job.  Should I remove everywhere that I've added a new
line to avoid imposing my style on the rest of the code?  The next
version will eliminate the add_config move thanks to Jan's constructive
suggestion, so I hope it meets your standards.  Thanks,

Alex