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 <berra...@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 <ebl...@redhat.com>
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
signature.asc
Description: OpenPGP digital signature
