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 <js...@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
