* [Qemu-devel] [PATCH 1/8] sockets: add ability to disable DNS resolution for InetSocketAddress
2017-01-05 16:03 [Qemu-devel] [PATCH 0/8] io: enable DNS resolving separately of socket create Daniel P. Berrange
@ 2017-01-05 16:03 ` Daniel P. Berrange
2017-01-05 16:22 ` Eric Blake
2017-01-05 16:03 ` [Qemu-devel] [PATCH 2/8] io: stop incrementing reference in qio_task_get_source Daniel P. Berrange
` (6 subsequent siblings)
7 siblings, 1 reply; 22+ messages in thread
From: Daniel P. Berrange @ 2017-01-05 16:03 UTC (permalink / raw)
To: qemu-devel
Add a 'numeric' flag to the InetSocketAddress struct to allow the
caller to indicate that DNS should be skipped for the host/port
fields. This is useful if the caller knows the address is already
numeric and wants to guarantee no (potentially blocking) DNS
lookups are attempted.
Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
---
qapi-schema.json | 5 +++++
util/qemu-sockets.c | 3 +++
2 files changed, 8 insertions(+)
diff --git a/qapi-schema.json b/qapi-schema.json
index a0d3b5d..d605c1e 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -3070,6 +3070,10 @@
#
# @port: port part of the address, or lowest port if @to is present
#
+# @numeric: #optional true if the host/port are guaranteed to be numeric,
+# false if name resolution should be attempted. Defaults to false.
+# Since 2.8
+#
# @to: highest port to try
#
# @ipv4: whether to accept IPv4 addresses, default try both IPv4 and IPv6
@@ -3084,6 +3088,7 @@
'data': {
'host': 'str',
'port': 'str',
+ 'numeric': 'bool',
'*to': 'uint16',
'*ipv4': 'bool',
'*ipv6': 'bool' } }
diff --git a/util/qemu-sockets.c b/util/qemu-sockets.c
index fe1d07a..6f1c10a 100644
--- a/util/qemu-sockets.c
+++ b/util/qemu-sockets.c
@@ -141,6 +141,9 @@ static int inet_listen_saddr(InetSocketAddress *saddr,
memset(&ai,0, sizeof(ai));
ai.ai_flags = AI_PASSIVE;
+ if (saddr->numeric) {
+ ai.ai_flags |= AI_NUMERICHOST | AI_NUMERICSERV;
+ }
ai.ai_family = inet_ai_family_from_address(saddr, &err);
ai.ai_socktype = SOCK_STREAM;
--
2.9.3
^ permalink raw reply related [flat|nested] 22+ messages in thread* Re: [Qemu-devel] [PATCH 1/8] sockets: add ability to disable DNS resolution for InetSocketAddress
2017-01-05 16:03 ` [Qemu-devel] [PATCH 1/8] sockets: add ability to disable DNS resolution for InetSocketAddress Daniel P. Berrange
@ 2017-01-05 16:22 ` Eric Blake
2017-01-05 16:42 ` Daniel P. Berrange
0 siblings, 1 reply; 22+ messages in thread
From: Eric Blake @ 2017-01-05 16:22 UTC (permalink / raw)
To: Daniel P. Berrange, qemu-devel
[-- Attachment #1: Type: text/plain, Size: 2504 bytes --]
On 01/05/2017 10:03 AM, Daniel P. Berrange wrote:
> Add a 'numeric' flag to the InetSocketAddress struct to allow the
> caller to indicate that DNS should be skipped for the host/port
> fields. This is useful if the caller knows the address is already
> numeric and wants to guarantee no (potentially blocking) DNS
> lookups are attempted.
>
> Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
> ---
> qapi-schema.json | 5 +++++
> util/qemu-sockets.c | 3 +++
> 2 files changed, 8 insertions(+)
>
> diff --git a/qapi-schema.json b/qapi-schema.json
> index a0d3b5d..d605c1e 100644
> --- a/qapi-schema.json
> +++ b/qapi-schema.json
> @@ -3070,6 +3070,10 @@
> #
> # @port: port part of the address, or lowest port if @to is present
> #
> +# @numeric: #optional true if the host/port are guaranteed to be numeric,
> +# false if name resolution should be attempted. Defaults to false.
> +# Since 2.8
2.9, actually. I'm also not sure if Marc-Andre's work requires ()
brackets around the since designation on a per-member listing.
> @@ -3084,6 +3088,7 @@
> 'data': {
> 'host': 'str',
> 'port': 'str',
> + 'numeric': 'bool',
In order to be optional, it must be spelled '*numeric'.
> '*to': 'uint16',
> '*ipv4': 'bool',
> '*ipv6': 'bool' } }
Thinking out loud: Do we even need a 'numeric' field? If we could
create an alternate type that distinguishes between a 'str' (name, DNS
resolution required) and an 'int' (numeric, skip DNS), then we don't
need a field. Except that an 'int' for IPv4 addresses is awkward, and
an 'int' for IPv6 addresses in insufficient - so even when the address
is numeric, you STILL have to pass it in as 'str'. Okay, experiment
failed, your interface seems like the correct thing to do.
> diff --git a/util/qemu-sockets.c b/util/qemu-sockets.c
> index fe1d07a..6f1c10a 100644
> --- a/util/qemu-sockets.c
> +++ b/util/qemu-sockets.c
> @@ -141,6 +141,9 @@ static int inet_listen_saddr(InetSocketAddress *saddr,
>
> memset(&ai,0, sizeof(ai));
> ai.ai_flags = AI_PASSIVE;
> + if (saddr->numeric) {
if (saddr->has_numeric && saddr->numeric)
> + ai.ai_flags |= AI_NUMERICHOST | AI_NUMERICSERV;
> + }
> ai.ai_family = inet_ai_family_from_address(saddr, &err);
> ai.ai_socktype = SOCK_STREAM;
>
>
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]
^ permalink raw reply [flat|nested] 22+ messages in thread* Re: [Qemu-devel] [PATCH 1/8] sockets: add ability to disable DNS resolution for InetSocketAddress
2017-01-05 16:22 ` Eric Blake
@ 2017-01-05 16:42 ` Daniel P. Berrange
0 siblings, 0 replies; 22+ messages in thread
From: Daniel P. Berrange @ 2017-01-05 16:42 UTC (permalink / raw)
To: Eric Blake; +Cc: qemu-devel
On Thu, Jan 05, 2017 at 10:22:43AM -0600, Eric Blake wrote:
> On 01/05/2017 10:03 AM, Daniel P. Berrange wrote:
> > Add a 'numeric' flag to the InetSocketAddress struct to allow the
> > caller to indicate that DNS should be skipped for the host/port
> > fields. This is useful if the caller knows the address is already
> > numeric and wants to guarantee no (potentially blocking) DNS
> > lookups are attempted.
> >
> > Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
> > ---
> > qapi-schema.json | 5 +++++
> > util/qemu-sockets.c | 3 +++
> > 2 files changed, 8 insertions(+)
> >
> > diff --git a/qapi-schema.json b/qapi-schema.json
> > index a0d3b5d..d605c1e 100644
> > --- a/qapi-schema.json
> > +++ b/qapi-schema.json
> > @@ -3070,6 +3070,10 @@
> > #
> > # @port: port part of the address, or lowest port if @to is present
> > #
> > +# @numeric: #optional true if the host/port are guaranteed to be numeric,
> > +# false if name resolution should be attempted. Defaults to false.
> > +# Since 2.8
>
> 2.9, actually. I'm also not sure if Marc-Andre's work requires ()
> brackets around the since designation on a per-member listing.
>
> > @@ -3084,6 +3088,7 @@
> > 'data': {
> > 'host': 'str',
> > 'port': 'str',
> > + 'numeric': 'bool',
>
> In order to be optional, it must be spelled '*numeric'.
Sigh, yes.
> > '*to': 'uint16',
> > '*ipv4': 'bool',
> > '*ipv6': 'bool' } }
>
> Thinking out loud: Do we even need a 'numeric' field? If we could
> create an alternate type that distinguishes between a 'str' (name, DNS
> resolution required) and an 'int' (numeric, skip DNS), then we don't
> need a field. Except that an 'int' for IPv4 addresses is awkward, and
> an 'int' for IPv6 addresses in insufficient - so even when the address
> is numeric, you STILL have to pass it in as 'str'. Okay, experiment
> failed, your interface seems like the correct thing to do.
Basically think of this 'numeric' flag as being the same as
the AI_NUMERIC flag to getaddrinfo. You're still using the
string format as input so that you can reuse existing code
for converting from user friendly string format to raw
byte format.
Regards,
Daniel
--
|: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org -o- http://virt-manager.org :|
|: http://entangle-photo.org -o- http://search.cpan.org/~danberr/ :|
^ permalink raw reply [flat|nested] 22+ messages in thread
* [Qemu-devel] [PATCH 2/8] io: stop incrementing reference in qio_task_get_source
2017-01-05 16:03 [Qemu-devel] [PATCH 0/8] io: enable DNS resolving separately of socket create Daniel P. Berrange
2017-01-05 16:03 ` [Qemu-devel] [PATCH 1/8] sockets: add ability to disable DNS resolution for InetSocketAddress Daniel P. Berrange
@ 2017-01-05 16:03 ` Daniel P. Berrange
2017-01-05 16:30 ` Eric Blake
2017-01-05 16:03 ` [Qemu-devel] [PATCH 3/8] io: fix typo in docs for QIOTask Daniel P. Berrange
` (5 subsequent siblings)
7 siblings, 1 reply; 22+ messages in thread
From: Daniel P. Berrange @ 2017-01-05 16:03 UTC (permalink / raw)
To: qemu-devel
Incrementing the reference in qio_task_get_source is
not neccessary, since we're not running concurrently
with any other code touching the QIOTask. This
minimizes chances of further memory leaks.
Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
---
include/io/task.h | 7 ++++---
io/channel-socket.c | 3 ---
io/channel-tls.c | 2 --
io/task.c | 1 -
tests/test-io-task.c | 1 -
5 files changed, 4 insertions(+), 10 deletions(-)
diff --git a/include/io/task.h b/include/io/task.h
index 42028cb..c268eb0 100644
--- a/include/io/task.h
+++ b/include/io/task.h
@@ -244,9 +244,10 @@ void qio_task_abort(QIOTask *task,
* @task: the task struct
*
* Get the source object associated with the background
- * task. This returns a new reference to the object,
- * which the caller must released with object_unref()
- * when no longer required.
+ * task. The caller does not own a reference on the
+ * returned Object, and so should call object_ref()
+ * if it wants to keep the object pointer outside the
+ * lifetime of the QIOTask object.
*
* Returns: the source object
*/
diff --git a/io/channel-socket.c b/io/channel-socket.c
index d7e03f6..45df819 100644
--- a/io/channel-socket.c
+++ b/io/channel-socket.c
@@ -168,7 +168,6 @@ static int qio_channel_socket_connect_worker(QIOTask *task,
addr,
errp);
- object_unref(OBJECT(ioc));
return ret;
}
@@ -231,7 +230,6 @@ static int qio_channel_socket_listen_worker(QIOTask *task,
addr,
errp);
- object_unref(OBJECT(ioc));
return ret;
}
@@ -309,7 +307,6 @@ static int qio_channel_socket_dgram_worker(QIOTask *task,
data->remoteAddr,
errp);
- object_unref(OBJECT(ioc));
return ret;
}
diff --git a/io/channel-tls.c b/io/channel-tls.c
index d24dc8c..cf3bcca 100644
--- a/io/channel-tls.c
+++ b/io/channel-tls.c
@@ -200,8 +200,6 @@ static gboolean qio_channel_tls_handshake_io(QIOChannel *ioc,
qio_channel_tls_handshake_task(
tioc, task);
- object_unref(OBJECT(tioc));
-
return FALSE;
}
diff --git a/io/task.c b/io/task.c
index c7f97a9..a763990 100644
--- a/io/task.c
+++ b/io/task.c
@@ -156,6 +156,5 @@ void qio_task_abort(QIOTask *task,
Object *qio_task_get_source(QIOTask *task)
{
- object_ref(task->source);
return task->source;
}
diff --git a/tests/test-io-task.c b/tests/test-io-task.c
index e091c12..024eb58 100644
--- a/tests/test-io-task.c
+++ b/tests/test-io-task.c
@@ -76,7 +76,6 @@ static void test_task_complete(void)
g_assert(obj == src);
object_unref(obj);
- object_unref(src);
g_assert(data.source == obj);
g_assert(data.err == NULL);
--
2.9.3
^ permalink raw reply related [flat|nested] 22+ messages in thread* [Qemu-devel] [PATCH 3/8] io: fix typo in docs for QIOTask
2017-01-05 16:03 [Qemu-devel] [PATCH 0/8] io: enable DNS resolving separately of socket create Daniel P. Berrange
2017-01-05 16:03 ` [Qemu-devel] [PATCH 1/8] sockets: add ability to disable DNS resolution for InetSocketAddress Daniel P. Berrange
2017-01-05 16:03 ` [Qemu-devel] [PATCH 2/8] io: stop incrementing reference in qio_task_get_source Daniel P. Berrange
@ 2017-01-05 16:03 ` Daniel P. Berrange
2017-01-05 20:29 ` Eric Blake
2017-01-05 16:03 ` [Qemu-devel] [PATCH 4/8] io: add ability to associate an opaque "result" with with a task Daniel P. Berrange
` (4 subsequent siblings)
7 siblings, 1 reply; 22+ messages in thread
From: Daniel P. Berrange @ 2017-01-05 16:03 UTC (permalink / raw)
To: qemu-devel
The GDestroyNotify parameter is already a pointer, so does
not need a '*' suffix on the type.
Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
---
include/io/task.h | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/include/io/task.h b/include/io/task.h
index c268eb0..1407747 100644
--- a/include/io/task.h
+++ b/include/io/task.h
@@ -49,7 +49,7 @@ typedef int (*QIOTaskWorker)(QIOTask *task,
* void myobject_operation(QMyObject *obj,
* QIOTaskFunc *func,
* gpointer opaque,
- * GDestroyNotify *notify);
+ * GDestroyNotify notify);
* </programlisting>
* </example>
*
@@ -67,7 +67,7 @@ typedef int (*QIOTaskWorker)(QIOTask *task,
* void myobject_operation(QMyObject *obj,
* QIOTaskFunc *func,
* gpointer opaque,
- * GDestroyNotify *notify)
+ * GDestroyNotify notify)
* {
* QIOTask *task;
*
@@ -154,7 +154,7 @@ typedef int (*QIOTaskWorker)(QIOTask *task,
* SocketAddress *addr,
* QIOTaskFunc *func,
* gpointer opaque,
- * GDestroyNotify *notify)
+ * GDestroyNotify notify)
* {
* QIOTask *task;
* SocketAddress *addrCopy;
--
2.9.3
^ permalink raw reply related [flat|nested] 22+ messages in thread* [Qemu-devel] [PATCH 4/8] io: add ability to associate an opaque "result" with with a task
2017-01-05 16:03 [Qemu-devel] [PATCH 0/8] io: enable DNS resolving separately of socket create Daniel P. Berrange
` (2 preceding siblings ...)
2017-01-05 16:03 ` [Qemu-devel] [PATCH 3/8] io: fix typo in docs for QIOTask Daniel P. Berrange
@ 2017-01-05 16:03 ` Daniel P. Berrange
2017-01-05 20:32 ` Eric Blake
2017-01-05 16:03 ` [Qemu-devel] [PATCH 5/8] io: add ability to associate an error " Daniel P. Berrange
` (3 subsequent siblings)
7 siblings, 1 reply; 22+ messages in thread
From: Daniel P. Berrange @ 2017-01-05 16:03 UTC (permalink / raw)
To: qemu-devel
Currently there is no data associated with a successful
task completion. This adds an opaque pointer to the task
to store an arbitrary result.
Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
---
include/io/task.h | 27 +++++++++++++++++++++++++++
io/task.c | 20 ++++++++++++++++++++
2 files changed, 47 insertions(+)
diff --git a/include/io/task.h b/include/io/task.h
index 1407747..ece1372 100644
--- a/include/io/task.h
+++ b/include/io/task.h
@@ -240,6 +240,33 @@ void qio_task_abort(QIOTask *task,
/**
+ * qio_task_set_result_pointer:
+ * @task: the task struct
+ * @result: pointer to the result data
+ *
+ * Associate an opaque result with the task,
+ * which can later be retrieved with the
+ * qio_task_get_result_pointer() method
+ *
+ */
+void qio_task_set_result_pointer(QIOTask *task,
+ gpointer result,
+ GDestroyNotify notify);
+
+
+/**
+ * qio_task_get_result_pointer:
+ * @task: the task struct
+ *
+ * Retrieve the opaque result data associated
+ * with the task, if any.
+ *
+ * Returns: the task result, or NULL
+ */
+gpointer qio_task_get_result_pointer(QIOTask *task);
+
+
+/**
* qio_task_get_source:
* @task: the task struct
*
diff --git a/io/task.c b/io/task.c
index a763990..675e196 100644
--- a/io/task.c
+++ b/io/task.c
@@ -29,6 +29,8 @@ struct QIOTask {
QIOTaskFunc func;
gpointer opaque;
GDestroyNotify destroy;
+ gpointer result;
+ GDestroyNotify destroyResult;
};
@@ -57,6 +59,9 @@ static void qio_task_free(QIOTask *task)
if (task->destroy) {
task->destroy(task->opaque);
}
+ if (task->destroyResult) {
+ task->destroyResult(task->result);
+ }
object_unref(task->source);
g_free(task);
@@ -154,6 +159,21 @@ void qio_task_abort(QIOTask *task,
}
+void qio_task_set_result_pointer(QIOTask *task,
+ gpointer result,
+ GDestroyNotify destroy)
+{
+ task->result = result;
+ task->destroyResult = destroy;
+}
+
+
+gpointer qio_task_get_result_pointer(QIOTask *task)
+{
+ return task->result;
+}
+
+
Object *qio_task_get_source(QIOTask *task)
{
return task->source;
--
2.9.3
^ permalink raw reply related [flat|nested] 22+ messages in thread* Re: [Qemu-devel] [PATCH 4/8] io: add ability to associate an opaque "result" with with a task
2017-01-05 16:03 ` [Qemu-devel] [PATCH 4/8] io: add ability to associate an opaque "result" with with a task Daniel P. Berrange
@ 2017-01-05 20:32 ` Eric Blake
2017-01-06 9:14 ` Daniel P. Berrange
0 siblings, 1 reply; 22+ messages in thread
From: Eric Blake @ 2017-01-05 20:32 UTC (permalink / raw)
To: Daniel P. Berrange, qemu-devel
[-- Attachment #1: Type: text/plain, Size: 811 bytes --]
On 01/05/2017 10:03 AM, Daniel P. Berrange wrote:
> Currently there is no data associated with a successful
> task completion. This adds an opaque pointer to the task
> to store an arbitrary result.
>
> Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
> ---
> include/io/task.h | 27 +++++++++++++++++++++++++++
> io/task.c | 20 ++++++++++++++++++++
> 2 files changed, 47 insertions(+)
>
I suppose a different approach would have been changing QIOTaskFunc() to
have a void* return, but that would be more invasive. Grabbing the
result separately from the task completing is a bit weird, but looks
like it will work.
Reviewed-by: Eric Blake <eblake@redhat.com>
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [Qemu-devel] [PATCH 4/8] io: add ability to associate an opaque "result" with with a task
2017-01-05 20:32 ` Eric Blake
@ 2017-01-06 9:14 ` Daniel P. Berrange
0 siblings, 0 replies; 22+ messages in thread
From: Daniel P. Berrange @ 2017-01-06 9:14 UTC (permalink / raw)
To: Eric Blake; +Cc: qemu-devel
On Thu, Jan 05, 2017 at 02:32:46PM -0600, Eric Blake wrote:
> On 01/05/2017 10:03 AM, Daniel P. Berrange wrote:
> > Currently there is no data associated with a successful
> > task completion. This adds an opaque pointer to the task
> > to store an arbitrary result.
> >
> > Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
> > ---
> > include/io/task.h | 27 +++++++++++++++++++++++++++
> > io/task.c | 20 ++++++++++++++++++++
> > 2 files changed, 47 insertions(+)
> >
>
> I suppose a different approach would have been changing QIOTaskFunc() to
> have a void* return, but that would be more invasive. Grabbing the
> result separately from the task completing is a bit weird, but looks
> like it will work.
FYI, the design I've chosen here closely matches that used by GLib in its
own GTask / GAsyncResult, on which the QIOTask was modelled. The idea is
that in future when we eventually bump to a newer glib, we can switch
over to GTask easily.
Regards,
Daniel
--
|: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org -o- http://virt-manager.org :|
|: http://entangle-photo.org -o- http://search.cpan.org/~danberr/ :|
^ permalink raw reply [flat|nested] 22+ messages in thread
* [Qemu-devel] [PATCH 5/8] io: add ability to associate an error with a task
2017-01-05 16:03 [Qemu-devel] [PATCH 0/8] io: enable DNS resolving separately of socket create Daniel P. Berrange
` (3 preceding siblings ...)
2017-01-05 16:03 ` [Qemu-devel] [PATCH 4/8] io: add ability to associate an opaque "result" with with a task Daniel P. Berrange
@ 2017-01-05 16:03 ` Daniel P. Berrange
2017-01-05 21:03 ` Eric Blake
2017-01-05 16:03 ` [Qemu-devel] [PATCH 6/8] io: change the QIOTask callback signature Daniel P. Berrange
` (2 subsequent siblings)
7 siblings, 1 reply; 22+ messages in thread
From: Daniel P. Berrange @ 2017-01-05 16:03 UTC (permalink / raw)
To: qemu-devel
Currently when a task fails, the error is never explicitly
associated with the task object, it is just passed along
through the completion callback. This adds ability to
explicitly associate an error with the task.
Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
---
include/io/task.h | 29 +++++++++++++++++++++++++++++
io/task.c | 23 +++++++++++++++++++++++
2 files changed, 52 insertions(+)
diff --git a/include/io/task.h b/include/io/task.h
index ece1372..244c1a1 100644
--- a/include/io/task.h
+++ b/include/io/task.h
@@ -240,6 +240,35 @@ void qio_task_abort(QIOTask *task,
/**
+ * qio_task_set_error:
+ * @task: the task struct
+ * @err: pointer to the error
+ *
+ * Associate an error with the task, which can later
+ * be retrieved with the qio_task_propagate_error()
+ * method. This method takes ownership of @err, so
+ * it is not valid to access it after this call
+ * completes.
+ */
+void qio_task_set_error(QIOTask *task,
+ Error *err);
+
+
+/**
+ * qio_task_propagate_error:
+ * @task: the task struct
+ * @errp: pointer to a NULL-initialized error object
+ *
+ * Propagate the error associated with @task
+ * into @errp.
+ *
+ * Returns: true if an error was propagated, false otherwise
+ */
+gboolean qio_task_propagate_error(QIOTask *task,
+ Error **errp);
+
+
+/**
* qio_task_set_result_pointer:
* @task: the task struct
* @result: pointer to the result data
diff --git a/io/task.c b/io/task.c
index 675e196..1136c75 100644
--- a/io/task.c
+++ b/io/task.c
@@ -29,6 +29,7 @@ struct QIOTask {
QIOTaskFunc func;
gpointer opaque;
GDestroyNotify destroy;
+ Error *err;
gpointer result;
GDestroyNotify destroyResult;
};
@@ -62,6 +63,9 @@ static void qio_task_free(QIOTask *task)
if (task->destroyResult) {
task->destroyResult(task->result);
}
+ if (task->err) {
+ error_free(task->err);
+ }
object_unref(task->source);
g_free(task);
@@ -159,6 +163,25 @@ void qio_task_abort(QIOTask *task,
}
+void qio_task_set_error(QIOTask *task,
+ Error *err)
+{
+ error_propagate(&task->err, err);
+}
+
+
+gboolean qio_task_propagate_error(QIOTask *task,
+ Error **errp)
+{
+ if (task->err) {
+ error_propagate(errp, task->err);
+ return TRUE;
+ }
+
+ return FALSE;
+}
+
+
void qio_task_set_result_pointer(QIOTask *task,
gpointer result,
GDestroyNotify destroy)
--
2.9.3
^ permalink raw reply related [flat|nested] 22+ messages in thread* Re: [Qemu-devel] [PATCH 5/8] io: add ability to associate an error with a task
2017-01-05 16:03 ` [Qemu-devel] [PATCH 5/8] io: add ability to associate an error " Daniel P. Berrange
@ 2017-01-05 21:03 ` Eric Blake
2017-01-06 9:16 ` Daniel P. Berrange
0 siblings, 1 reply; 22+ messages in thread
From: Eric Blake @ 2017-01-05 21:03 UTC (permalink / raw)
To: Daniel P. Berrange, qemu-devel
[-- Attachment #1: Type: text/plain, Size: 4002 bytes --]
On 01/05/2017 10:03 AM, Daniel P. Berrange wrote:
> Currently when a task fails, the error is never explicitly
> associated with the task object, it is just passed along
> through the completion callback. This adds ability to
s/adds/adds the/
> explicitly associate an error with the task.
>
> Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
> ---
> include/io/task.h | 29 +++++++++++++++++++++++++++++
> io/task.c | 23 +++++++++++++++++++++++
> 2 files changed, 52 insertions(+)
>
> diff --git a/include/io/task.h b/include/io/task.h
> index ece1372..244c1a1 100644
> --- a/include/io/task.h
> +++ b/include/io/task.h
> @@ -240,6 +240,35 @@ void qio_task_abort(QIOTask *task,
>
>
> /**
> + * qio_task_set_error:
> + * @task: the task struct
> + * @err: pointer to the error
> + *
> + * Associate an error with the task, which can later
> + * be retrieved with the qio_task_propagate_error()
> + * method. This method takes ownership of @err, so
> + * it is not valid to access it after this call
> + * completes.
> + */
> +void qio_task_set_error(QIOTask *task,
> + Error *err);
Is it valid for @err to be NULL (or put another way, may callers blindly
call this function whether or not an error has occurred)?...
> +
> +
> +/**
> + * qio_task_propagate_error:
> + * @task: the task struct
> + * @errp: pointer to a NULL-initialized error object
> + *
> + * Propagate the error associated with @task
> + * into @errp.
> + *
> + * Returns: true if an error was propagated, false otherwise
> + */
> +gboolean qio_task_propagate_error(QIOTask *task,
Must this return 'gboolean' (in which case the docs should mention
TRUE/FALSE rather than true/false)? Or can we make it return the nicer
'bool'?
> + Error **errp);
> +
> +
> +/**
> * qio_task_set_result_pointer:
> * @task: the task struct
> * @result: pointer to the result data
> diff --git a/io/task.c b/io/task.c
> index 675e196..1136c75 100644
> --- a/io/task.c
> +++ b/io/task.c
> @@ -29,6 +29,7 @@ struct QIOTask {
> QIOTaskFunc func;
> gpointer opaque;
> GDestroyNotify destroy;
> + Error *err;
> gpointer result;
> GDestroyNotify destroyResult;
> };
> @@ -62,6 +63,9 @@ static void qio_task_free(QIOTask *task)
> if (task->destroyResult) {
> task->destroyResult(task->result);
> }
> + if (task->err) {
> + error_free(task->err);
> + }
> object_unref(task->source);
>
> g_free(task);
> @@ -159,6 +163,25 @@ void qio_task_abort(QIOTask *task,
> }
>
>
> +void qio_task_set_error(QIOTask *task,
> + Error *err)
> +{
> + error_propagate(&task->err, err);
...As written, qio_task_set_error(task, NULL) is thus valid as a no-op;
and in turn, qio_task_set_error(task, err) is unconditionally valid a
cleanup path regardless of whether the cleanup was reached on success
(err is still NULL) or failure (err is set). But it's worth documenting.
In fact, error_propagate() can be called more than once (first call
wins, all later calls silently drop the subsequent error in favor of a
first one already present), and therefore qio_task_set_error() gains
that same property. Worth documenting?
> +}
> +
> +
> +gboolean qio_task_propagate_error(QIOTask *task,
> + Error **errp)
> +{
> + if (task->err) {
> + error_propagate(errp, task->err);
> + return TRUE;
> + }
> +
> + return FALSE;
Again, I think a 'bool' return (and true/false) is nicer.
> +}
> +
> +
> void qio_task_set_result_pointer(QIOTask *task,
> gpointer result,
> GDestroyNotify destroy)
>
The idea makes sense, though.
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]
^ permalink raw reply [flat|nested] 22+ messages in thread* Re: [Qemu-devel] [PATCH 5/8] io: add ability to associate an error with a task
2017-01-05 21:03 ` Eric Blake
@ 2017-01-06 9:16 ` Daniel P. Berrange
0 siblings, 0 replies; 22+ messages in thread
From: Daniel P. Berrange @ 2017-01-06 9:16 UTC (permalink / raw)
To: Eric Blake; +Cc: qemu-devel
On Thu, Jan 05, 2017 at 03:03:08PM -0600, Eric Blake wrote:
> On 01/05/2017 10:03 AM, Daniel P. Berrange wrote:
> > Currently when a task fails, the error is never explicitly
> > associated with the task object, it is just passed along
> > through the completion callback. This adds ability to
>
> s/adds/adds the/
>
> > explicitly associate an error with the task.
> >
> > Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
> > ---
> > include/io/task.h | 29 +++++++++++++++++++++++++++++
> > io/task.c | 23 +++++++++++++++++++++++
> > 2 files changed, 52 insertions(+)
> >
> > diff --git a/include/io/task.h b/include/io/task.h
> > index ece1372..244c1a1 100644
> > --- a/include/io/task.h
> > +++ b/include/io/task.h
> > @@ -240,6 +240,35 @@ void qio_task_abort(QIOTask *task,
> >
> >
> > /**
> > + * qio_task_set_error:
> > + * @task: the task struct
> > + * @err: pointer to the error
> > + *
> > + * Associate an error with the task, which can later
> > + * be retrieved with the qio_task_propagate_error()
> > + * method. This method takes ownership of @err, so
> > + * it is not valid to access it after this call
> > + * completes.
> > + */
> > +void qio_task_set_error(QIOTask *task,
> > + Error *err);
>
> Is it valid for @err to be NULL (or put another way, may callers blindly
> call this function whether or not an error has occurred)?...
>
> > +
> > +
> > +/**
> > + * qio_task_propagate_error:
> > + * @task: the task struct
> > + * @errp: pointer to a NULL-initialized error object
> > + *
> > + * Propagate the error associated with @task
> > + * into @errp.
> > + *
> > + * Returns: true if an error was propagated, false otherwise
> > + */
> > +gboolean qio_task_propagate_error(QIOTask *task,
>
> Must this return 'gboolean' (in which case the docs should mention
> TRUE/FALSE rather than true/false)? Or can we make it return the nicer
> 'bool'?
bool is fine - gboolean is just from my habit of using glib
> > @@ -159,6 +163,25 @@ void qio_task_abort(QIOTask *task,
> > }
> >
> >
> > +void qio_task_set_error(QIOTask *task,
> > + Error *err)
> > +{
> > + error_propagate(&task->err, err);
>
> ...As written, qio_task_set_error(task, NULL) is thus valid as a no-op;
> and in turn, qio_task_set_error(task, err) is unconditionally valid a
> cleanup path regardless of whether the cleanup was reached on success
> (err is still NULL) or failure (err is set). But it's worth documenting.
>
> In fact, error_propagate() can be called more than once (first call
> wins, all later calls silently drop the subsequent error in favor of a
> first one already present), and therefore qio_task_set_error() gains
> that same property. Worth documenting?
Ok, will expand docs.
Regards,
Daniel
--
|: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org -o- http://virt-manager.org :|
|: http://entangle-photo.org -o- http://search.cpan.org/~danberr/ :|
^ permalink raw reply [flat|nested] 22+ messages in thread
* [Qemu-devel] [PATCH 6/8] io: change the QIOTask callback signature
2017-01-05 16:03 [Qemu-devel] [PATCH 0/8] io: enable DNS resolving separately of socket create Daniel P. Berrange
` (4 preceding siblings ...)
2017-01-05 16:03 ` [Qemu-devel] [PATCH 5/8] io: add ability to associate an error " Daniel P. Berrange
@ 2017-01-05 16:03 ` Daniel P. Berrange
2017-01-05 21:47 ` Eric Blake
2017-01-05 16:03 ` [Qemu-devel] [PATCH 7/8] io: remove Error parameter from QIOTask thread worker Daniel P. Berrange
2017-01-05 16:03 ` [Qemu-devel] [PATCH 8/8] io: introduce a DNS resolver API Daniel P. Berrange
7 siblings, 1 reply; 22+ messages in thread
From: Daniel P. Berrange @ 2017-01-05 16:03 UTC (permalink / raw)
To: qemu-devel
Currently the QIOTaskFunc signature takes an Object * for
the source, and an Error * for any error. We also need to
be able to provide a result pointer. Rather than continue
to add parameters to QIOTaskFunc, remove the existing
ones and simply pass the QIOTask object instead. This
has methods to access all the other data items required
in the callback impl.
Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
---
include/io/task.h | 73 ++++++++++++++++++++++++------------------
io/channel-tls.c | 14 ++++----
io/channel-websock.c | 8 ++---
io/task.c | 18 +++--------
io/trace-events | 1 -
migration/socket.c | 11 ++++---
migration/tls.c | 19 +++++------
nbd/common.c | 8 ++---
nbd/nbd-internal.h | 3 +-
qemu-char.c | 18 ++++++-----
tests/test-io-channel-socket.c | 5 ++-
tests/test-io-channel-tls.c | 5 ++-
tests/test-io-task.c | 18 +++++------
ui/vnc-auth-vencrypt.c | 7 ++--
ui/vnc-ws.c | 14 ++++----
15 files changed, 110 insertions(+), 112 deletions(-)
diff --git a/include/io/task.h b/include/io/task.h
index 244c1a1..7b5bc43 100644
--- a/include/io/task.h
+++ b/include/io/task.h
@@ -26,8 +26,7 @@
typedef struct QIOTask QIOTask;
-typedef void (*QIOTaskFunc)(Object *source,
- Error *err,
+typedef void (*QIOTaskFunc)(QIOTask *task,
gpointer opaque);
typedef int (*QIOTaskWorker)(QIOTask *task,
@@ -44,7 +43,7 @@ typedef int (*QIOTaskWorker)(QIOTask *task,
* a public API which accepts a task callback:
*
* <example>
- * <title>Task callback function signature</title>
+ * <title>Task function signature</title>
* <programlisting>
* void myobject_operation(QMyObject *obj,
* QIOTaskFunc *func,
@@ -57,12 +56,36 @@ typedef int (*QIOTaskWorker)(QIOTask *task,
* is data to pass to it. The optional 'notify' function is used
* to free 'opaque' when no longer needed.
*
- * Now, lets say the implementation of this method wants to set
- * a timer to run once a second checking for completion of some
- * activity. It would do something like
+ * When the operation completes, the 'func' callback will be
+ * invoked, allowing the calling code to determine the result
+ * of the operation. An example QIOTaskFunc impl may look
+ * like
*
* <example>
- * <title>Task callback function implementation</title>
+ * <title>Task callback implementation</title>
+ * <programlisting>
+ * static void myobject_operation_notify(QIOTask *task,
+ * gpointer opaque)
+ * {
+ * Error *err = NULL;
+ * if (qio_task_propagate_error(task, &err)) {
+ * ...deal with the failure...
+ * error_free(err);
+ * } else {
+ * QMyObject *src = QMY_OBJECT(qio_task_get_source(task));
+ * ...deal with the completion...
+ * }
+ * }
+ * </programlisting>
+ * </example>
+ *
+ * Now, lets say the implementation of the method using the
+ * task wants to set a timer to run once a second checking
+ * for completion of some activity. It would do something
+ * like
+ *
+ * <example>
+ * <title>Task function implementation</title>
* <programlisting>
* void myobject_operation(QMyObject *obj,
* QIOTaskFunc *func,
@@ -102,8 +125,8 @@ typedef int (*QIOTaskWorker)(QIOTask *task,
*
* ...check something important...
* if (err) {
- * qio_task_abort(task, err);
- * error_free(task);
+ * qio_task_set_error(task, err);
+ * qio_task_complete(task);
* return FALSE;
* } else if (...work is completed ...) {
* qio_task_complete(task);
@@ -115,6 +138,10 @@ typedef int (*QIOTaskWorker)(QIOTask *task,
* </programlisting>
* </example>
*
+ * The 'qio_task_complete' call in this method will trigger
+ * the callback func 'myobject_operation_notify' shown
+ * earlier to deal with the results.
+ *
* Once this function returns false, object_unref will be called
* automatically on the task causing it to be released and the
* ref on QMyObject dropped too.
@@ -187,8 +214,8 @@ typedef int (*QIOTaskWorker)(QIOTask *task,
* 'err' attribute in the task object to determine if
* the operation was successful or not.
*
- * The returned task will be released when one of
- * qio_task_abort() or qio_task_complete() are invoked.
+ * The returned task will be released when qio_task_complete()
+ * is invoked.
*
* Returns: the task struct
*/
@@ -204,10 +231,8 @@ QIOTask *qio_task_new(Object *source,
* @opaque: opaque data to pass to @worker
* @destroy: function to free @opaque
*
- * Run a task in a background thread. If @worker
- * returns 0 it will call qio_task_complete() in
- * the main event thread context. If @worker
- * returns -1 it will call qio_task_abort() in
+ * Run a task in a background thread. When @worker
+ * returns it will call qio_task_complete() in
* the main event thread context.
*/
void qio_task_run_in_thread(QIOTask *task,
@@ -219,25 +244,11 @@ void qio_task_run_in_thread(QIOTask *task,
* qio_task_complete:
* @task: the task struct
*
- * Mark the operation as successfully completed
- * and free the memory for @task.
+ * Invoke the completion callback for @task and
+ * then free its memory.
*/
void qio_task_complete(QIOTask *task);
-/**
- * qio_task_abort:
- * @task: the task struct
- * @err: the error to record for the operation
- *
- * Mark the operation as failed, with @err providing
- * details about the failure. The @err may be freed
- * afer the function returns, as the notification
- * callback is invoked synchronously. The @task will
- * be freed when this call completes.
- */
-void qio_task_abort(QIOTask *task,
- Error *err);
-
/**
* qio_task_set_error:
diff --git a/io/channel-tls.c b/io/channel-tls.c
index cf3bcca..f25ab0a 100644
--- a/io/channel-tls.c
+++ b/io/channel-tls.c
@@ -153,8 +153,9 @@ static void qio_channel_tls_handshake_task(QIOChannelTLS *ioc,
if (qcrypto_tls_session_handshake(ioc->session, &err) < 0) {
trace_qio_channel_tls_handshake_fail(ioc);
- qio_task_abort(task, err);
- goto cleanup;
+ qio_task_set_error(task, err);
+ qio_task_complete(task);
+ return;
}
status = qcrypto_tls_session_get_handshake_status(ioc->session);
@@ -163,10 +164,10 @@ static void qio_channel_tls_handshake_task(QIOChannelTLS *ioc,
if (qcrypto_tls_session_check_credentials(ioc->session,
&err) < 0) {
trace_qio_channel_tls_credentials_deny(ioc);
- qio_task_abort(task, err);
- goto cleanup;
+ qio_task_set_error(task, err);
+ } else {
+ trace_qio_channel_tls_credentials_allow(ioc);
}
- trace_qio_channel_tls_credentials_allow(ioc);
qio_task_complete(task);
} else {
GIOCondition condition;
@@ -183,9 +184,6 @@ static void qio_channel_tls_handshake_task(QIOChannelTLS *ioc,
task,
NULL);
}
-
- cleanup:
- error_free(err);
}
diff --git a/io/channel-websock.c b/io/channel-websock.c
index f45bced..e47279a 100644
--- a/io/channel-websock.c
+++ b/io/channel-websock.c
@@ -279,8 +279,8 @@ static gboolean qio_channel_websock_handshake_send(QIOChannel *ioc,
if (ret < 0) {
trace_qio_channel_websock_handshake_fail(ioc);
- qio_task_abort(task, err);
- error_free(err);
+ qio_task_set_error(task, err);
+ qio_task_complete(task);
return FALSE;
}
@@ -307,8 +307,8 @@ static gboolean qio_channel_websock_handshake_io(QIOChannel *ioc,
ret = qio_channel_websock_handshake_read(wioc, &err);
if (ret < 0) {
trace_qio_channel_websock_handshake_fail(ioc);
- qio_task_abort(task, err);
- error_free(err);
+ qio_task_set_error(task, err);
+ qio_task_complete(task);
return FALSE;
}
if (ret == 0) {
diff --git a/io/task.c b/io/task.c
index 1136c75..7da6bdd 100644
--- a/io/task.c
+++ b/io/task.c
@@ -87,13 +87,11 @@ static gboolean gio_task_thread_result(gpointer opaque)
struct QIOTaskThreadData *data = opaque;
trace_qio_task_thread_result(data->task);
- if (data->ret == 0) {
- qio_task_complete(data->task);
- } else {
- qio_task_abort(data->task, data->err);
+ if (data->err) {
+ qio_task_set_error(data->task, data->err);
}
+ qio_task_complete(data->task);
- error_free(data->err);
if (data->destroy) {
data->destroy(data->opaque);
}
@@ -149,19 +147,11 @@ void qio_task_run_in_thread(QIOTask *task,
void qio_task_complete(QIOTask *task)
{
- task->func(task->source, NULL, task->opaque);
+ task->func(task, task->opaque);
trace_qio_task_complete(task);
qio_task_free(task);
}
-void qio_task_abort(QIOTask *task,
- Error *err)
-{
- task->func(task->source, err, task->opaque);
- trace_qio_task_abort(task);
- qio_task_free(task);
-}
-
void qio_task_set_error(QIOTask *task,
Error *err)
diff --git a/io/trace-events b/io/trace-events
index e31b596..ff993be 100644
--- a/io/trace-events
+++ b/io/trace-events
@@ -3,7 +3,6 @@
# io/task.c
qio_task_new(void *task, void *source, void *func, void *opaque) "Task new task=%p source=%p func=%p opaque=%p"
qio_task_complete(void *task) "Task complete task=%p"
-qio_task_abort(void *task) "Task abort task=%p"
qio_task_thread_start(void *task, void *worker, void *opaque) "Task thread start task=%p worker=%p opaque=%p"
qio_task_thread_run(void *task) "Task thread run task=%p"
qio_task_thread_exit(void *task) "Task thread exit task=%p"
diff --git a/migration/socket.c b/migration/socket.c
index 11f80b1..13966f1 100644
--- a/migration/socket.c
+++ b/migration/socket.c
@@ -70,22 +70,23 @@ static void socket_connect_data_free(void *opaque)
g_free(data);
}
-static void socket_outgoing_migration(Object *src,
- Error *err,
+static void socket_outgoing_migration(QIOTask *task,
gpointer opaque)
{
struct SocketConnectData *data = opaque;
- QIOChannel *sioc = QIO_CHANNEL(src);
+ QIOChannel *sioc = QIO_CHANNEL(qio_task_get_source(task));
+ Error *err = NULL;
- if (err) {
+ if (qio_task_propagate_error(task, &err)) {
trace_migration_socket_outgoing_error(error_get_pretty(err));
data->s->to_dst_file = NULL;
migrate_fd_error(data->s, err);
+ error_free(err);
} else {
trace_migration_socket_outgoing_connected(data->hostname);
migration_channel_connect(data->s, sioc, data->hostname);
}
- object_unref(src);
+ object_unref(OBJECT(sioc));
}
static void socket_start_outgoing_migration(MigrationState *s,
diff --git a/migration/tls.c b/migration/tls.c
index 49ca9a8..203c11d 100644
--- a/migration/tls.c
+++ b/migration/tls.c
@@ -61,15 +61,15 @@ migration_tls_get_creds(MigrationState *s,
}
-static void migration_tls_incoming_handshake(Object *src,
- Error *err,
+static void migration_tls_incoming_handshake(QIOTask *task,
gpointer opaque)
{
- QIOChannel *ioc = QIO_CHANNEL(src);
+ QIOChannel *ioc = QIO_CHANNEL(qio_task_get_source(task));
+ Error *err = NULL;
- if (err) {
+ if (qio_task_propagate_error(task, &err)) {
trace_migration_tls_incoming_handshake_error(error_get_pretty(err));
- error_report("%s", error_get_pretty(err));
+ error_report_err(err);
} else {
trace_migration_tls_incoming_handshake_complete();
migration_channel_process_incoming(migrate_get_current(), ioc);
@@ -107,17 +107,18 @@ void migration_tls_channel_process_incoming(MigrationState *s,
}
-static void migration_tls_outgoing_handshake(Object *src,
- Error *err,
+static void migration_tls_outgoing_handshake(QIOTask *task,
gpointer opaque)
{
MigrationState *s = opaque;
- QIOChannel *ioc = QIO_CHANNEL(src);
+ QIOChannel *ioc = QIO_CHANNEL(qio_task_get_source(task));
+ Error *err = NULL;
- if (err) {
+ if (qio_task_propagate_error(task, &err)) {
trace_migration_tls_outgoing_handshake_error(error_get_pretty(err));
s->to_dst_file = NULL;
migrate_fd_error(s, err);
+ error_free(err);
} else {
trace_migration_tls_outgoing_handshake_complete();
migration_channel_connect(s, ioc, NULL);
diff --git a/nbd/common.c b/nbd/common.c
index b583a4f..a5f39ea 100644
--- a/nbd/common.c
+++ b/nbd/common.c
@@ -78,15 +78,13 @@ ssize_t nbd_wr_syncv(QIOChannel *ioc,
}
-void nbd_tls_handshake(Object *src,
- Error *err,
+void nbd_tls_handshake(QIOTask *task,
void *opaque)
{
struct NBDTLSHandshakeData *data = opaque;
- if (err) {
- TRACE("TLS failed %s", error_get_pretty(err));
- data->error = error_copy(err);
+ if (qio_task_propagate_error(task, &data->error)) {
+ TRACE("TLS failed %s", error_get_pretty(data->error));
}
data->complete = true;
g_main_loop_quit(data->loop);
diff --git a/nbd/nbd-internal.h b/nbd/nbd-internal.h
index eee20ab..f43d990 100644
--- a/nbd/nbd-internal.h
+++ b/nbd/nbd-internal.h
@@ -120,8 +120,7 @@ struct NBDTLSHandshakeData {
};
-void nbd_tls_handshake(Object *src,
- Error *err,
+void nbd_tls_handshake(QIOTask *task,
void *opaque);
#endif
diff --git a/qemu-char.c b/qemu-char.c
index 2c9940c..502d50a 100644
--- a/qemu-char.c
+++ b/qemu-char.c
@@ -3272,14 +3272,13 @@ static void tcp_chr_telnet_init(CharDriverState *chr)
}
-static void tcp_chr_tls_handshake(Object *source,
- Error *err,
+static void tcp_chr_tls_handshake(QIOTask *task,
gpointer user_data)
{
CharDriverState *chr = user_data;
TCPCharDriver *s = chr->opaque;
- if (err) {
+ if (qio_task_propagate_error(task, NULL)) {
tcp_chr_disconnect(chr);
} else {
if (s->do_telnetopt) {
@@ -3487,20 +3486,23 @@ static void tcp_chr_free(CharDriverState *chr)
}
-static void qemu_chr_socket_connected(Object *src, Error *err, void *opaque)
+static void qemu_chr_socket_connected(QIOTask *task, void *opaque)
{
- QIOChannelSocket *sioc = QIO_CHANNEL_SOCKET(src);
+ QIOChannelSocket *sioc = QIO_CHANNEL_SOCKET(qio_task_get_source(task));
CharDriverState *chr = opaque;
TCPCharDriver *s = chr->opaque;
+ Error *err = NULL;
- if (err) {
+ if (qio_task_propagate_error(task, &err)) {
check_report_connect_error(chr, err);
- object_unref(src);
- return;
+ error_free(err);
+ goto cleanup;
}
s->connect_err_reported = false;
tcp_chr_new_client(chr, sioc);
+
+ cleanup:
object_unref(OBJECT(sioc));
}
diff --git a/tests/test-io-channel-socket.c b/tests/test-io-channel-socket.c
index aa88c3c..aaa9116 100644
--- a/tests/test-io-channel-socket.c
+++ b/tests/test-io-channel-socket.c
@@ -156,12 +156,11 @@ struct TestIOChannelData {
};
-static void test_io_channel_complete(Object *src,
- Error *err,
+static void test_io_channel_complete(QIOTask *task,
gpointer opaque)
{
struct TestIOChannelData *data = opaque;
- data->err = err != NULL;
+ data->err = qio_task_propagate_error(task, NULL);
g_main_loop_quit(data->loop);
}
diff --git a/tests/test-io-channel-tls.c b/tests/test-io-channel-tls.c
index bd3ae2b..8eaa208 100644
--- a/tests/test-io-channel-tls.c
+++ b/tests/test-io-channel-tls.c
@@ -53,14 +53,13 @@ struct QIOChannelTLSHandshakeData {
bool failed;
};
-static void test_tls_handshake_done(Object *source,
- Error *err,
+static void test_tls_handshake_done(QIOTask *task,
gpointer opaque)
{
struct QIOChannelTLSHandshakeData *data = opaque;
data->finished = true;
- data->failed = err != NULL;
+ data->failed = qio_task_propagate_error(task, NULL);
}
diff --git a/tests/test-io-task.c b/tests/test-io-task.c
index 024eb58..84144c9 100644
--- a/tests/test-io-task.c
+++ b/tests/test-io-task.c
@@ -50,14 +50,13 @@ struct TestTaskData {
};
-static void task_callback(Object *source,
- Error *err,
+static void task_callback(QIOTask *task,
gpointer opaque)
{
struct TestTaskData *data = opaque;
- data->source = source;
- data->err = err;
+ data->source = qio_task_get_source(task);
+ qio_task_propagate_error(task, &data->err);
}
@@ -120,9 +119,9 @@ static void test_task_failure(void)
error_setg(&err, "Some error");
- qio_task_abort(task, err);
+ qio_task_set_error(task, err);
+ qio_task_complete(task);
- error_free(err);
object_unref(obj);
g_assert(data.source == obj);
@@ -158,14 +157,13 @@ static int test_task_thread_worker(QIOTask *task,
}
-static void test_task_thread_callback(Object *source,
- Error *err,
+static void test_task_thread_callback(QIOTask *task,
gpointer opaque)
{
struct TestThreadWorkerData *data = opaque;
- data->source = source;
- data->err = err;
+ data->source = qio_task_get_source(task);
+ qio_task_propagate_error(task, &data->err);
data->complete = g_thread_self();
diff --git a/ui/vnc-auth-vencrypt.c b/ui/vnc-auth-vencrypt.c
index c0c29a5..ffaab57 100644
--- a/ui/vnc-auth-vencrypt.c
+++ b/ui/vnc-auth-vencrypt.c
@@ -65,16 +65,17 @@ static void start_auth_vencrypt_subauth(VncState *vs)
}
}
-static void vnc_tls_handshake_done(Object *source,
- Error *err,
+static void vnc_tls_handshake_done(QIOTask *task,
gpointer user_data)
{
VncState *vs = user_data;
+ Error *err = NULL;
- if (err) {
+ if (qio_task_propagate_error(task, &err)) {
VNC_DEBUG("Handshake failed %s\n",
error_get_pretty(err));
vnc_client_error(vs);
+ error_free(err);
} else {
vs->ioc_tag = qio_channel_add_watch(
vs->ioc, G_IO_IN | G_IO_OUT, vnc_client_io, vs, NULL);
diff --git a/ui/vnc-ws.c b/ui/vnc-ws.c
index bffb484..f530cd5 100644
--- a/ui/vnc-ws.c
+++ b/ui/vnc-ws.c
@@ -24,15 +24,16 @@
#include "io/channel-websock.h"
#include "qemu/bswap.h"
-static void vncws_tls_handshake_done(Object *source,
- Error *err,
+static void vncws_tls_handshake_done(QIOTask *task,
gpointer user_data)
{
VncState *vs = user_data;
+ Error *err = NULL;
- if (err) {
+ if (qio_task_propagate_error(task, &err)) {
VNC_DEBUG("Handshake failed %s\n", error_get_pretty(err));
vnc_client_error(vs);
+ error_free(err);
} else {
VNC_DEBUG("TLS handshake complete, starting websocket handshake\n");
vs->ioc_tag = qio_channel_add_watch(
@@ -83,15 +84,16 @@ gboolean vncws_tls_handshake_io(QIOChannel *ioc G_GNUC_UNUSED,
}
-static void vncws_handshake_done(Object *source,
- Error *err,
+static void vncws_handshake_done(QIOTask *task,
gpointer user_data)
{
VncState *vs = user_data;
+ Error *err = NULL;
- if (err) {
+ if (qio_task_propagate_error(task, &err)) {
VNC_DEBUG("Websock handshake failed %s\n", error_get_pretty(err));
vnc_client_error(vs);
+ error_free(err);
} else {
VNC_DEBUG("Websock handshake complete, starting VNC protocol\n");
vnc_start_protocol(vs);
--
2.9.3
^ permalink raw reply related [flat|nested] 22+ messages in thread* Re: [Qemu-devel] [PATCH 6/8] io: change the QIOTask callback signature
2017-01-05 16:03 ` [Qemu-devel] [PATCH 6/8] io: change the QIOTask callback signature Daniel P. Berrange
@ 2017-01-05 21:47 ` Eric Blake
2017-01-06 12:05 ` Daniel P. Berrange
0 siblings, 1 reply; 22+ messages in thread
From: Eric Blake @ 2017-01-05 21:47 UTC (permalink / raw)
To: Daniel P. Berrange, qemu-devel
[-- Attachment #1: Type: text/plain, Size: 7843 bytes --]
On 01/05/2017 10:03 AM, Daniel P. Berrange wrote:
> Currently the QIOTaskFunc signature takes an Object * for
> the source, and an Error * for any error. We also need to
> be able to provide a result pointer. Rather than continue
> to add parameters to QIOTaskFunc, remove the existing
> ones and simply pass the QIOTask object instead. This
> has methods to access all the other data items required
> in the callback impl.
Hmmm. If you're going to change the callback signature after all, then
is it worth considering having the callback return void* instead of the
clunky way of passing the result pointer around, with existing callers
updated to return NULL if they have nothing better to do? Accessing the
error pointer through the QIOTask object seems okay, it's just the
return type that feels clunky.
But that's bike-shedding, so I'll review as written.
>
> Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
> ---
> include/io/task.h | 73 ++++++++++++++++++++++++------------------
> io/channel-tls.c | 14 ++++----
> io/channel-websock.c | 8 ++---
> io/task.c | 18 +++--------
> io/trace-events | 1 -
> migration/socket.c | 11 ++++---
> migration/tls.c | 19 +++++------
> nbd/common.c | 8 ++---
> nbd/nbd-internal.h | 3 +-
> qemu-char.c | 18 ++++++-----
> tests/test-io-channel-socket.c | 5 ++-
> tests/test-io-channel-tls.c | 5 ++-
> tests/test-io-task.c | 18 +++++------
> ui/vnc-auth-vencrypt.c | 7 ++--
> ui/vnc-ws.c | 14 ++++----
> 15 files changed, 110 insertions(+), 112 deletions(-)
>
> diff --git a/include/io/task.h b/include/io/task.h
> index 244c1a1..7b5bc43 100644
> --- a/include/io/task.h
> +++ b/include/io/task.h
> @@ -26,8 +26,7 @@
>
> typedef struct QIOTask QIOTask;
>
> -typedef void (*QIOTaskFunc)(Object *source,
> - Error *err,
> +typedef void (*QIOTaskFunc)(QIOTask *task,
> gpointer opaque);
>
> *
> - * Now, lets say the implementation of this method wants to set
> - * a timer to run once a second checking for completion of some
> - * activity. It would do something like
> + * When the operation completes, the 'func' callback will be
> + * invoked, allowing the calling code to determine the result
> + * of the operation. An example QIOTaskFunc impl may look
Worth spelling out 'implementation' in the docs?
> + * like
> *
> * <example>
> - * <title>Task callback function implementation</title>
> + * <title>Task callback implementation</title>
> + * <programlisting>
> + * static void myobject_operation_notify(QIOTask *task,
> + * gpointer opaque)
> + * {
> + * Error *err = NULL;
> + * if (qio_task_propagate_error(task, &err)) {
> + * ...deal with the failure...
> + * error_free(err);
> + * } else {
> + * QMyObject *src = QMY_OBJECT(qio_task_get_source(task));
> + * ...deal with the completion...
> + * }
> + * }
> + * </programlisting>
> + * </example>
> + *
> + * Now, lets say the implementation of the method using the
> + * task wants to set a timer to run once a second checking
> + * for completion of some activity. It would do something
Why two spaces after 'some'?
> + * like
> + *
> + * <example>
> + * <title>Task function implementation</title>
> * <programlisting>
> * void myobject_operation(QMyObject *obj,
> * QIOTaskFunc *func,
> @@ -102,8 +125,8 @@ typedef int (*QIOTaskWorker)(QIOTask *task,
> *
> * ...check something important...
> * if (err) {
> - * qio_task_abort(task, err);
> - * error_free(task);
> + * qio_task_set_error(task, err);
> + * qio_task_complete(task);
Interesting - by storing the error object directly in the task, you no
longer have to special-case task abort vs. complete (rather, the
notifier checks whether the task completed successfully or in error).
> * return FALSE;
> * } else if (...work is completed ...) {
> * qio_task_complete(task);
> @@ -115,6 +138,10 @@ typedef int (*QIOTaskWorker)(QIOTask *task,
> * </programlisting>
> * </example>
> *
> + * The 'qio_task_complete' call in this method will trigger
> + * the callback func 'myobject_operation_notify' shown
> + * earlier to deal with the results.
> + *
> * Once this function returns false, object_unref will be called
> * automatically on the task causing it to be released and the
> * ref on QMyObject dropped too.
> @@ -187,8 +214,8 @@ typedef int (*QIOTaskWorker)(QIOTask *task,
> * 'err' attribute in the task object to determine if
> * the operation was successful or not.
> *
> - * The returned task will be released when one of
> - * qio_task_abort() or qio_task_complete() are invoked.
> + * The returned task will be released when qio_task_complete()
> + * is invoked.
> *
> * Returns: the task struct
> */
> @@ -204,10 +231,8 @@ QIOTask *qio_task_new(Object *source,
> * @opaque: opaque data to pass to @worker
> * @destroy: function to free @opaque
> *
> - * Run a task in a background thread. If @worker
> - * returns 0 it will call qio_task_complete() in
> - * the main event thread context. If @worker
> - * returns -1 it will call qio_task_abort() in
> + * Run a task in a background thread. When @worker
> + * returns it will call qio_task_complete() in
> * the main event thread context.
> */
> void qio_task_run_in_thread(QIOTask *task,
> @@ -219,25 +244,11 @@ void qio_task_run_in_thread(QIOTask *task,
> * qio_task_complete:
> * @task: the task struct
> *
> - * Mark the operation as successfully completed
> - * and free the memory for @task.
> + * Invoke the completion callback for @task and
> + * then free its memory.
> */
> void qio_task_complete(QIOTask *task);
>
> -/**
> - * qio_task_abort:
> - * @task: the task struct
> - * @err: the error to record for the operation
> - *
> - * Mark the operation as failed, with @err providing
> - * details about the failure. The @err may be freed
> - * afer the function returns, as the notification
And you're eliminating a typo :)
> - * callback is invoked synchronously. The @task will
> - * be freed when this call completes.
> - */
> -void qio_task_abort(QIOTask *task,
> - Error *err);
> -
>
> /**
> * qio_task_set_error:
> diff --git a/io/channel-tls.c b/io/channel-tls.c
> index cf3bcca..f25ab0a 100644
> --- a/io/channel-tls.c
> +++ b/tests/test-io-channel-socket.c
> @@ -156,12 +156,11 @@ struct TestIOChannelData {
> };
>
>
> -static void test_io_channel_complete(Object *src,
> - Error *err,
> +static void test_io_channel_complete(QIOTask *task,
> gpointer opaque)
> {
> struct TestIOChannelData *data = opaque;
> - data->err = err != NULL;
> + data->err = qio_task_propagate_error(task, NULL);
Cool way to ignore and free task->err while still tracking if it had
captured an error. I had to go re-read patch 5/8 to make sure this did
the right thing.
I'm still not sure if there is a better signature to be using, but as
written, your conversion to your choice of signature looks correct
(modulo nits I pointed out above), so you can add:
Reviewed-by: Eric Blake <eblake@redhat.com>
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]
^ permalink raw reply [flat|nested] 22+ messages in thread* Re: [Qemu-devel] [PATCH 6/8] io: change the QIOTask callback signature
2017-01-05 21:47 ` Eric Blake
@ 2017-01-06 12:05 ` Daniel P. Berrange
0 siblings, 0 replies; 22+ messages in thread
From: Daniel P. Berrange @ 2017-01-06 12:05 UTC (permalink / raw)
To: Eric Blake; +Cc: qemu-devel
On Thu, Jan 05, 2017 at 03:47:57PM -0600, Eric Blake wrote:
> On 01/05/2017 10:03 AM, Daniel P. Berrange wrote:
> > Currently the QIOTaskFunc signature takes an Object * for
> > the source, and an Error * for any error. We also need to
> > be able to provide a result pointer. Rather than continue
> > to add parameters to QIOTaskFunc, remove the existing
> > ones and simply pass the QIOTask object instead. This
> > has methods to access all the other data items required
> > in the callback impl.
>
> Hmmm. If you're going to change the callback signature after all, then
> is it worth considering having the callback return void* instead of the
> clunky way of passing the result pointer around, with existing callers
> updated to return NULL if they have nothing better to do? Accessing the
> error pointer through the QIOTask object seems okay, it's just the
> return type that feels clunky.
>
> But that's bike-shedding, so I'll review as written.
Per my comments in previous patch, the approach taken here was chosen
to align with GLib's GTask/GAsyncResult design more closely.
Regards,
Daniel
--
|: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org -o- http://virt-manager.org :|
|: http://entangle-photo.org -o- http://search.cpan.org/~danberr/ :|
^ permalink raw reply [flat|nested] 22+ messages in thread
* [Qemu-devel] [PATCH 7/8] io: remove Error parameter from QIOTask thread worker
2017-01-05 16:03 [Qemu-devel] [PATCH 0/8] io: enable DNS resolving separately of socket create Daniel P. Berrange
` (5 preceding siblings ...)
2017-01-05 16:03 ` [Qemu-devel] [PATCH 6/8] io: change the QIOTask callback signature Daniel P. Berrange
@ 2017-01-05 16:03 ` Daniel P. Berrange
2017-01-05 22:09 ` Eric Blake
2017-01-05 16:03 ` [Qemu-devel] [PATCH 8/8] io: introduce a DNS resolver API Daniel P. Berrange
7 siblings, 1 reply; 22+ messages in thread
From: Daniel P. Berrange @ 2017-01-05 16:03 UTC (permalink / raw)
To: qemu-devel
Now that task objects have a directly associated error,
there's no need for an an Error **errp parameter to
the QIOTask thread worker function. It already has a
QIOTask object, so can directly set the error on it.
Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
---
include/io/task.h | 19 +++++++++----------
io/channel-socket.c | 47 ++++++++++++++++++++++-------------------------
io/task.c | 10 +---------
tests/test-io-task.c | 12 +++++-------
4 files changed, 37 insertions(+), 51 deletions(-)
diff --git a/include/io/task.h b/include/io/task.h
index 7b5bc43..dca57dc 100644
--- a/include/io/task.h
+++ b/include/io/task.h
@@ -29,9 +29,8 @@ typedef struct QIOTask QIOTask;
typedef void (*QIOTaskFunc)(QIOTask *task,
gpointer opaque);
-typedef int (*QIOTaskWorker)(QIOTask *task,
- Error **errp,
- gpointer opaque);
+typedef void (*QIOTaskWorker)(QIOTask *task,
+ gpointer opaque);
/**
* QIOTask:
@@ -163,18 +162,18 @@ typedef int (*QIOTaskWorker)(QIOTask *task,
* socket listen using QIOTask would require:
*
* <example>
- * static int myobject_listen_worker(QIOTask *task,
- * Error **errp,
- * gpointer opaque)
+ * static void myobject_listen_worker(QIOTask *task,
+ * gpointer opaque)
* {
* QMyObject obj = QMY_OBJECT(qio_task_get_source(task));
* SocketAddress *addr = opaque;
+ * Error *err = NULL;
*
- * obj->fd = socket_listen(addr, errp);
- * if (obj->fd < 0) {
- * return -1;
+ * obj->fd = socket_listen(addr, &err);
+ *
+ * if (err) {
+ * qio_task_set_error(task, err);
* }
- * return 0;
* }
*
* void myobject_listen_async(QMyObject *obj,
diff --git a/io/channel-socket.c b/io/channel-socket.c
index 45df819..8d32a0b 100644
--- a/io/channel-socket.c
+++ b/io/channel-socket.c
@@ -156,19 +156,18 @@ int qio_channel_socket_connect_sync(QIOChannelSocket *ioc,
}
-static int qio_channel_socket_connect_worker(QIOTask *task,
- Error **errp,
- gpointer opaque)
+static void qio_channel_socket_connect_worker(QIOTask *task,
+ gpointer opaque)
{
QIOChannelSocket *ioc = QIO_CHANNEL_SOCKET(qio_task_get_source(task));
SocketAddress *addr = opaque;
- int ret;
+ Error *err = NULL;
- ret = qio_channel_socket_connect_sync(ioc,
- addr,
- errp);
+ qio_channel_socket_connect_sync(ioc, addr, &err);
- return ret;
+ if (err) {
+ qio_task_set_error(task, err);
+ }
}
@@ -218,19 +217,18 @@ int qio_channel_socket_listen_sync(QIOChannelSocket *ioc,
}
-static int qio_channel_socket_listen_worker(QIOTask *task,
- Error **errp,
- gpointer opaque)
+static void qio_channel_socket_listen_worker(QIOTask *task,
+ gpointer opaque)
{
QIOChannelSocket *ioc = QIO_CHANNEL_SOCKET(qio_task_get_source(task));
SocketAddress *addr = opaque;
- int ret;
+ Error *err = NULL;
- ret = qio_channel_socket_listen_sync(ioc,
- addr,
- errp);
+ qio_channel_socket_listen_sync(ioc, addr, &err);
- return ret;
+ if (err) {
+ qio_task_set_error(task, err);
+ }
}
@@ -293,21 +291,20 @@ static void qio_channel_socket_dgram_worker_free(gpointer opaque)
g_free(data);
}
-static int qio_channel_socket_dgram_worker(QIOTask *task,
- Error **errp,
- gpointer opaque)
+static void qio_channel_socket_dgram_worker(QIOTask *task,
+ gpointer opaque)
{
QIOChannelSocket *ioc = QIO_CHANNEL_SOCKET(qio_task_get_source(task));
struct QIOChannelSocketDGramWorkerData *data = opaque;
- int ret;
+ Error *err = NULL;
/* socket_dgram() blocks in DNS lookups, so we must use a thread */
- ret = qio_channel_socket_dgram_sync(ioc,
- data->localAddr,
- data->remoteAddr,
- errp);
+ qio_channel_socket_dgram_sync(ioc, data->localAddr,
+ data->remoteAddr, &err);
- return ret;
+ if (err) {
+ qio_task_set_error(task, err);
+ }
}
diff --git a/io/task.c b/io/task.c
index 7da6bdd..448ec67 100644
--- a/io/task.c
+++ b/io/task.c
@@ -77,8 +77,6 @@ struct QIOTaskThreadData {
QIOTaskWorker worker;
gpointer opaque;
GDestroyNotify destroy;
- Error *err;
- int ret;
};
@@ -87,9 +85,6 @@ static gboolean gio_task_thread_result(gpointer opaque)
struct QIOTaskThreadData *data = opaque;
trace_qio_task_thread_result(data->task);
- if (data->err) {
- qio_task_set_error(data->task, data->err);
- }
qio_task_complete(data->task);
if (data->destroy) {
@@ -107,10 +102,7 @@ static gpointer qio_task_thread_worker(gpointer opaque)
struct QIOTaskThreadData *data = opaque;
trace_qio_task_thread_run(data->task);
- data->ret = data->worker(data->task, &data->err, data->opaque);
- if (data->ret < 0 && data->err == NULL) {
- error_setg(&data->err, "Task worker failed but did not set an error");
- }
+ data->worker(data->task, data->opaque);
/* We're running in the background thread, and must only
* ever report the task results in the main event loop
diff --git a/tests/test-io-task.c b/tests/test-io-task.c
index 84144c9..ff62272 100644
--- a/tests/test-io-task.c
+++ b/tests/test-io-task.c
@@ -140,20 +140,18 @@ struct TestThreadWorkerData {
GMainLoop *loop;
};
-static int test_task_thread_worker(QIOTask *task,
- Error **errp,
- gpointer opaque)
+static void test_task_thread_worker(QIOTask *task,
+ gpointer opaque)
{
struct TestThreadWorkerData *data = opaque;
data->worker = g_thread_self();
if (data->fail) {
- error_setg(errp, "Testing fail");
- return -1;
+ Error *err = NULL;
+ error_setg(&err, "Testing fail");
+ qio_task_set_error(task, err);
}
-
- return 0;
}
--
2.9.3
^ permalink raw reply related [flat|nested] 22+ messages in thread* Re: [Qemu-devel] [PATCH 7/8] io: remove Error parameter from QIOTask thread worker
2017-01-05 16:03 ` [Qemu-devel] [PATCH 7/8] io: remove Error parameter from QIOTask thread worker Daniel P. Berrange
@ 2017-01-05 22:09 ` Eric Blake
0 siblings, 0 replies; 22+ messages in thread
From: Eric Blake @ 2017-01-05 22:09 UTC (permalink / raw)
To: Daniel P. Berrange, qemu-devel
[-- Attachment #1: Type: text/plain, Size: 4138 bytes --]
On 01/05/2017 10:03 AM, Daniel P. Berrange wrote:
> Now that task objects have a directly associated error,
> there's no need for an an Error **errp parameter to
> the QIOTask thread worker function. It already has a
> QIOTask object, so can directly set the error on it.
>
> Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
> ---
> include/io/task.h | 19 +++++++++----------
> io/channel-socket.c | 47 ++++++++++++++++++++++-------------------------
> io/task.c | 10 +---------
> tests/test-io-task.c | 12 +++++-------
> 4 files changed, 37 insertions(+), 51 deletions(-)
>
> diff --git a/include/io/task.h b/include/io/task.h
> index 7b5bc43..dca57dc 100644
> --- a/include/io/task.h
> +++ b/include/io/task.h
> @@ -29,9 +29,8 @@ typedef struct QIOTask QIOTask;
> typedef void (*QIOTaskFunc)(QIOTask *task,
> gpointer opaque);
>
> -typedef int (*QIOTaskWorker)(QIOTask *task,
> - Error **errp,
> - gpointer opaque);
> +typedef void (*QIOTaskWorker)(QIOTask *task,
> + gpointer opaque);
Hmm, so you're getting rid of the return type here, because the QIOTask
now holds everything. I'm still not sure whether a void* return would be
easier, but I'm not going to reject your patch because of my bikeshedding.
>
> /**
> * QIOTask:
> @@ -163,18 +162,18 @@ typedef int (*QIOTaskWorker)(QIOTask *task,
> * socket listen using QIOTask would require:
> *
> * <example>
> - * static int myobject_listen_worker(QIOTask *task,
> - * Error **errp,
> - * gpointer opaque)
> + * static void myobject_listen_worker(QIOTask *task,
> + * gpointer opaque)
> * {
> * QMyObject obj = QMY_OBJECT(qio_task_get_source(task));
> * SocketAddress *addr = opaque;
> + * Error *err = NULL;
> *
> - * obj->fd = socket_listen(addr, errp);
> - * if (obj->fd < 0) {
> - * return -1;
> + * obj->fd = socket_listen(addr, &err);
> + *
> + * if (err) {
> + * qio_task_set_error(task, err);
I argued earlier that you can call this unconditionally, dropping the
'if (err)'. Both here in the doc example...
> +++ b/io/channel-socket.c
> @@ -156,19 +156,18 @@ int qio_channel_socket_connect_sync(QIOChannelSocket *ioc,
> }
>
>
> -static int qio_channel_socket_connect_worker(QIOTask *task,
> - Error **errp,
> - gpointer opaque)
> +static void qio_channel_socket_connect_worker(QIOTask *task,
> + gpointer opaque)
> {
> QIOChannelSocket *ioc = QIO_CHANNEL_SOCKET(qio_task_get_source(task));
> SocketAddress *addr = opaque;
> - int ret;
> + Error *err = NULL;
>
> - ret = qio_channel_socket_connect_sync(ioc,
> - addr,
> - errp);
> + qio_channel_socket_connect_sync(ioc, addr, &err);
>
> - return ret;
> + if (err) {
> + qio_task_set_error(task, err);
...and in the actual code. But I guess leaving it in doesn't hurt much,
either.
> @@ -107,10 +102,7 @@ static gpointer qio_task_thread_worker(gpointer opaque)
> struct QIOTaskThreadData *data = opaque;
>
> trace_qio_task_thread_run(data->task);
> - data->ret = data->worker(data->task, &data->err, data->opaque);
> - if (data->ret < 0 && data->err == NULL) {
> - error_setg(&data->err, "Task worker failed but did not set an error");
> - }
> + data->worker(data->task, data->opaque);
I like that your choice of making the error part of the QIOTask
simplifies the workers.
Up to you whether to simplify the conditionals.
Reviewed-by: Eric Blake <eblake@redhat.com>
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]
^ permalink raw reply [flat|nested] 22+ messages in thread
* [Qemu-devel] [PATCH 8/8] io: introduce a DNS resolver API
2017-01-05 16:03 [Qemu-devel] [PATCH 0/8] io: enable DNS resolving separately of socket create Daniel P. Berrange
` (6 preceding siblings ...)
2017-01-05 16:03 ` [Qemu-devel] [PATCH 7/8] io: remove Error parameter from QIOTask thread worker Daniel P. Berrange
@ 2017-01-05 16:03 ` Daniel P. Berrange
2017-01-05 22:51 ` Eric Blake
7 siblings, 1 reply; 22+ messages in thread
From: Daniel P. Berrange @ 2017-01-05 16:03 UTC (permalink / raw)
To: qemu-devel
Currently DNS resolution is done automatically as part
of the creation of a QIOChannelSocket object instance.
This works ok for network clients where you just end
up a single network socket, but for servers, the results
of DNS resolution may require creation of multiple
sockets.
Introducing a DNS resolver API allows DNS resolution
to be separated from the socket object creation. This
will make it practical to create multiple QIOChannelSocket
instances for servers.
Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
---
include/io/dns-resolver.h | 224 +++++++++++++++++++++++++++++++++++++++
include/qemu/sockets.h | 2 +
io/Makefile.objs | 1 +
io/dns-resolver.c | 265 ++++++++++++++++++++++++++++++++++++++++++++++
util/qemu-sockets.c | 4 +-
5 files changed, 494 insertions(+), 2 deletions(-)
create mode 100644 include/io/dns-resolver.h
create mode 100644 io/dns-resolver.c
diff --git a/include/io/dns-resolver.h b/include/io/dns-resolver.h
new file mode 100644
index 0000000..5121e65
--- /dev/null
+++ b/include/io/dns-resolver.h
@@ -0,0 +1,224 @@
+/*
+ * QEMU DNS resolver
+ *
+ * Copyright (c) 2016 Red Hat, Inc.
+ *
+ * This library is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public
+ * License as published by the Free Software Foundation; either
+ * version 2 of the License, or (at your option) any later version.
+ *
+ * This library is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public
+ * License along with this library; if not, see <http://www.gnu.org/licenses/>.
+ *
+ */
+
+#ifndef QIO_DNS_RESOLVER_H
+#define QIO_DNS_RESOLVER_H
+
+#include "qemu-common.h"
+#include "qom/object.h"
+#include "io/task.h"
+
+#define TYPE_QIO_DNS_RESOLVER "qio-dns-resolver"
+#define QIO_DNS_RESOLVER(obj) \
+ OBJECT_CHECK(QIODNSResolver, (obj), TYPE_QIO_DNS_RESOLVER)
+#define QIO_DNS_RESOLVER_CLASS(klass) \
+ OBJECT_CLASS_CHECK(QIODNSResolverClass, klass, TYPE_QIO_DNS_RESOLVER)
+#define QIO_DNS_RESOLVER_GET_CLASS(obj) \
+ OBJECT_GET_CLASS(QIODNSResolverClass, obj, TYPE_QIO_DNS_RESOLVER)
+
+typedef struct QIODNSResolver QIODNSResolver;
+typedef struct QIODNSResolverClass QIODNSResolverClass;
+
+/**
+ * QIODNSResolver:
+ *
+ * The QIODNSResolver class provides a framework for doing
+ * DNS resolution on SocketAddress objects, independently
+ * of socket creation.
+ *
+ * <example>
+ * <title>Resolving addresses synchronously</title>
+ * <programlisting>
+ * int mylisten(SocketAddress *addr, Error **errp) {
+ * QIODNSResolver *resolver = qio_dns_resolver_get_instance();
+ * SocketAddress **rawaddrs = NULL;
+ * size_t nrawaddrs = 0;
+ * Error *err = NULL;
+ * QIOChannel **socks = NULL;
+ * size_t nsocks = 0;
+ *
+ * if (qio_dns_resolver_lookup_sync(dns, addr, &nrawaddrs,
+ * &rawaddrs, &err) < 0) {
+ * error_propagate(errp, err);
+ * return -1;
+ * }
+ *
+ * for (i = 0; i < nrawaddrs; i++) {
+ * QIOChannel *sock = qio_channel_new();
+ * Error *local_err = NULL;
+ * qio_channel_listen_sync(sock, rawaddrs[i], &local_err);
+ * if (local_err && !err) {
+ * error_propagate(err, local_err);
+ * } else {
+ * socks = g_renew(QIOChannelSocket *, socks, nsocks + 1);
+ * socks[nsocks++] = sock;
+ * }
+ * }
+ *
+ * if (nsocks == 0) {
+ * error_propagate(errp, err);
+ * } else {
+ * error_free(err);
+ * }
+ * }
+ * </programlisting>
+ * </example>
+ *
+ * <example>
+ * <title>Resolving addresses asynchronously</title>
+ * <programlisting>
+ * typedef struct MyListenData {
+ * Error *err;
+ * QIOChannelSocket **socks;
+ * size_t nsocks;
+ * } MyListenData;
+ *
+ * void mylistenresult(QIOTask *task, void *opaque) {
+ * MyListenData *data = opaque;
+ * QIODNSResolver *resolver =
+ * QIO_DNS_RESOLVER(qio_task_get_source(task);
+ * SocketAddress **rawaddrs = NULL;
+ * size_t nrawaddrs = 0;
+ * Error *err = NULL;
+ *
+ * if (qio_task_propagate_error(task, &data->err)) {
+ * return;
+ * }
+ *
+ * qio_dns_resolver_lookup_result(resolver, task,
+ * &nrawaddrs, &rawaddrs);
+ *
+ * for (i = 0; i < nrawaddrs; i++) {
+ * QIOChannel *sock = qio_channel_new();
+ * Error *local_err = NULL;
+ * qio_channel_listen_sync(sock, rawaddrs[i], &local_err);
+ * if (local_err && !err) {
+ * error_propagate(err, local_err);
+ * } else {
+ * socks = g_renew(QIOChannelSocket *, socks, nsocks + 1);
+ * socks[nsocks++] = sock;
+ * }
+ * }
+ *
+ * if (nsocks == 0) {
+ * error_propagate(&data->err, err);
+ * } else {
+ * error_free(err);
+ * }
+ * }
+ *
+ * void mylisten(SocketAddress *addr, MyListenData *data) {
+ * QIODNSResolver *resolver = qio_dns_resolver_get_instance();
+ * qio_dns_resolver_lookup_async(dns, addr,
+ * mylistenresult, data, NULL);
+ * }
+ * </programlisting>
+ * </example>
+ */
+struct QIODNSResolver {
+ Object parent;
+};
+
+struct QIODNSResolverClass {
+ ObjectClass parent;
+};
+
+
+/**
+ * qio_dns_resolver_get_instance:
+ *
+ * Get the singleton dns resolver instance. The caller
+ * does not own a reference on the returned object.
+ *
+ * Returns: the single dns resolver instance
+ */
+QIODNSResolver *qio_dns_resolver_get_instance(void);
+
+/**
+ * qio_dns_resolver_lookup_sync:
+ * @resolver: the DNS resolver instance
+ * @addr: the address to resolve
+ * @naddr: pointer to hold number of resolved addresses
+ * @addrs: pointer to hold resolved addresses
+ * @errp: pointer to NULL initialized error object
+ *
+ * This will attempt to resolve the address provided
+ * in @addr. If resolution succeeds, @addrs will be filled
+ * with all the resolved addresses. @naddrs will specify
+ * the number of entries allocated in @addrs. The caller
+ * is responsible for freeing each entry in @addrs, as
+ * well as @addrs itself.
+ *
+ * DNS resolution will be done synchronously so execution
+ * of the caller may be blocked for an arbitrary length
+ * of time.
+ *
+ * Returns: 0 if resolution was successful, -1 on error
+ */
+int qio_dns_resolver_lookup_sync(QIODNSResolver *resolver,
+ SocketAddress *addr,
+ size_t *naddrs,
+ SocketAddress ***addrs,
+ Error **errp);
+
+/**
+ * qio_dns_resolver_lookup_sync:
+ * @resolver: the DNS resolver instance
+ * @addr: the address to resolve
+ * @naddr: pointer to hold number of resolved addresses
+ * @addrs: pointer to hold resolved addresses
+ * @errp: pointer to NULL initialized error object
+ *
+ * This will attempt to resolve the address provided
+ * in @addr. The callback @func will be invoked when
+ * resolution has either completed or failed. On
+ * success, the @func should call the method
+ * qio_dns_resolver_lookup_result() to obtain the
+ * results.
+ *
+ * DNS resolution will be done asynchronously so execution
+ * of the caller will not be blocked.
+ */
+void qio_dns_resolver_lookup_async(QIODNSResolver *resolver,
+ SocketAddress *addr,
+ QIOTaskFunc func,
+ gpointer opaque,
+ GDestroyNotify notify);
+
+/**
+ * qio_dns_resolver_lookup_result:
+ * @resolver: the DNS resolver instance
+ * @task: the task object to get results for
+ * @naddr: pointer to hold number of resolved addresses
+ * @addrs: pointer to hold resolved addresses
+ *
+ * This method should be called from the callback passed
+ * to qio_dns_resolver_lookup_async() in order to obtain
+ * results. @addrs will be filled with all the resolved
+ * addresses. @naddrs will specify the number of entries
+ * allocated in @addrs. The caller is responsible for
+ * freeing each entry in @addrs, as well as @addrs itself.
+ */
+void qio_dns_resolver_lookup_result(QIODNSResolver *resolver,
+ QIOTask *task,
+ size_t *naddrs,
+ SocketAddress ***addrs);
+
+#endif /* QIO_DNS_RESOLVER_H */
diff --git a/include/qemu/sockets.h b/include/qemu/sockets.h
index 5589e68..5f1bab9 100644
--- a/include/qemu/sockets.h
+++ b/include/qemu/sockets.h
@@ -32,6 +32,8 @@ int socket_set_fast_reuse(int fd);
*/
typedef void NonBlockingConnectHandler(int fd, Error *err, void *opaque);
+int inet_ai_family_from_address(InetSocketAddress *addr,
+ Error **errp);
InetSocketAddress *inet_parse(const char *str, Error **errp);
int inet_connect(const char *str, Error **errp);
int inet_connect_saddr(InetSocketAddress *saddr, Error **errp,
diff --git a/io/Makefile.objs b/io/Makefile.objs
index 9d8337d..12983cc 100644
--- a/io/Makefile.objs
+++ b/io/Makefile.objs
@@ -7,4 +7,5 @@ io-obj-y += channel-tls.o
io-obj-y += channel-watch.o
io-obj-y += channel-websock.o
io-obj-y += channel-util.o
+io-obj-y += dns-resolver.o
io-obj-y += task.o
diff --git a/io/dns-resolver.c b/io/dns-resolver.c
new file mode 100644
index 0000000..029e54f
--- /dev/null
+++ b/io/dns-resolver.c
@@ -0,0 +1,265 @@
+/*
+ * QEMU DNS resolver
+ *
+ * Copyright (c) 2016 Red Hat, Inc.
+ *
+ * This library is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public
+ * License as published by the Free Software Foundation; either
+ * version 2 of the License, or (at your option) any later version.
+ *
+ * This library is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public
+ * License along with this library; if not, see <http://www.gnu.org/licenses/>.
+ *
+ */
+
+#include "qemu/osdep.h"
+#include "io/dns-resolver.h"
+#include "qapi/clone-visitor.h"
+#include "qemu/sockets.h"
+#include "qapi/error.h"
+#include "qemu/cutils.h"
+
+
+static QIODNSResolver *instance;
+
+QIODNSResolver *qio_dns_resolver_get_instance(void)
+{
+ return instance;
+}
+
+static int qio_dns_resolver_lookup_sync_inet(QIODNSResolver *resolver,
+ SocketAddress *addr,
+ size_t *naddrs,
+ SocketAddress ***addrs,
+ Error **errp)
+{
+ struct addrinfo ai, *res, *e;
+ InetSocketAddress *iaddr = addr->u.inet.data;
+ char port[33];
+ char uaddr[INET6_ADDRSTRLEN + 1];
+ char uport[33];
+ int rc;
+ Error *err = NULL;
+ size_t i;
+
+ *naddrs = 0;
+ *addrs = NULL;
+
+ memset(&ai, 0, sizeof(ai));
+ ai.ai_flags = AI_PASSIVE;
+ if (iaddr->numeric) {
+ ai.ai_flags |= AI_NUMERICHOST | AI_NUMERICSERV;
+ }
+ ai.ai_family = inet_ai_family_from_address(iaddr, &err);
+ ai.ai_socktype = SOCK_STREAM;
+
+ if (err) {
+ error_propagate(errp, err);
+ return -1;
+ }
+
+ if (iaddr->host == NULL) {
+ error_setg(errp, "host not specified");
+ return -1;
+ }
+ if (iaddr->port != NULL) {
+ pstrcpy(port, sizeof(port), iaddr->port);
+ } else {
+ port[0] = '\0';
+ }
+
+ rc = getaddrinfo(strlen(iaddr->host) ? iaddr->host : NULL,
+ strlen(port) ? port : NULL, &ai, &res);
+ if (rc != 0) {
+ error_setg(errp, "address resolution failed for %s:%s: %s",
+ iaddr->host, port, gai_strerror(rc));
+ return -1;
+ }
+
+ for (e = res; e != NULL; e = e->ai_next) {
+ (*naddrs)++;
+ }
+
+ *addrs = g_new0(SocketAddress *, *naddrs);
+
+ /* create socket + bind */
+ for (i = 0, e = res; e != NULL; i++, e = e->ai_next) {
+ SocketAddress *newaddr = g_new0(SocketAddress, 1);
+ InetSocketAddress *newiaddr = g_new0(InetSocketAddress, 1);
+ newaddr->u.inet.data = newiaddr;
+ newaddr->type = SOCKET_ADDRESS_KIND_INET;
+
+ getnameinfo((struct sockaddr *)e->ai_addr, e->ai_addrlen,
+ uaddr, INET6_ADDRSTRLEN, uport, 32,
+ NI_NUMERICHOST | NI_NUMERICSERV);
+
+ *newiaddr = (InetSocketAddress){
+ .host = g_strdup(uaddr),
+ .port = g_strdup(uport),
+ .numeric = true,
+ .has_to = iaddr->has_to,
+ .to = iaddr->to,
+ .has_ipv4 = false,
+ .has_ipv6 = false,
+ };
+
+ (*addrs)[i] = newaddr;
+ }
+ freeaddrinfo(res);
+ return 0;
+}
+
+
+static int qio_dns_resolver_lookup_sync_unix(QIODNSResolver *resolver,
+ SocketAddress *addr,
+ size_t *naddrs,
+ SocketAddress ***addrs,
+ Error **errp)
+{
+ *naddrs = 1;
+ *addrs = g_new0(SocketAddress *, 1);
+ (*addrs)[0] = QAPI_CLONE(SocketAddress, addr);
+
+ return 0;
+}
+
+
+int qio_dns_resolver_lookup_sync(QIODNSResolver *resolver,
+ SocketAddress *addr,
+ size_t *naddrs,
+ SocketAddress ***addrs,
+ Error **errp)
+{
+ switch (addr->type) {
+ case SOCKET_ADDRESS_KIND_INET:
+ return qio_dns_resolver_lookup_sync_inet(resolver,
+ addr,
+ naddrs,
+ addrs,
+ errp);
+
+ case SOCKET_ADDRESS_KIND_UNIX:
+ return qio_dns_resolver_lookup_sync_unix(resolver,
+ addr,
+ naddrs,
+ addrs,
+ errp);
+
+ default:
+ error_setg(errp, "Unknown socket address kind");
+ return -1;
+ }
+}
+
+
+struct QIODNSResolverLookupData {
+ SocketAddress *addr;
+ SocketAddress **addrs;
+ size_t naddrs;
+};
+
+
+static void qio_dns_resolver_lookup_data_free(gpointer opaque)
+{
+ struct QIODNSResolverLookupData *data = opaque;
+ size_t i;
+
+ qapi_free_SocketAddress(data->addr);
+ for (i = 0; i < data->naddrs; i++) {
+ qapi_free_SocketAddress(data->addrs[i]);
+ }
+
+ g_free(data->addrs);
+ g_free(data);
+}
+
+
+static void qio_dns_resolver_lookup_worker(QIOTask *task,
+ gpointer opaque)
+{
+ QIODNSResolver *resolver = QIO_DNS_RESOLVER(qio_task_get_source(task));
+ struct QIODNSResolverLookupData *data = opaque;
+ Error *err = NULL;
+
+ qio_dns_resolver_lookup_sync(resolver,
+ data->addr,
+ &data->naddrs,
+ &data->addrs,
+ &err);
+ if (err) {
+ qio_task_set_error(task, err);
+ } else {
+ qio_task_set_result_pointer(task, opaque, NULL);
+ }
+
+ object_unref(OBJECT(resolver));
+}
+
+
+void qio_dns_resolver_lookup_async(QIODNSResolver *resolver,
+ SocketAddress *addr,
+ QIOTaskFunc func,
+ gpointer opaque,
+ GDestroyNotify notify)
+{
+ QIOTask *task;
+ struct QIODNSResolverLookupData *data =
+ g_new0(struct QIODNSResolverLookupData, 1);
+
+ data->addr = QAPI_CLONE(SocketAddress, addr);
+
+ task = qio_task_new(OBJECT(resolver), func, opaque, notify);
+
+ qio_task_run_in_thread(task,
+ qio_dns_resolver_lookup_worker,
+ data,
+ qio_dns_resolver_lookup_data_free);
+}
+
+
+void qio_dns_resolver_lookup_result(QIODNSResolver *resolver,
+ QIOTask *task,
+ size_t *naddrs,
+ SocketAddress ***addrs)
+{
+ struct QIODNSResolverLookupData *data =
+ qio_task_get_result_pointer(task);
+ size_t i;
+
+ *naddrs = 0;
+ *addrs = NULL;
+ if (!data) {
+ return;
+ }
+
+ *naddrs = data->naddrs;
+ *addrs = g_new0(SocketAddress *, data->naddrs);
+ for (i = 0; i < data->naddrs; i++) {
+ (*addrs)[i] = QAPI_CLONE(SocketAddress, data->addrs[i]);
+ }
+}
+
+
+static const TypeInfo qio_dns_resolver_info = {
+ .parent = TYPE_OBJECT,
+ .name = TYPE_QIO_DNS_RESOLVER,
+ .instance_size = sizeof(QIODNSResolver),
+ .class_size = sizeof(QIODNSResolverClass),
+};
+
+
+static void qio_dns_resolver_register_types(void)
+{
+ type_register_static(&qio_dns_resolver_info);
+
+ instance = QIO_DNS_RESOLVER(object_new(TYPE_QIO_DNS_RESOLVER));
+}
+
+
+type_init(qio_dns_resolver_register_types);
diff --git a/util/qemu-sockets.c b/util/qemu-sockets.c
index 6f1c10a..ed31a1d 100644
--- a/util/qemu-sockets.c
+++ b/util/qemu-sockets.c
@@ -110,8 +110,8 @@ NetworkAddressFamily inet_netfamily(int family)
* outside scope of this method and not currently handled by
* callers at all.
*/
-static int inet_ai_family_from_address(InetSocketAddress *addr,
- Error **errp)
+int inet_ai_family_from_address(InetSocketAddress *addr,
+ Error **errp)
{
if (addr->has_ipv6 && addr->has_ipv4 &&
!addr->ipv6 && !addr->ipv4) {
--
2.9.3
^ permalink raw reply related [flat|nested] 22+ messages in thread* Re: [Qemu-devel] [PATCH 8/8] io: introduce a DNS resolver API
2017-01-05 16:03 ` [Qemu-devel] [PATCH 8/8] io: introduce a DNS resolver API Daniel P. Berrange
@ 2017-01-05 22:51 ` Eric Blake
2017-01-06 12:19 ` Daniel P. Berrange
0 siblings, 1 reply; 22+ messages in thread
From: Eric Blake @ 2017-01-05 22:51 UTC (permalink / raw)
To: Daniel P. Berrange, qemu-devel
[-- Attachment #1: Type: text/plain, Size: 14034 bytes --]
On 01/05/2017 10:03 AM, Daniel P. Berrange wrote:
> Currently DNS resolution is done automatically as part
> of the creation of a QIOChannelSocket object instance.
> This works ok for network clients where you just end
> up a single network socket, but for servers, the results
> of DNS resolution may require creation of multiple
> sockets.
>
> Introducing a DNS resolver API allows DNS resolution
> to be separated from the socket object creation. This
> will make it practical to create multiple QIOChannelSocket
> instances for servers.
>
> Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
> ---
> include/io/dns-resolver.h | 224 +++++++++++++++++++++++++++++++++++++++
> include/qemu/sockets.h | 2 +
> io/Makefile.objs | 1 +
> io/dns-resolver.c | 265 ++++++++++++++++++++++++++++++++++++++++++++++
> util/qemu-sockets.c | 4 +-
> 5 files changed, 494 insertions(+), 2 deletions(-)
> create mode 100644 include/io/dns-resolver.h
> create mode 100644 io/dns-resolver.c
>
> diff --git a/include/io/dns-resolver.h b/include/io/dns-resolver.h
> new file mode 100644
> index 0000000..5121e65
> --- /dev/null
> +++ b/include/io/dns-resolver.h
> @@ -0,0 +1,224 @@
> +/*
> + * QEMU DNS resolver
> + *
> + * Copyright (c) 2016 Red Hat, Inc.
Want to add 2017?
> +
> +/**
> + * QIODNSResolver:
> + *
> + * The QIODNSResolver class provides a framework for doing
> + * DNS resolution on SocketAddress objects, independently
> + * of socket creation.
> + *
> + * <example>
> + * <title>Resolving addresses synchronously</title>
> + * <programlisting>
> + * int mylisten(SocketAddress *addr, Error **errp) {
> + * QIODNSResolver *resolver = qio_dns_resolver_get_instance();
> + * SocketAddress **rawaddrs = NULL;
> + * size_t nrawaddrs = 0;
> + * Error *err = NULL;
> + * QIOChannel **socks = NULL;
> + * size_t nsocks = 0;
> + *
> + * if (qio_dns_resolver_lookup_sync(dns, addr, &nrawaddrs,
> + * &rawaddrs, &err) < 0) {
> + * error_propagate(errp, err);
You aren't using the local err here; why not just call
qio_dns_resolver_lookup_sync(, errp) directly, then you don't need to
propagate?
Is it guaranteed that 'err' is set only when
qio_dns_resolver_lookup_sync() returns negative, and conversely that
nrawaddrs is > 0 when it succeeded? It matters later...[3]
> + * return -1;
> + * }
> + *
> + * for (i = 0; i < nrawaddrs; i++) {
> + * QIOChannel *sock = qio_channel_new();
> + * Error *local_err = NULL;
It looks weird that you are declaring two different local Error*
variables. But I read further down, and finally figured out why...[1]
> + * qio_channel_listen_sync(sock, rawaddrs[i], &local_err);
> + * if (local_err && !err) {
> + * error_propagate(err, local_err);
Won't compile as written; you need error_propagate(&err, local_err);
error_propagate() is safe to call more than once (first error wins), so
you could simplify this to 'if (local_err) {'. In fact, if you don't
rewrite the condition, then on the second error, you end up falling
through to the else...[2]
> + * } else {
> + * socks = g_renew(QIOChannelSocket *, socks, nsocks + 1);
[2]...and allocating a slot in spite of the failure, plus leaking
local_err at the end of the loop. Oops.
As long as nsocks is not going to be arbitrarily huge, it shouldn't
matter that this particular style of array growth is O(n^2) in
complexity (quadratic complexity is fine on small lists, but for large
lists, you need to realloc in geometrically-larger increments to keep
things amortized to linear costs that otherwise explode due to copying
old-to-new array on each iteration).
> + * socks[nsocks++] = sock;
> + * }
> + * }
> + *
> + * if (nsocks == 0) {
> + * error_propagate(errp, err);
[3]...if the DNS lookup can succeed with nrawaddrs == 0, then you have
nsocks == 0 but no err set. Is that a problem? Or is nrawaddrs always
greater than 0 on success, such that nsocks == 0 always implies that we
failed to open every single socket and thus have err set?
> + * } else {
> + * error_free(err);
[1]...and this is why you have two error variables. You've chosen to
explicitly succeed if you get at least one socket open, even in the case
where resolution returns multiple possible addresses and some of them fail.
> + * }
> + * }
> + * </programlisting>
> + * </example>
> + *
> + * <example>
> + * <title>Resolving addresses asynchronously</title>
> + * <programlisting>
> + * typedef struct MyListenData {
> + * Error *err;
> + * QIOChannelSocket **socks;
> + * size_t nsocks;
> + * } MyListenData;
> + *
> + * void mylistenresult(QIOTask *task, void *opaque) {
> + * MyListenData *data = opaque;
> + * QIODNSResolver *resolver =
> + * QIO_DNS_RESOLVER(qio_task_get_source(task);
> + * SocketAddress **rawaddrs = NULL;
> + * size_t nrawaddrs = 0;
> + * Error *err = NULL;
> + *
> + * if (qio_task_propagate_error(task, &data->err)) {
> + * return;
> + * }
> + *
> + * qio_dns_resolver_lookup_result(resolver, task,
> + * &nrawaddrs, &rawaddrs);
> + *
> + * for (i = 0; i < nrawaddrs; i++) {
> + * QIOChannel *sock = qio_channel_new();
> + * Error *local_err = NULL;
> + * qio_channel_listen_sync(sock, rawaddrs[i], &local_err);
> + * if (local_err && !err) {
> + * error_propagate(err, local_err);
Same problem as in the last example, where you don't handle
double-failure correctly, and where the code won't compile without &.
> + * } else {
> + * socks = g_renew(QIOChannelSocket *, socks, nsocks + 1);
> + * socks[nsocks++] = sock;
> + * }
> + * }
> + *
> + * if (nsocks == 0) {
> + * error_propagate(&data->err, err);
> + * } else {
> + * error_free(err);
> + * }
> + * }
> + *
> + * void mylisten(SocketAddress *addr, MyListenData *data) {
> + * QIODNSResolver *resolver = qio_dns_resolver_get_instance();
> + * qio_dns_resolver_lookup_async(dns, addr,
> + * mylistenresult, data, NULL);
> + * }
> + * </programlisting>
> + * </example>
> + */
The examples are much appreciated; looking forward to v2.
> +/**
> + * qio_dns_resolver_lookup_sync:
> + * @resolver: the DNS resolver instance
> + * @addr: the address to resolve
> + * @naddr: pointer to hold number of resolved addresses
> + * @addrs: pointer to hold resolved addresses
> + * @errp: pointer to NULL initialized error object
> + *
> + * This will attempt to resolve the address provided
> + * in @addr. If resolution succeeds, @addrs will be filled
> + * with all the resolved addresses. @naddrs will specify
> + * the number of entries allocated in @addrs. The caller
> + * is responsible for freeing each entry in @addrs, as
> + * well as @addrs itself.
Where in your example code above do you free the memory? Or is that a
leak you need to plug?
Are we guaranteed that naddrs > 0 on success? (point [3] above)
> + *
> + * DNS resolution will be done synchronously so execution
> + * of the caller may be blocked for an arbitrary length
> + * of time.
> + *
> + * Returns: 0 if resolution was successful, -1 on error
> + */
> +int qio_dns_resolver_lookup_sync(QIODNSResolver *resolver,
> + SocketAddress *addr,
> + size_t *naddrs,
> + SocketAddress ***addrs,
> + Error **errp);
> +
> +/**
> + * qio_dns_resolver_lookup_sync:
s/sync/async/
> + * @resolver: the DNS resolver instance
> + * @addr: the address to resolve
> + * @naddr: pointer to hold number of resolved addresses
> + * @addrs: pointer to hold resolved addresses
> + * @errp: pointer to NULL initialized error object
Wrong parameters; naddr/addrs/errp should be replaced with
func/opaque/notify.
> + *
> + * This will attempt to resolve the address provided
> + * in @addr. The callback @func will be invoked when
> + * resolution has either completed or failed. On
> + * success, the @func should call the method
> + * qio_dns_resolver_lookup_result() to obtain the
> + * results.
> + *
> + * DNS resolution will be done asynchronously so execution
> + * of the caller will not be blocked.
> + */
> +void qio_dns_resolver_lookup_async(QIODNSResolver *resolver,
> + SocketAddress *addr,
> + QIOTaskFunc func,
> + gpointer opaque,
> + GDestroyNotify notify);
> +
> +/**
> + * qio_dns_resolver_lookup_result:
> + * @resolver: the DNS resolver instance
> + * @task: the task object to get results for
> + * @naddr: pointer to hold number of resolved addresses
> + * @addrs: pointer to hold resolved addresses
> + *
> + * This method should be called from the callback passed
> + * to qio_dns_resolver_lookup_async() in order to obtain
> + * results. @addrs will be filled with all the resolved
> + * addresses. @naddrs will specify the number of entries
> + * allocated in @addrs. The caller is responsible for
> + * freeing each entry in @addrs, as well as @addrs itself.
Again, the free seems to be missing in the example above.
> + */
> +void qio_dns_resolver_lookup_result(QIODNSResolver *resolver,
> + QIOTask *task,
> + size_t *naddrs,
> + SocketAddress ***addrs);
> +
> +#endif /* QIO_DNS_RESOLVER_H */
Overall the interface looks reasonable.
> +++ b/io/dns-resolver.c
> @@ -0,0 +1,265 @@
> +/*
> + * QEMU DNS resolver
> + *
> + * Copyright (c) 2016 Red Hat, Inc.
and 2017?
> +static int qio_dns_resolver_lookup_sync_inet(QIODNSResolver *resolver,
> + SocketAddress *addr,
> + size_t *naddrs,
> + SocketAddress ***addrs,
> + Error **errp)
> +{
> + struct addrinfo ai, *res, *e;
> + InetSocketAddress *iaddr = addr->u.inet.data;
> + char port[33];
> + char uaddr[INET6_ADDRSTRLEN + 1];
> + char uport[33];
> + int rc;
> + Error *err = NULL;
> + size_t i;
> +
> + *naddrs = 0;
> + *addrs = NULL;
> +
> + memset(&ai, 0, sizeof(ai));
> + ai.ai_flags = AI_PASSIVE;
> + if (iaddr->numeric) {
'iaddr->has_numeric && iaddr->numeric', unless you make sure that all
possible initialization paths have a sane value of iaddr->numeric==false
even when iaddr->has_numeric is false (qapi guarantees 0 initialization,
but I'm not sure if all SocketAddress come from qapi).
> + /* create socket + bind */
> + for (i = 0, e = res; e != NULL; i++, e = e->ai_next) {
> + SocketAddress *newaddr = g_new0(SocketAddress, 1);
> + InetSocketAddress *newiaddr = g_new0(InetSocketAddress, 1);
> + newaddr->u.inet.data = newiaddr;
> + newaddr->type = SOCKET_ADDRESS_KIND_INET;
> +
> + getnameinfo((struct sockaddr *)e->ai_addr, e->ai_addrlen,
> + uaddr, INET6_ADDRSTRLEN, uport, 32,
> + NI_NUMERICHOST | NI_NUMERICSERV);
> +
> + *newiaddr = (InetSocketAddress){
> + .host = g_strdup(uaddr),
> + .port = g_strdup(uport),
> + .numeric = true,
Also need .has_numeric = true
> + .has_to = iaddr->has_to,
> + .to = iaddr->to,
> + .has_ipv4 = false,
> + .has_ipv6 = false,
> + };
> +
> + (*addrs)[i] = newaddr;
> + }
> + freeaddrinfo(res);
> + return 0;
> +}
> +
> +
> +static int qio_dns_resolver_lookup_sync_unix(QIODNSResolver *resolver,
> + SocketAddress *addr,
> + size_t *naddrs,
> + SocketAddress ***addrs,
> + Error **errp)
> +{
> + *naddrs = 1;
> + *addrs = g_new0(SocketAddress *, 1);
> + (*addrs)[0] = QAPI_CLONE(SocketAddress, addr);
Cool - I'm glad to see more use of my clone visitor :)
> +
> + return 0;
> +}
> +
> +
> +int qio_dns_resolver_lookup_sync(QIODNSResolver *resolver,
> + SocketAddress *addr,
> + size_t *naddrs,
> + SocketAddress ***addrs,
> + Error **errp)
> +{
> + switch (addr->type) {
> + case SOCKET_ADDRESS_KIND_INET:
> + return qio_dns_resolver_lookup_sync_inet(resolver,
> + addr,
> + naddrs,
> + addrs,
> + errp);
> +
> + case SOCKET_ADDRESS_KIND_UNIX:
> + return qio_dns_resolver_lookup_sync_unix(resolver,
> + addr,
> + naddrs,
> + addrs,
> + errp);
> +
> + default:
Do we need to play with Stefan's vsock stuff?
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]
^ permalink raw reply [flat|nested] 22+ messages in thread* Re: [Qemu-devel] [PATCH 8/8] io: introduce a DNS resolver API
2017-01-05 22:51 ` Eric Blake
@ 2017-01-06 12:19 ` Daniel P. Berrange
0 siblings, 0 replies; 22+ messages in thread
From: Daniel P. Berrange @ 2017-01-06 12:19 UTC (permalink / raw)
To: Eric Blake; +Cc: qemu-devel
On Thu, Jan 05, 2017 at 04:51:53PM -0600, Eric Blake wrote:
> On 01/05/2017 10:03 AM, Daniel P. Berrange wrote:
> > + * <example>
> > + * <title>Resolving addresses synchronously</title>
> > + * <programlisting>
> > + * int mylisten(SocketAddress *addr, Error **errp) {
> > + * QIODNSResolver *resolver = qio_dns_resolver_get_instance();
> > + * SocketAddress **rawaddrs = NULL;
> > + * size_t nrawaddrs = 0;
> > + * Error *err = NULL;
> > + * QIOChannel **socks = NULL;
> > + * size_t nsocks = 0;
> > + *
> > + * if (qio_dns_resolver_lookup_sync(dns, addr, &nrawaddrs,
> > + * &rawaddrs, &err) < 0) {
> > + * error_propagate(errp, err);
>
> You aren't using the local err here; why not just call
> qio_dns_resolver_lookup_sync(, errp) directly, then you don't need to
> propagate?
Yep
> Is it guaranteed that 'err' is set only when
> qio_dns_resolver_lookup_sync() returns negative, and conversely that
> nrawaddrs is > 0 when it succeeded? It matters later...[3]
getaddrinfo() returns an error code of EAI_NODATA if the dns name
has no corresponding records, so we should be guaranteed > 0 for
nrawaddrs on success.
>
> > + * return -1;
> > + * }
> > + *
> > + * for (i = 0; i < nrawaddrs; i++) {
> > + * QIOChannel *sock = qio_channel_new();
> > + * Error *local_err = NULL;
>
> It looks weird that you are declaring two different local Error*
> variables. But I read further down, and finally figured out why...[1]
Yeah, the peculiarty of getaddrinfo where you should only report an
error if every address failed.
>
> > + * qio_channel_listen_sync(sock, rawaddrs[i], &local_err);
> > + * if (local_err && !err) {
> > + * error_propagate(err, local_err);
>
> Won't compile as written; you need error_propagate(&err, local_err);
>
> error_propagate() is safe to call more than once (first error wins), so
> you could simplify this to 'if (local_err) {'. In fact, if you don't
> rewrite the condition, then on the second error, you end up falling
> through to the else...[2]
>
> > + * } else {
> > + * socks = g_renew(QIOChannelSocket *, socks, nsocks + 1);
>
> [2]...and allocating a slot in spite of the failure, plus leaking
> local_err at the end of the loop. Oops.
Oh, nice spot.
> As long as nsocks is not going to be arbitrarily huge, it shouldn't
> matter that this particular style of array growth is O(n^2) in
> complexity (quadratic complexity is fine on small lists, but for large
> lists, you need to realloc in geometrically-larger increments to keep
> things amortized to linear costs that otherwise explode due to copying
> old-to-new array on each iteration).
Yeah, realistically 99% of the time there will be 2 results (ipv4 and
ipv6). Only in niche cases where there's a host with multiple public
IPs will we have more addressses and even then it'll be less than
10. So the complexity is not an issue here IMHO - the actual dns
network lookup time will dwarf any inefficiency.
> > + * } else {
> > + * error_free(err);
>
> [1]...and this is why you have two error variables. You've chosen to
> explicitly succeed if you get at least one socket open, even in the case
> where resolution returns multiple possible addresses and some of them fail.
Yep, glibc recommended behaviour for listening with getaddrinfo().
> > +/**
> > + * qio_dns_resolver_lookup_sync:
> > + * @resolver: the DNS resolver instance
> > + * @addr: the address to resolve
> > + * @naddr: pointer to hold number of resolved addresses
> > + * @addrs: pointer to hold resolved addresses
> > + * @errp: pointer to NULL initialized error object
> > + *
> > + * This will attempt to resolve the address provided
> > + * in @addr. If resolution succeeds, @addrs will be filled
> > + * with all the resolved addresses. @naddrs will specify
> > + * the number of entries allocated in @addrs. The caller
> > + * is responsible for freeing each entry in @addrs, as
> > + * well as @addrs itself.
>
> Where in your example code above do you free the memory? Or is that a
> leak you need to plug?
Opps, yes, a leak
> Are we guaranteed that naddrs > 0 on success? (point [3] above)
Yes and will document it.
>
> > + *
> > + * DNS resolution will be done synchronously so execution
> > + * of the caller may be blocked for an arbitrary length
> > + * of time.
> > + *
> > + * Returns: 0 if resolution was successful, -1 on error
> > + */
> > +int qio_dns_resolver_lookup_sync(QIODNSResolver *resolver,
> > + SocketAddress *addr,
> > + size_t *naddrs,
> > + SocketAddress ***addrs,
> > + Error **errp);
> > +
> > +/**
> > + * qio_dns_resolver_lookup_sync:
>
> s/sync/async/
>
> > + * @resolver: the DNS resolver instance
> > + * @addr: the address to resolve
> > + * @naddr: pointer to hold number of resolved addresses
> > + * @addrs: pointer to hold resolved addresses
> > + * @errp: pointer to NULL initialized error object
>
> Wrong parameters; naddr/addrs/errp should be replaced with
> func/opaque/notify.
>
> > + *
> > + * This will attempt to resolve the address provided
> > + * in @addr. The callback @func will be invoked when
> > + * resolution has either completed or failed. On
> > + * success, the @func should call the method
> > + * qio_dns_resolver_lookup_result() to obtain the
> > + * results.
> > + *
> > + * DNS resolution will be done asynchronously so execution
> > + * of the caller will not be blocked.
> > + */
> > +void qio_dns_resolver_lookup_async(QIODNSResolver *resolver,
> > + SocketAddress *addr,
> > + QIOTaskFunc func,
> > + gpointer opaque,
> > + GDestroyNotify notify);
> > +
> > +/**
> > + * qio_dns_resolver_lookup_result:
> > + * @resolver: the DNS resolver instance
> > + * @task: the task object to get results for
> > + * @naddr: pointer to hold number of resolved addresses
> > + * @addrs: pointer to hold resolved addresses
> > + *
> > + * This method should be called from the callback passed
> > + * to qio_dns_resolver_lookup_async() in order to obtain
> > + * results. @addrs will be filled with all the resolved
> > + * addresses. @naddrs will specify the number of entries
> > + * allocated in @addrs. The caller is responsible for
> > + * freeing each entry in @addrs, as well as @addrs itself.
>
> Again, the free seems to be missing in the example above.
Yep, will fix.
> > +static int qio_dns_resolver_lookup_sync_inet(QIODNSResolver *resolver,
> > + SocketAddress *addr,
> > + size_t *naddrs,
> > + SocketAddress ***addrs,
> > + Error **errp)
> > +{
> > + struct addrinfo ai, *res, *e;
> > + InetSocketAddress *iaddr = addr->u.inet.data;
> > + char port[33];
> > + char uaddr[INET6_ADDRSTRLEN + 1];
> > + char uport[33];
> > + int rc;
> > + Error *err = NULL;
> > + size_t i;
> > +
> > + *naddrs = 0;
> > + *addrs = NULL;
> > +
> > + memset(&ai, 0, sizeof(ai));
> > + ai.ai_flags = AI_PASSIVE;
> > + if (iaddr->numeric) {
>
> 'iaddr->has_numeric && iaddr->numeric', unless you make sure that all
> possible initialization paths have a sane value of iaddr->numeric==false
> even when iaddr->has_numeric is false (qapi guarantees 0 initialization,
> but I'm not sure if all SocketAddress come from qapi).
Yeah, makes sense to be explicit about it anyway
> > + /* create socket + bind */
> > + for (i = 0, e = res; e != NULL; i++, e = e->ai_next) {
> > + SocketAddress *newaddr = g_new0(SocketAddress, 1);
> > + InetSocketAddress *newiaddr = g_new0(InetSocketAddress, 1);
> > + newaddr->u.inet.data = newiaddr;
> > + newaddr->type = SOCKET_ADDRESS_KIND_INET;
> > +
> > + getnameinfo((struct sockaddr *)e->ai_addr, e->ai_addrlen,
> > + uaddr, INET6_ADDRSTRLEN, uport, 32,
> > + NI_NUMERICHOST | NI_NUMERICSERV);
> > +
> > + *newiaddr = (InetSocketAddress){
> > + .host = g_strdup(uaddr),
> > + .port = g_strdup(uport),
> > + .numeric = true,
>
> Also need .has_numeric = true
>
> > + .has_to = iaddr->has_to,
> > + .to = iaddr->to,
> > + .has_ipv4 = false,
> > + .has_ipv6 = false,
> > + };
> > +
> > + (*addrs)[i] = newaddr;
> > + }
> > + freeaddrinfo(res);
> > + return 0;
> > +}
> > +
> > +
> > +static int qio_dns_resolver_lookup_sync_unix(QIODNSResolver *resolver,
> > + SocketAddress *addr,
> > + size_t *naddrs,
> > + SocketAddress ***addrs,
> > + Error **errp)
> > +{
> > + *naddrs = 1;
> > + *addrs = g_new0(SocketAddress *, 1);
> > + (*addrs)[0] = QAPI_CLONE(SocketAddress, addr);
>
> Cool - I'm glad to see more use of my clone visitor :)
>
> > +
> > + return 0;
> > +}
> > +
> > +
> > +int qio_dns_resolver_lookup_sync(QIODNSResolver *resolver,
> > + SocketAddress *addr,
> > + size_t *naddrs,
> > + SocketAddress ***addrs,
> > + Error **errp)
> > +{
> > + switch (addr->type) {
> > + case SOCKET_ADDRESS_KIND_INET:
> > + return qio_dns_resolver_lookup_sync_inet(resolver,
> > + addr,
> > + naddrs,
> > + addrs,
> > + errp);
> > +
> > + case SOCKET_ADDRESS_KIND_UNIX:
> > + return qio_dns_resolver_lookup_sync_unix(resolver,
> > + addr,
> > + naddrs,
> > + addrs,
> > + errp);
> > +
> > + default:
>
> Do we need to play with Stefan's vsock stuff?
Oh yes, I didn't realize that was merged. It should be just
cloned as we do for unix sock.
Regards,
Daniel
--
|: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org -o- http://virt-manager.org :|
|: http://entangle-photo.org -o- http://search.cpan.org/~danberr/ :|
^ permalink raw reply [flat|nested] 22+ messages in thread