On Wed, Jul 27, 2011 at 03:50:11PM +0530, Amit Shah wrote:
> On (Wed) 27 Jul 2011 [10:07:56], Alon Levy wrote:
> > On Wed, Jul 27, 2011 at 07:45:25AM +0200, Markus Armbruster wrote:
> > > Alon Levy <al...@redhat.com> writes:
> > >
> > > > Signed-off-by: Alon Levy <al...@redhat.com>
> > > > ---
> > > > hw/virtio-serial-bus.c | 8 +++++++-
> > > > 1 files changed, 7 insertions(+), 1 deletions(-)
> > > >
> > > > diff --git a/hw/virtio-serial-bus.c b/hw/virtio-serial-bus.c
> > > > index c5eb931..7a652ff 100644
> > > > --- a/hw/virtio-serial-bus.c
> > > > +++ b/hw/virtio-serial-bus.c
> > > > @@ -618,14 +618,20 @@ static int virtio_serial_load(QEMUFile *f, void
> > > > *opaque, int version_id)
> > > > for (i = 0; i < nr_active_ports; i++) {
> > > > uint32_t id;
> > > > bool host_connected;
> > > > + VirtIOSerialPortInfo *info;
> > > >
> > > > id = qemu_get_be32(f);
> > > > port = find_port_by_id(s, id);
> > > > if (!port) {
> > > > return -EINVAL;
> > > > }
> > > > -
> > > > port->guest_connected = qemu_get_byte(f);
> > > > + info = DO_UPCAST(VirtIOSerialPortInfo, qdev, port->dev.info);
> > > > + if (port->guest_connected && info->guest_open) {
> > > > + /* replay guest open */
> > > > + info->guest_open(port);
> > > > +
> > > > + }
> > > > host_connected = qemu_get_byte(f);
> > > > if (host_connected != port->host_connected) {
> > > > /*
> > >
> > > The patch makes enough sense to me, but the commit message is
> > > insufficient. Why do you have to replay? And what's being fixed?
> >
> > When migrating a host with with a spice agent running the mouse becomes
> > non operational after the migration. This is rhbz #718463, currently on
> > spice-server but it seems this is a qemu-kvm issue. The problem is that
> > after migration spice doesn't know the guest agent is open. Spice is just
> > a char dev here. And a chardev cannot query it's device, the device has
> > to let the chardev know when it is open. Right now after migration the
> > chardev which is recreated is in it's default state, which assumes the
> > guest is disconnected. Char devices carry no information across migration,
> > but the virtio-serial does already carry the guest_connected state. This
> > patch passes that bit to the chardev.
>
> It's not guaranteed all ports will be chardevs.
>
The wording may be off, but the code isn't - it doesn't check for chardev,
it calls the same guest_open callback that is called when guest_connected
is changed from 0 to 1. So the logic is:
1. Start from stratch.
2. guest_connected 0->1
3. info->guest_open(port)
And on migration target:
5. if (guest_connected)
6. info->guest_open(port)
If that callback was non NULL it would be called in a non migration scenario
as well, so no reason to care if it is specifically a chardev or not, let alone
specifically a spice chardev or not.
> My thinking was this can be handled by qemu-char-spice.c since it can
> add a new migration section and if an image is being restored and the
> guest agent channel is open after migration finishes, it can continue
> its work from there. What's the benefit of all virtio-serial ports
> receiving a guest_open() event in this case?
>
Consistency between non migration guest open and migrating when guest connected.
> Also, we'll be lying that a guest opened, since a guest was opened
> much earlier, before migration. Nothing has changed as far as the
> guest is concerned, this is just some host-side tracking that has to
> be done post-migrate, which belongs in individual devices / ports.
The callback is there on purpose, some qemu side users exist surely. While
I understand the lying part about the time, it is worst to lie completely
by not mentioning the guest has opened the port.
>
> So I'm not completely sure that this is the right place for such a
> notification. However, if others feel this is fine, I'll accept the
> patch.
>
> (Also, when resending, make sure the whitespace changes don't go
> through.)
>
I removed them. We can continue this on the patch - I didn't cc you since
I thought you were already agreed and just wanted Armbru/Juan to take a look.
> Thanks,
> Amit
>
