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