Re: [Qemu-devel] [PATCH 1/2] char: add qemu_chr_be_is_fe_connected

[Hans de Goede]

Hi,

On 03/22/2013 06:11 PM, Anthony Liguori wrote:
> Hans de Goede <address@hidden> 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