Patchwork [2/9] char: introduce tcp_chr_detach()

login
register
mail settings
Submitter Amit Shah
Date Aug. 28, 2013, 5:10 a.m.
Message ID <f1a7d37fcd2b6fb21d4f536628aad0cd67521ff5.1377666450.git.amit.shah@redhat.com>
Download mbox | patch
Permalink /patch/270330/
State New
Headers show

Comments

Amit Shah - Aug. 28, 2013, 5:10 a.m.
Remove any registered callbacks if a frontend is detached.

CC: <qemu-stable@nongnu.org>
Signed-off-by: Amit Shah <amit.shah@redhat.com>
---
 qemu-char.c | 11 +++++++++++
 1 file changed, 11 insertions(+)
Gerd Hoffmann - Aug. 28, 2013, 7:09 a.m.
Hi,

> +static void tcp_chr_detach(CharDriverState *chr)
> +{
> +    TCPCharDriver *s = chr->opaque;
> +
> +    if (s->tag) {
> +        io_remove_watch_poll(s->tag);
> +        s->tag = 0;
> +    }
> +}

Lots of simliar functions in the other patches.

Doesn't it make sense to move the tag field from TCPCharDriver to
CharDriverState instead, so we don't need a new callback in the first
place?

cheers,
  Gerd
Amit Shah - Aug. 28, 2013, 8:10 a.m.
On (Wed) 28 Aug 2013 [09:09:47], Gerd Hoffmann wrote:
>   Hi,
> 
> > +static void tcp_chr_detach(CharDriverState *chr)
> > +{
> > +    TCPCharDriver *s = chr->opaque;
> > +
> > +    if (s->tag) {
> > +        io_remove_watch_poll(s->tag);
> > +        s->tag = 0;
> > +    }
> > +}
> 
> Lots of simliar functions in the other patches.
> 
> Doesn't it make sense to move the tag field from TCPCharDriver to
> CharDriverState instead, so we don't need a new callback in the first
> place?

Yep, I thought about it, but it might get tricky to handle it:
tcp needs two, one for listening sockets and one for connected ones.

We don't need to worry about the listening socket for this patchset,
should we then just keep that in the tcp struct, and use the tag as
the generic one in CharDriverState for all of the backends?

		Amit
Gerd Hoffmann - Aug. 28, 2013, 11:38 a.m.
> We don't need to worry about the listening socket for this patchset,
> should we then just keep that in the tcp struct, and use the tag as
> the generic one in CharDriverState for all of the backends?

Yes, I think that will simplify the series.  And maybe name the one one
in CharDriverState 'fd_in_tag' to make more clear what it is.

cheers,
  Gerd

Patch

diff --git a/qemu-char.c b/qemu-char.c
index f27fdb6..e235334 100644
--- a/qemu-char.c
+++ b/qemu-char.c
@@ -2321,6 +2321,16 @@  typedef struct {
     int msgfd;
 } TCPCharDriver;
 
+static void tcp_chr_detach(CharDriverState *chr)
+{
+    TCPCharDriver *s = chr->opaque;
+
+    if (s->tag) {
+        io_remove_watch_poll(s->tag);
+        s->tag = 0;
+    }
+}
+
 static gboolean tcp_chr_accept(GIOChannel *chan, GIOCondition cond, void *opaque);
 
 static int tcp_chr_write(CharDriverState *chr, const uint8_t *buf, int len)
@@ -2689,6 +2699,7 @@  static CharDriverState *qemu_chr_open_socket_fd(int fd, bool do_nodelay,
     chr->opaque = s;
     chr->chr_write = tcp_chr_write;
     chr->chr_close = tcp_chr_close;
+    chr->chr_detach = tcp_chr_detach;
     chr->get_msgfd = tcp_get_msgfd;
     chr->chr_add_client = tcp_chr_add_client;
     chr->chr_add_watch = tcp_chr_add_watch;