From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:54855) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1UIl7F-0004JN-UJ for qemu-devel@nongnu.org; Thu, 21 Mar 2013 15:25:48 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1UIl7B-0006km-4f for qemu-devel@nongnu.org; Thu, 21 Mar 2013 15:25:33 -0400 Received: from e23smtp08.au.ibm.com ([202.81.31.141]:51760) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1UIl7A-0006g5-93 for qemu-devel@nongnu.org; Thu, 21 Mar 2013 15:25:28 -0400 Received: from /spool/local by e23smtp08.au.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Fri, 22 Mar 2013 05:23:15 +1000 Received: from d23relay05.au.ibm.com (d23relay05.au.ibm.com [9.190.235.152]) by d23dlp03.au.ibm.com (Postfix) with ESMTP id 1F16D357804A for ; Fri, 22 Mar 2013 06:25:14 +1100 (EST) Received: from d23av03.au.ibm.com (d23av03.au.ibm.com [9.190.234.97]) by d23relay05.au.ibm.com (8.13.8/8.13.8/NCO v10.0) with ESMTP id r2LJBeem10551560 for ; Fri, 22 Mar 2013 06:11:40 +1100 Received: from d23av03.au.ibm.com (loopback [127.0.0.1]) by d23av03.au.ibm.com (8.14.4/8.13.1/NCO v10.0 AVout) with ESMTP id r2LJOhfW025476 for ; Fri, 22 Mar 2013 06:24:43 +1100 From: Anthony Liguori In-Reply-To: <1179249577.12798614.1363890915793.JavaMail.root@redhat.com> References: <1179249577.12798614.1363890915793.JavaMail.root@redhat.com> Date: Thu, 21 Mar 2013 14:24:37 -0500 Message-ID: <87ppysa74a.fsf@codemonkey.ws> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii 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: Alon Levy Cc: amit shah , hdegoede@redhat.com, kraxel@redhat.com, qemu-devel@nongnu.org Alon Levy writes: >> Alon Levy writes: >> >> > Note that the handler is called chr_is_guest_connected and not >> > chr_is_fe_connected, consistent with other members of >> > CharDriverState. >> >> Sorry, I don't get it. >> >> There isn't a notion of "connected" for the front-ends in the char >> layer. The closest thing is whether add_handlers() have been called >> or >> not. > > It makes sense for virtio-console - it matches exactly the internal > guest_connected port state. I still don't understand why you need to know that detail in the backend. Hint: you should explain this in future commit messages/cover letters. >> I really dislike the idea of introduction a new concept to the char >> layer in a half baked way. > > Is the fact there is only one user, virtio-console, the reason you > call this half baked? You are introducing a function: qemu_chr_be_is_virtio_console_connnected() And calling pretending like it's a generic character device interface. It's not. If spicevmc only works with virtio-console and has no hope of working with anything else, don't use the character device layer! It's trying to fit a square peg into a round hole. If it's a concept that makes sense for all character devices front ends, then it should be a patch making it be a meaningful to all character device front end implementations. >> Why can't migration notifiers be used for this? I think Gerd >> objected >> to using a migration *handler* but not necessarily a state notifier. > > Because if you have two chardevices, i.e. > -chardev spicevmc,name=vdagent,id=spice1 -chardev spicevmc,name=vdagent,id=spice2 > > Then the two on-the-wire vmstates will be identical. I don't understand why this matters but I don't understand what the problem you're trying to solve is either so that's not surprising. Regards, Anthony Liguori > >> >> Regards, >> >> Anthony Liguori >> >> > >> > Signed-off-by: Alon Levy >> > --- >> > hw/virtio-console.c | 9 +++++++++ >> > include/char/char.h | 11 +++++++++++ >> > qemu-char.c | 9 +++++++++ >> > 3 files changed, 29 insertions(+) >> > >> > diff --git a/hw/virtio-console.c b/hw/virtio-console.c >> > index e2d1c58..643e24e 100644 >> > --- a/hw/virtio-console.c >> > +++ b/hw/virtio-console.c >> > @@ -120,6 +120,13 @@ static void chr_event(void *opaque, int event) >> > } >> > } >> > >> > +static bool chr_is_guest_connected(void *opaque) >> > +{ >> > + VirtConsole *vcon = opaque; >> > + >> > + return vcon->port.guest_connected; >> > +} >> > + >> > static int virtconsole_initfn(VirtIOSerialPort *port) >> > { >> > VirtConsole *vcon = DO_UPCAST(VirtConsole, port, port); >> > @@ -133,6 +140,8 @@ static int virtconsole_initfn(VirtIOSerialPort >> > *port) >> > if (vcon->chr) { >> > qemu_chr_add_handlers(vcon->chr, chr_can_read, chr_read, >> > chr_event, >> > vcon); >> > + /* only user of chr_is_guest_connected so leave it as >> > special cased*/ >> > + vcon->chr->chr_is_guest_connected = >> > chr_is_guest_connected; >> > } >> > >> > return 0; >> > diff --git a/include/char/char.h b/include/char/char.h >> > index 0326b2a..b41ddc0 100644 >> > --- a/include/char/char.h >> > +++ b/include/char/char.h >> > @@ -52,6 +52,7 @@ typedef struct { >> > #define CHR_TIOCM_RTS 0x004 >> > >> > typedef void IOEventHandler(void *opaque, int event); >> > +typedef bool IOIsGuestConnectedHandler(void *opaque); >> > >> > struct CharDriverState { >> > void (*init)(struct CharDriverState *s); >> > @@ -64,6 +65,7 @@ struct CharDriverState { >> > IOEventHandler *chr_event; >> > IOCanReadHandler *chr_can_read; >> > IOReadHandler *chr_read; >> > + IOIsGuestConnectedHandler *chr_is_guest_connected; >> > void *handler_opaque; >> > void (*chr_close)(struct CharDriverState *chr); >> > void (*chr_accept_input)(struct CharDriverState *chr); >> > @@ -229,6 +231,15 @@ void qemu_chr_be_write(CharDriverState *s, >> > uint8_t *buf, int len); >> > */ >> > void qemu_chr_be_event(CharDriverState *s, int event); >> > >> > +/** >> > + * @qemu_chr_be_is_fe_connected: >> > + * >> > + * Back end calls this to check if the front end is connected. >> > + * >> > + * Returns: true if the guest (front end) is connected, false >> > otherwise. >> > + */ >> > +bool qemu_chr_be_is_fe_connected(CharDriverState *s); >> > + >> > void qemu_chr_add_handlers(CharDriverState *s, >> > IOCanReadHandler *fd_can_read, >> > IOReadHandler *fd_read, >> > diff --git a/qemu-char.c b/qemu-char.c >> > index 4e011df..77a501a 100644 >> > --- a/qemu-char.c >> > +++ b/qemu-char.c >> > @@ -120,6 +120,15 @@ void qemu_chr_be_event(CharDriverState *s, int >> > event) >> > s->chr_event(s->handler_opaque, event); >> > } >> > >> > +bool qemu_chr_be_is_fe_connected(CharDriverState *s) >> > +{ >> > + if (s->chr_is_guest_connected) { >> > + return s->chr_is_guest_connected(s->handler_opaque); >> > + } >> > + /* default to always connected */ >> > + return true; >> > +} >> > + >> > static gboolean qemu_chr_generic_open_bh(gpointer opaque) >> > { >> > CharDriverState *s = opaque; >> > -- >> > 1.8.1.4 >> >> >>