On Tue, Oct 15, 2013 at 7:20 AM, Michael S. Tsirkin <m...@redhat.com> wrote: > On Tue, Oct 15, 2013 at 06:51:30AM -0700, Anthony Liguori wrote: >> On Mon, Oct 14, 2013 at 10:28 PM, Michael S. Tsirkin <m...@redhat.com> wrote: >> > On Mon, Oct 14, 2013 at 03:42:37PM -0700, Anthony Liguori wrote: >> >> "Michael S. Tsirkin" <m...@redhat.com> writes: >> >> >> >> > Anthony, I know you wanted to review some of the patches, >> >> > since you didn't respond either all's well or you >> >> > could not find the time. >> >> > I think we are better off merging them for 1.7 and then - worst case, >> >> > if major issues surface - disabling the functionality at the last minute >> >> > than delaying the merge even more. >> >> >> >> There is no way I'll pull this for 1.7. Changes like this aren't going >> >> to get merged at the last minute. >> > >> > Last minute? This has been on list for months. >> >> It doesn't matter how long the patches have been on the list. We have >> a very short testing cycle for releases. >> >> This pull request lacks any automated testing. Something like this >> really should come with at least some qtest validation that we are >> still generating the right ACPI tables but certainly could have >> simpler unit tests too. > > It did go through autotest testing though.
This specific tree or some previous version of the series? A full run or just a restricted run? >> There is no statement about what manual testing you actually did. > > Manually I loaded tables and verified that they match > the bios bit for bit except pointer values. > >> Have you run kvm autotest? Have you tested a variety of Windows >> guests? > > Yes, both. > >> The pull request has a patch with a binary diff and a comment of: >> >> "update generated file, not sure what changed" >> >> And that didn't concern you prior to sending the pull request? > > > Sorry, I forgot to update the description. V2 has it right: > IASL sticks its own version when it builds tables, > this is what changed. > >> This series is not ready to merge. > > Because a single commit message was out of date? > >> >> A good chunk of the series lacks >> >> any Reviewed-bys including the actual hotplug behind a pci bridge bits >> >> which is the whole point of the series. >> > >> > It isn't. The point is getting ACPI out of seabios. >> > OK what if I drop the bridge hotplug part? >> >> How does that address the above? > > It addresses the issues you have raised which was with > the bridge. > >> >> This is a huge series and I still am not convinced this is the right >> >> path forward. The alternative to this series is a small set of changes >> >> to SeaBIOS to support PCI bridge hotplug, no? >> > >> > No, we also get alternative firmwares working correctly with QEMU. >> > >> >> Or 10k SLOC of code into QEMU that includes breaking migration >> >> compatibility. >> > >> > AFAIK it doesn't break migration compatibility. >> >> >From 41/43: >> >> "The interface is actually backwards-compatible with >> existing PIIX4 ACPI (though not migration compatible)." >> >> And does "AFAIK" translate to, "I have tested migration from new and >> old and old and new with this series"? I suspect the answer is no. >> >> Regards, >> >> Anthony Liguori > > But the code to handle it is there, at least. > I will test it but I think minor fixes like this can go > in after soft freeze. I cannot reasonable revert a series like this before we cut GA. We would have to delay the release until everything was fixed. The release is a month away and most of us will lose at least a week to KVM Forum so our ducks need to be in a row here. Please put together a summary of the testing this series has gone through. I still think there should be automated testing as part of this but if the manual testing is sufficiently thorough I'll reconsider for 1.7. Regards, Anthony Liguori > > -- > MST
