Re: [Qemu-devel] [PATCH 11/14] qio/chardev: return QIOTask when connect async

2018-02-28 Thread Peter Xu
On Wed, Feb 28, 2018 at 09:20:47AM +, Daniel P. Berrangé wrote:
> On Wed, Feb 28, 2018 at 01:06:30PM +0800, Peter Xu wrote:
> > Let qio_channel_socket_connect_async() return the created QIOTask object
> > for the async connection.  In tcp chardev, cache that in SocketChardev
> > for further use.  With the QIOTask refcount, this is pretty safe.
> 
> Why do you want to return QIOTask ? This is going against the intended
> design pattern for QIOTask (that was based on that in GLib). The task
> supposed to be an internal use only helper that callers should never
> be touching until the completion callback is invoked.

I proposed another solution in the other comment reply to split the
threaded QIO task into create() and run().  If you like that, I can
try.  Any other suggestion would be welcomed too.  Thanks,

-- 
Peter Xu



Re: [Qemu-devel] [PATCH 11/14] qio/chardev: return QIOTask when connect async

2018-02-28 Thread Daniel P . Berrangé
On Wed, Feb 28, 2018 at 01:06:30PM +0800, Peter Xu wrote:
> Let qio_channel_socket_connect_async() return the created QIOTask object
> for the async connection.  In tcp chardev, cache that in SocketChardev
> for further use.  With the QIOTask refcount, this is pretty safe.

Why do you want to return QIOTask ? This is going against the intended
design pattern for QIOTask (that was based on that in GLib). The task
supposed to be an internal use only helper that callers should never
be touching until the completion callback is invoked.


Regards,
Daniel
-- 
|: https://berrange.com  -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|



[Qemu-devel] [PATCH 11/14] qio/chardev: return QIOTask when connect async

2018-02-27 Thread Peter Xu
Let qio_channel_socket_connect_async() return the created QIOTask object
for the async connection.  In tcp chardev, cache that in SocketChardev
for further use.  With the QIOTask refcount, this is pretty safe.

Since at it, generalize out tcp_chr_socket_connect_async() since the
logic is used in both initial phase and reconnect timeout.

Signed-off-by: Peter Xu 
---
 chardev/char-socket.c   | 33 ++---
 include/io/channel-socket.h | 14 +-
 io/channel-socket.c | 12 +++-
 3 files changed, 38 insertions(+), 21 deletions(-)

diff --git a/chardev/char-socket.c b/chardev/char-socket.c
index a16d894c40..9d51b8da07 100644
--- a/chardev/char-socket.c
+++ b/chardev/char-socket.c
@@ -64,6 +64,7 @@ typedef struct {
 GSource *reconnect_timer;
 int64_t reconnect_time;
 bool connect_err_reported;
+QIOTask *thread_task;
 } SocketChardev;
 
 #define SOCKET_CHARDEV(obj) \
@@ -879,14 +880,32 @@ static void qemu_chr_socket_connected(QIOTask *task, void 
*opaque)
 tcp_chr_new_client(chr, sioc);
 
 cleanup:
+assert(s->thread_task == task);
+qio_task_unref(task);
+s->thread_task = NULL;
 object_unref(OBJECT(sioc));
 }
 
+static void tcp_chr_socket_connect_async(SocketChardev *s)
+{
+QIOChannelSocket *sioc = qio_channel_socket_new();
+Chardev *chr = CHARDEV(s);
+QIOTask *task;
+
+assert(s->thread_task == NULL);
+
+tcp_chr_set_client_ioc_name(chr, sioc);
+task = qio_channel_socket_connect_async(sioc, s->addr,
+qemu_chr_socket_connected,
+chr, NULL);
+qio_task_ref(task);
+s->thread_task = task;
+}
+
 static gboolean socket_reconnect_timeout(gpointer opaque)
 {
 Chardev *chr = CHARDEV(opaque);
 SocketChardev *s = SOCKET_CHARDEV(opaque);
-QIOChannelSocket *sioc;
 
 g_source_unref(s->reconnect_timer);
 s->reconnect_timer = NULL;
@@ -895,11 +914,7 @@ static gboolean socket_reconnect_timeout(gpointer opaque)
 return false;
 }
 
-sioc = qio_channel_socket_new();
-tcp_chr_set_client_ioc_name(chr, sioc);
-qio_channel_socket_connect_async(sioc, s->addr,
- qemu_chr_socket_connected,
- chr, NULL);
+tcp_chr_socket_connect_async(s);
 
 return false;
 }
@@ -979,11 +994,7 @@ static void qmp_chardev_open_socket(Chardev *chr,
 }
 
 if (s->reconnect_time) {
-sioc = qio_channel_socket_new();
-tcp_chr_set_client_ioc_name(chr, sioc);
-qio_channel_socket_connect_async(sioc, s->addr,
- qemu_chr_socket_connected,
- chr, NULL);
+tcp_chr_socket_connect_async(s);
 } else {
 if (s->is_listen) {
 char *name;
diff --git a/include/io/channel-socket.h b/include/io/channel-socket.h
index 53801f6042..5cfa9e2b7c 100644
--- a/include/io/channel-socket.h
+++ b/include/io/channel-socket.h
@@ -108,12 +108,16 @@ int qio_channel_socket_connect_sync(QIOChannelSocket *ioc,
  * will be invoked on completion or failure. The @addr
  * parameter will be copied, so may be freed as soon
  * as this function returns without waiting for completion.
+ *
+ * Returns the IOTask created.  NOTE: if the caller is going to use
+ * the returned QIOTask, the caller is responsible to reference the
+ * task and unref it when it's not needed any more.
  */
-void qio_channel_socket_connect_async(QIOChannelSocket *ioc,
-  SocketAddress *addr,
-  QIOTaskFunc callback,
-  gpointer opaque,
-  GDestroyNotify destroy);
+QIOTask *qio_channel_socket_connect_async(QIOChannelSocket *ioc,
+  SocketAddress *addr,
+  QIOTaskFunc callback,
+  gpointer opaque,
+  GDestroyNotify destroy);
 
 
 /**
diff --git a/io/channel-socket.c b/io/channel-socket.c
index 4224ce323a..f420502290 100644
--- a/io/channel-socket.c
+++ b/io/channel-socket.c
@@ -169,11 +169,11 @@ static void qio_channel_socket_connect_worker(QIOTask 
*task,
 }
 
 
-void qio_channel_socket_connect_async(QIOChannelSocket *ioc,
-  SocketAddress *addr,
-  QIOTaskFunc callback,
-  gpointer opaque,
-  GDestroyNotify destroy)
+QIOTask *qio_channel_socket_connect_async(QIOChannelSocket *ioc,
+  SocketAddress *addr,
+  QIOTaskFunc callback,
+  gpointer opaque,
+