Re: [Qemu-devel] [PULL 00/47] virtio, pc: fixes and features

[Michael S. Tsirkin]

On Tue, Nov 01, 2016 at 02:21:07PM +0100, Igor Mammedov wrote:
> On Tue, 1 Nov 2016 00:48:11 +0200
> "Michael S. Tsirkin" <address@hidden> wrote:
> 
> > On Mon, Oct 31, 2016 at 10:50:31AM +0100, Igor Mammedov wrote:
> > > On Sun, 30 Oct 2016 23:23:18 +0200
> > > "Michael S. Tsirkin" <address@hidden> wrote:
> > >   
> > > > The following changes since commit 
> > > > 5b2ecabaeabc17f032197246c4846b9ba95ba8a6:
> > > > 
> > > >   Merge remote-tracking branch 'remotes/kraxel/tags/pull-ui-20161028-1' 
> > > > into staging (2016-10-28 17:59:04 +0100)
> > > > 
> > > > are available in the git repository at:
> > > > 
> > > >   git://git.kernel.org/pub/scm/virt/kvm/mst/qemu.git tags/for_upstream
> > > > 
> > > > for you to fetch changes up to f082ec0225bd15c71e0b4697d2df3af7bad65d7f:
> > > > 
> > > >   acpi: fix assert failure caused by commit 35c5a52d (2016-10-30 
> > > > 20:06:25 +0200)
> > > > 
> > > > ----------------------------------------------------------------
> > > > virtio, pc: fixes and features
> > > > 
> > > > nvdimm hotplug support  
> > > Michael,
> > > 
> > > Could you drop nvdimm hotplug from pull request (I should review at least 
> > > once as it touches not only NVDIMMs but a generic hotplug infrastructure)
> > > 
> > > and keep only nvdimm fixes/cleanups for now?  
> > 
> > If I drop it now it won't be in the next QEMU and it seems like
> > a valuable feature. The comments so far are about minor style
> > improvements that IMO can be addressed by patches on top.
> wrt nvdimm hotplug support it's not about style issues but rather
> design issues: for example:
>  - it extends general hotplug framework unnecessarily instead of
>    figuring out how it works.
>  - adds not needed locks
> maybe there is more and all of that was posted just a day before
> this pull request so I haven't even had a chance to review it properly.
> 
> 
> 
> > We can always revert if you see bigger issues, but let's enable the
> > testing of this feature.
> if it didn't mess with general infrastructure, I wouldn't care much.
> But it does so I'd rather avoid merging not yet ready series just for
> the sake of getting it in.
> 
> I haven't reviewed 28-35 patches either but they are all cleanups/
> fixes of current nvdimm code and local to it so don't mind them
> getting merged.
> 
> However I suggest dropping 36-39 patches from this pull request
> as not yet ready for merging.

So I think it's ok to keep them from now as that should help
the feature converge faster, on the understanding the
style issues are addressed quickly.

Let's merge as is and I'll revert if this does not happen
within two weeks. Ack?

-- 
MST