[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH 7/8] io: remove Error parameter from QIOTask thr
From: |
Eric Blake |
Subject: |
Re: [Qemu-devel] [PATCH 7/8] io: remove Error parameter from QIOTask thread worker |
Date: |
Thu, 5 Jan 2017 16:09:47 -0600 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.5.1 |
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 <address@hidden>
> ---
> 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 <address@hidden>
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
signature.asc
Description: OpenPGP digital signature
- [Qemu-devel] [PATCH 5/8] io: add ability to associate an error with a task, (continued)