Re: [Qemu-devel] [PATCH v2 02/11] blockjob: centralize QMP event emissions

[Kevin Wolf <kwolf@redhat.com>]

Am 01.10.2016 um 00:00 hat John Snow geschrieben:
> There's no reason to leave this to blockdev; we can do it in blockjobs
> directly and get rid of an extra callback for most users.
> 
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>  blockdev.c | 37 ++++++-------------------------------
>  blockjob.c | 16 ++++++++++++++--
>  2 files changed, 20 insertions(+), 33 deletions(-)
> 
> diff --git a/blockdev.c b/blockdev.c
> index 29c6561..03200e7 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -2957,31 +2957,6 @@ out:
>      aio_context_release(aio_context);
>  }
>  
> -static void block_job_cb(void *opaque, int ret)
> -{
> -    /* Note that this function may be executed from another AioContext besides
> -     * the QEMU main loop.  If you need to access anything that assumes the
> -     * QEMU global mutex, use a BH or introduce a mutex.
> -     */
> -
> -    BlockDriverState *bs = opaque;
> -    const char *msg = NULL;
> -
> -    trace_block_job_cb(bs, bs->job, ret);

This trace event is removed from the code, but not from trace-events.

> -
> -    assert(bs->job);
> -
> -    if (ret < 0) {
> -        msg = strerror(-ret);
> -    }
> -
> -    if (block_job_is_cancelled(bs->job)) {
> -        block_job_event_cancelled(bs->job);
> -    } else {
> -        block_job_event_completed(bs->job, msg);

block_job_event_cancelled/completed can become static now.

> -    }
> -}
> -
>  void qmp_block_stream(bool has_job_id, const char *job_id, const char *device,
>                        bool has_base, const char *base,
>                        bool has_backing_file, const char *backing_file,
> @@ -3033,7 +3008,7 @@ void qmp_block_stream(bool has_job_id, const char *job_id, const char *device,
>      base_name = has_backing_file ? backing_file : base_name;
>  
>      stream_start(has_job_id ? job_id : NULL, bs, base_bs, base_name,
> -                 has_speed ? speed : 0, on_error, block_job_cb, bs, &local_err);
> +                 has_speed ? speed : 0, on_error, NULL, bs, &local_err);

Passing cb == NULL, but opaque != NULL is harmless, but feels odd.

And actually this is the only caller of stream_start, so the parameters
could just be dropped.

>      if (local_err) {
>          error_propagate(errp, local_err);
>          goto out;
> @@ -3136,10 +3111,10 @@ void qmp_block_commit(bool has_job_id, const char *job_id, const char *device,
>              goto out;
>          }
>          commit_active_start(has_job_id ? job_id : NULL, bs, base_bs, speed,
> -                            on_error, block_job_cb, bs, &local_err, false);
> +                            on_error, NULL, bs, &local_err, false);

Here we have an additional caller in block/replication.c and qemu-img,
so the parameters must stay. For qemu-img, nothing changes. For
replication, the block job events are added as a side effect.

Not sure if we want to emit such events for an internal block job, but
if we do want the change, it should be explicit.

>      } else {
>          commit_start(has_job_id ? job_id : NULL, bs, base_bs, top_bs, speed,
> -                     on_error, block_job_cb, bs,
> +                     on_error, NULL, bs,
>                       has_backing_file ? backing_file : NULL, &local_err);

Like stream_start, drop the parameters.

>      }
>      if (local_err != NULL) {
> @@ -3260,7 +3235,7 @@ static void do_drive_backup(DriveBackup *backup, BlockJobTxn *txn, Error **errp)
>  
>      backup_start(backup->job_id, bs, target_bs, backup->speed, backup->sync,
>                   bmap, backup->compress, backup->on_source_error,
> -                 backup->on_target_error, block_job_cb, bs, txn, &local_err);
> +                 backup->on_target_error, NULL, bs, txn, &local_err);
>      bdrv_unref(target_bs);
>      if (local_err != NULL) {
>          error_propagate(errp, local_err);
> @@ -3330,7 +3305,7 @@ void do_blockdev_backup(BlockdevBackup *backup, BlockJobTxn *txn, Error **errp)
>      }
>      backup_start(backup->job_id, bs, target_bs, backup->speed, backup->sync,
>                   NULL, backup->compress, backup->on_source_error,
> -                 backup->on_target_error, block_job_cb, bs, txn, &local_err);
> +                 backup->on_target_error, NULL, bs, txn, &local_err);

Backup is another job used by replication, too. Same question as above.

>      if (local_err != NULL) {
>          error_propagate(errp, local_err);
>      }
> @@ -3410,7 +3385,7 @@ static void blockdev_mirror_common(const char *job_id, BlockDriverState *bs,
>                   has_replaces ? replaces : NULL,
>                   speed, granularity, buf_size, sync, backing_mode,
>                   on_source_error, on_target_error, unmap,
> -                 block_job_cb, bs, errp);
> +                 NULL, bs, errp);

And again, the parameters can be dropped.

Kevin