On Sun, Dec 12, 2010 at 09:29:05AM -0700, Alex Williamson wrote: > On Sun, 2010-12-12 at 20:07 +0530, Juan Quintela wrote: > > "Michael S. Tsirkin" <m...@redhat.com> wrote: > > > On Sun, Dec 12, 2010 at 05:23:39PM +0530, Juan Quintela wrote: > > >> "Michael S. Tsirkin" <m...@redhat.com> wrote: > > >> > On Thu, Dec 09, 2010 at 03:14:17PM -0700, Alex Williamson wrote: > > > > >> > How about we keep migrating the index for the benefit of > > >> > old versions, but ignore the value on load? > > >> > Something like the following: > > >> > > >> This was my 1st suggestion to Alex O:-) > > > > > > The difference here is that instead of sending garbage to the > > > old version we send an actual index value. > > > > > >> So, I am in. he think this is bad for upstream, I don't think so (but > > >> I understand that it is oppinable). > > >> > > >> Later, Juan. > > > > > > I think it makes sense to fix this for the stable branch, > > > and I think we should try as hard as we can to avoid bumping up the > > > version number there. > > > > > > For master we can bump the version number but it might be easier to > > > just keep the code the same there. > > > > I think that your solution is better. For older versions, it works as > > expected. For new versions, problem is fixed. Solution is not the > > "purest", but you can say the same about uping the version for a state > > that is exactly the same length & fields O:-) > > I disagree, without bumping the version number, we can never guarantee > the problem is behind us. We can always migrate to the bad version, > which puts our users at risk.
Well, they can also migrate between old versions, right? > The responsible behavior is to allow > forward migrations and prevent migrations to a version with an issue > known to compromise VM integrity. Perhaps I feel more strongly about > this because I actually had to debug this problem. Obvious in > retrospect, but a huge pain in the butt to get there. > > I had sent Juan a similar patch to the one Michael proposed, but with > the following change: > > + if (s->dev.qdev.hotplugged) { > + s->rtl8139_mmio_io_addr_dummy = IO_MEM_UNASSIGNED; > + } else { > + s->rtl8139_mmio_io_addr_dummy = s->rtl8139_mmio_io_addr; > + } > > With this, we at least limit the damage that the hotplugged NIC can do, > but all it takes is a reboot of the VM to touch the BARs before the > device becomes unusable (which is better than taking out the whole VM). Looks good to me. > If we need something like this for stable, I will begrudgingly agree, > but for master I think we have to bump the version. Thanks, > > Alex