From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:54306) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1UJk82-0006Vr-RW for qemu-devel@nongnu.org; Sun, 24 Mar 2013 08:34:28 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1UJk81-0007de-6T for qemu-devel@nongnu.org; Sun, 24 Mar 2013 08:34:26 -0400 Received: from mx1.redhat.com ([209.132.183.28]:16320) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1UJk80-0007dR-VK for qemu-devel@nongnu.org; Sun, 24 Mar 2013 08:34:25 -0400 Message-ID: <514EF39B.1000904@redhat.com> Date: Sun, 24 Mar 2013 13:37:47 +0100 From: Hans de Goede MIME-Version: 1.0 References: <87boadc2yp.fsf@codemonkey.ws> <1363883716-30289-1-git-send-email-alevy@redhat.com> <1363883716-30289-2-git-send-email-alevy@redhat.com> <87sj3o62gd.fsf@codemonkey.ws> <514C0EC3.3090202@redhat.com> <87wqszv8zr.fsf@codemonkey.ws> <514C8BCD.4020107@redhat.com> <87wqsz2wbv.fsf@codemonkey.ws> In-Reply-To: <87wqsz2wbv.fsf@codemonkey.ws> Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH 1/2] char: add qemu_chr_be_is_fe_connected List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Anthony Liguori Cc: amit.shah@redhat.com, Alon Levy , qemu-devel@nongnu.org, kraxel@redhat.com Hi, On 03/22/2013 06:11 PM, Anthony Liguori wrote: > Hans de Goede writes: > 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. >> 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