Re: [Qemu-devel] [PATCH 7/8] io: remove Error parameter from QIOTask thread worker

[Eric Blake]

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