Hi,

On 03/22/2013 06:11 PM, Anthony Liguori wrote:
Hans de Goede <hdego...@redhat.com> writes:

<snip>

If the qemu-char.c code did:

int qemu_chr_fe_write(...) {
     if (!s->fe_open) {
         qemu_chr_fe_open(s);
     }
     ...
}

That would be one thing.  It's a hack, but a more reasonable hack than
doing this in the backend like we do today.

Agreed.

And in fact, herein lies exactly what the problem is.  There is no
"s->fe_open".  And if such a thing did exist, we would note that this is
in fact guest state and that it needs to be taken care of during
migration.

Agreed too, I've prepared a patch set adding fe_open and cleaning up the
whole existing code around the fe_open stuff. I'll send it directly after
this mail.

<snip>

We could do the same and have a qemu_fe_generic_open for frontends which
does the qemu_chr_fe_open call.

You mean, in qemu_chr_fe_write()?  Yes, I think not doing this bit in
the backend and making it part of qemu-char would be a significant
improvement and would also guide in solving this problem correctly.

I believe that qemu_chr_fe_write is too late, this assumes that the
frontend is always the first one to do a write (assuming fe_open aware
backends won't do anything until the fe_open happens). But what if the
protocol going over the chardev expects the backend to do the first
write? Then the backend will just sit there waiting for the fe_open,
and the frontend will just sit there waiting for the first write before
sending this fe_open.

I believe that your previous comments on qemu_chr_add_handlers being
the closest thing to a fe_open for many frontends is correct, esp. since
some frontends and hw/qdev-properties-system.c do a NULL
qemu_chr_add_handlers when a frontend is being unregistered / removed /
closed. Which gives us a central place to detect frontend closes too.

So this is what I've used in my patch-set.

Also, you can get reset handling for free by unconditionally clearly
s->fe_open with a reset handler.  That would also simplify the
virtio-serial code since it would no longer need the reset logic.

And for me, the most logical thing is to call qemu_chr_fe_open() in
post_load for the device.

With the device I assume you mean the frontend? That is what we originally
suggested and submitted a patch for (for virtio-console) but Amit didn't
like it.

As Avi would say, this is "derived guest state" and derived guest state
has to be recomputed in post_load handlers.

Agreed, which brings us back to the original patch posted a long time
ago which Amit did not like:
http://lists.gnu.org/archive/html/qemu-devel/2012-11/msg02721.html

Amit can you take a second look at this? Note that after the cleanup patchset
which I'll send right after this mail, it will look slightly different, 
something
like:

@@ -653,6 +654,10 @@ static void virtio_serial_post_load_timer_cb(void *opaque)
             send_control_event(port, VIRTIO_CONSOLE_PORT_OPEN,
                                port->host_connected);
         }
+        vsc = VIRTIO_SERIAL_PORT_GET_CLASS(port);
+        if (vsc->set_guest_connected) {
+            vsc->set_guest_connected(port, port->guest_connected);
+        }
     }
     g_free(s->post_load.connected);
     s->post_load.connected = NULL;

Which to me looks like a reasonable thing to do on post-load.

Regards,

Hans

Reply via email to