Re: [Qemu-devel] [PATCH v3 1/9] jobs: change start callback to run callback
On Wed, Aug 29, 2018 at 09:57:26PM -0400, John Snow wrote:
> Presently we codify the entry point for a job as the "start" callback,
> but a more apt name would be "run" to clarify the idea that when this
> function returns we consider the job to have "finished," except for
> any cleanup which occurs in separate callbacks later.
>
> As part of this clarification, change the signature to include an error
> object and a return code. The error ptr is not yet used, and the return
> code while captured, will be overwritten by actions in the job_completed
> function.
>
> Signed-off-by: John Snow
> Reviewed-by: Max Reitz
Reviewed-by: Jeff Cody
> ---
> block/backup.c| 7 ---
> block/commit.c| 7 ---
> block/create.c| 8 +---
> block/mirror.c| 10 ++
> block/stream.c| 7 ---
> include/qemu/job.h| 2 +-
> job.c | 6 +++---
> tests/test-bdrv-drain.c | 7 ---
> tests/test-blockjob-txn.c | 16
> tests/test-blockjob.c | 7 ---
> 10 files changed, 43 insertions(+), 34 deletions(-)
>
> diff --git a/block/backup.c b/block/backup.c
> index 8630d32926..5d47781840 100644
> --- a/block/backup.c
> +++ b/block/backup.c
> @@ -480,9 +480,9 @@ static void
> backup_incremental_init_copy_bitmap(BackupBlockJob *job)
> bdrv_dirty_iter_free(dbi);
> }
>
> -static void coroutine_fn backup_run(void *opaque)
> +static int coroutine_fn backup_run(Job *opaque_job, Error **errp)
> {
> -BackupBlockJob *job = opaque;
> +BackupBlockJob *job = container_of(opaque_job, BackupBlockJob,
> common.job);
> BackupCompleteData *data;
> BlockDriverState *bs = blk_bs(job->common.blk);
> int64_t offset, nb_clusters;
> @@ -587,6 +587,7 @@ static void coroutine_fn backup_run(void *opaque)
> data = g_malloc(sizeof(*data));
> data->ret = ret;
> job_defer_to_main_loop(&job->common.job, backup_complete, data);
> +return ret;
> }
>
> static const BlockJobDriver backup_job_driver = {
> @@ -596,7 +597,7 @@ static const BlockJobDriver backup_job_driver = {
> .free = block_job_free,
> .user_resume= block_job_user_resume,
> .drain = block_job_drain,
> -.start = backup_run,
> +.run= backup_run,
> .commit = backup_commit,
> .abort = backup_abort,
> .clean = backup_clean,
> diff --git a/block/commit.c b/block/commit.c
> index eb414579bd..a0ea86ff64 100644
> --- a/block/commit.c
> +++ b/block/commit.c
> @@ -134,9 +134,9 @@ static void commit_complete(Job *job, void *opaque)
> bdrv_unref(top);
> }
>
> -static void coroutine_fn commit_run(void *opaque)
> +static int coroutine_fn commit_run(Job *job, Error **errp)
> {
> -CommitBlockJob *s = opaque;
> +CommitBlockJob *s = container_of(job, CommitBlockJob, common.job);
> CommitCompleteData *data;
> int64_t offset;
> uint64_t delay_ns = 0;
> @@ -213,6 +213,7 @@ out:
> data = g_malloc(sizeof(*data));
> data->ret = ret;
> job_defer_to_main_loop(&s->common.job, commit_complete, data);
> +return ret;
> }
>
> static const BlockJobDriver commit_job_driver = {
> @@ -222,7 +223,7 @@ static const BlockJobDriver commit_job_driver = {
> .free = block_job_free,
> .user_resume = block_job_user_resume,
> .drain = block_job_drain,
> -.start = commit_run,
> +.run = commit_run,
> },
> };
>
> diff --git a/block/create.c b/block/create.c
> index 915cd41bcc..04733c3618 100644
> --- a/block/create.c
> +++ b/block/create.c
> @@ -45,9 +45,9 @@ static void blockdev_create_complete(Job *job, void *opaque)
> job_completed(job, s->ret, s->err);
> }
>
> -static void coroutine_fn blockdev_create_run(void *opaque)
> +static int coroutine_fn blockdev_create_run(Job *job, Error **errp)
> {
> -BlockdevCreateJob *s = opaque;
> +BlockdevCreateJob *s = container_of(job, BlockdevCreateJob, common);
>
> job_progress_set_remaining(&s->common, 1);
> s->ret = s->drv->bdrv_co_create(s->opts, &s->err);
> @@ -55,12 +55,14 @@ static void coroutine_fn blockdev_create_run(void *opaque)
>
> qapi_free_BlockdevCreateOptions(s->opts);
> job_defer_to_main_loop(&s->common, blockdev_create_complete, NULL);
> +
> +return s->ret;
> }
>
&
Re: [Qemu-devel] [PATCH v3 3/9] jobs: add exit shim
On Wed, Aug 29, 2018 at 09:57:28PM -0400, John Snow wrote:
> All jobs do the same thing when they leave their running loop:
> - Store the return code in a structure
> - wait to receive this structure in the main thread
> - signal job completion via job_completed
>
> Few jobs do anything beyond exactly this. Consolidate this exit
> logic for a net reduction in SLOC.
>
> More seriously, when we utilize job_defer_to_main_loop_bh to call
> a function that calls job_completed, job_finalize_single will run
> in a context where it has recursively taken the aio_context lock,
> which can cause hangs if it puts down a reference that causes a flush.
>
> You can observe this in practice by looking at mirror_exit's careful
> placement of job_completed and bdrv_unref calls.
>
> If we centralize job exiting, we can signal job completion from outside
> of the aio_context, which should allow for job cleanup code to run with
> only one lock, which makes cleanup callbacks less tricky to write.
>
> Signed-off-by: John Snow
> Reviewed-by: Max Reitz
Reviewed-by: Jeff Cody
> ---
> include/qemu/job.h | 11 +++
> job.c | 18 ++
> 2 files changed, 29 insertions(+)
>
> diff --git a/include/qemu/job.h b/include/qemu/job.h
> index e0e99870a1..1144d671a1 100644
> --- a/include/qemu/job.h
> +++ b/include/qemu/job.h
> @@ -208,6 +208,17 @@ struct JobDriver {
> */
> void (*drain)(Job *job);
>
> +/**
> + * If the callback is not NULL, exit will be invoked from the main thread
> + * when the job's coroutine has finished, but before transactional
> + * convergence; before @prepare or @abort.
> + *
> + * FIXME TODO: This callback is only temporary to transition remaining
> jobs
> + * to prepare/commit/abort/clean callbacks and will be removed before
> 3.1.
> + * is released.
> + */
> +void (*exit)(Job *job);
> +
> /**
> * If the callback is not NULL, prepare will be invoked when all the jobs
> * belonging to the same transaction complete; or upon this job's
> completion
> diff --git a/job.c b/job.c
> index bc1d970df4..bc8dad4e71 100644
> --- a/job.c
> +++ b/job.c
> @@ -535,6 +535,18 @@ void job_drain(Job *job)
> }
> }
>
> +static void job_exit(void *opaque)
> +{
> +Job *job = (Job *)opaque;
> +AioContext *aio_context = job->aio_context;
> +
> +if (job->driver->exit) {
> +aio_context_acquire(aio_context);
> +job->driver->exit(job);
> +aio_context_release(aio_context);
> +}
> +job_completed(job, job->ret);
> +}
>
> /**
> * All jobs must allow a pause point before entering their job proper. This
> @@ -547,6 +559,12 @@ static void coroutine_fn job_co_entry(void *opaque)
> assert(job && job->driver && job->driver->run);
> job_pause_point(job);
> job->ret = job->driver->run(job, &job->err);
> +if (!job->deferred_to_main_loop) {
> +job->deferred_to_main_loop = true;
> +aio_bh_schedule_oneshot(qemu_get_aio_context(),
> +job_exit,
> +job);
> +}
> }
>
>
> --
> 2.14.4
>
Re: [Qemu-devel] [PATCH v3 4/9] block/commit: utilize job_exit shim
On Wed, Aug 29, 2018 at 09:57:29PM -0400, John Snow wrote:
> Change the manual deferment to commit_complete into the implicit
> callback to job_exit, renaming commit_complete to commit_exit.
>
> This conversion does change the timing of when job_completed is
> called to after the bdrv_replace_node and bdrv_unref calls, which
> could have implications for bjob->blk which will now be put down
> after this cleanup.
>
> Kevin highlights that we did not take any permissions for that backend
> at job creation time, so it is safe to reorder these operations.
>
> Signed-off-by: John Snow
> Reviewed-by: Max Reitz
> ---
> block/commit.c | 22 +-
> 1 file changed, 5 insertions(+), 17 deletions(-)
>
> diff --git a/block/commit.c b/block/commit.c
> index 4a17bb73ec..da69165de3 100644
> --- a/block/commit.c
> +++ b/block/commit.c
> @@ -68,19 +68,13 @@ static int coroutine_fn commit_populate(BlockBackend *bs,
> BlockBackend *base,
> return 0;
> }
>
> -typedef struct {
> -int ret;
> -} CommitCompleteData;
> -
> -static void commit_complete(Job *job, void *opaque)
> +static void commit_exit(Job *job)
> {
> CommitBlockJob *s = container_of(job, CommitBlockJob, common.job);
> BlockJob *bjob = &s->common;
> -CommitCompleteData *data = opaque;
> BlockDriverState *top = blk_bs(s->top);
> BlockDriverState *base = blk_bs(s->base);
> BlockDriverState *commit_top_bs = s->commit_top_bs;
> -int ret = data->ret;
> bool remove_commit_top_bs = false;
>
> /* Make sure commit_top_bs and top stay around until bdrv_replace_node()
> */
> @@ -91,10 +85,10 @@ static void commit_complete(Job *job, void *opaque)
> * the normal backing chain can be restored. */
> blk_unref(s->base);
>
> -if (!job_is_cancelled(job) && ret == 0) {
> +if (!job_is_cancelled(job) && job->ret == 0) {
> /* success */
> -ret = bdrv_drop_intermediate(s->commit_top_bs, base,
> - s->backing_file_str);
> +job->ret = bdrv_drop_intermediate(s->commit_top_bs, base,
> + s->backing_file_str);
> } else {
> /* XXX Can (or should) we somehow keep 'consistent read' blocked even
> * after the failed/cancelled commit job is gone? If we already wrote
> @@ -117,9 +111,6 @@ static void commit_complete(Job *job, void *opaque)
> * bdrv_set_backing_hd() to fail. */
> block_job_remove_all_bdrv(bjob);
>
> -job_completed(job, ret);
> -g_free(data);
> -
Not having to allocate, track, and free this cumbersome return value for
each of these job specific completions now is pretty nice.
Reviewed-by: Jeff Cody
> /* If bdrv_drop_intermediate() didn't already do that, remove the commit
> * filter driver from the backing chain. Do this as the final step so
> that
> * the 'consistent read' permission can be granted. */
> @@ -137,7 +128,6 @@ static void commit_complete(Job *job, void *opaque)
> static int coroutine_fn commit_run(Job *job, Error **errp)
> {
> CommitBlockJob *s = container_of(job, CommitBlockJob, common.job);
> -CommitCompleteData *data;
> int64_t offset;
> uint64_t delay_ns = 0;
> int ret = 0;
> @@ -210,9 +200,6 @@ static int coroutine_fn commit_run(Job *job, Error **errp)
> out:
> qemu_vfree(buf);
>
> -data = g_malloc(sizeof(*data));
> -data->ret = ret;
> -job_defer_to_main_loop(&s->common.job, commit_complete, data);
> return ret;
> }
>
> @@ -224,6 +211,7 @@ static const BlockJobDriver commit_job_driver = {
> .user_resume = block_job_user_resume,
> .drain = block_job_drain,
> .run = commit_run,
> +.exit = commit_exit,
> },
> };
>
> --
> 2.14.4
>
Re: [Qemu-devel] [PATCH v3 5/9] block/mirror: utilize job_exit shim
On Wed, Aug 29, 2018 at 09:57:30PM -0400, John Snow wrote:
> Change the manual deferment to mirror_exit into the implicit
> callback to job_exit and the mirror_exit callback.
>
> This does change the order of some bdrv_unref calls and job_completed,
> but thanks to the new context in which we call .exit, this is safe to
> defer the possible flushing of any nodes to the job_finalize_single
> cleanup stage.
>
> Signed-off-by: John Snow
Reviewed-by: Jeff Cody
> ---
> block/mirror.c | 29 +++--
> 1 file changed, 11 insertions(+), 18 deletions(-)
>
> diff --git a/block/mirror.c b/block/mirror.c
> index be5dc6b7b0..b8941db6c1 100644
> --- a/block/mirror.c
> +++ b/block/mirror.c
> @@ -607,26 +607,22 @@ static void mirror_wait_for_all_io(MirrorBlockJob *s)
> }
> }
>
> -typedef struct {
> -int ret;
> -} MirrorExitData;
> -
> -static void mirror_exit(Job *job, void *opaque)
> +static void mirror_exit(Job *job)
> {
> MirrorBlockJob *s = container_of(job, MirrorBlockJob, common.job);
> BlockJob *bjob = &s->common;
> -MirrorExitData *data = opaque;
> MirrorBDSOpaque *bs_opaque = s->mirror_top_bs->opaque;
> AioContext *replace_aio_context = NULL;
> BlockDriverState *src = s->mirror_top_bs->backing->bs;
> BlockDriverState *target_bs = blk_bs(s->target);
> BlockDriverState *mirror_top_bs = s->mirror_top_bs;
> Error *local_err = NULL;
> +int ret = job->ret;
>
> bdrv_release_dirty_bitmap(src, s->dirty_bitmap);
>
> -/* Make sure that the source BDS doesn't go away before we called
> - * job_completed(). */
> +/* Make sure that the source BDS doesn't go away during
> bdrv_replace_node,
> + * before we can call bdrv_drained_end */
> bdrv_ref(src);
> bdrv_ref(mirror_top_bs);
> bdrv_ref(target_bs);
> @@ -652,7 +648,7 @@ static void mirror_exit(Job *job, void *opaque)
> bdrv_set_backing_hd(target_bs, backing, &local_err);
> if (local_err) {
> error_report_err(local_err);
> -data->ret = -EPERM;
> +ret = -EPERM;
> }
> }
> }
> @@ -662,7 +658,7 @@ static void mirror_exit(Job *job, void *opaque)
> aio_context_acquire(replace_aio_context);
> }
>
> -if (s->should_complete && data->ret == 0) {
> +if (s->should_complete && ret == 0) {
> BlockDriverState *to_replace = src;
> if (s->to_replace) {
> to_replace = s->to_replace;
> @@ -679,7 +675,7 @@ static void mirror_exit(Job *job, void *opaque)
> bdrv_drained_end(target_bs);
> if (local_err) {
> error_report_err(local_err);
> -data->ret = -EPERM;
> +ret = -EPERM;
> }
> }
> if (s->to_replace) {
> @@ -710,12 +706,12 @@ static void mirror_exit(Job *job, void *opaque)
> blk_insert_bs(bjob->blk, mirror_top_bs, &error_abort);
>
> bs_opaque->job = NULL;
> -job_completed(job, data->ret);
>
> -g_free(data);
> bdrv_drained_end(src);
> bdrv_unref(mirror_top_bs);
> bdrv_unref(src);
> +
> +job->ret = ret;
> }
>
> static void mirror_throttle(MirrorBlockJob *s)
> @@ -815,7 +811,6 @@ static int mirror_flush(MirrorBlockJob *s)
> static int coroutine_fn mirror_run(Job *job, Error **errp)
> {
> MirrorBlockJob *s = container_of(job, MirrorBlockJob, common.job);
> -MirrorExitData *data;
> BlockDriverState *bs = s->mirror_top_bs->backing->bs;
> BlockDriverState *target_bs = blk_bs(s->target);
> bool need_drain = true;
> @@ -1035,14 +1030,10 @@ immediate_exit:
> g_free(s->in_flight_bitmap);
> bdrv_dirty_iter_free(s->dbi);
>
> -data = g_malloc(sizeof(*data));
> -data->ret = ret;
> -
> if (need_drain) {
> bdrv_drained_begin(bs);
> }
>
> -job_defer_to_main_loop(&s->common.job, mirror_exit, data);
> return ret;
> }
>
> @@ -1141,6 +1132,7 @@ static const BlockJobDriver mirror_job_driver = {
> .user_resume= block_job_user_resume,
> .drain = block_job_drain,
> .run= mirror_run,
> +.exit = mirror_exit,
> .pause = mirror_pause,
> .complete = mirror_complete,
> },
> @@ -1157,6 +1149,7 @@ static const BlockJobDriver commit_active_job_driver = {
> .user_resume= block_job_user_resume,
> .drain = block_job_drain,
> .run= mirror_run,
> +.exit = mirror_exit,
> .pause = mirror_pause,
> .complete = mirror_complete,
> },
> --
> 2.14.4
>
Re: [Qemu-devel] [PATCH v3 01/15] block/commit: add block job creation flags
On Fri, Aug 31, 2018 at 06:28:53PM -0400, John Snow wrote:
> Add support for taking and passing forward job creation flags.
>
> Signed-off-by: John Snow
> Reviewed-by: Max Reitz
Reviewed-by: Jeff Cody
> ---
> block/commit.c| 5 +++--
> blockdev.c| 7 ---
> include/block/block_int.h | 5 -
> 3 files changed, 11 insertions(+), 6 deletions(-)
>
> diff --git a/block/commit.c b/block/commit.c
> index da69165de3..b6e8969877 100644
> --- a/block/commit.c
> +++ b/block/commit.c
> @@ -249,7 +249,8 @@ static BlockDriver bdrv_commit_top = {
> };
>
> void commit_start(const char *job_id, BlockDriverState *bs,
> - BlockDriverState *base, BlockDriverState *top, int64_t
> speed,
> + BlockDriverState *base, BlockDriverState *top,
> + int creation_flags, int64_t speed,
>BlockdevOnError on_error, const char *backing_file_str,
>const char *filter_node_name, Error **errp)
> {
> @@ -267,7 +268,7 @@ void commit_start(const char *job_id, BlockDriverState
> *bs,
> }
>
> s = block_job_create(job_id, &commit_job_driver, NULL, bs, 0,
> BLK_PERM_ALL,
> - speed, JOB_DEFAULT, NULL, NULL, errp);
> + speed, creation_flags, NULL, NULL, errp);
> if (!s) {
> return;
> }
> diff --git a/blockdev.c b/blockdev.c
> index 72f5347df5..c15a1e624b 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -3214,6 +3214,7 @@ void qmp_block_commit(bool has_job_id, const char
> *job_id, const char *device,
> * BlockdevOnError change for blkmirror makes it in
> */
> BlockdevOnError on_error = BLOCKDEV_ON_ERROR_REPORT;
> +int job_flags = JOB_DEFAULT;
>
> if (!has_speed) {
> speed = 0;
> @@ -3295,15 +3296,15 @@ 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,
> -JOB_DEFAULT, speed, on_error,
> +job_flags, speed, on_error,
> filter_node_name, NULL, NULL, false, &local_err);
> } else {
> BlockDriverState *overlay_bs = bdrv_find_overlay(bs, top_bs);
> if (bdrv_op_is_blocked(overlay_bs, BLOCK_OP_TYPE_COMMIT_TARGET,
> errp)) {
> goto out;
> }
> -commit_start(has_job_id ? job_id : NULL, bs, base_bs, top_bs, speed,
> - on_error, has_backing_file ? backing_file : NULL,
> +commit_start(has_job_id ? job_id : NULL, bs, base_bs, top_bs,
> job_flags,
> + speed, on_error, has_backing_file ? backing_file : NULL,
> filter_node_name, &local_err);
> }
> if (local_err != NULL) {
> diff --git a/include/block/block_int.h b/include/block/block_int.h
> index 903b9c1034..ffab0b4d3e 100644
> --- a/include/block/block_int.h
> +++ b/include/block/block_int.h
> @@ -980,6 +980,8 @@ void stream_start(const char *job_id, BlockDriverState
> *bs,
> * @bs: Active block device.
> * @top: Top block device to be committed.
> * @base: Block device that will be written into, and become the new top.
> + * @creation_flags: Flags that control the behavior of the Job lifetime.
> + * See @BlockJobCreateFlags
> * @speed: The maximum speed, in bytes per second, or 0 for unlimited.
> * @on_error: The action to take upon error.
> * @backing_file_str: String to use as the backing file in @top's overlay
> @@ -990,7 +992,8 @@ void stream_start(const char *job_id, BlockDriverState
> *bs,
> *
> */
> void commit_start(const char *job_id, BlockDriverState *bs,
> - BlockDriverState *base, BlockDriverState *top, int64_t
> speed,
> + BlockDriverState *base, BlockDriverState *top,
> + int creation_flags, int64_t speed,
>BlockdevOnError on_error, const char *backing_file_str,
>const char *filter_node_name, Error **errp);
> /**
> --
> 2.14.4
>
Re: [Qemu-devel] [PATCH v3 03/15] block/stream: add block job creation flags
On Fri, Aug 31, 2018 at 06:28:55PM -0400, John Snow wrote:
> Add support for taking and passing forward job creaton flags.
>
> Signed-off-by: John Snow
> Reviewed-by: Max Reitz
(with the misspelling that Eric pointed out fixed):
Reviewed-by: Jeff Cody
> ---
> block/stream.c| 5 +++--
> blockdev.c| 3 ++-
> include/block/block_int.h | 5 -
> 3 files changed, 9 insertions(+), 4 deletions(-)
>
> diff --git a/block/stream.c b/block/stream.c
> index 67e1e72e23..700eb239e4 100644
> --- a/block/stream.c
> +++ b/block/stream.c
> @@ -214,7 +214,8 @@ static const BlockJobDriver stream_job_driver = {
>
> void stream_start(const char *job_id, BlockDriverState *bs,
>BlockDriverState *base, const char *backing_file_str,
> - int64_t speed, BlockdevOnError on_error, Error **errp)
> + int creation_flags, int64_t speed,
> + BlockdevOnError on_error, Error **errp)
> {
> StreamBlockJob *s;
> BlockDriverState *iter;
> @@ -236,7 +237,7 @@ void stream_start(const char *job_id, BlockDriverState
> *bs,
> BLK_PERM_GRAPH_MOD,
> BLK_PERM_CONSISTENT_READ | BLK_PERM_WRITE_UNCHANGED
> |
> BLK_PERM_WRITE,
> - speed, JOB_DEFAULT, NULL, NULL, errp);
> + speed, creation_flags, NULL, NULL, errp);
> if (!s) {
> goto fail;
> }
> diff --git a/blockdev.c b/blockdev.c
> index 6574356708..ec90eb1cf9 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -3123,6 +3123,7 @@ void qmp_block_stream(bool has_job_id, const char
> *job_id, const char *device,
> AioContext *aio_context;
> Error *local_err = NULL;
> const char *base_name = NULL;
> +int job_flags = JOB_DEFAULT;
>
> if (!has_on_error) {
> on_error = BLOCKDEV_ON_ERROR_REPORT;
> @@ -3185,7 +3186,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, &local_err);
> + job_flags, has_speed ? speed : 0, on_error, &local_err);
> if (local_err) {
> error_propagate(errp, local_err);
> goto out;
> diff --git a/include/block/block_int.h b/include/block/block_int.h
> index b40f0bfc9b..4000d2af45 100644
> --- a/include/block/block_int.h
> +++ b/include/block/block_int.h
> @@ -958,6 +958,8 @@ int is_windows_drive(const char *filename);
> * flatten the whole backing file chain onto @bs.
> * @backing_file_str: The file name that will be written to @bs as the
> * the new backing file if the job completes. Ignored if @base is %NULL.
> + * @creation_flags: Flags that control the behavior of the Job lifetime.
> + * See @BlockJobCreateFlags
> * @speed: The maximum speed, in bytes per second, or 0 for unlimited.
> * @on_error: The action to take upon error.
> * @errp: Error object.
> @@ -971,7 +973,8 @@ int is_windows_drive(const char *filename);
> */
> void stream_start(const char *job_id, BlockDriverState *bs,
>BlockDriverState *base, const char *backing_file_str,
> - int64_t speed, BlockdevOnError on_error, Error **errp);
> + int creation_flags, int64_t speed,
> + BlockdevOnError on_error, Error **errp);
>
> /**
> * commit_start:
> --
> 2.14.4
>
Re: [Qemu-devel] [PATCH v3 02/15] block/mirror: add block job creation flags
On Fri, Aug 31, 2018 at 06:28:54PM -0400, John Snow wrote:
> Add support for taking and passing forward job creaton flags.
>
> Signed-off-by: John Snow
> Reviewed-by: Max Reitz
> ---
> block/mirror.c| 5 +++--
> blockdev.c| 3 ++-
> include/block/block_int.h | 5 -
> 3 files changed, 9 insertions(+), 4 deletions(-)
>
> diff --git a/block/mirror.c b/block/mirror.c
> index b8941db6c1..cba555b4ef 100644
> --- a/block/mirror.c
> +++ b/block/mirror.c
> @@ -1639,7 +1639,8 @@ fail:
>
> void mirror_start(const char *job_id, BlockDriverState *bs,
>BlockDriverState *target, const char *replaces,
> - int64_t speed, uint32_t granularity, int64_t buf_size,
> + int creation_flags, int64_t speed,
> + uint32_t granularity, int64_t buf_size,
>MirrorSyncMode mode, BlockMirrorBackingMode backing_mode,
>BlockdevOnError on_source_error,
>BlockdevOnError on_target_error,
> @@ -1655,7 +1656,7 @@ void mirror_start(const char *job_id, BlockDriverState
> *bs,
> }
> is_none_mode = mode == MIRROR_SYNC_MODE_NONE;
> base = mode == MIRROR_SYNC_MODE_TOP ? backing_bs(bs) : NULL;
> -mirror_start_job(job_id, bs, JOB_DEFAULT, target, replaces,
> +mirror_start_job(job_id, bs, creation_flags, target, replaces,
> speed, granularity, buf_size, backing_mode,
> on_source_error, on_target_error, unmap, NULL, NULL,
> &mirror_job_driver, is_none_mode, base, false,
This is another one of those comments that do not pertain directly to this
patch, so it doesn't affect my r-b. So let's get that out of the way:
Reviewed-by: Jeff Cody
Some of the block job functions have an impressive number of arguments now
(and have for a while, this isn't all that new). I don't know if it is a
sign that our data design is a bit out of whack, or what. But for instance,
mirror_start_job() takes 22 (!) parameters. Seems like something worth
examining at some point.
-Jeff
> diff --git a/blockdev.c b/blockdev.c
> index c15a1e624b..6574356708 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -3590,6 +3590,7 @@ static void blockdev_mirror_common(const char *job_id,
> BlockDriverState *bs,
> bool has_copy_mode, MirrorCopyMode
> copy_mode,
> Error **errp)
> {
> +int job_flags = JOB_DEFAULT;
>
> if (!has_speed) {
> speed = 0;
> @@ -3642,7 +3643,7 @@ static void blockdev_mirror_common(const char *job_id,
> BlockDriverState *bs,
> * and will allow to check whether the node still exist at mirror
> completion
> */
> mirror_start(job_id, bs, target,
> - has_replaces ? replaces : NULL,
> + has_replaces ? replaces : NULL, job_flags,
> speed, granularity, buf_size, sync, backing_mode,
> on_source_error, on_target_error, unmap, filter_node_name,
> copy_mode, errp);
> diff --git a/include/block/block_int.h b/include/block/block_int.h
> index ffab0b4d3e..b40f0bfc9b 100644
> --- a/include/block/block_int.h
> +++ b/include/block/block_int.h
> @@ -1029,6 +1029,8 @@ void commit_active_start(const char *job_id,
> BlockDriverState *bs,
> * @target: Block device to write to.
> * @replaces: Block graph node name to replace once the mirror is done. Can
> *only be used when full mirroring is selected.
> + * @creation_flags: Flags that control the behavior of the Job lifetime.
> + * See @BlockJobCreateFlags
> * @speed: The maximum speed, in bytes per second, or 0 for unlimited.
> * @granularity: The chosen granularity for the dirty bitmap.
> * @buf_size: The amount of data that can be in flight at one time.
> @@ -1050,7 +1052,8 @@ void commit_active_start(const char *job_id,
> BlockDriverState *bs,
> */
> void mirror_start(const char *job_id, BlockDriverState *bs,
>BlockDriverState *target, const char *replaces,
> - int64_t speed, uint32_t granularity, int64_t buf_size,
> + int creation_flags, int64_t speed,
> + uint32_t granularity, int64_t buf_size,
>MirrorSyncMode mode, BlockMirrorBackingMode backing_mode,
>BlockdevOnError on_source_error,
>BlockdevOnError on_target_error,
> --
> 2.14.4
>
Re: [Qemu-devel] [PATCH v4 04/15] block/commit: refactor commit to use job callbacks
On Tue, Sep 04, 2018 at 01:09:19PM -0400, John Snow wrote:
> Use the component callbacks; prepare, abort, and clean.
>
> NB: prepare is only called when the job has not yet failed;
> and abort can be called after prepare.
>
> complete -> prepare -> abort -> clean
> complete -> abort -> clean
>
> Signed-off-by: John Snow
> Reviewed-by: Max Reitz
> ---
> block/commit.c | 90
> --
> 1 file changed, 49 insertions(+), 41 deletions(-)
>
> diff --git a/block/commit.c b/block/commit.c
> index b6e8969877..eb3941e545 100644
> --- a/block/commit.c
> +++ b/block/commit.c
> @@ -36,6 +36,7 @@ typedef struct CommitBlockJob {
> BlockDriverState *commit_top_bs;
> BlockBackend *top;
> BlockBackend *base;
> +BlockDriverState *base_bs;
> BlockdevOnError on_error;
> int base_flags;
> char *backing_file_str;
> @@ -68,61 +69,65 @@ static int coroutine_fn commit_populate(BlockBackend *bs,
> BlockBackend *base,
> return 0;
> }
>
> -static void commit_exit(Job *job)
> +static int commit_prepare(Job *job)
> {
> CommitBlockJob *s = container_of(job, CommitBlockJob, common.job);
> -BlockJob *bjob = &s->common;
> -BlockDriverState *top = blk_bs(s->top);
> -BlockDriverState *base = blk_bs(s->base);
> -BlockDriverState *commit_top_bs = s->commit_top_bs;
> -bool remove_commit_top_bs = false;
> -
> -/* Make sure commit_top_bs and top stay around until bdrv_replace_node()
> */
> -bdrv_ref(top);
> -bdrv_ref(commit_top_bs);
>
> /* Remove base node parent that still uses BLK_PERM_WRITE/RESIZE before
> * the normal backing chain can be restored. */
> blk_unref(s->base);
> +s->base = NULL;
>
> -if (!job_is_cancelled(job) && job->ret == 0) {
> -/* success */
> -job->ret = bdrv_drop_intermediate(s->commit_top_bs, base,
> - s->backing_file_str);
> -} else {
> -/* XXX Can (or should) we somehow keep 'consistent read' blocked even
> - * after the failed/cancelled commit job is gone? If we already wrote
> - * something to base, the intermediate images aren't valid any more.
> */
> -remove_commit_top_bs = true;
> +return bdrv_drop_intermediate(s->commit_top_bs, s->base_bs,
> + s->backing_file_str);
> +}
If we can go from prepare->abort->clean, then that means to me that every
failure case of .prepare() can be resolved without permanent changes / data
loss. Is this necessarily the case?
>From bdrv_drop_intermediate():
QLIST_FOREACH_SAFE(c, &top->parents, next_parent, next) {
/* Check whether we are allowed to switch c from top to base */
GSList *ignore_children = g_slist_prepend(NULL, c);
bdrv_check_update_perm(base, NULL, c->perm, c->shared_perm,
ignore_children, &local_err);
g_slist_free(ignore_children);
if (local_err) {
ret = -EPERM;
error_report_err(local_err);
goto exit;
}
/* If so, update the backing file path in the image file */
if (c->role->update_filename) {
ret = c->role->update_filename(c, base, backing_file_str,
&local_err);
if (ret < 0) {
bdrv_abort_perm_update(base);
error_report_err(local_err);
goto exit;
}
}
[...]
}
We could fail this but still have modified an image file backing filenames,
right?
Or am I incorrect about the intention here, that abort() can always be clean?
-Jeff
> +
> +static void commit_abort(Job *job)
> +{
> +CommitBlockJob *s = container_of(job, CommitBlockJob, common.job);
> +BlockDriverState *top_bs = blk_bs(s->top);
> +
> +/* Make sure commit_top_bs and top stay around until bdrv_replace_node()
> */
> +bdrv_ref(top_bs);
> +bdrv_ref(s->commit_top_bs);
> +
> +if (s->base) {
> +blk_unref(s->base);
> }
>
> +/* free the blockers on the intermediate nodes so that bdrv_replace_nodes
> + * can succeed */
> +block_job_remove_all_bdrv(&s->common);
> +
> +/* If bdrv_drop_intermediate() failed (or was not invoked), remove the
> + * commit filter driver from the backing chain now. Do this as the final
> + * step so that the 'consistent read' permission can be granted.
> + *
> + * XXX Can (or should) we somehow keep 'consistent read' blocked even
> + * after the failed/cancelled commit job is gone? If we already wrote
> + * something to base, the intermediate images aren't valid any more. */
> +bdrv_child_try_set_perm(s->commit_top_bs->backing, 0, BLK_PERM_ALL,
> +&error_abort);
> +bdrv_replace_node(s->commit_top_bs, backing_bs(s->commit_top_bs),
> + &error_abort);
> +
> +bdrv_unref(s->commit_top_bs);
Re: [Qemu-devel] [PATCH v5 04/16] block/commit: refactor commit to use job callbacks
On Thu, Sep 06, 2018 at 09:02:13AM -0400, John Snow wrote:
> Use the component callbacks; prepare, abort, and clean.
>
> NB: prepare is only called when the job has not yet failed;
> and abort can be called after prepare.
>
> complete -> prepare -> abort -> clean
> complete -> abort -> clean
>
> During refactor, a potential problem with bdrv_drop_intermediate
> was identified, The patched behavior is no worse than the pre-patch
> behavior, so leave a FIXME for now to be fixed in a future patch.
>
> Signed-off-by: John Snow
> Reviewed-by: Max Reitz
Reviewed-by: Jeff Cody
> ---
> block/commit.c | 92
> --
> 1 file changed, 51 insertions(+), 41 deletions(-)
>
> diff --git a/block/commit.c b/block/commit.c
> index b6e8969877..a2da5740b0 100644
> --- a/block/commit.c
> +++ b/block/commit.c
> @@ -36,6 +36,7 @@ typedef struct CommitBlockJob {
> BlockDriverState *commit_top_bs;
> BlockBackend *top;
> BlockBackend *base;
> +BlockDriverState *base_bs;
> BlockdevOnError on_error;
> int base_flags;
> char *backing_file_str;
> @@ -68,61 +69,67 @@ static int coroutine_fn commit_populate(BlockBackend *bs,
> BlockBackend *base,
> return 0;
> }
>
> -static void commit_exit(Job *job)
> +static int commit_prepare(Job *job)
> {
> CommitBlockJob *s = container_of(job, CommitBlockJob, common.job);
> -BlockJob *bjob = &s->common;
> -BlockDriverState *top = blk_bs(s->top);
> -BlockDriverState *base = blk_bs(s->base);
> -BlockDriverState *commit_top_bs = s->commit_top_bs;
> -bool remove_commit_top_bs = false;
> -
> -/* Make sure commit_top_bs and top stay around until bdrv_replace_node()
> */
> -bdrv_ref(top);
> -bdrv_ref(commit_top_bs);
>
> /* Remove base node parent that still uses BLK_PERM_WRITE/RESIZE before
> * the normal backing chain can be restored. */
> blk_unref(s->base);
> +s->base = NULL;
>
> -if (!job_is_cancelled(job) && job->ret == 0) {
> -/* success */
> -job->ret = bdrv_drop_intermediate(s->commit_top_bs, base,
> - s->backing_file_str);
> -} else {
> -/* XXX Can (or should) we somehow keep 'consistent read' blocked even
> - * after the failed/cancelled commit job is gone? If we already wrote
> - * something to base, the intermediate images aren't valid any more.
> */
> -remove_commit_top_bs = true;
> +/* FIXME: bdrv_drop_intermediate treats total failures and partial
> failures
> + * identically. Further work is needed to disambiguate these cases. */
> +return bdrv_drop_intermediate(s->commit_top_bs, s->base_bs,
> + s->backing_file_str);
> +}
> +
> +static void commit_abort(Job *job)
> +{
> +CommitBlockJob *s = container_of(job, CommitBlockJob, common.job);
> +BlockDriverState *top_bs = blk_bs(s->top);
> +
> +/* Make sure commit_top_bs and top stay around until bdrv_replace_node()
> */
> +bdrv_ref(top_bs);
> +bdrv_ref(s->commit_top_bs);
> +
> +if (s->base) {
> +blk_unref(s->base);
> }
>
> +/* free the blockers on the intermediate nodes so that bdrv_replace_nodes
> + * can succeed */
> +block_job_remove_all_bdrv(&s->common);
> +
> +/* If bdrv_drop_intermediate() failed (or was not invoked), remove the
> + * commit filter driver from the backing chain now. Do this as the final
> + * step so that the 'consistent read' permission can be granted.
> + *
> + * XXX Can (or should) we somehow keep 'consistent read' blocked even
> + * after the failed/cancelled commit job is gone? If we already wrote
> + * something to base, the intermediate images aren't valid any more. */
> +bdrv_child_try_set_perm(s->commit_top_bs->backing, 0, BLK_PERM_ALL,
> +&error_abort);
> +bdrv_replace_node(s->commit_top_bs, backing_bs(s->commit_top_bs),
> + &error_abort);
> +
> +bdrv_unref(s->commit_top_bs);
> +bdrv_unref(top_bs);
> +}
> +
> +static void commit_clean(Job *job)
> +{
> +CommitBlockJob *s = container_of(job, CommitBlockJob, common.job);
> +
> /* restore base open flags here if appropriate (e.g., change the base
> back
> * to r/o). These reopens do not need to be atomic, since we won't abort
> * even on failure here */
> -if (s->base
Re: [Qemu-devel] [PATCH v5 05/16] block/mirror: don't install backing chain on abort
On Thu, Sep 06, 2018 at 09:02:14AM -0400, John Snow wrote:
> In cases where we abort the block/mirror job, there's no point in
> installing the new backing chain before we finish aborting.
>
> Signed-off-by: John Snow
Reviewed-by: Jeff Cody
> ---
> block/mirror.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/block/mirror.c b/block/mirror.c
> index cba555b4ef..bd3e908710 100644
> --- a/block/mirror.c
> +++ b/block/mirror.c
> @@ -642,7 +642,7 @@ static void mirror_exit(Job *job)
> * required before it could become a backing file of target_bs. */
> bdrv_child_try_set_perm(mirror_top_bs->backing, 0, BLK_PERM_ALL,
> &error_abort);
> -if (s->backing_mode == MIRROR_SOURCE_BACKING_CHAIN) {
> +if (ret == 0 && s->backing_mode == MIRROR_SOURCE_BACKING_CHAIN) {
> BlockDriverState *backing = s->is_none_mode ? src : s->base;
> if (backing_bs(target_bs) != backing) {
> bdrv_set_backing_hd(target_bs, backing, &local_err);
> --
> 2.14.4
>
Re: [Qemu-devel] [PATCH v5 06/16] block/mirror: conservative mirror_exit refactor
On Thu, Sep 06, 2018 at 09:02:15AM -0400, John Snow wrote:
> For purposes of minimum code movement, refactor the mirror_exit
> callback to use the post-finalization callbacks in a trivial way.
>
> Signed-off-by: John Snow
> ---
> block/mirror.c | 39 ---
> 1 file changed, 28 insertions(+), 11 deletions(-)
>
> diff --git a/block/mirror.c b/block/mirror.c
> index bd3e908710..a92b4702c5 100644
> --- a/block/mirror.c
> +++ b/block/mirror.c
> @@ -79,6 +79,7 @@ typedef struct MirrorBlockJob {
> int max_iov;
> bool initial_zeroing_ongoing;
> int in_active_write_counter;
> +bool prepared;
> } MirrorBlockJob;
>
> typedef struct MirrorBDSOpaque {
> @@ -607,7 +608,7 @@ static void mirror_wait_for_all_io(MirrorBlockJob *s)
> }
> }
>
> -static void mirror_exit(Job *job)
> +static int mirror_exit_common(Job *job)
> {
> MirrorBlockJob *s = container_of(job, MirrorBlockJob, common.job);
> BlockJob *bjob = &s->common;
> @@ -617,7 +618,13 @@ static void mirror_exit(Job *job)
> BlockDriverState *target_bs = blk_bs(s->target);
> BlockDriverState *mirror_top_bs = s->mirror_top_bs;
> Error *local_err = NULL;
> -int ret = job->ret;
> +bool abort = job->ret < 0;
> +int ret = 0;
> +
> +if (s->prepared) {
> +return 0;
> +}
> +s->prepared = true;
>
> bdrv_release_dirty_bitmap(src, s->dirty_bitmap);
>
> @@ -642,7 +649,7 @@ static void mirror_exit(Job *job)
> * required before it could become a backing file of target_bs. */
> bdrv_child_try_set_perm(mirror_top_bs->backing, 0, BLK_PERM_ALL,
> &error_abort);
> -if (ret == 0 && s->backing_mode == MIRROR_SOURCE_BACKING_CHAIN) {
> +if (!abort && s->backing_mode == MIRROR_SOURCE_BACKING_CHAIN) {
> BlockDriverState *backing = s->is_none_mode ? src : s->base;
> if (backing_bs(target_bs) != backing) {
> bdrv_set_backing_hd(target_bs, backing, &local_err);
> @@ -658,11 +665,8 @@ static void mirror_exit(Job *job)
> aio_context_acquire(replace_aio_context);
> }
>
> -if (s->should_complete && ret == 0) {
> -BlockDriverState *to_replace = src;
> -if (s->to_replace) {
> -to_replace = s->to_replace;
> -}
> +if (s->should_complete && !abort) {
> +BlockDriverState *to_replace = s->to_replace ?: src;
>
> if (bdrv_get_flags(target_bs) != bdrv_get_flags(to_replace)) {
> bdrv_reopen(target_bs, bdrv_get_flags(to_replace), NULL);
> @@ -711,7 +715,18 @@ static void mirror_exit(Job *job)
> bdrv_unref(mirror_top_bs);
> bdrv_unref(src);
>
> -job->ret = ret;
> +return ret;
> +}
> +
> +static int mirror_prepare(Job *job)
> +{
> +return mirror_exit_common(job);
> +}
> +
> +static void mirror_abort(Job *job)
> +{
> +int ret = mirror_exit_common(job);
> +assert(ret == 0);
Not something to hold the series up, but in case a v6 is called for due to
other changes: I think it may be worth a comment in mirror_exit_common()
that if abort is true, and we don't return success, QEMU will hit an assert.
Mainly to prevent someone from including a call with a potential error
return in the abort path in the future.
Reviewed-by: Jeff Cody
> }
>
> static void mirror_throttle(MirrorBlockJob *s)
> @@ -1132,7 +1147,8 @@ static const BlockJobDriver mirror_job_driver = {
> .user_resume= block_job_user_resume,
> .drain = block_job_drain,
> .run= mirror_run,
> -.exit = mirror_exit,
> +.prepare= mirror_prepare,
> +.abort = mirror_abort,
> .pause = mirror_pause,
> .complete = mirror_complete,
> },
> @@ -1149,7 +1165,8 @@ static const BlockJobDriver commit_active_job_driver = {
> .user_resume= block_job_user_resume,
> .drain = block_job_drain,
> .run= mirror_run,
> -.exit = mirror_exit,
> +.prepare= mirror_prepare,
> +.abort = mirror_abort,
> .pause = mirror_pause,
> .complete = mirror_complete,
> },
> --
> 2.14.4
>
Re: [Qemu-devel] [PATCH v5 07/16] block/stream: refactor stream to use job callbacks
On Thu, Sep 06, 2018 at 09:02:16AM -0400, John Snow wrote:
> Signed-off-by: John Snow
> Reviewed-by: Max Reitz
Reviewed-by: Jeff Cody
> ---
> block/stream.c | 23 +++
> 1 file changed, 15 insertions(+), 8 deletions(-)
>
> diff --git a/block/stream.c b/block/stream.c
> index 700eb239e4..81a7ec8ece 100644
> --- a/block/stream.c
> +++ b/block/stream.c
> @@ -54,16 +54,16 @@ static int coroutine_fn stream_populate(BlockBackend *blk,
> return blk_co_preadv(blk, offset, qiov.size, &qiov,
> BDRV_REQ_COPY_ON_READ);
> }
>
> -static void stream_exit(Job *job)
> +static int stream_prepare(Job *job)
> {
> StreamBlockJob *s = container_of(job, StreamBlockJob, common.job);
> BlockJob *bjob = &s->common;
> BlockDriverState *bs = blk_bs(bjob->blk);
> BlockDriverState *base = s->base;
> Error *local_err = NULL;
> -int ret = job->ret;
> +int ret = 0;
>
> -if (!job_is_cancelled(job) && bs->backing && ret == 0) {
> +if (bs->backing) {
> const char *base_id = NULL, *base_fmt = NULL;
> if (base) {
> base_id = s->backing_file_str;
> @@ -75,12 +75,19 @@ static void stream_exit(Job *job)
> bdrv_set_backing_hd(bs, base, &local_err);
> if (local_err) {
> error_report_err(local_err);
> -ret = -EPERM;
> -goto out;
> +return -EPERM;
> }
> }
>
> -out:
> +return ret;
> +}
> +
> +static void stream_clean(Job *job)
> +{
> +StreamBlockJob *s = container_of(job, StreamBlockJob, common.job);
> +BlockJob *bjob = &s->common;
> +BlockDriverState *bs = blk_bs(bjob->blk);
> +
> /* Reopen the image back in read-only mode if necessary */
> if (s->bs_flags != bdrv_get_flags(bs)) {
> /* Give up write permissions before making it read-only */
> @@ -89,7 +96,6 @@ out:
> }
>
> g_free(s->backing_file_str);
> -job->ret = ret;
> }
>
> static int coroutine_fn stream_run(Job *job, Error **errp)
> @@ -206,7 +212,8 @@ static const BlockJobDriver stream_job_driver = {
> .job_type = JOB_TYPE_STREAM,
> .free = block_job_free,
> .run = stream_run,
> -.exit = stream_exit,
> +.prepare = stream_prepare,
> +.clean = stream_clean,
> .user_resume = block_job_user_resume,
> .drain = block_job_drain,
> },
> --
> 2.14.4
>
Re: [Qemu-devel] [PATCH v5 08/16] tests/blockjob: replace Blockjob with Job
On Thu, Sep 06, 2018 at 09:02:17AM -0400, John Snow wrote:
> These tests don't actually test blockjobs anymore, they test
> generic Job lifetimes. Change the types accordingly.
>
> Signed-off-by: John Snow
> Reviewed-by: Max Reitz
Reviewed-by: Jeff Cody
> ---
> tests/test-blockjob.c | 98
> ++-
> 1 file changed, 50 insertions(+), 48 deletions(-)
>
> diff --git a/tests/test-blockjob.c b/tests/test-blockjob.c
> index ad4a65bc78..8e8b680416 100644
> --- a/tests/test-blockjob.c
> +++ b/tests/test-blockjob.c
> @@ -206,18 +206,20 @@ static const BlockJobDriver test_cancel_driver = {
> },
> };
>
> -static CancelJob *create_common(BlockJob **pjob)
> +static CancelJob *create_common(Job **pjob)
> {
> BlockBackend *blk;
> -BlockJob *job;
> +Job *job;
> +BlockJob *bjob;
> CancelJob *s;
>
> blk = create_blk(NULL);
> -job = mk_job(blk, "Steve", &test_cancel_driver, true,
> - JOB_MANUAL_FINALIZE | JOB_MANUAL_DISMISS);
> -job_ref(&job->job);
> -assert(job->job.status == JOB_STATUS_CREATED);
> -s = container_of(job, CancelJob, common);
> +bjob = mk_job(blk, "Steve", &test_cancel_driver, true,
> + JOB_MANUAL_FINALIZE | JOB_MANUAL_DISMISS);
> +job = &bjob->job;
> +job_ref(job);
> +assert(job->status == JOB_STATUS_CREATED);
> +s = container_of(bjob, CancelJob, common);
> s->blk = blk;
>
> *pjob = job;
> @@ -242,7 +244,7 @@ static void cancel_common(CancelJob *s)
>
> static void test_cancel_created(void)
> {
> -BlockJob *job;
> +Job *job;
> CancelJob *s;
>
> s = create_common(&job);
> @@ -251,119 +253,119 @@ static void test_cancel_created(void)
>
> static void test_cancel_running(void)
> {
> -BlockJob *job;
> +Job *job;
> CancelJob *s;
>
> s = create_common(&job);
>
> -job_start(&job->job);
> -assert(job->job.status == JOB_STATUS_RUNNING);
> +job_start(job);
> +assert(job->status == JOB_STATUS_RUNNING);
>
> cancel_common(s);
> }
>
> static void test_cancel_paused(void)
> {
> -BlockJob *job;
> +Job *job;
> CancelJob *s;
>
> s = create_common(&job);
>
> -job_start(&job->job);
> -assert(job->job.status == JOB_STATUS_RUNNING);
> +job_start(job);
> +assert(job->status == JOB_STATUS_RUNNING);
>
> -job_user_pause(&job->job, &error_abort);
> -job_enter(&job->job);
> -assert(job->job.status == JOB_STATUS_PAUSED);
> +job_user_pause(job, &error_abort);
> +job_enter(job);
> +assert(job->status == JOB_STATUS_PAUSED);
>
> cancel_common(s);
> }
>
> static void test_cancel_ready(void)
> {
> -BlockJob *job;
> +Job *job;
> CancelJob *s;
>
> s = create_common(&job);
>
> -job_start(&job->job);
> -assert(job->job.status == JOB_STATUS_RUNNING);
> +job_start(job);
> +assert(job->status == JOB_STATUS_RUNNING);
>
> s->should_converge = true;
> -job_enter(&job->job);
> -assert(job->job.status == JOB_STATUS_READY);
> +job_enter(job);
> +assert(job->status == JOB_STATUS_READY);
>
> cancel_common(s);
> }
>
> static void test_cancel_standby(void)
> {
> -BlockJob *job;
> +Job *job;
> CancelJob *s;
>
> s = create_common(&job);
>
> -job_start(&job->job);
> -assert(job->job.status == JOB_STATUS_RUNNING);
> +job_start(job);
> +assert(job->status == JOB_STATUS_RUNNING);
>
> s->should_converge = true;
> -job_enter(&job->job);
> -assert(job->job.status == JOB_STATUS_READY);
> +job_enter(job);
> +assert(job->status == JOB_STATUS_READY);
>
> -job_user_pause(&job->job, &error_abort);
> -job_enter(&job->job);
> -assert(job->job.status == JOB_STATUS_STANDBY);
> +job_user_pause(job, &error_abort);
> +job_enter(job);
> +assert(job->status == JOB_STATUS_STANDBY);
>
> cancel_common(s);
> }
>
> static void test_cancel_pending(void)
> {
> -BlockJob *job;
> +Job *job;
> CancelJob *s;
>
> s = create_common(&job);
>
> -job_start(&job->job);
> -assert(job->job.status == JOB_STATUS_RUNNING);
> +job_start(job);
> +assert(job->status == JOB_STATUS_RUNNING);
>
Re: [Qemu-devel] [PATCH v5 10/16] tests/test-blockjob-txn: move .exit to .clean
On Thu, Sep 06, 2018 at 09:02:19AM -0400, John Snow wrote:
> The exit callback in this test actually only performs cleanup.
>
> Signed-off-by: John Snow
> Reviewed-by: Max Reitz
Reviewed-by: Jeff Cody
> ---
> tests/test-blockjob-txn.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/tests/test-blockjob-txn.c b/tests/test-blockjob-txn.c
> index ef29f35e44..86606f92b3 100644
> --- a/tests/test-blockjob-txn.c
> +++ b/tests/test-blockjob-txn.c
> @@ -24,7 +24,7 @@ typedef struct {
> int *result;
> } TestBlockJob;
>
> -static void test_block_job_exit(Job *job)
> +static void test_block_job_clean(Job *job)
> {
> BlockJob *bjob = container_of(job, BlockJob, job);
> BlockDriverState *bs = blk_bs(bjob->blk);
> @@ -73,7 +73,7 @@ static const BlockJobDriver test_block_job_driver = {
> .user_resume = block_job_user_resume,
> .drain = block_job_drain,
> .run = test_block_job_run,
> -.exit = test_block_job_exit,
> +.clean = test_block_job_clean,
> },
> };
>
> --
> 2.14.4
>
Re: [Qemu-devel] [PATCH v5 11/16] jobs: remove .exit callback
On Thu, Sep 06, 2018 at 09:02:20AM -0400, John Snow wrote:
> Now that all of the jobs use the component finalization callbacks,
> there's no use for the heavy-hammer .exit callback anymore.
>
> job_exit becomes a glorified type shim so that we can call
> job_completed from aio_bh_schedule_oneshot.
>
> Move these three functions down into job.c to eliminate a
> forward reference.
>
> Signed-off-by: John Snow
> Reviewed-by: Max Reitz
Reviewed-by: Jeff Cody
> ---
> include/qemu/job.h | 11
> job.c | 77
> --
> 2 files changed, 34 insertions(+), 54 deletions(-)
>
> diff --git a/include/qemu/job.h b/include/qemu/job.h
> index e0cff702b7..5cb0681834 100644
> --- a/include/qemu/job.h
> +++ b/include/qemu/job.h
> @@ -221,17 +221,6 @@ struct JobDriver {
> */
> void (*drain)(Job *job);
>
> -/**
> - * If the callback is not NULL, exit will be invoked from the main thread
> - * when the job's coroutine has finished, but before transactional
> - * convergence; before @prepare or @abort.
> - *
> - * FIXME TODO: This callback is only temporary to transition remaining
> jobs
> - * to prepare/commit/abort/clean callbacks and will be removed before
> 3.1.
> - * is released.
> - */
> -void (*exit)(Job *job);
> -
> /**
> * If the callback is not NULL, prepare will be invoked when all the jobs
> * belonging to the same transaction complete; or upon this job's
> completion
> diff --git a/job.c b/job.c
> index 01dd97fee3..72f7de1f36 100644
> --- a/job.c
> +++ b/job.c
> @@ -535,49 +535,6 @@ void job_drain(Job *job)
> }
> }
>
> -static void job_completed(Job *job);
> -
> -static void job_exit(void *opaque)
> -{
> -Job *job = (Job *)opaque;
> -AioContext *aio_context = job->aio_context;
> -
> -if (job->driver->exit) {
> -aio_context_acquire(aio_context);
> -job->driver->exit(job);
> -aio_context_release(aio_context);
> -}
> -job_completed(job);
> -}
> -
> -/**
> - * All jobs must allow a pause point before entering their job proper. This
> - * ensures that jobs can be paused prior to being started, then resumed
> later.
> - */
> -static void coroutine_fn job_co_entry(void *opaque)
> -{
> -Job *job = opaque;
> -
> -assert(job && job->driver && job->driver->run);
> -job_pause_point(job);
> -job->ret = job->driver->run(job, &job->err);
> -job->deferred_to_main_loop = true;
> -aio_bh_schedule_oneshot(qemu_get_aio_context(), job_exit, job);
> -}
> -
> -
> -void job_start(Job *job)
> -{
> -assert(job && !job_started(job) && job->paused &&
> - job->driver && job->driver->run);
> -job->co = qemu_coroutine_create(job_co_entry, job);
> -job->pause_count--;
> -job->busy = true;
> -job->paused = false;
> -job_state_transition(job, JOB_STATUS_RUNNING);
> -aio_co_enter(job->aio_context, job->co);
> -}
> -
> /* Assumes the block_job_mutex is held */
> static bool job_timer_not_pending(Job *job)
> {
> @@ -894,6 +851,40 @@ static void job_completed(Job *job)
> }
> }
>
> +/** Useful only as a type shim for aio_bh_schedule_oneshot. */
> +static void job_exit(void *opaque)
> +{
> +Job *job = (Job *)opaque;
> +job_completed(job);
> +}
> +
> +/**
> + * All jobs must allow a pause point before entering their job proper. This
> + * ensures that jobs can be paused prior to being started, then resumed
> later.
> + */
> +static void coroutine_fn job_co_entry(void *opaque)
> +{
> +Job *job = opaque;
> +
> +assert(job && job->driver && job->driver->run);
> +job_pause_point(job);
> +job->ret = job->driver->run(job, &job->err);
> +job->deferred_to_main_loop = true;
> +aio_bh_schedule_oneshot(qemu_get_aio_context(), job_exit, job);
> +}
> +
> +void job_start(Job *job)
> +{
> +assert(job && !job_started(job) && job->paused &&
> + job->driver && job->driver->run);
> +job->co = qemu_coroutine_create(job_co_entry, job);
> +job->pause_count--;
> +job->busy = true;
> +job->paused = false;
> +job_state_transition(job, JOB_STATUS_RUNNING);
> +aio_co_enter(job->aio_context, job->co);
> +}
> +
> void job_cancel(Job *job, bool force)
> {
> if (job->status == JOB_STATUS_CONCLUDED) {
> --
> 2.14.4
>
Re: [Qemu-devel] [PATCH v5 09/16] tests/test-blockjob: remove exit callback
On Thu, Sep 06, 2018 at 09:02:18AM -0400, John Snow wrote:
> We remove the exit callback and the completed boolean along with it.
> We can simulate it just fine by waiting for the job to defer to the
> main loop, and then giving it one final kick to get the main loop
> portion to run.
>
> Signed-off-by: John Snow
> Reviewed-by: Max Reitz
Reviewed-by: Jeff Cody
> ---
> tests/test-blockjob.c | 16 ++--
> 1 file changed, 6 insertions(+), 10 deletions(-)
>
> diff --git a/tests/test-blockjob.c b/tests/test-blockjob.c
> index 8e8b680416..de4c1c20aa 100644
> --- a/tests/test-blockjob.c
> +++ b/tests/test-blockjob.c
> @@ -160,15 +160,8 @@ typedef struct CancelJob {
> BlockBackend *blk;
> bool should_converge;
> bool should_complete;
> -bool completed;
> } CancelJob;
>
> -static void cancel_job_exit(Job *job)
> -{
> -CancelJob *s = container_of(job, CancelJob, common.job);
> -s->completed = true;
> -}
> -
> static void cancel_job_complete(Job *job, Error **errp)
> {
> CancelJob *s = container_of(job, CancelJob, common.job);
> @@ -201,7 +194,6 @@ static const BlockJobDriver test_cancel_driver = {
> .user_resume = block_job_user_resume,
> .drain = block_job_drain,
> .run = cancel_job_run,
> -.exit = cancel_job_exit,
> .complete = cancel_job_complete,
> },
> };
> @@ -335,9 +327,11 @@ static void test_cancel_pending(void)
>
> job_complete(job, &error_abort);
> job_enter(job);
> -while (!s->completed) {
> +while (!job->deferred_to_main_loop) {
> aio_poll(qemu_get_aio_context(), true);
> }
> +assert(job->status == JOB_STATUS_READY);
> +aio_poll(qemu_get_aio_context(), true);
> assert(job->status == JOB_STATUS_PENDING);
>
> cancel_common(s);
> @@ -359,9 +353,11 @@ static void test_cancel_concluded(void)
>
> job_complete(job, &error_abort);
> job_enter(job);
> -while (!s->completed) {
> +while (!job->deferred_to_main_loop) {
> aio_poll(qemu_get_aio_context(), true);
> }
> +assert(job->status == JOB_STATUS_READY);
> +aio_poll(qemu_get_aio_context(), true);
> assert(job->status == JOB_STATUS_PENDING);
>
> job_finalize(job, &error_abort);
> --
> 2.14.4
>
Re: [Qemu-devel] [PATCH v5 12/16] qapi/block-commit: expose new job properties
On Thu, Sep 06, 2018 at 09:02:21AM -0400, John Snow wrote:
> Signed-off-by: John Snow
> Reviewed-by: Max Reitz
Reviewed-by: Jeff Cody
> ---
> blockdev.c | 8
> qapi/block-core.json | 16 +++-
> 2 files changed, 23 insertions(+), 1 deletion(-)
>
> diff --git a/blockdev.c b/blockdev.c
> index ec90eb1cf9..98b91e75a7 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -3204,6 +3204,8 @@ void qmp_block_commit(bool has_job_id, const char
> *job_id, const char *device,
>bool has_backing_file, const char *backing_file,
>bool has_speed, int64_t speed,
>bool has_filter_node_name, const char
> *filter_node_name,
> + bool has_auto_finalize, bool auto_finalize,
> + bool has_auto_dismiss, bool auto_dismiss,
>Error **errp)
> {
> BlockDriverState *bs;
> @@ -3223,6 +3225,12 @@ void qmp_block_commit(bool has_job_id, const char
> *job_id, const char *device,
> if (!has_filter_node_name) {
> filter_node_name = NULL;
> }
> +if (has_auto_finalize && !auto_finalize) {
> +job_flags |= JOB_MANUAL_FINALIZE;
> +}
> +if (has_auto_dismiss && !auto_dismiss) {
> +job_flags |= JOB_MANUAL_DISMISS;
> +}
>
> /* Important Note:
> * libvirt relies on the DeviceNotFound error class in order to probe
> for
> diff --git a/qapi/block-core.json b/qapi/block-core.json
> index 4c7a37afdc..d5b62e50d7 100644
> --- a/qapi/block-core.json
> +++ b/qapi/block-core.json
> @@ -1498,6 +1498,19 @@
> #above @top. If this option is not given, a node name is
> #autogenerated. (Since: 2.9)
> #
> +# @auto-finalize: When false, this job will wait in a PENDING state after it
> has
> +# finished its work, waiting for @block-job-finalize before
> +# making any block graph changes.
> +# When true, this job will automatically
> +# perform its abort or commit actions.
> +# Defaults to true. (Since 3.1)
> +#
> +# @auto-dismiss: When false, this job will wait in a CONCLUDED state after it
> +#has completely ceased all work, and awaits
> @block-job-dismiss.
> +#When true, this job will automatically disappear from the
> query
> +#list without user intervention.
> +#Defaults to true. (Since 3.1)
> +#
> # Returns: Nothing on success
> # If @device does not exist, DeviceNotFound
> # Any other error returns a GenericError.
> @@ -1515,7 +1528,8 @@
> { 'command': 'block-commit',
>'data': { '*job-id': 'str', 'device': 'str', '*base': 'str', '*top': 'str',
> '*backing-file': 'str', '*speed': 'int',
> -'*filter-node-name': 'str' } }
> +'*filter-node-name': 'str',
> +'*auto-finalize': 'bool', '*auto-dismiss': 'bool' } }
>
> ##
> # @drive-backup:
> --
> 2.14.4
>
Re: [Qemu-devel] [PATCH v5 16/16] blockdev: document transactional shortcomings
On Thu, Sep 06, 2018 at 09:02:25AM -0400, John Snow wrote:
> Presently only the backup job really guarantees what one would consider
> transactional semantics. To guard against someone helpfully adding them
> in the future, document that there are shortcomings in the model that
> would need to be audited at that time.
>
> Signed-off-by: John Snow
Reviewed-by: Jeff Cody
> ---
> blockdev.c | 8 +++-
> 1 file changed, 7 insertions(+), 1 deletion(-)
>
> diff --git a/blockdev.c b/blockdev.c
> index 0cf8febe6c..d4b42403df 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -2182,7 +2182,13 @@ static const BlkActionOps actions[] = {
> .instance_size = sizeof(BlockDirtyBitmapState),
> .prepare = block_dirty_bitmap_disable_prepare,
> .abort = block_dirty_bitmap_disable_abort,
> - }
> +},
> +/* Where are transactions for MIRROR, COMMIT and STREAM?
> + * Although these blockjobs use transaction callbacks like the backup
> job,
> + * these jobs do not necessarily adhere to transaction semantics.
> + * These jobs may not fully undo all of their actions on abort, nor do
> they
> + * necessarily work in transactions with more than one job in them.
> + */
> };
>
> /**
> --
> 2.14.4
>
Re: [Qemu-devel] [PATCH v5 14/16] qapi/block-stream: expose new job properties
On Thu, Sep 06, 2018 at 09:02:23AM -0400, John Snow wrote:
> Signed-off-by: John Snow
> Reviewed-by: Max Reitz
Reviewed-by: Jeff Cody
> ---
> blockdev.c | 9 +
> hmp.c| 5 +++--
> qapi/block-core.json | 16 +++-
> 3 files changed, 27 insertions(+), 3 deletions(-)
>
> diff --git a/blockdev.c b/blockdev.c
> index 429cdf9901..0cf8febe6c 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -3116,6 +3116,8 @@ void qmp_block_stream(bool has_job_id, const char
> *job_id, const char *device,
>bool has_backing_file, const char *backing_file,
>bool has_speed, int64_t speed,
>bool has_on_error, BlockdevOnError on_error,
> + bool has_auto_finalize, bool auto_finalize,
> + bool has_auto_dismiss, bool auto_dismiss,
>Error **errp)
> {
> BlockDriverState *bs, *iter;
> @@ -3185,6 +3187,13 @@ void qmp_block_stream(bool has_job_id, const char
> *job_id, const char *device,
> /* backing_file string overrides base bs filename */
> base_name = has_backing_file ? backing_file : base_name;
>
> +if (has_auto_finalize && !auto_finalize) {
> +job_flags |= JOB_MANUAL_FINALIZE;
> +}
> +if (has_auto_dismiss && !auto_dismiss) {
> +job_flags |= JOB_MANUAL_DISMISS;
> +}
> +
> stream_start(has_job_id ? job_id : NULL, bs, base_bs, base_name,
> job_flags, has_speed ? speed : 0, on_error, &local_err);
> if (local_err) {
> diff --git a/hmp.c b/hmp.c
> index 4975fa56b0..868c1a049d 100644
> --- a/hmp.c
> +++ b/hmp.c
> @@ -1905,8 +1905,9 @@ void hmp_block_stream(Monitor *mon, const QDict *qdict)
> int64_t speed = qdict_get_try_int(qdict, "speed", 0);
>
> qmp_block_stream(true, device, device, base != NULL, base, false, NULL,
> - false, NULL, qdict_haskey(qdict, "speed"), speed,
> - true, BLOCKDEV_ON_ERROR_REPORT, &error);
> + false, NULL, qdict_haskey(qdict, "speed"), speed, true,
> + BLOCKDEV_ON_ERROR_REPORT, false, false, false, false,
> + &error);
>
> hmp_handle_error(mon, &error);
> }
> diff --git a/qapi/block-core.json b/qapi/block-core.json
> index e785c2e9fe..f877e9e414 100644
> --- a/qapi/block-core.json
> +++ b/qapi/block-core.json
> @@ -2317,6 +2317,19 @@
> #'stop' and 'enospc' can only be used if the block device
> #supports io-status (see BlockInfo). Since 1.3.
> #
> +# @auto-finalize: When false, this job will wait in a PENDING state after it
> has
> +# finished its work, waiting for @block-job-finalize before
> +# making any block graph changes.
> +# When true, this job will automatically
> +# perform its abort or commit actions.
> +# Defaults to true. (Since 3.1)
> +#
> +# @auto-dismiss: When false, this job will wait in a CONCLUDED state after it
> +#has completely ceased all work, and awaits
> @block-job-dismiss.
> +#When true, this job will automatically disappear from the
> query
> +#list without user intervention.
> +#Defaults to true. (Since 3.1)
> +#
> # Returns: Nothing on success. If @device does not exist, DeviceNotFound.
> #
> # Since: 1.1
> @@ -2332,7 +2345,8 @@
> { 'command': 'block-stream',
>'data': { '*job-id': 'str', 'device': 'str', '*base': 'str',
> '*base-node': 'str', '*backing-file': 'str', '*speed': 'int',
> -'*on-error': 'BlockdevOnError' } }
> +'*on-error': 'BlockdevOnError',
> +'*auto-finalize': 'bool', '*auto-dismiss': 'bool' } }
>
> ##
> # @block-job-set-speed:
> --
> 2.14.4
>
Re: [Qemu-devel] [PATCH v5 15/16] block/backup: qapi documentation fixup
On Thu, Sep 06, 2018 at 09:02:24AM -0400, John Snow wrote: > Fix documentation to match the other jobs amended for 3.1. > > Signed-off-by: John Snow > Reviewed-by: Max Reitz Reviewed-by: Jeff Cody > --- > qapi/block-core.json | 18 ++ > 1 file changed, 10 insertions(+), 8 deletions(-) > > diff --git a/qapi/block-core.json b/qapi/block-core.json > index f877e9e414..c0b3d33dbb 100644 > --- a/qapi/block-core.json > +++ b/qapi/block-core.json > @@ -1272,13 +1272,14 @@ > # a different block device than @device). > # > # @auto-finalize: When false, this job will wait in a PENDING state after it > has > -# finished its work, waiting for @block-job-finalize. > -# When true, this job will automatically perform its abort or > -# commit actions. > +# finished its work, waiting for @block-job-finalize before > +# making any block graph changes. > +# When true, this job will automatically > +# perform its abort or commit actions. > # Defaults to true. (Since 2.12) > # > # @auto-dismiss: When false, this job will wait in a CONCLUDED state after it > -#has completed ceased all work, and wait for > @block-job-dismiss. > +#has completely ceased all work, and awaits > @block-job-dismiss. > #When true, this job will automatically disappear from the > query > #list without user intervention. > #Defaults to true. (Since 2.12) > @@ -1327,13 +1328,14 @@ > # a different block device than @device). > # > # @auto-finalize: When false, this job will wait in a PENDING state after it > has > -# finished its work, waiting for @block-job-finalize. > -# When true, this job will automatically perform its abort or > -# commit actions. > +# finished its work, waiting for @block-job-finalize before > +# making any block graph changes. > +# When true, this job will automatically > +# perform its abort or commit actions. > # Defaults to true. (Since 2.12) > # > # @auto-dismiss: When false, this job will wait in a CONCLUDED state after it > -#has completed ceased all work, and wait for > @block-job-dismiss. > +#has completely ceased all work, and awaits > @block-job-dismiss. > #When true, this job will automatically disappear from the > query > #list without user intervention. > #Defaults to true. (Since 2.12) > -- > 2.14.4 >
Re: [Qemu-devel] [PATCH v5 13/16] qapi/block-mirror: expose new job properties
On Thu, Sep 06, 2018 at 09:02:22AM -0400, John Snow wrote:
> Signed-off-by: John Snow
> Reviewed-by: Max Reitz
Reviewed-by: Jeff Cody
> ---
> blockdev.c | 14 ++
> qapi/block-core.json | 30 --
> 2 files changed, 42 insertions(+), 2 deletions(-)
>
> diff --git a/blockdev.c b/blockdev.c
> index 98b91e75a7..429cdf9901 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -3597,6 +3597,8 @@ static void blockdev_mirror_common(const char *job_id,
> BlockDriverState *bs,
> bool has_filter_node_name,
> const char *filter_node_name,
> bool has_copy_mode, MirrorCopyMode
> copy_mode,
> + bool has_auto_finalize, bool
> auto_finalize,
> + bool has_auto_dismiss, bool auto_dismiss,
> Error **errp)
> {
> int job_flags = JOB_DEFAULT;
> @@ -3625,6 +3627,12 @@ static void blockdev_mirror_common(const char *job_id,
> BlockDriverState *bs,
> if (!has_copy_mode) {
> copy_mode = MIRROR_COPY_MODE_BACKGROUND;
> }
> +if (has_auto_finalize && !auto_finalize) {
> +job_flags |= JOB_MANUAL_FINALIZE;
> +}
> +if (has_auto_dismiss && !auto_dismiss) {
> +job_flags |= JOB_MANUAL_DISMISS;
> +}
>
> if (granularity != 0 && (granularity < 512 || granularity > 1048576 *
> 64)) {
> error_setg(errp, QERR_INVALID_PARAMETER_VALUE, "granularity",
> @@ -3802,6 +3810,8 @@ void qmp_drive_mirror(DriveMirror *arg, Error **errp)
> arg->has_unmap, arg->unmap,
> false, NULL,
> arg->has_copy_mode, arg->copy_mode,
> + arg->has_auto_finalize, arg->auto_finalize,
> + arg->has_auto_dismiss, arg->auto_dismiss,
> &local_err);
> bdrv_unref(target_bs);
> error_propagate(errp, local_err);
> @@ -3823,6 +3833,8 @@ void qmp_blockdev_mirror(bool has_job_id, const char
> *job_id,
> bool has_filter_node_name,
> const char *filter_node_name,
> bool has_copy_mode, MirrorCopyMode copy_mode,
> + bool has_auto_finalize, bool auto_finalize,
> + bool has_auto_dismiss, bool auto_dismiss,
> Error **errp)
> {
> BlockDriverState *bs;
> @@ -3856,6 +3868,8 @@ void qmp_blockdev_mirror(bool has_job_id, const char
> *job_id,
> true, true,
> has_filter_node_name, filter_node_name,
> has_copy_mode, copy_mode,
> + has_auto_finalize, auto_finalize,
> + has_auto_dismiss, auto_dismiss,
> &local_err);
> error_propagate(errp, local_err);
>
> diff --git a/qapi/block-core.json b/qapi/block-core.json
> index d5b62e50d7..e785c2e9fe 100644
> --- a/qapi/block-core.json
> +++ b/qapi/block-core.json
> @@ -1729,6 +1729,18 @@
> # @copy-mode: when to copy data to the destination; defaults to 'background'
> # (Since: 3.0)
> #
> +# @auto-finalize: When false, this job will wait in a PENDING state after it
> has
> +# finished its work, waiting for @block-job-finalize before
> +# making any block graph changes.
> +# When true, this job will automatically
> +# perform its abort or commit actions.
> +# Defaults to true. (Since 3.1)
> +#
> +# @auto-dismiss: When false, this job will wait in a CONCLUDED state after it
> +#has completely ceased all work, and awaits
> @block-job-dismiss.
> +#When true, this job will automatically disappear from the
> query
> +#list without user intervention.
> +#Defaults to true. (Since 3.1)
> # Since: 1.3
> ##
> { 'struct': 'DriveMirror',
> @@ -1738,7 +1750,8 @@
> '*speed': 'int', '*granularity': 'uint32',
> '*buf-size': 'int', '*on-source-error': 'BlockdevOnError',
> '*on-target-error': 'BlockdevOnError',
> -'*unmap': 'bool', '*copy-mode': 'MirrorCopyMode' } }
> +'*unmap': 'bool&#
[Qemu-devel] [PATCH 1/2] block/rbd: pull out qemu_rbd_convert_options
Code movement to pull the conversion from Qdict to BlockdevOptionsRbd
into a helper function.
Signed-off-by: Jeff Cody
---
block/rbd.c | 36
1 file changed, 24 insertions(+), 12 deletions(-)
diff --git a/block/rbd.c b/block/rbd.c
index ca8e5bbace..a8e79d01d2 100644
--- a/block/rbd.c
+++ b/block/rbd.c
@@ -655,12 +655,34 @@ failed_opts:
return r;
}
+static int qemu_rbd_convert_options(BlockDriverState *bs, QDict *options,
+BlockdevOptionsRbd **opts, Error **errp)
+{
+Visitor *v;
+Error *local_err = NULL;
+
+/* Convert the remaining options into a QAPI object */
+v = qobject_input_visitor_new_flat_confused(options, errp);
+if (!v) {
+return -EINVAL;
+}
+
+visit_type_BlockdevOptionsRbd(v, NULL, opts, &local_err);
+visit_free(v);
+
+if (local_err) {
+error_propagate(errp, local_err);
+return -EINVAL;
+}
+
+return 0;
+}
+
static int qemu_rbd_open(BlockDriverState *bs, QDict *options, int flags,
Error **errp)
{
BDRVRBDState *s = bs->opaque;
BlockdevOptionsRbd *opts = NULL;
-Visitor *v;
const QDictEntry *e;
Error *local_err = NULL;
char *keypairs, *secretid;
@@ -676,19 +698,9 @@ static int qemu_rbd_open(BlockDriverState *bs, QDict
*options, int flags,
qdict_del(options, "password-secret");
}
-/* Convert the remaining options into a QAPI object */
-v = qobject_input_visitor_new_flat_confused(options, errp);
-if (!v) {
-r = -EINVAL;
-goto out;
-}
-
-visit_type_BlockdevOptionsRbd(v, NULL, &opts, &local_err);
-visit_free(v);
-
+r = qemu_rbd_convert_options(bs, options, &opts, &local_err);
if (local_err) {
error_propagate(errp, local_err);
-r = -EINVAL;
goto out;
}
--
2.17.1
[Qemu-devel] [PATCH 2/2] block/rbd: Attempt to parse legacy filenames
When we converted rbd to get rid of the older key/value-centric
encoding format, we broke compatibility with image files with backing
file strings encoded in the old format.
This leaves a bit of an ugly conundrum, and a hacky solution.
If the initial attempt to parse the "proper" options fails, it assumes
that we may have an older key/value encoded filename. Fall back to
attempting to parse the filename, and extract the required options from
it. If that fails, pass along the original error message.
This approach has a potential drawback: if for some reason there are
some options supplied the new way, and some the old way, we may not
catch all the old options if they are not required options (since it
won't cause the initial failure).
Signed-off-by: Jeff Cody
---
block/rbd.c | 30 +++---
1 file changed, 27 insertions(+), 3 deletions(-)
diff --git a/block/rbd.c b/block/rbd.c
index a8e79d01d2..bce86b8bde 100644
--- a/block/rbd.c
+++ b/block/rbd.c
@@ -685,7 +685,7 @@ static int qemu_rbd_open(BlockDriverState *bs, QDict
*options, int flags,
BlockdevOptionsRbd *opts = NULL;
const QDictEntry *e;
Error *local_err = NULL;
-char *keypairs, *secretid;
+char *keypairs, *secretid, *filename;
int r;
keypairs = g_strdup(qdict_get_try_str(options, "=keyvalue-pairs"));
@@ -700,8 +700,32 @@ static int qemu_rbd_open(BlockDriverState *bs, QDict
*options, int flags,
r = qemu_rbd_convert_options(bs, options, &opts, &local_err);
if (local_err) {
-error_propagate(errp, local_err);
-goto out;
+/* If the initial attempt to convert and process the options failed,
+ * we may be attempting to open an image file that has the rbd options
+ * specified in the older format consisting of all key/value pairs
+ * encoded in the filename. Go ahead and attempt to parse the
+ * filename, and see if we can pull out the required options */
+Error *parse_err = NULL;
+
+filename = g_strdup(qdict_get_try_str(options, "filename"));
+qdict_del(options, "filename");
+
+qemu_rbd_parse_filename(filename, options, NULL);
+
+g_free(keypairs);
+keypairs = g_strdup(qdict_get_try_str(options, "=keyvalue-pairs"));
+if (keypairs) {
+qdict_del(options, "=keyvalue-pairs");
+}
+
+r = qemu_rbd_convert_options(bs, options, &opts, &parse_err);
+if (parse_err) {
+/* if the second attempt failed, pass along the original error
+ * message for the current format */
+error_propagate(errp, local_err);
+error_free(parse_err);
+goto out;
+}
}
/* Remove the processed options from the QDict (the visitor processes
--
2.17.1
[Qemu-devel] [PATCH 0/2] block/rbd: enable filename parsing on open
This series enables filename parsing on open, on the old key/value pair. Recent changes to new option formats for rbd broke some images. See Patch 2 for more details. Jeff Cody (2): block/rbd: pull out qemu_rbd_convert_options block/rbd: Attempt to parse legacy filenames block/rbd.c | 66 + 1 file changed, 51 insertions(+), 15 deletions(-) -- 2.17.1
Re: [Qemu-devel] [PATCH 2/2] block/rbd: Attempt to parse legacy filenames
On Tue, Sep 11, 2018 at 01:15:49AM -0400, Jeff Cody wrote:
> When we converted rbd to get rid of the older key/value-centric
> encoding format, we broke compatibility with image files with backing
> file strings encoded in the old format.
>
> This leaves a bit of an ugly conundrum, and a hacky solution.
>
> If the initial attempt to parse the "proper" options fails, it assumes
> that we may have an older key/value encoded filename. Fall back to
> attempting to parse the filename, and extract the required options from
> it. If that fails, pass along the original error message.
>
> This approach has a potential drawback: if for some reason there are
> some options supplied the new way, and some the old way, we may not
> catch all the old options if they are not required options (since it
> won't cause the initial failure).
>
> Signed-off-by: Jeff Cody
> ---
> block/rbd.c | 30 +++---
> 1 file changed, 27 insertions(+), 3 deletions(-)
>
> diff --git a/block/rbd.c b/block/rbd.c
> index a8e79d01d2..bce86b8bde 100644
> --- a/block/rbd.c
> +++ b/block/rbd.c
> @@ -685,7 +685,7 @@ static int qemu_rbd_open(BlockDriverState *bs, QDict
> *options, int flags,
> BlockdevOptionsRbd *opts = NULL;
> const QDictEntry *e;
> Error *local_err = NULL;
> -char *keypairs, *secretid;
> +char *keypairs, *secretid, *filename;
> int r;
>
> keypairs = g_strdup(qdict_get_try_str(options, "=keyvalue-pairs"));
> @@ -700,8 +700,32 @@ static int qemu_rbd_open(BlockDriverState *bs, QDict
> *options, int flags,
>
> r = qemu_rbd_convert_options(bs, options, &opts, &local_err);
> if (local_err) {
> -error_propagate(errp, local_err);
> -goto out;
> +/* If the initial attempt to convert and process the options failed,
> + * we may be attempting to open an image file that has the rbd
> options
> + * specified in the older format consisting of all key/value pairs
> + * encoded in the filename. Go ahead and attempt to parse the
> + * filename, and see if we can pull out the required options */
> +Error *parse_err = NULL;
> +
> +filename = g_strdup(qdict_get_try_str(options, "filename"));
I'm leaking filename, I'll clean that up (along with any other comments) in
v2.
> +qdict_del(options, "filename");
> +
> +qemu_rbd_parse_filename(filename, options, NULL);
> +
> +g_free(keypairs);
> +keypairs = g_strdup(qdict_get_try_str(options, "=keyvalue-pairs"));
> +if (keypairs) {
> +qdict_del(options, "=keyvalue-pairs");
> +}
> +
> +r = qemu_rbd_convert_options(bs, options, &opts, &parse_err);
> +if (parse_err) {
> +/* if the second attempt failed, pass along the original error
> + * message for the current format */
> +error_propagate(errp, local_err);
> +error_free(parse_err);
> +goto out;
> +}
> }
>
> /* Remove the processed options from the QDict (the visitor processes
> --
> 2.17.1
>
Re: [Qemu-devel] [PATCH 2/2] block/rbd: Attempt to parse legacy filenames
On Tue, Sep 11, 2018 at 01:03:44PM -0500, Eric Blake wrote:
> On 9/11/18 12:15 AM, Jeff Cody wrote:
> >When we converted rbd to get rid of the older key/value-centric
> >encoding format, we broke compatibility with image files with backing
> >file strings encoded in the old format.
> >
> >This leaves a bit of an ugly conundrum, and a hacky solution.
> >
> >If the initial attempt to parse the "proper" options fails, it assumes
> >that we may have an older key/value encoded filename. Fall back to
> >attempting to parse the filename, and extract the required options from
> >it. If that fails, pass along the original error message.
> >
> >This approach has a potential drawback: if for some reason there are
> >some options supplied the new way, and some the old way, we may not
> >catch all the old options if they are not required options (since it
> >won't cause the initial failure).
>
> No one should be mixing new and old, though.
>
> >
> >Signed-off-by: Jeff Cody
> >---
> > block/rbd.c | 30 +++---
> > 1 file changed, 27 insertions(+), 3 deletions(-)
> >
> >diff --git a/block/rbd.c b/block/rbd.c
> >index a8e79d01d2..bce86b8bde 100644
> >--- a/block/rbd.c
> >+++ b/block/rbd.c
> >@@ -685,7 +685,7 @@ static int qemu_rbd_open(BlockDriverState *bs, QDict
> >*options, int flags,
> > BlockdevOptionsRbd *opts = NULL;
> > const QDictEntry *e;
> > Error *local_err = NULL;
> >-char *keypairs, *secretid;
> >+char *keypairs, *secretid, *filename;
> > int r;
> > keypairs = g_strdup(qdict_get_try_str(options, "=keyvalue-pairs"));
> >@@ -700,8 +700,32 @@ static int qemu_rbd_open(BlockDriverState *bs, QDict
> >*options, int flags,
> > r = qemu_rbd_convert_options(bs, options, &opts, &local_err);
> > if (local_err) {
> >-error_propagate(errp, local_err);
> >-goto out;
>
> Oh, my comment about simplifying this in 1/2 is probably moot, now that you
> are doing a lot more based on local_err rather than just blindly propagating
> it.
>
> >+/* If the initial attempt to convert and process the options failed,
> >+ * we may be attempting to open an image file that has the rbd
> >options
> >+ * specified in the older format consisting of all key/value pairs
> >+ * encoded in the filename. Go ahead and attempt to parse the
> >+ * filename, and see if we can pull out the required options */
> >+Error *parse_err = NULL;
> >+
> >+filename = g_strdup(qdict_get_try_str(options, "filename"));
>
> You already spotted your leak.
>
> >+qdict_del(options, "filename");
> >+
> >+qemu_rbd_parse_filename(filename, options, NULL);
> >+
> >+g_free(keypairs);
>
> Wait. Why are you unilaterally freeing any previously-parsed keypairs in
> favor of the ones parsed out of the filename? I'd rather that we insist on
> only old behavior, or only new, and not some mix. Thus, if we already
> detected keypairs at all, we should declare this situation as an error,
> rather than throwing them away.
>
Good point. I'll flag (local_err && keypairs) as an error.
I also just realized I need to check for NULL filename, and error out as
well in that case.
> >+keypairs = g_strdup(qdict_get_try_str(options, "=keyvalue-pairs"));
> >+if (keypairs) {
> >+qdict_del(options, "=keyvalue-pairs");
> >+}
> >+
> >+r = qemu_rbd_convert_options(bs, options, &opts, &parse_err);
> >+if (parse_err) {
> >+/* if the second attempt failed, pass along the original error
> >+ * message for the current format */
> >+error_propagate(errp, local_err);
> >+error_free(parse_err);
> >+goto out;
> >+}
> > }
>
> The idea of trying two parses makes sense, but I'm hoping v2 better handles
> the case of detecting bad attempts to mix-and-match behavior. Furthermore,
> is there an iotests that you can modify (or add) as a regression test for
> this working the way we want?
Hmm... yes, that should be doable. I'm not sure if I can do it without the
'success' path being a timeout when there is no ceph server present, though.
I'll see what I can do.
-Jeff
Re: [Qemu-devel] [PATCH 2/2] block/rbd: Attempt to parse legacy filenames
On Tue, Sep 11, 2018 at 02:22:31PM -0400, John Snow wrote:
>
>
> On 09/11/2018 01:15 AM, Jeff Cody wrote:
> > When we converted rbd to get rid of the older key/value-centric
> > encoding format, we broke compatibility with image files with backing
> > file strings encoded in the old format.
> >
> > This leaves a bit of an ugly conundrum, and a hacky solution.
> >
> > If the initial attempt to parse the "proper" options fails, it assumes
> > that we may have an older key/value encoded filename. Fall back to
> > attempting to parse the filename, and extract the required options from
> > it. If that fails, pass along the original error message.
> >
> > This approach has a potential drawback: if for some reason there are
> > some options supplied the new way, and some the old way, we may not
> > catch all the old options if they are not required options (since it
> > won't cause the initial failure).
> >
> > Signed-off-by: Jeff Cody
> > ---
> > block/rbd.c | 30 +++---
> > 1 file changed, 27 insertions(+), 3 deletions(-)
> >
> > diff --git a/block/rbd.c b/block/rbd.c
> > index a8e79d01d2..bce86b8bde 100644
> > --- a/block/rbd.c
> > +++ b/block/rbd.c
> > @@ -685,7 +685,7 @@ static int qemu_rbd_open(BlockDriverState *bs, QDict
> > *options, int flags,
> > BlockdevOptionsRbd *opts = NULL;
> > const QDictEntry *e;
> > Error *local_err = NULL;
> > -char *keypairs, *secretid;
> > +char *keypairs, *secretid, *filename;
> > int r;
> >
> > keypairs = g_strdup(qdict_get_try_str(options, "=keyvalue-pairs"));
> > @@ -700,8 +700,32 @@ static int qemu_rbd_open(BlockDriverState *bs, QDict
> > *options, int flags,
> >
> > r = qemu_rbd_convert_options(bs, options, &opts, &local_err);
> > if (local_err) {
> > -error_propagate(errp, local_err);
> > -goto out;
> > +/* If the initial attempt to convert and process the options
> > failed,
> > + * we may be attempting to open an image file that has the rbd
> > options
> > + * specified in the older format consisting of all key/value pairs
> > + * encoded in the filename. Go ahead and attempt to parse the
> > + * filename, and see if we can pull out the required options */
>
> Is it worth splitting out the legacy parsing routine here into its own
> function, given that we will generally depend on it not being invoked?
> i.e., for readability, it doesn't need to distract us.
>
Yeah, that would probably be good.
> > +Error *parse_err = NULL;
> > +
> > +filename = g_strdup(qdict_get_try_str(options, "filename"));
> > +qdict_del(options, "filename");
> > +
> > +qemu_rbd_parse_filename(filename, options, NULL);
> > +
> > +g_free(keypairs);
>
> As Eric already noticed, better to just return with error if this is
> already set.
>
> > +keypairs = g_strdup(qdict_get_try_str(options, "=keyvalue-pairs"));
> > +if (keypairs) {
> > +qdict_del(options, "=keyvalue-pairs");
> > +}
> > +
> > +r = qemu_rbd_convert_options(bs, options, &opts, &parse_err);
> > +if (parse_err) {
> > +/* if the second attempt failed, pass along the original error
> > + * message for the current format */
> > +error_propagate(errp, local_err);
> > +error_free(parse_err);
> > +goto out;
> > +}
>
> If it does succeed, though, ought we emit a deprecated warning that the
> old specification syntax is not supported?
>
I don't know. Without this support, we can't open some existing images. At
what point would we actually remove that support?
> Once we load the image, will the header get rewritten into a compliant
> format?
>
Hmm - I think in some code paths, but not all. I don't think the answer is
'yes' universally, alas.
[Qemu-devel] [PATCH v2 1/3] block/rbd: pull out qemu_rbd_convert_options
Code movement to pull the conversion from Qdict to BlockdevOptionsRbd
into a helper function.
Reviewed-by: Eric Blake
Reviewed-by: John Snow
Signed-off-by: Jeff Cody
---
block/rbd.c | 36
1 file changed, 24 insertions(+), 12 deletions(-)
diff --git a/block/rbd.c b/block/rbd.c
index ca8e5bbace..b199450f9f 100644
--- a/block/rbd.c
+++ b/block/rbd.c
@@ -655,12 +655,34 @@ failed_opts:
return r;
}
+static int qemu_rbd_convert_options(QDict *options, BlockdevOptionsRbd **opts,
+Error **errp)
+{
+Visitor *v;
+Error *local_err = NULL;
+
+/* Convert the remaining options into a QAPI object */
+v = qobject_input_visitor_new_flat_confused(options, errp);
+if (!v) {
+return -EINVAL;
+}
+
+visit_type_BlockdevOptionsRbd(v, NULL, opts, &local_err);
+visit_free(v);
+
+if (local_err) {
+error_propagate(errp, local_err);
+return -EINVAL;
+}
+
+return 0;
+}
+
static int qemu_rbd_open(BlockDriverState *bs, QDict *options, int flags,
Error **errp)
{
BDRVRBDState *s = bs->opaque;
BlockdevOptionsRbd *opts = NULL;
-Visitor *v;
const QDictEntry *e;
Error *local_err = NULL;
char *keypairs, *secretid;
@@ -676,19 +698,9 @@ static int qemu_rbd_open(BlockDriverState *bs, QDict
*options, int flags,
qdict_del(options, "password-secret");
}
-/* Convert the remaining options into a QAPI object */
-v = qobject_input_visitor_new_flat_confused(options, errp);
-if (!v) {
-r = -EINVAL;
-goto out;
-}
-
-visit_type_BlockdevOptionsRbd(v, NULL, &opts, &local_err);
-visit_free(v);
-
+r = qemu_rbd_convert_options(options, &opts, &local_err);
if (local_err) {
error_propagate(errp, local_err);
-r = -EINVAL;
goto out;
}
--
2.17.1
[Qemu-devel] [PATCH v2 2/3] block/rbd: Attempt to parse legacy filenames
When we converted rbd to get rid of the older key/value-centric
encoding format, we broke compatibility with image files with backing
file strings encoded in the old format.
This leaves a bit of an ugly conundrum, and a hacky solution.
If the initial attempt to parse the "proper" options fails, it assumes
that we may have an older key/value encoded filename. Fall back to
attempting to parse the filename, and extract the required options from
it. If that fails, pass along the original error message.
We do not support mixed modern usage alongside legacy keyvalue pair
usage.
A deprecation warning has been added, although care should be taken
when actually deprecating since the impact is not limited to
commandline or qapi usage, but also opening existing images.
Signed-off-by: Jeff Cody
---
block/rbd.c | 53 +++--
1 file changed, 51 insertions(+), 2 deletions(-)
diff --git a/block/rbd.c b/block/rbd.c
index b199450f9f..5090e4f662 100644
--- a/block/rbd.c
+++ b/block/rbd.c
@@ -678,6 +678,33 @@ static int qemu_rbd_convert_options(QDict *options,
BlockdevOptionsRbd **opts,
return 0;
}
+static int qemu_rbd_attempt_legacy_options(QDict *options,
+ BlockdevOptionsRbd **opts,
+ char **keypairs)
+{
+char *filename;
+int r;
+
+filename = g_strdup(qdict_get_try_str(options, "filename"));
+if (!filename) {
+return -EINVAL;
+}
+qdict_del(options, "filename");
+
+qemu_rbd_parse_filename(filename, options, NULL);
+
+/* keypairs freed by caller */
+*keypairs = g_strdup(qdict_get_try_str(options, "=keyvalue-pairs"));
+if (*keypairs) {
+qdict_del(options, "=keyvalue-pairs");
+}
+
+r = qemu_rbd_convert_options(options, opts, NULL);
+
+g_free(filename);
+return r;
+}
+
static int qemu_rbd_open(BlockDriverState *bs, QDict *options, int flags,
Error **errp)
{
@@ -700,8 +727,30 @@ static int qemu_rbd_open(BlockDriverState *bs, QDict
*options, int flags,
r = qemu_rbd_convert_options(options, &opts, &local_err);
if (local_err) {
-error_propagate(errp, local_err);
-goto out;
+/* If keypairs are present, that means some options are present in
+ * the modern option format. Don't attempt to parse legacy option
+ * formats, as we won't support mixed usage. */
+if (keypairs) {
+error_propagate(errp, local_err);
+goto out;
+}
+
+/* If the initial attempt to convert and process the options failed,
+ * we may be attempting to open an image file that has the rbd options
+ * specified in the older format consisting of all key/value pairs
+ * encoded in the filename. Go ahead and attempt to parse the
+ * filename, and see if we can pull out the required options. */
+r = qemu_rbd_attempt_legacy_options(options, &opts, &keypairs);
+if (r < 0) {
+error_propagate(errp, local_err);
+goto out;
+}
+/* Take care whenever deciding to actually deprecate; once this ability
+ * is removed, we will not be able to open any images with
legacy-styled
+ * backing image strings. */
+error_report("RBD options encoded in the filename as keyvalue pairs "
+ "is deprecated. Future versions may cease to parse "
+ "these options in the future.");
}
/* Remove the processed options from the QDict (the visitor processes
--
2.17.1
[Qemu-devel] [PATCH v2 3/3] block/rbd: add iotest for rbd legacy keyvalue filename parsing
This is a small test that will check for the ability to parse
both legacy and modern options for rbd.
The way the test is set up is for failure to occur, but without
having to wait to timeout on a non-existent rbd server. The error
messages in the success path show that the arguments were parsed.
The failure behavior prior to the patch series that has this test, is
qemu-img complaining about mandatory options (e.g. 'pool') not being
provided.
Signed-off-by: Jeff Cody
---
tests/qemu-iotests/231 | 62 ++
tests/qemu-iotests/231.out | 9 ++
tests/qemu-iotests/group | 1 +
3 files changed, 72 insertions(+)
create mode 100755 tests/qemu-iotests/231
create mode 100644 tests/qemu-iotests/231.out
diff --git a/tests/qemu-iotests/231 b/tests/qemu-iotests/231
new file mode 100755
index 00..3e283708b4
--- /dev/null
+++ b/tests/qemu-iotests/231
@@ -0,0 +1,62 @@
+#!/bin/bash
+#
+# Test legacy and modern option parsing for rbd/ceph. This will not
+# actually connect to a ceph server, but rather looks for the appropriate
+# error message that indicates we parsed the options correctly.
+#
+# Copyright (C) 2018 Red Hat, Inc.
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 2 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program. If not, see <http://www.gnu.org/licenses/>.
+#
+
+# creator
+owner=jc...@redhat.com
+
+seq=`basename $0`
+echo "QA output created by $seq"
+
+here=`pwd`
+status=1 # failure is the default!
+
+_cleanup()
+{
+rm "${BOGUS_CONF}"
+}
+trap "_cleanup; exit \$status" 0 1 2 3 15
+
+# get standard environment, filters and checks
+. ./common.rc
+. ./common.filter
+
+_supported_fmt generic
+_supported_proto rbd
+_supported_os Linux
+
+BOGUS_CONF=${TEST_DIR}/ceph-$$.conf
+touch "${BOGUS_CONF}"
+
+_filter_conf()
+{
+sed -e "s#$BOGUS_CONF#BOGUS_CONF#g"
+}
+
+# We expect this to fail, with no monitor ip provided and a null conf file.
Just want it
+# to fail in the right way.
+$QEMU_IMG info
"json:{'file.driver':'rbd','file.filename':'rbd:rbd/bogus:conf=${BOGUS_CONF}'}"
2>&1 | _filter_conf
+$QEMU_IMG info
"json:{'file.driver':'rbd','file.pool':'rbd','file.image':'bogus','file.conf':'${BOGUS_CONF}'}"
2>&1 | _filter_conf
+
+# success, all done
+echo "*** done"
+rm -f $seq.full
+status=0
diff --git a/tests/qemu-iotests/231.out b/tests/qemu-iotests/231.out
new file mode 100644
index 00..579ba11c16
--- /dev/null
+++ b/tests/qemu-iotests/231.out
@@ -0,0 +1,9 @@
+QA output created by 231
+qemu-img: RBD options encoded in the filename as keyvalue pairs is deprecated.
Future versions may cease to parse these options in the future.
+unable to get monitor info from DNS SRV with service name: ceph-mon
+no monitors specified to connect to.
+qemu-img: Could not open
'json:{'file.driver':'rbd','file.filename':'rbd:rbd/bogus:conf=BOGUS_CONF'}':
error connecting: No such file or directory
+unable to get monitor info from DNS SRV with service name: ceph-mon
+no monitors specified to connect to.
+qemu-img: Could not open
'json:{'file.driver':'rbd','file.pool':'rbd','file.image':'bogus','file.conf':'BOGUS_CONF'}':
error connecting: No such file or directory
+*** done
diff --git a/tests/qemu-iotests/group b/tests/qemu-iotests/group
index 743790745b..31f6e77dcb 100644
--- a/tests/qemu-iotests/group
+++ b/tests/qemu-iotests/group
@@ -226,3 +226,4 @@
226 auto quick
227 auto quick
229 auto quick
+231 auto quick
--
2.17.1
[Qemu-devel] [PATCH v2 0/3] block/rbd: enable filename parsing on open
Changes from v1:
Patch 1: Don't pass unused BlockDriverState to helper function
Patch 2: Do not allow mixed usage; fail if keyvalue is present [Eric]
Add deprecation warning [John]
Pull legacy parsing code into function [John]
Fixed filename leak
Patch 3: New; iotest 231. [Eric]
iotest failure on current master:
QA output created by 231
-qemu-img: RBD options encoded in the filename as keyvalue pairs is deprecated.
Future versions may cease to parse these options in the future.
-unable to get monitor info from DNS SRV with service name: ceph-mon
-no monitors specified to connect to.
-qemu-img: Could not open
'json:{'file.driver':'rbd','file.filename':'rbd:rbd/bogus:conf=BOGUS_CONF'}':
error connecting: No such file or directory
+qemu-img: Could not open
'json:{'file.driver':'rbd','file.filename':'rbd:rbd/bogus:conf=BOGUS_CONF'}':
Parameter 'pool' is missing
unable to get monitor info from DNS SRV with service name: ceph-mon
no monitors specified to connect to.
qemu-img: Could not open
'json:{'file.driver':'rbd','file.pool':'rbd','file.image':'bogus','file.conf':'BOGUS_CONF'}':
error connecting: No such file or directory
Failures: 231
Failed 1 of 1 tests
Jeff Cody (3):
block/rbd: pull out qemu_rbd_convert_options
block/rbd: Attempt to parse legacy filenames
block/rbd: add iotest for rbd legacy keyvalue filename parsing
block/rbd.c| 89 --
tests/qemu-iotests/231 | 62 ++
tests/qemu-iotests/231.out | 9
tests/qemu-iotests/group | 1 +
4 files changed, 147 insertions(+), 14 deletions(-)
create mode 100755 tests/qemu-iotests/231
create mode 100644 tests/qemu-iotests/231.out
--
2.17.1
Re: [Qemu-devel] [PATCH v2 2/3] block/rbd: Attempt to parse legacy filenames
On Tue, Sep 11, 2018 at 04:05:45PM -0500, Eric Blake wrote:
> On 9/11/18 3:43 PM, Jeff Cody wrote:
> >When we converted rbd to get rid of the older key/value-centric
> >encoding format, we broke compatibility with image files with backing
> >file strings encoded in the old format.
> >
> >This leaves a bit of an ugly conundrum, and a hacky solution.
> >
> >If the initial attempt to parse the "proper" options fails, it assumes
> >that we may have an older key/value encoded filename. Fall back to
> >attempting to parse the filename, and extract the required options from
> >it. If that fails, pass along the original error message.
> >
> >We do not support mixed modern usage alongside legacy keyvalue pair
> >usage.
> >
> >A deprecation warning has been added, although care should be taken
> >when actually deprecating since the impact is not limited to
> >commandline or qapi usage, but also opening existing images.
> >
> >Signed-off-by: Jeff Cody
> >---
> > block/rbd.c | 53 +++--
> > 1 file changed, 51 insertions(+), 2 deletions(-)
>
> Where's the patch to qemu/qemu-deprecated.texi to mention the new message
> and our advice on upgrading images to avoid triggering it?
>
Argh, thanks - forgot that. I'll spin a v3 with that as patch 4.
> >
> >diff --git a/block/rbd.c b/block/rbd.c
> >index b199450f9f..5090e4f662 100644
> >--- a/block/rbd.c
> >+++ b/block/rbd.c
> >@@ -678,6 +678,33 @@ static int qemu_rbd_convert_options(QDict *options,
> >BlockdevOptionsRbd **opts,
> > return 0;
> > }
> >+static int qemu_rbd_attempt_legacy_options(QDict *options,
> >+ BlockdevOptionsRbd **opts,
> >+ char **keypairs)
> >+{
> >+char *filename;
> >+int r;
> >+
> >+filename = g_strdup(qdict_get_try_str(options, "filename"));
> >+if (!filename) {
> >+return -EINVAL;
> >+}
> >+qdict_del(options, "filename");
> >+
> >+qemu_rbd_parse_filename(filename, options, NULL);
>
> Intentionally ignoring errors here,
>
> >+
> >+/* keypairs freed by caller */
> >+*keypairs = g_strdup(qdict_get_try_str(options, "=keyvalue-pairs"));
> >+if (*keypairs) {
> >+qdict_del(options, "=keyvalue-pairs");
> >+}
> >+
> >+r = qemu_rbd_convert_options(options, opts, NULL);
>
> and here. I guess we'll see how the caller is expecting things to behave.
>
> >+
> >+g_free(filename);
> >+return r;
> >+}
> >+
> > static int qemu_rbd_open(BlockDriverState *bs, QDict *options, int flags,
> > Error **errp)
> > {
> >@@ -700,8 +727,30 @@ static int qemu_rbd_open(BlockDriverState *bs, QDict
> >*options, int flags,
> > r = qemu_rbd_convert_options(options, &opts, &local_err);
> > if (local_err) {
> >-error_propagate(errp, local_err);
> >-goto out;
> >+/* If keypairs are present, that means some options are present in
> >+ * the modern option format. Don't attempt to parse legacy option
> >+ * formats, as we won't support mixed usage. */
> >+if (keypairs) {
> >+error_propagate(errp, local_err);
> >+goto out;
> >+}
> >+
> >+/* If the initial attempt to convert and process the options failed,
> >+ * we may be attempting to open an image file that has the rbd
> >options
> >+ * specified in the older format consisting of all key/value pairs
> >+ * encoded in the filename. Go ahead and attempt to parse the
> >+ * filename, and see if we can pull out the required options. */
> >+r = qemu_rbd_attempt_legacy_options(options, &opts, &keypairs);
> >+if (r < 0) {
> >+error_propagate(errp, local_err);
> >+goto out;
> >+}
> >+/* Take care whenever deciding to actually deprecate; once this
> >ability
> >+ * is removed, we will not be able to open any images with
> >legacy-styled
> >+ * backing image strings. */
> >+error_report("RBD options encoded in the filename as keyvalue pairs
> >"
> >+ "is deprecated. Future versions may cease to parse "
> >+ "these options in the future."
[Qemu-devel] [PATCH v3 4/4] block/rbd: add deprecation documenation for filename keyvalue pairs
Signed-off-by: Jeff Cody --- qemu-deprecated.texi | 7 +++ 1 file changed, 7 insertions(+) diff --git a/qemu-deprecated.texi b/qemu-deprecated.texi index 1b9c007f12..4df8ac442d 100644 --- a/qemu-deprecated.texi +++ b/qemu-deprecated.texi @@ -190,6 +190,13 @@ used instead. In order to prevent QEMU from automatically opening an image's backing chain, use ``"backing": null'' instead. +@subsubsection "rbd keyvalue pair encoded filenames": "" (since 3.1.0) + +Options for ``rbd'' should be specified according to its runtime options, +like other block drivers. Legacy parsing of keyvalue pair encoded +filenames is useful to open images with the old format for backing files; +These image files should be updated to use the current format. + @subsection vio-spapr-device device options @subsubsection "irq": "" (since 3.0.0) -- 2.17.1
[Qemu-devel] [PATCH v3 3/4] block/rbd: add iotest for rbd legacy keyvalue filename parsing
This is a small test that will check for the ability to parse
both legacy and modern options for rbd.
The way the test is set up is for failure to occur, but without
having to wait to timeout on a non-existent rbd server. The error
messages in the success path show that the arguments were parsed.
The failure behavior prior to the patch series that has this test, is
qemu-img complaining about mandatory options (e.g. 'pool') not being
provided.
Reviewed-by: Eric Blake
Signed-off-by: Jeff Cody
---
tests/qemu-iotests/231 | 62 ++
tests/qemu-iotests/231.out | 9 ++
tests/qemu-iotests/group | 1 +
3 files changed, 72 insertions(+)
create mode 100755 tests/qemu-iotests/231
create mode 100644 tests/qemu-iotests/231.out
diff --git a/tests/qemu-iotests/231 b/tests/qemu-iotests/231
new file mode 100755
index 00..3e283708b4
--- /dev/null
+++ b/tests/qemu-iotests/231
@@ -0,0 +1,62 @@
+#!/bin/bash
+#
+# Test legacy and modern option parsing for rbd/ceph. This will not
+# actually connect to a ceph server, but rather looks for the appropriate
+# error message that indicates we parsed the options correctly.
+#
+# Copyright (C) 2018 Red Hat, Inc.
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 2 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program. If not, see <http://www.gnu.org/licenses/>.
+#
+
+# creator
+owner=jc...@redhat.com
+
+seq=`basename $0`
+echo "QA output created by $seq"
+
+here=`pwd`
+status=1 # failure is the default!
+
+_cleanup()
+{
+rm "${BOGUS_CONF}"
+}
+trap "_cleanup; exit \$status" 0 1 2 3 15
+
+# get standard environment, filters and checks
+. ./common.rc
+. ./common.filter
+
+_supported_fmt generic
+_supported_proto rbd
+_supported_os Linux
+
+BOGUS_CONF=${TEST_DIR}/ceph-$$.conf
+touch "${BOGUS_CONF}"
+
+_filter_conf()
+{
+sed -e "s#$BOGUS_CONF#BOGUS_CONF#g"
+}
+
+# We expect this to fail, with no monitor ip provided and a null conf file.
Just want it
+# to fail in the right way.
+$QEMU_IMG info
"json:{'file.driver':'rbd','file.filename':'rbd:rbd/bogus:conf=${BOGUS_CONF}'}"
2>&1 | _filter_conf
+$QEMU_IMG info
"json:{'file.driver':'rbd','file.pool':'rbd','file.image':'bogus','file.conf':'${BOGUS_CONF}'}"
2>&1 | _filter_conf
+
+# success, all done
+echo "*** done"
+rm -f $seq.full
+status=0
diff --git a/tests/qemu-iotests/231.out b/tests/qemu-iotests/231.out
new file mode 100644
index 00..579ba11c16
--- /dev/null
+++ b/tests/qemu-iotests/231.out
@@ -0,0 +1,9 @@
+QA output created by 231
+qemu-img: RBD options encoded in the filename as keyvalue pairs is deprecated.
Future versions may cease to parse these options in the future.
+unable to get monitor info from DNS SRV with service name: ceph-mon
+no monitors specified to connect to.
+qemu-img: Could not open
'json:{'file.driver':'rbd','file.filename':'rbd:rbd/bogus:conf=BOGUS_CONF'}':
error connecting: No such file or directory
+unable to get monitor info from DNS SRV with service name: ceph-mon
+no monitors specified to connect to.
+qemu-img: Could not open
'json:{'file.driver':'rbd','file.pool':'rbd','file.image':'bogus','file.conf':'BOGUS_CONF'}':
error connecting: No such file or directory
+*** done
diff --git a/tests/qemu-iotests/group b/tests/qemu-iotests/group
index 743790745b..31f6e77dcb 100644
--- a/tests/qemu-iotests/group
+++ b/tests/qemu-iotests/group
@@ -226,3 +226,4 @@
226 auto quick
227 auto quick
229 auto quick
+231 auto quick
--
2.17.1
[Qemu-devel] [PATCH v3 0/4] block/rbd: enable filename parsing on open
Changes from v2:
=
Patch 4: New, document deprecation. [Eric]
Patch 3,2: Add r-b's
Changes from v1:
=
Patch 1: Don't pass unused BlockDriverState to helper function
Patch 2: Do not allow mixed usage; fail if keyvalue is present [Eric]
Add deprecation warning [John]
Pull legacy parsing code into function [John]
Fixed filename leak
Patch 3: New; iotest 231. [Eric]
iotest failure on current master:
QA output created by 231
-qemu-img: RBD options encoded in the filename as keyvalue pairs is deprecated.
Future versions may cease to parse these options in the future.
-unable to get monitor info from DNS SRV with service name: ceph-mon
-no monitors specified to connect to.
-qemu-img: Could not open
'json:{'file.driver':'rbd','file.filename':'rbd:rbd/bogus:conf=BOGUS_CONF'}':
error connecting: No such file or directory
+qemu-img: Could not open
'json:{'file.driver':'rbd','file.filename':'rbd:rbd/bogus:conf=BOGUS_CONF'}':
Parameter 'pool' is missing
unable to get monitor info from DNS SRV with service name: ceph-mon
no monitors specified to connect to.
qemu-img: Could not open
'json:{'file.driver':'rbd','file.pool':'rbd','file.image':'bogus','file.conf':'BOGUS_CONF'}':
error connecting: No such file or directory
Failures: 231
Failed 1 of 1 tests
Jeff Cody (4):
block/rbd: pull out qemu_rbd_convert_options
block/rbd: Attempt to parse legacy filenames
block/rbd: add iotest for rbd legacy keyvalue filename parsing
block/rbd: add deprecation documenation for filename keyvalue pairs
block/rbd.c| 89 --
qemu-deprecated.texi | 7 +++
tests/qemu-iotests/231 | 62 ++
tests/qemu-iotests/231.out | 9
tests/qemu-iotests/group | 1 +
5 files changed, 154 insertions(+), 14 deletions(-)
create mode 100755 tests/qemu-iotests/231
create mode 100644 tests/qemu-iotests/231.out
--
2.17.1
[Qemu-devel] [PATCH v3 1/4] block/rbd: pull out qemu_rbd_convert_options
Code movement to pull the conversion from Qdict to BlockdevOptionsRbd
into a helper function.
Reviewed-by: Eric Blake
Reviewed-by: John Snow
Signed-off-by: Jeff Cody
---
block/rbd.c | 36
1 file changed, 24 insertions(+), 12 deletions(-)
diff --git a/block/rbd.c b/block/rbd.c
index ca8e5bbace..b199450f9f 100644
--- a/block/rbd.c
+++ b/block/rbd.c
@@ -655,12 +655,34 @@ failed_opts:
return r;
}
+static int qemu_rbd_convert_options(QDict *options, BlockdevOptionsRbd **opts,
+Error **errp)
+{
+Visitor *v;
+Error *local_err = NULL;
+
+/* Convert the remaining options into a QAPI object */
+v = qobject_input_visitor_new_flat_confused(options, errp);
+if (!v) {
+return -EINVAL;
+}
+
+visit_type_BlockdevOptionsRbd(v, NULL, opts, &local_err);
+visit_free(v);
+
+if (local_err) {
+error_propagate(errp, local_err);
+return -EINVAL;
+}
+
+return 0;
+}
+
static int qemu_rbd_open(BlockDriverState *bs, QDict *options, int flags,
Error **errp)
{
BDRVRBDState *s = bs->opaque;
BlockdevOptionsRbd *opts = NULL;
-Visitor *v;
const QDictEntry *e;
Error *local_err = NULL;
char *keypairs, *secretid;
@@ -676,19 +698,9 @@ static int qemu_rbd_open(BlockDriverState *bs, QDict
*options, int flags,
qdict_del(options, "password-secret");
}
-/* Convert the remaining options into a QAPI object */
-v = qobject_input_visitor_new_flat_confused(options, errp);
-if (!v) {
-r = -EINVAL;
-goto out;
-}
-
-visit_type_BlockdevOptionsRbd(v, NULL, &opts, &local_err);
-visit_free(v);
-
+r = qemu_rbd_convert_options(options, &opts, &local_err);
if (local_err) {
error_propagate(errp, local_err);
-r = -EINVAL;
goto out;
}
--
2.17.1
[Qemu-devel] [PATCH v3 2/4] block/rbd: Attempt to parse legacy filenames
When we converted rbd to get rid of the older key/value-centric
encoding format, we broke compatibility with image files with backing
file strings encoded in the old format.
This leaves a bit of an ugly conundrum, and a hacky solution.
If the initial attempt to parse the "proper" options fails, it assumes
that we may have an older key/value encoded filename. Fall back to
attempting to parse the filename, and extract the required options from
it. If that fails, pass along the original error message.
We do not support mixed modern usage alongside legacy keyvalue pair
usage.
A deprecation warning has been added, although care should be taken
when actually deprecating since the impact is not limited to
commandline or qapi usage, but also opening existing images.
Reviewed-by: Eric Blake
Signed-off-by: Jeff Cody
---
block/rbd.c | 53 +++--
1 file changed, 51 insertions(+), 2 deletions(-)
diff --git a/block/rbd.c b/block/rbd.c
index b199450f9f..5090e4f662 100644
--- a/block/rbd.c
+++ b/block/rbd.c
@@ -678,6 +678,33 @@ static int qemu_rbd_convert_options(QDict *options,
BlockdevOptionsRbd **opts,
return 0;
}
+static int qemu_rbd_attempt_legacy_options(QDict *options,
+ BlockdevOptionsRbd **opts,
+ char **keypairs)
+{
+char *filename;
+int r;
+
+filename = g_strdup(qdict_get_try_str(options, "filename"));
+if (!filename) {
+return -EINVAL;
+}
+qdict_del(options, "filename");
+
+qemu_rbd_parse_filename(filename, options, NULL);
+
+/* keypairs freed by caller */
+*keypairs = g_strdup(qdict_get_try_str(options, "=keyvalue-pairs"));
+if (*keypairs) {
+qdict_del(options, "=keyvalue-pairs");
+}
+
+r = qemu_rbd_convert_options(options, opts, NULL);
+
+g_free(filename);
+return r;
+}
+
static int qemu_rbd_open(BlockDriverState *bs, QDict *options, int flags,
Error **errp)
{
@@ -700,8 +727,30 @@ static int qemu_rbd_open(BlockDriverState *bs, QDict
*options, int flags,
r = qemu_rbd_convert_options(options, &opts, &local_err);
if (local_err) {
-error_propagate(errp, local_err);
-goto out;
+/* If keypairs are present, that means some options are present in
+ * the modern option format. Don't attempt to parse legacy option
+ * formats, as we won't support mixed usage. */
+if (keypairs) {
+error_propagate(errp, local_err);
+goto out;
+}
+
+/* If the initial attempt to convert and process the options failed,
+ * we may be attempting to open an image file that has the rbd options
+ * specified in the older format consisting of all key/value pairs
+ * encoded in the filename. Go ahead and attempt to parse the
+ * filename, and see if we can pull out the required options. */
+r = qemu_rbd_attempt_legacy_options(options, &opts, &keypairs);
+if (r < 0) {
+error_propagate(errp, local_err);
+goto out;
+}
+/* Take care whenever deciding to actually deprecate; once this ability
+ * is removed, we will not be able to open any images with
legacy-styled
+ * backing image strings. */
+error_report("RBD options encoded in the filename as keyvalue pairs "
+ "is deprecated. Future versions may cease to parse "
+ "these options in the future.");
}
/* Remove the processed options from the QDict (the visitor processes
--
2.17.1
Re: [Qemu-devel] [PATCH v3 4/4] block/rbd: add deprecation documenation for filename keyvalue pairs
On Tue, Sep 11, 2018 at 04:56:36PM -0500, Eric Blake wrote:
> [MAINTAINERS says libvir-list should have been cc'd; not sure why that
> didn't happen]
>
Thanks
> On 9/11/18 4:34 PM, Jeff Cody wrote:
> >Signed-off-by: Jeff Cody
>
> In the subject: s/documenation/documentation/
>
> >---
> > qemu-deprecated.texi | 7 +++
> > 1 file changed, 7 insertions(+)
> >
> >diff --git a/qemu-deprecated.texi b/qemu-deprecated.texi
> >index 1b9c007f12..4df8ac442d 100644
> >--- a/qemu-deprecated.texi
> >+++ b/qemu-deprecated.texi
> >@@ -190,6 +190,13 @@ used instead.
> > In order to prevent QEMU from automatically opening an image's backing
> > chain, use ``"backing": null'' instead.
> >+@subsubsection "rbd keyvalue pair encoded filenames": "" (since 3.1.0)
> >+
> >+Options for ``rbd'' should be specified according to its runtime options,
> >+like other block drivers. Legacy parsing of keyvalue pair encoded
> >+filenames is useful to open images with the old format for backing files;
> >+These image files should be updated to use the current format.
>
> Can we give an example? Cribbing from patch 3, an example might look like
> changing:
>
> json:{"file.driver":"rbd", "file.filename":"rbd:rbd/name"}
>
> into:
>
> json:{"file.driver":"rbd", "file.pool":"rbd", "file.image":"name"}
>
That is a good example, I'll include it.
> I'll let Peter or John comment on whether libvirt's RBD pool handler is
> impacted by this deprecation, but it seems reasonable to me.
>
Thanks!
[Qemu-devel] [PATCH v4 3/4] block/rbd: add iotest for rbd legacy keyvalue filename parsing
This is a small test that will check for the ability to parse
both legacy and modern options for rbd.
The way the test is set up is for failure to occur, but without
having to wait to timeout on a non-existent rbd server. The error
messages in the success path show that the arguments were parsed.
The failure behavior prior to the patch series that has this test, is
qemu-img complaining about mandatory options (e.g. 'pool') not being
provided.
Reviewed-by: Eric Blake
Signed-off-by: Jeff Cody
---
tests/qemu-iotests/231 | 62 ++
tests/qemu-iotests/231.out | 9 ++
tests/qemu-iotests/group | 1 +
3 files changed, 72 insertions(+)
create mode 100755 tests/qemu-iotests/231
create mode 100644 tests/qemu-iotests/231.out
diff --git a/tests/qemu-iotests/231 b/tests/qemu-iotests/231
new file mode 100755
index 00..3e283708b4
--- /dev/null
+++ b/tests/qemu-iotests/231
@@ -0,0 +1,62 @@
+#!/bin/bash
+#
+# Test legacy and modern option parsing for rbd/ceph. This will not
+# actually connect to a ceph server, but rather looks for the appropriate
+# error message that indicates we parsed the options correctly.
+#
+# Copyright (C) 2018 Red Hat, Inc.
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 2 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program. If not, see <http://www.gnu.org/licenses/>.
+#
+
+# creator
+owner=jc...@redhat.com
+
+seq=`basename $0`
+echo "QA output created by $seq"
+
+here=`pwd`
+status=1 # failure is the default!
+
+_cleanup()
+{
+rm "${BOGUS_CONF}"
+}
+trap "_cleanup; exit \$status" 0 1 2 3 15
+
+# get standard environment, filters and checks
+. ./common.rc
+. ./common.filter
+
+_supported_fmt generic
+_supported_proto rbd
+_supported_os Linux
+
+BOGUS_CONF=${TEST_DIR}/ceph-$$.conf
+touch "${BOGUS_CONF}"
+
+_filter_conf()
+{
+sed -e "s#$BOGUS_CONF#BOGUS_CONF#g"
+}
+
+# We expect this to fail, with no monitor ip provided and a null conf file.
Just want it
+# to fail in the right way.
+$QEMU_IMG info
"json:{'file.driver':'rbd','file.filename':'rbd:rbd/bogus:conf=${BOGUS_CONF}'}"
2>&1 | _filter_conf
+$QEMU_IMG info
"json:{'file.driver':'rbd','file.pool':'rbd','file.image':'bogus','file.conf':'${BOGUS_CONF}'}"
2>&1 | _filter_conf
+
+# success, all done
+echo "*** done"
+rm -f $seq.full
+status=0
diff --git a/tests/qemu-iotests/231.out b/tests/qemu-iotests/231.out
new file mode 100644
index 00..579ba11c16
--- /dev/null
+++ b/tests/qemu-iotests/231.out
@@ -0,0 +1,9 @@
+QA output created by 231
+qemu-img: RBD options encoded in the filename as keyvalue pairs is deprecated.
Future versions may cease to parse these options in the future.
+unable to get monitor info from DNS SRV with service name: ceph-mon
+no monitors specified to connect to.
+qemu-img: Could not open
'json:{'file.driver':'rbd','file.filename':'rbd:rbd/bogus:conf=BOGUS_CONF'}':
error connecting: No such file or directory
+unable to get monitor info from DNS SRV with service name: ceph-mon
+no monitors specified to connect to.
+qemu-img: Could not open
'json:{'file.driver':'rbd','file.pool':'rbd','file.image':'bogus','file.conf':'BOGUS_CONF'}':
error connecting: No such file or directory
+*** done
diff --git a/tests/qemu-iotests/group b/tests/qemu-iotests/group
index 743790745b..31f6e77dcb 100644
--- a/tests/qemu-iotests/group
+++ b/tests/qemu-iotests/group
@@ -226,3 +226,4 @@
226 auto quick
227 auto quick
229 auto quick
+231 auto quick
--
2.17.1
[Qemu-devel] [PATCH v4 2/4] block/rbd: Attempt to parse legacy filenames
When we converted rbd to get rid of the older key/value-centric
encoding format, we broke compatibility with image files with backing
file strings encoded in the old format.
This leaves a bit of an ugly conundrum, and a hacky solution.
If the initial attempt to parse the "proper" options fails, it assumes
that we may have an older key/value encoded filename. Fall back to
attempting to parse the filename, and extract the required options from
it. If that fails, pass along the original error message.
We do not support mixed modern usage alongside legacy keyvalue pair
usage.
A deprecation warning has been added, although care should be taken
when actually deprecating since the impact is not limited to
commandline or qapi usage, but also opening existing images.
Reviewed-by: Eric Blake
Signed-off-by: Jeff Cody
---
block/rbd.c | 53 +++--
1 file changed, 51 insertions(+), 2 deletions(-)
diff --git a/block/rbd.c b/block/rbd.c
index b199450f9f..5090e4f662 100644
--- a/block/rbd.c
+++ b/block/rbd.c
@@ -678,6 +678,33 @@ static int qemu_rbd_convert_options(QDict *options,
BlockdevOptionsRbd **opts,
return 0;
}
+static int qemu_rbd_attempt_legacy_options(QDict *options,
+ BlockdevOptionsRbd **opts,
+ char **keypairs)
+{
+char *filename;
+int r;
+
+filename = g_strdup(qdict_get_try_str(options, "filename"));
+if (!filename) {
+return -EINVAL;
+}
+qdict_del(options, "filename");
+
+qemu_rbd_parse_filename(filename, options, NULL);
+
+/* keypairs freed by caller */
+*keypairs = g_strdup(qdict_get_try_str(options, "=keyvalue-pairs"));
+if (*keypairs) {
+qdict_del(options, "=keyvalue-pairs");
+}
+
+r = qemu_rbd_convert_options(options, opts, NULL);
+
+g_free(filename);
+return r;
+}
+
static int qemu_rbd_open(BlockDriverState *bs, QDict *options, int flags,
Error **errp)
{
@@ -700,8 +727,30 @@ static int qemu_rbd_open(BlockDriverState *bs, QDict
*options, int flags,
r = qemu_rbd_convert_options(options, &opts, &local_err);
if (local_err) {
-error_propagate(errp, local_err);
-goto out;
+/* If keypairs are present, that means some options are present in
+ * the modern option format. Don't attempt to parse legacy option
+ * formats, as we won't support mixed usage. */
+if (keypairs) {
+error_propagate(errp, local_err);
+goto out;
+}
+
+/* If the initial attempt to convert and process the options failed,
+ * we may be attempting to open an image file that has the rbd options
+ * specified in the older format consisting of all key/value pairs
+ * encoded in the filename. Go ahead and attempt to parse the
+ * filename, and see if we can pull out the required options. */
+r = qemu_rbd_attempt_legacy_options(options, &opts, &keypairs);
+if (r < 0) {
+error_propagate(errp, local_err);
+goto out;
+}
+/* Take care whenever deciding to actually deprecate; once this ability
+ * is removed, we will not be able to open any images with
legacy-styled
+ * backing image strings. */
+error_report("RBD options encoded in the filename as keyvalue pairs "
+ "is deprecated. Future versions may cease to parse "
+ "these options in the future.");
}
/* Remove the processed options from the QDict (the visitor processes
--
2.17.1
[Qemu-devel] [PATCH v4 0/4] block/rbd: enable filename parsing on open
Changes from v3:
Patch 4: Typo fixed [Eric]
Added examples [Eric]
Changes from v2:
=
Patch 4: New, document deprecation. [Eric]
Patch 3,2: Add r-b's
Changes from v1:
=
Patch 1: Don't pass unused BlockDriverState to helper function
Patch 2: Do not allow mixed usage; fail if keyvalue is present [Eric]
Add deprecation warning [John]
Pull legacy parsing code into function [John]
Fixed filename leak
Patch 3: New; iotest 231. [Eric]
iotest failure on current master:
QA output created by 231
-qemu-img: RBD options encoded in the filename as keyvalue pairs is deprecated.
Future versions may cease to parse these options in the future.
-unable to get monitor info from DNS SRV with service name: ceph-mon
-no monitors specified to connect to.
-qemu-img: Could not open
'json:{'file.driver':'rbd','file.filename':'rbd:rbd/bogus:conf=BOGUS_CONF'}':
error connecting: No such file or directory
+qemu-img: Could not open
'json:{'file.driver':'rbd','file.filename':'rbd:rbd/bogus:conf=BOGUS_CONF'}':
Parameter 'pool' is missing
unable to get monitor info from DNS SRV with service name: ceph-mon
no monitors specified to connect to.
qemu-img: Could not open
'json:{'file.driver':'rbd','file.pool':'rbd','file.image':'bogus','file.conf':'BOGUS_CONF'}':
error connecting: No such file or directory
Failures: 231
Failed 1 of 1 tests
Jeff Cody (4):
block/rbd: pull out qemu_rbd_convert_options
block/rbd: Attempt to parse legacy filenames
block/rbd: add iotest for rbd legacy keyvalue filename parsing
block/rbd: add deprecation documentation for filename keyvalue pairs
block/rbd.c| 89 --
qemu-deprecated.texi | 15 +++
tests/qemu-iotests/231 | 62 ++
tests/qemu-iotests/231.out | 9
tests/qemu-iotests/group | 1 +
5 files changed, 162 insertions(+), 14 deletions(-)
create mode 100755 tests/qemu-iotests/231
create mode 100644 tests/qemu-iotests/231.out
--
2.17.1
[Qemu-devel] [PATCH v4 1/4] block/rbd: pull out qemu_rbd_convert_options
Code movement to pull the conversion from Qdict to BlockdevOptionsRbd
into a helper function.
Reviewed-by: Eric Blake
Reviewed-by: John Snow
Signed-off-by: Jeff Cody
---
block/rbd.c | 36
1 file changed, 24 insertions(+), 12 deletions(-)
diff --git a/block/rbd.c b/block/rbd.c
index ca8e5bbace..b199450f9f 100644
--- a/block/rbd.c
+++ b/block/rbd.c
@@ -655,12 +655,34 @@ failed_opts:
return r;
}
+static int qemu_rbd_convert_options(QDict *options, BlockdevOptionsRbd **opts,
+Error **errp)
+{
+Visitor *v;
+Error *local_err = NULL;
+
+/* Convert the remaining options into a QAPI object */
+v = qobject_input_visitor_new_flat_confused(options, errp);
+if (!v) {
+return -EINVAL;
+}
+
+visit_type_BlockdevOptionsRbd(v, NULL, opts, &local_err);
+visit_free(v);
+
+if (local_err) {
+error_propagate(errp, local_err);
+return -EINVAL;
+}
+
+return 0;
+}
+
static int qemu_rbd_open(BlockDriverState *bs, QDict *options, int flags,
Error **errp)
{
BDRVRBDState *s = bs->opaque;
BlockdevOptionsRbd *opts = NULL;
-Visitor *v;
const QDictEntry *e;
Error *local_err = NULL;
char *keypairs, *secretid;
@@ -676,19 +698,9 @@ static int qemu_rbd_open(BlockDriverState *bs, QDict
*options, int flags,
qdict_del(options, "password-secret");
}
-/* Convert the remaining options into a QAPI object */
-v = qobject_input_visitor_new_flat_confused(options, errp);
-if (!v) {
-r = -EINVAL;
-goto out;
-}
-
-visit_type_BlockdevOptionsRbd(v, NULL, &opts, &local_err);
-visit_free(v);
-
+r = qemu_rbd_convert_options(options, &opts, &local_err);
if (local_err) {
error_propagate(errp, local_err);
-r = -EINVAL;
goto out;
}
--
2.17.1
[Qemu-devel] [PATCH v4 4/4] block/rbd: add deprecation documentation for filename keyvalue pairs
Signed-off-by: Jeff Cody
---
qemu-deprecated.texi | 15 +++
1 file changed, 15 insertions(+)
diff --git a/qemu-deprecated.texi b/qemu-deprecated.texi
index 1b9c007f12..8d285b281e 100644
--- a/qemu-deprecated.texi
+++ b/qemu-deprecated.texi
@@ -190,6 +190,21 @@ used instead.
In order to prevent QEMU from automatically opening an image's backing
chain, use ``"backing": null'' instead.
+@subsubsection rbd keyvalue pair encoded filenames: "" (since 3.1.0)
+
+Options for ``rbd'' should be specified according to its runtime options,
+like other block drivers. Legacy parsing of keyvalue pair encoded
+filenames is useful to open images with the old format for backing files;
+These image files should be updated to use the current format.
+
+Example of legacy encoding:
+
+@code{json:@{"file.driver":"rbd", "file.filename":"rbd:rbd/name"@}}
+
+The above, converted to the current supported format:
+
+@code{json:@{"file.driver":"rbd", "file.pool":"rbd", "file.image":"name"@}}
+
@subsection vio-spapr-device device options
@subsubsection "irq": "" (since 3.0.0)
--
2.17.1
Re: [Qemu-devel] [PATCH 2/2] block/rbd: Attempt to parse legacy filenames
On Wed, Sep 12, 2018 at 12:38:56PM +0200, Kevin Wolf wrote: > Am 11.09.2018 um 20:37 hat Jeff Cody geschrieben: > > On Tue, Sep 11, 2018 at 02:22:31PM -0400, John Snow wrote: > > > Once we load the image, will the header get rewritten into a compliant > > > format? > > > > Hmm - I think in some code paths, but not all. I don't think the answer is > > 'yes' universally, alas. > > Can't we explicitly call BdrvChildRole.update_filename() for all parents > when we open a legacy filename? We'd just need to add the callback to > child_file, which would propagate it to the parents of the format layer, > and then just opening the image with a legacy backing file link once > would fix the problem for this image. > Yes, that is a good idea. I will spin a v5 with that added.
Re: [Qemu-devel] [PATCH 2/2] block/rbd: Attempt to parse legacy filenames
On Wed, Sep 12, 2018 at 08:42:15AM -0400, Jeff Cody wrote: > On Wed, Sep 12, 2018 at 12:38:56PM +0200, Kevin Wolf wrote: > > Am 11.09.2018 um 20:37 hat Jeff Cody geschrieben: > > > On Tue, Sep 11, 2018 at 02:22:31PM -0400, John Snow wrote: > > > > Once we load the image, will the header get rewritten into a compliant > > > > format? > > > > > > Hmm - I think in some code paths, but not all. I don't think the answer > > > is > > > 'yes' universally, alas. > > > > Can't we explicitly call BdrvChildRole.update_filename() for all parents > > when we open a legacy filename? We'd just need to add the callback to > > child_file, which would propagate it to the parents of the format layer, > > and then just opening the image with a legacy backing file link once > > would fix the problem for this image. > > > > Yes, that is a good idea. I will spin a v5 with that added. On second thought; how about we address updating the legacy filename header in a separate patch series? -Jeff
Re: [Qemu-devel] [PATCH v4 0/4] block/rbd: enable filename parsing on open
On Tue, Sep 11, 2018 at 06:32:29PM -0400, Jeff Cody wrote:
> Changes from v3:
>
>
> Patch 4: Typo fixed [Eric]
> Added examples [Eric]
>
> Changes from v2:
> =
>
> Patch 4: New, document deprecation. [Eric]
> Patch 3,2: Add r-b's
>
>
> Changes from v1:
> =
>
> Patch 1: Don't pass unused BlockDriverState to helper function
>
> Patch 2: Do not allow mixed usage; fail if keyvalue is present [Eric]
> Add deprecation warning [John]
> Pull legacy parsing code into function [John]
> Fixed filename leak
>
> Patch 3: New; iotest 231. [Eric]
>
>
> iotest failure on current master:
>
> QA output created by 231
> -qemu-img: RBD options encoded in the filename as keyvalue pairs is
> deprecated. Future versions may cease to parse these options in the future.
> -unable to get monitor info from DNS SRV with service name: ceph-mon
> -no monitors specified to connect to.
> -qemu-img: Could not open
> 'json:{'file.driver':'rbd','file.filename':'rbd:rbd/bogus:conf=BOGUS_CONF'}':
> error connecting: No such file or directory
> +qemu-img: Could not open
> 'json:{'file.driver':'rbd','file.filename':'rbd:rbd/bogus:conf=BOGUS_CONF'}':
> Parameter 'pool' is missing
> unable to get monitor info from DNS SRV with service name: ceph-mon
> no monitors specified to connect to.
> qemu-img: Could not open
> 'json:{'file.driver':'rbd','file.pool':'rbd','file.image':'bogus','file.conf':'BOGUS_CONF'}':
> error connecting: No such file or directory
> Failures: 231
> Failed 1 of 1 tests
>
> Jeff Cody (4):
> block/rbd: pull out qemu_rbd_convert_options
> block/rbd: Attempt to parse legacy filenames
> block/rbd: add iotest for rbd legacy keyvalue filename parsing
> block/rbd: add deprecation documentation for filename keyvalue pairs
>
> block/rbd.c| 89 --
> qemu-deprecated.texi | 15 +++
> tests/qemu-iotests/231 | 62 ++
> tests/qemu-iotests/231.out | 9
> tests/qemu-iotests/group | 1 +
> 5 files changed, 162 insertions(+), 14 deletions(-)
> create mode 100755 tests/qemu-iotests/231
> create mode 100644 tests/qemu-iotests/231.out
>
> --
> 2.17.1
>
Thanks,
Applied to my block branch:
git://github.com/codyprime/qemu-kvm-jtc block
-Jeff
[Qemu-devel] [PULL 0/4] Block patches
The following changes since commit 19b599f7664b2ebfd0f405fb79c14dd241557452: Merge remote-tracking branch 'remotes/armbru/tags/pull-error-2018-08-27-v2' into staging (2018-08-27 16:44:20 +0100) are available in the Git repository at: git://github.com/codyprime/qemu-kvm-jtc.git tags/block-pull-request for you to fetch changes up to 8af2eb7b43a1b694fd6d1d090025027d6b72caac: block/rbd: add deprecation documentation for filename keyvalue pairs (2018-09-12 08:51:45 -0400) Block patches for RBD ---- Jeff Cody (4): block/rbd: pull out qemu_rbd_convert_options block/rbd: Attempt to parse legacy filenames block/rbd: add iotest for rbd legacy keyvalue filename parsing block/rbd: add deprecation documentation for filename keyvalue pairs block/rbd.c| 89 -- qemu-deprecated.texi | 15 +++ tests/qemu-iotests/231 | 62 ++ tests/qemu-iotests/231.out | 9 tests/qemu-iotests/group | 1 + 5 files changed, 162 insertions(+), 14 deletions(-) create mode 100755 tests/qemu-iotests/231 create mode 100644 tests/qemu-iotests/231.out -- 2.17.1
[Qemu-devel] [PULL 4/4] block/rbd: add deprecation documentation for filename keyvalue pairs
Signed-off-by: Jeff Cody
Message-id:
647f5b5ab7efd8bf567a504c832b1d2d6f719b23.1536704901.git.jc...@redhat.com
Signed-off-by: Jeff Cody
---
qemu-deprecated.texi | 15 +++
1 file changed, 15 insertions(+)
diff --git a/qemu-deprecated.texi b/qemu-deprecated.texi
index 1b9c007f12..8d285b281e 100644
--- a/qemu-deprecated.texi
+++ b/qemu-deprecated.texi
@@ -190,6 +190,21 @@ used instead.
In order to prevent QEMU from automatically opening an image's backing
chain, use ``"backing": null'' instead.
+@subsubsection rbd keyvalue pair encoded filenames: "" (since 3.1.0)
+
+Options for ``rbd'' should be specified according to its runtime options,
+like other block drivers. Legacy parsing of keyvalue pair encoded
+filenames is useful to open images with the old format for backing files;
+These image files should be updated to use the current format.
+
+Example of legacy encoding:
+
+@code{json:@{"file.driver":"rbd", "file.filename":"rbd:rbd/name"@}}
+
+The above, converted to the current supported format:
+
+@code{json:@{"file.driver":"rbd", "file.pool":"rbd", "file.image":"name"@}}
+
@subsection vio-spapr-device device options
@subsubsection "irq": "" (since 3.0.0)
--
2.17.1
[Qemu-devel] [PULL 2/4] block/rbd: Attempt to parse legacy filenames
When we converted rbd to get rid of the older key/value-centric
encoding format, we broke compatibility with image files with backing
file strings encoded in the old format.
This leaves a bit of an ugly conundrum, and a hacky solution.
If the initial attempt to parse the "proper" options fails, it assumes
that we may have an older key/value encoded filename. Fall back to
attempting to parse the filename, and extract the required options from
it. If that fails, pass along the original error message.
We do not support mixed modern usage alongside legacy keyvalue pair
usage.
A deprecation warning has been added, although care should be taken
when actually deprecating since the impact is not limited to
commandline or qapi usage, but also opening existing images.
Reviewed-by: Eric Blake
Signed-off-by: Jeff Cody
Message-id:
15b332e5432ad069441f7275a46080f465d789a0.1536704901.git.jc...@redhat.com
Signed-off-by: Jeff Cody
---
block/rbd.c | 53 +++--
1 file changed, 51 insertions(+), 2 deletions(-)
diff --git a/block/rbd.c b/block/rbd.c
index b199450f9f..5090e4f662 100644
--- a/block/rbd.c
+++ b/block/rbd.c
@@ -678,6 +678,33 @@ static int qemu_rbd_convert_options(QDict *options,
BlockdevOptionsRbd **opts,
return 0;
}
+static int qemu_rbd_attempt_legacy_options(QDict *options,
+ BlockdevOptionsRbd **opts,
+ char **keypairs)
+{
+char *filename;
+int r;
+
+filename = g_strdup(qdict_get_try_str(options, "filename"));
+if (!filename) {
+return -EINVAL;
+}
+qdict_del(options, "filename");
+
+qemu_rbd_parse_filename(filename, options, NULL);
+
+/* keypairs freed by caller */
+*keypairs = g_strdup(qdict_get_try_str(options, "=keyvalue-pairs"));
+if (*keypairs) {
+qdict_del(options, "=keyvalue-pairs");
+}
+
+r = qemu_rbd_convert_options(options, opts, NULL);
+
+g_free(filename);
+return r;
+}
+
static int qemu_rbd_open(BlockDriverState *bs, QDict *options, int flags,
Error **errp)
{
@@ -700,8 +727,30 @@ static int qemu_rbd_open(BlockDriverState *bs, QDict
*options, int flags,
r = qemu_rbd_convert_options(options, &opts, &local_err);
if (local_err) {
-error_propagate(errp, local_err);
-goto out;
+/* If keypairs are present, that means some options are present in
+ * the modern option format. Don't attempt to parse legacy option
+ * formats, as we won't support mixed usage. */
+if (keypairs) {
+error_propagate(errp, local_err);
+goto out;
+}
+
+/* If the initial attempt to convert and process the options failed,
+ * we may be attempting to open an image file that has the rbd options
+ * specified in the older format consisting of all key/value pairs
+ * encoded in the filename. Go ahead and attempt to parse the
+ * filename, and see if we can pull out the required options. */
+r = qemu_rbd_attempt_legacy_options(options, &opts, &keypairs);
+if (r < 0) {
+error_propagate(errp, local_err);
+goto out;
+}
+/* Take care whenever deciding to actually deprecate; once this ability
+ * is removed, we will not be able to open any images with
legacy-styled
+ * backing image strings. */
+error_report("RBD options encoded in the filename as keyvalue pairs "
+ "is deprecated. Future versions may cease to parse "
+ "these options in the future.");
}
/* Remove the processed options from the QDict (the visitor processes
--
2.17.1
[Qemu-devel] [PULL 3/4] block/rbd: add iotest for rbd legacy keyvalue filename parsing
This is a small test that will check for the ability to parse
both legacy and modern options for rbd.
The way the test is set up is for failure to occur, but without
having to wait to timeout on a non-existent rbd server. The error
messages in the success path show that the arguments were parsed.
The failure behavior prior to the patch series that has this test, is
qemu-img complaining about mandatory options (e.g. 'pool') not being
provided.
Reviewed-by: Eric Blake
Signed-off-by: Jeff Cody
Message-id:
f830580e339b974a83ed4870d11adcdc17f49a47.1536704901.git.jc...@redhat.com
Signed-off-by: Jeff Cody
---
tests/qemu-iotests/231 | 62 ++
tests/qemu-iotests/231.out | 9 ++
tests/qemu-iotests/group | 1 +
3 files changed, 72 insertions(+)
create mode 100755 tests/qemu-iotests/231
create mode 100644 tests/qemu-iotests/231.out
diff --git a/tests/qemu-iotests/231 b/tests/qemu-iotests/231
new file mode 100755
index 00..3e283708b4
--- /dev/null
+++ b/tests/qemu-iotests/231
@@ -0,0 +1,62 @@
+#!/bin/bash
+#
+# Test legacy and modern option parsing for rbd/ceph. This will not
+# actually connect to a ceph server, but rather looks for the appropriate
+# error message that indicates we parsed the options correctly.
+#
+# Copyright (C) 2018 Red Hat, Inc.
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 2 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program. If not, see <http://www.gnu.org/licenses/>.
+#
+
+# creator
+owner=jc...@redhat.com
+
+seq=`basename $0`
+echo "QA output created by $seq"
+
+here=`pwd`
+status=1 # failure is the default!
+
+_cleanup()
+{
+rm "${BOGUS_CONF}"
+}
+trap "_cleanup; exit \$status" 0 1 2 3 15
+
+# get standard environment, filters and checks
+. ./common.rc
+. ./common.filter
+
+_supported_fmt generic
+_supported_proto rbd
+_supported_os Linux
+
+BOGUS_CONF=${TEST_DIR}/ceph-$$.conf
+touch "${BOGUS_CONF}"
+
+_filter_conf()
+{
+sed -e "s#$BOGUS_CONF#BOGUS_CONF#g"
+}
+
+# We expect this to fail, with no monitor ip provided and a null conf file.
Just want it
+# to fail in the right way.
+$QEMU_IMG info
"json:{'file.driver':'rbd','file.filename':'rbd:rbd/bogus:conf=${BOGUS_CONF}'}"
2>&1 | _filter_conf
+$QEMU_IMG info
"json:{'file.driver':'rbd','file.pool':'rbd','file.image':'bogus','file.conf':'${BOGUS_CONF}'}"
2>&1 | _filter_conf
+
+# success, all done
+echo "*** done"
+rm -f $seq.full
+status=0
diff --git a/tests/qemu-iotests/231.out b/tests/qemu-iotests/231.out
new file mode 100644
index 00..579ba11c16
--- /dev/null
+++ b/tests/qemu-iotests/231.out
@@ -0,0 +1,9 @@
+QA output created by 231
+qemu-img: RBD options encoded in the filename as keyvalue pairs is deprecated.
Future versions may cease to parse these options in the future.
+unable to get monitor info from DNS SRV with service name: ceph-mon
+no monitors specified to connect to.
+qemu-img: Could not open
'json:{'file.driver':'rbd','file.filename':'rbd:rbd/bogus:conf=BOGUS_CONF'}':
error connecting: No such file or directory
+unable to get monitor info from DNS SRV with service name: ceph-mon
+no monitors specified to connect to.
+qemu-img: Could not open
'json:{'file.driver':'rbd','file.pool':'rbd','file.image':'bogus','file.conf':'BOGUS_CONF'}':
error connecting: No such file or directory
+*** done
diff --git a/tests/qemu-iotests/group b/tests/qemu-iotests/group
index 743790745b..31f6e77dcb 100644
--- a/tests/qemu-iotests/group
+++ b/tests/qemu-iotests/group
@@ -226,3 +226,4 @@
226 auto quick
227 auto quick
229 auto quick
+231 auto quick
--
2.17.1
[Qemu-devel] [PULL 1/4] block/rbd: pull out qemu_rbd_convert_options
Code movement to pull the conversion from Qdict to BlockdevOptionsRbd
into a helper function.
Reviewed-by: Eric Blake
Reviewed-by: John Snow
Signed-off-by: Jeff Cody
Message-id:
5b49a980f2cde6610ab1df41bb0277d00b5db893.1536704901.git.jc...@redhat.com
Signed-off-by: Jeff Cody
---
block/rbd.c | 36
1 file changed, 24 insertions(+), 12 deletions(-)
diff --git a/block/rbd.c b/block/rbd.c
index ca8e5bbace..b199450f9f 100644
--- a/block/rbd.c
+++ b/block/rbd.c
@@ -655,12 +655,34 @@ failed_opts:
return r;
}
+static int qemu_rbd_convert_options(QDict *options, BlockdevOptionsRbd **opts,
+Error **errp)
+{
+Visitor *v;
+Error *local_err = NULL;
+
+/* Convert the remaining options into a QAPI object */
+v = qobject_input_visitor_new_flat_confused(options, errp);
+if (!v) {
+return -EINVAL;
+}
+
+visit_type_BlockdevOptionsRbd(v, NULL, opts, &local_err);
+visit_free(v);
+
+if (local_err) {
+error_propagate(errp, local_err);
+return -EINVAL;
+}
+
+return 0;
+}
+
static int qemu_rbd_open(BlockDriverState *bs, QDict *options, int flags,
Error **errp)
{
BDRVRBDState *s = bs->opaque;
BlockdevOptionsRbd *opts = NULL;
-Visitor *v;
const QDictEntry *e;
Error *local_err = NULL;
char *keypairs, *secretid;
@@ -676,19 +698,9 @@ static int qemu_rbd_open(BlockDriverState *bs, QDict
*options, int flags,
qdict_del(options, "password-secret");
}
-/* Convert the remaining options into a QAPI object */
-v = qobject_input_visitor_new_flat_confused(options, errp);
-if (!v) {
-r = -EINVAL;
-goto out;
-}
-
-visit_type_BlockdevOptionsRbd(v, NULL, &opts, &local_err);
-visit_free(v);
-
+r = qemu_rbd_convert_options(options, &opts, &local_err);
if (local_err) {
error_propagate(errp, local_err);
-r = -EINVAL;
goto out;
}
--
2.17.1
Re: [Qemu-devel] [PATCH] curl: Make sslverify=off disable host as well as peer verification.
On Fri, Sep 14, 2018 at 10:56:22AM +0100, Richard W.M. Jones wrote:
> The sslverify setting is supposed to turn off all TLS certificate
> checks in libcurl. However because of the way we use it, it only
> turns off peer certificate authenticity checks
> (CURLOPT_SSL_VERIFYPEER). This patch makes it also turn off the check
> that the server name in the certificate is the same as the server
> you're connecting to (CURLOPT_SSL_VERIFYHOST).
>
> We can use Google's server at 8.8.8.8 which happens to have a bad TLS
> certificate to demonstrate this:
>
> $ ./qemu-img create -q -f qcow2 -b 'json: { "file.sslverify": "off",
> "file.driver": "https", "file.url": "https://8.8.8.8/foo"; }'
> /var/tmp/file.qcow2
> qemu-img: /var/tmp/file.qcow2: CURL: Error opening file: SSL: no alternative
> certificate subject name matches target host name '8.8.8.8'
> Could not open backing image to determine size.
>
> With this patch applied, qemu-img connects to the server regardless of
> the bad certificate:
>
> $ ./qemu-img create -q -f qcow2 -b 'json: { "file.sslverify": "off",
> "file.driver": "https", "file.url": "https://8.8.8.8/foo"; }'
> /var/tmp/file.qcow2
> qemu-img: /var/tmp/file.qcow2: CURL: Error opening file: The requested URL
> returned error: 404 Not Found
>
> (The 404 error is expected because 8.8.8.8 is not actually serving a
> file called "/foo".)
>
> Of course the default (without sslverify=off) remains to always check
> the certificate:
>
> $ ./qemu-img create -q -f qcow2 -b 'json: { "file.driver": "https",
> "file.url": "https://8.8.8.8/foo"; }' /var/tmp/file.qcow2
> qemu-img: /var/tmp/file.qcow2: CURL: Error opening file: SSL: no alternative
> certificate subject name matches target host name '8.8.8.8'
> Could not open backing image to determine size.
>
> Further information about the two settings is available here:
>
> https://curl.haxx.se/libcurl/c/CURLOPT_SSL_VERIFYPEER.html
> https://curl.haxx.se/libcurl/c/CURLOPT_SSL_VERIFYHOST.html
>
> Signed-off-by: Richard W.M. Jones
> ---
> block/curl.c | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/block/curl.c b/block/curl.c
> index 229bb84a27..fabb2b4da7 100644
> --- a/block/curl.c
> +++ b/block/curl.c
> @@ -483,6 +483,8 @@ static int curl_init_state(BDRVCURLState *s, CURLState
> *state)
> curl_easy_setopt(state->curl, CURLOPT_URL, s->url);
> curl_easy_setopt(state->curl, CURLOPT_SSL_VERIFYPEER,
> (long) s->sslverify);
> +curl_easy_setopt(state->curl, CURLOPT_SSL_VERIFYHOST,
> + s->sslverify ? 2L : 0L);
> if (s->cookie) {
> curl_easy_setopt(state->curl, CURLOPT_COOKIE, s->cookie);
> }
> --
> 2.19.0.rc0
>
Thanks,
Applied to my block branch:
git://github.com/codyprime/qemu-kvm-jtc block
-Jeff
Re: [Qemu-devel] [PATCH v4 2/4] block/rbd: Attempt to parse legacy filenames
On Sat, Sep 22, 2018 at 08:18:26AM +0200, Markus Armbruster wrote:
> Jeff Cody writes:
>
> > When we converted rbd to get rid of the older key/value-centric
> > encoding format, we broke compatibility with image files with backing
> > file strings encoded in the old format.
> >
> > This leaves a bit of an ugly conundrum, and a hacky solution.
> >
> > If the initial attempt to parse the "proper" options fails, it assumes
> > that we may have an older key/value encoded filename. Fall back to
> > attempting to parse the filename, and extract the required options from
> > it. If that fails, pass along the original error message.
> >
> > We do not support mixed modern usage alongside legacy keyvalue pair
> > usage.
> >
> > A deprecation warning has been added, although care should be taken
> > when actually deprecating since the impact is not limited to
> > commandline or qapi usage, but also opening existing images.
> >
> > Reviewed-by: Eric Blake
> > Signed-off-by: Jeff Cody
> > ---
> > block/rbd.c | 53 +++--
> > 1 file changed, 51 insertions(+), 2 deletions(-)
> >
> > diff --git a/block/rbd.c b/block/rbd.c
> > index b199450f9f..5090e4f662 100644
> > --- a/block/rbd.c
> > +++ b/block/rbd.c
> > @@ -678,6 +678,33 @@ static int qemu_rbd_convert_options(QDict *options,
> > BlockdevOptionsRbd **opts,
> > return 0;
> > }
> >
> > +static int qemu_rbd_attempt_legacy_options(QDict *options,
> > + BlockdevOptionsRbd **opts,
> > + char **keypairs)
> > +{
> > +char *filename;
> > +int r;
> > +
> > +filename = g_strdup(qdict_get_try_str(options, "filename"));
> > +if (!filename) {
> > +return -EINVAL;
> > +}
> > +qdict_del(options, "filename");
> > +
> > +qemu_rbd_parse_filename(filename, options, NULL);
> > +
> > +/* keypairs freed by caller */
> > +*keypairs = g_strdup(qdict_get_try_str(options, "=keyvalue-pairs"));
> > +if (*keypairs) {
> > +qdict_del(options, "=keyvalue-pairs");
> > +}
> > +
> > +r = qemu_rbd_convert_options(options, opts, NULL);
> > +
> > +g_free(filename);
> > +return r;
> > +}
> > +
> > static int qemu_rbd_open(BlockDriverState *bs, QDict *options, int flags,
> > Error **errp)
> > {
> > @@ -700,8 +727,30 @@ static int qemu_rbd_open(BlockDriverState *bs, QDict
> > *options, int flags,
> >
> > r = qemu_rbd_convert_options(options, &opts, &local_err);
> > if (local_err) {
> > -error_propagate(errp, local_err);
> > -goto out;
> > +/* If keypairs are present, that means some options are present in
> > + * the modern option format. Don't attempt to parse legacy option
> > + * formats, as we won't support mixed usage. */
> > +if (keypairs) {
> > +error_propagate(errp, local_err);
> > +goto out;
> > +}
> > +
> > +/* If the initial attempt to convert and process the options
> > failed,
> > + * we may be attempting to open an image file that has the rbd
> > options
> > + * specified in the older format consisting of all key/value pairs
> > + * encoded in the filename. Go ahead and attempt to parse the
> > + * filename, and see if we can pull out the required options. */
> > +r = qemu_rbd_attempt_legacy_options(options, &opts, &keypairs);
> > +if (r < 0) {
> > +error_propagate(errp, local_err);
> > +goto out;
>
> This reports the error from qemu_rbd_convert_options(), as you commit
> message explains. Would explaining it in a comment help future readers?
>
> > +}
> > +/* Take care whenever deciding to actually deprecate; once this
> > ability
> > + * is removed, we will not be able to open any images with
> > legacy-styled
> > + * backing image strings. */
> > +error_report("RBD options encoded in the filename as keyvalue
> > pairs "
> > + "is deprecated. Future versions may cease to parse "
> > + "these options in the future.");
>
> "Future versions may ... in the future": you're serious about this
> happening only in the future, aren't you? ;)
Eric noticed this as well :)
>
> Quote error_report()'s contract: "The resulting message should be a
> single phrase, with no newline or trailing punctuation."
>
> Let's scratch everything from the first period on.
>
Since the two requested changes are comments only and minor, and a PR has
already been sent, I went ahead and updated the patch and will send a v2
PR with these patches. I left the r-b for this patch untouched.
> > }
> >
> > /* Remove the processed options from the QDict (the visitor processes
[Qemu-devel] [PULL v2 1/5] block/rbd: pull out qemu_rbd_convert_options
Code movement to pull the conversion from Qdict to BlockdevOptionsRbd
into a helper function.
Reviewed-by: Eric Blake
Reviewed-by: John Snow
Signed-off-by: Jeff Cody
Message-id:
5b49a980f2cde6610ab1df41bb0277d00b5db893.1536704901.git.jc...@redhat.com
Signed-off-by: Jeff Cody
---
block/rbd.c | 36
1 file changed, 24 insertions(+), 12 deletions(-)
diff --git a/block/rbd.c b/block/rbd.c
index ca8e5bbace..b199450f9f 100644
--- a/block/rbd.c
+++ b/block/rbd.c
@@ -655,12 +655,34 @@ failed_opts:
return r;
}
+static int qemu_rbd_convert_options(QDict *options, BlockdevOptionsRbd **opts,
+Error **errp)
+{
+Visitor *v;
+Error *local_err = NULL;
+
+/* Convert the remaining options into a QAPI object */
+v = qobject_input_visitor_new_flat_confused(options, errp);
+if (!v) {
+return -EINVAL;
+}
+
+visit_type_BlockdevOptionsRbd(v, NULL, opts, &local_err);
+visit_free(v);
+
+if (local_err) {
+error_propagate(errp, local_err);
+return -EINVAL;
+}
+
+return 0;
+}
+
static int qemu_rbd_open(BlockDriverState *bs, QDict *options, int flags,
Error **errp)
{
BDRVRBDState *s = bs->opaque;
BlockdevOptionsRbd *opts = NULL;
-Visitor *v;
const QDictEntry *e;
Error *local_err = NULL;
char *keypairs, *secretid;
@@ -676,19 +698,9 @@ static int qemu_rbd_open(BlockDriverState *bs, QDict
*options, int flags,
qdict_del(options, "password-secret");
}
-/* Convert the remaining options into a QAPI object */
-v = qobject_input_visitor_new_flat_confused(options, errp);
-if (!v) {
-r = -EINVAL;
-goto out;
-}
-
-visit_type_BlockdevOptionsRbd(v, NULL, &opts, &local_err);
-visit_free(v);
-
+r = qemu_rbd_convert_options(options, &opts, &local_err);
if (local_err) {
error_propagate(errp, local_err);
-r = -EINVAL;
goto out;
}
--
2.17.1
[Qemu-devel] [PULL v2 5/5] curl: Make sslverify=off disable host as well as peer verification.
From: "Richard W.M. Jones"
The sslverify setting is supposed to turn off all TLS certificate
checks in libcurl. However because of the way we use it, it only
turns off peer certificate authenticity checks
(CURLOPT_SSL_VERIFYPEER). This patch makes it also turn off the check
that the server name in the certificate is the same as the server
you're connecting to (CURLOPT_SSL_VERIFYHOST).
We can use Google's server at 8.8.8.8 which happens to have a bad TLS
certificate to demonstrate this:
$ ./qemu-img create -q -f qcow2 -b 'json: { "file.sslverify": "off",
"file.driver": "https", "file.url": "https://8.8.8.8/foo"; }' /var/tmp/file.qcow2
qemu-img: /var/tmp/file.qcow2: CURL: Error opening file: SSL: no alternative
certificate subject name matches target host name '8.8.8.8'
Could not open backing image to determine size.
With this patch applied, qemu-img connects to the server regardless of
the bad certificate:
$ ./qemu-img create -q -f qcow2 -b 'json: { "file.sslverify": "off",
"file.driver": "https", "file.url": "https://8.8.8.8/foo"; }' /var/tmp/file.qcow2
qemu-img: /var/tmp/file.qcow2: CURL: Error opening file: The requested URL
returned error: 404 Not Found
(The 404 error is expected because 8.8.8.8 is not actually serving a
file called "/foo".)
Of course the default (without sslverify=off) remains to always check
the certificate:
$ ./qemu-img create -q -f qcow2 -b 'json: { "file.driver": "https", "file.url":
"https://8.8.8.8/foo"; }' /var/tmp/file.qcow2
qemu-img: /var/tmp/file.qcow2: CURL: Error opening file: SSL: no alternative
certificate subject name matches target host name '8.8.8.8'
Could not open backing image to determine size.
Further information about the two settings is available here:
https://curl.haxx.se/libcurl/c/CURLOPT_SSL_VERIFYPEER.html
https://curl.haxx.se/libcurl/c/CURLOPT_SSL_VERIFYHOST.html
Signed-off-by: Richard W.M. Jones
Message-id: 20180914095622.19698-1-rjo...@redhat.com
Signed-off-by: Jeff Cody
---
block/curl.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/block/curl.c b/block/curl.c
index 229bb84a27..fabb2b4da7 100644
--- a/block/curl.c
+++ b/block/curl.c
@@ -483,6 +483,8 @@ static int curl_init_state(BDRVCURLState *s, CURLState
*state)
curl_easy_setopt(state->curl, CURLOPT_URL, s->url);
curl_easy_setopt(state->curl, CURLOPT_SSL_VERIFYPEER,
(long) s->sslverify);
+curl_easy_setopt(state->curl, CURLOPT_SSL_VERIFYHOST,
+ s->sslverify ? 2L : 0L);
if (s->cookie) {
curl_easy_setopt(state->curl, CURLOPT_COOKIE, s->cookie);
}
--
2.17.1
[Qemu-devel] [PULL v2 3/5] block/rbd: add iotest for rbd legacy keyvalue filename parsing
This is a small test that will check for the ability to parse
both legacy and modern options for rbd.
The way the test is set up is for failure to occur, but without
having to wait to timeout on a non-existent rbd server. The error
messages in the success path show that the arguments were parsed.
The failure behavior prior to the patch series that has this test, is
qemu-img complaining about mandatory options (e.g. 'pool') not being
provided.
Reviewed-by: Eric Blake
Signed-off-by: Jeff Cody
Message-id:
f830580e339b974a83ed4870d11adcdc17f49a47.1536704901.git.jc...@redhat.com
Signed-off-by: Jeff Cody
---
tests/qemu-iotests/231 | 62 ++
tests/qemu-iotests/231.out | 9 ++
tests/qemu-iotests/group | 1 +
3 files changed, 72 insertions(+)
create mode 100755 tests/qemu-iotests/231
create mode 100644 tests/qemu-iotests/231.out
diff --git a/tests/qemu-iotests/231 b/tests/qemu-iotests/231
new file mode 100755
index 00..3e283708b4
--- /dev/null
+++ b/tests/qemu-iotests/231
@@ -0,0 +1,62 @@
+#!/bin/bash
+#
+# Test legacy and modern option parsing for rbd/ceph. This will not
+# actually connect to a ceph server, but rather looks for the appropriate
+# error message that indicates we parsed the options correctly.
+#
+# Copyright (C) 2018 Red Hat, Inc.
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 2 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program. If not, see <http://www.gnu.org/licenses/>.
+#
+
+# creator
+owner=jc...@redhat.com
+
+seq=`basename $0`
+echo "QA output created by $seq"
+
+here=`pwd`
+status=1 # failure is the default!
+
+_cleanup()
+{
+rm "${BOGUS_CONF}"
+}
+trap "_cleanup; exit \$status" 0 1 2 3 15
+
+# get standard environment, filters and checks
+. ./common.rc
+. ./common.filter
+
+_supported_fmt generic
+_supported_proto rbd
+_supported_os Linux
+
+BOGUS_CONF=${TEST_DIR}/ceph-$$.conf
+touch "${BOGUS_CONF}"
+
+_filter_conf()
+{
+sed -e "s#$BOGUS_CONF#BOGUS_CONF#g"
+}
+
+# We expect this to fail, with no monitor ip provided and a null conf file.
Just want it
+# to fail in the right way.
+$QEMU_IMG info
"json:{'file.driver':'rbd','file.filename':'rbd:rbd/bogus:conf=${BOGUS_CONF}'}"
2>&1 | _filter_conf
+$QEMU_IMG info
"json:{'file.driver':'rbd','file.pool':'rbd','file.image':'bogus','file.conf':'${BOGUS_CONF}'}"
2>&1 | _filter_conf
+
+# success, all done
+echo "*** done"
+rm -f $seq.full
+status=0
diff --git a/tests/qemu-iotests/231.out b/tests/qemu-iotests/231.out
new file mode 100644
index 00..579ba11c16
--- /dev/null
+++ b/tests/qemu-iotests/231.out
@@ -0,0 +1,9 @@
+QA output created by 231
+qemu-img: RBD options encoded in the filename as keyvalue pairs is deprecated.
Future versions may cease to parse these options in the future.
+unable to get monitor info from DNS SRV with service name: ceph-mon
+no monitors specified to connect to.
+qemu-img: Could not open
'json:{'file.driver':'rbd','file.filename':'rbd:rbd/bogus:conf=BOGUS_CONF'}':
error connecting: No such file or directory
+unable to get monitor info from DNS SRV with service name: ceph-mon
+no monitors specified to connect to.
+qemu-img: Could not open
'json:{'file.driver':'rbd','file.pool':'rbd','file.image':'bogus','file.conf':'BOGUS_CONF'}':
error connecting: No such file or directory
+*** done
diff --git a/tests/qemu-iotests/group b/tests/qemu-iotests/group
index 743790745b..31f6e77dcb 100644
--- a/tests/qemu-iotests/group
+++ b/tests/qemu-iotests/group
@@ -226,3 +226,4 @@
226 auto quick
227 auto quick
229 auto quick
+231 auto quick
--
2.17.1
[Qemu-devel] [PULL v2 0/5] Block patches
The following changes since commit 741e1a618b126e664f7b723e6fe1b7ace511caf7: Merge remote-tracking branch 'remotes/stefanberger/tags/pull-tpm-2018-09-07-1' into staging (2018-09-24 18:12:54 +0100) are available in the Git repository at: git://github.com/codyprime/qemu-kvm-jtc.git tags/block-pull-request for you to fetch changes up to 637fa44ab80c6b317adf1d117494325a95daad60: curl: Make sslverify=off disable host as well as peer verification. (2018-09-24 23:46:05 -0400) RBD and Curl patches ---- Jeff Cody (4): block/rbd: pull out qemu_rbd_convert_options block/rbd: Attempt to parse legacy filenames block/rbd: add iotest for rbd legacy keyvalue filename parsing block/rbd: add deprecation documentation for filename keyvalue pairs Richard W.M. Jones (1): curl: Make sslverify=off disable host as well as peer verification. block/curl.c | 2 + block/rbd.c| 90 -- qemu-deprecated.texi | 15 +++ tests/qemu-iotests/231 | 62 ++ tests/qemu-iotests/231.out | 9 tests/qemu-iotests/group | 1 + 6 files changed, 165 insertions(+), 14 deletions(-) create mode 100755 tests/qemu-iotests/231 create mode 100644 tests/qemu-iotests/231.out -- 2.17.1
[Qemu-devel] [PULL v2 2/5] block/rbd: Attempt to parse legacy filenames
When we converted rbd to get rid of the older key/value-centric
encoding format, we broke compatibility with image files with backing
file strings encoded in the old format.
This leaves a bit of an ugly conundrum, and a hacky solution.
If the initial attempt to parse the "proper" options fails, it assumes
that we may have an older key/value encoded filename. Fall back to
attempting to parse the filename, and extract the required options from
it. If that fails, pass along the original error message.
We do not support mixed modern usage alongside legacy keyvalue pair
usage.
A deprecation warning has been added, although care should be taken
when actually deprecating since the impact is not limited to
commandline or qapi usage, but also opening existing images.
Reviewed-by: Eric Blake
Signed-off-by: Jeff Cody
Message-id:
15b332e5432ad069441f7275a46080f465d789a0.1536704901.git.jc...@redhat.com
Signed-off-by: Jeff Cody
---
block/rbd.c | 54 +++--
1 file changed, 52 insertions(+), 2 deletions(-)
diff --git a/block/rbd.c b/block/rbd.c
index b199450f9f..014c68d629 100644
--- a/block/rbd.c
+++ b/block/rbd.c
@@ -678,6 +678,33 @@ static int qemu_rbd_convert_options(QDict *options,
BlockdevOptionsRbd **opts,
return 0;
}
+static int qemu_rbd_attempt_legacy_options(QDict *options,
+ BlockdevOptionsRbd **opts,
+ char **keypairs)
+{
+char *filename;
+int r;
+
+filename = g_strdup(qdict_get_try_str(options, "filename"));
+if (!filename) {
+return -EINVAL;
+}
+qdict_del(options, "filename");
+
+qemu_rbd_parse_filename(filename, options, NULL);
+
+/* keypairs freed by caller */
+*keypairs = g_strdup(qdict_get_try_str(options, "=keyvalue-pairs"));
+if (*keypairs) {
+qdict_del(options, "=keyvalue-pairs");
+}
+
+r = qemu_rbd_convert_options(options, opts, NULL);
+
+g_free(filename);
+return r;
+}
+
static int qemu_rbd_open(BlockDriverState *bs, QDict *options, int flags,
Error **errp)
{
@@ -700,8 +727,31 @@ static int qemu_rbd_open(BlockDriverState *bs, QDict
*options, int flags,
r = qemu_rbd_convert_options(options, &opts, &local_err);
if (local_err) {
-error_propagate(errp, local_err);
-goto out;
+/* If keypairs are present, that means some options are present in
+ * the modern option format. Don't attempt to parse legacy option
+ * formats, as we won't support mixed usage. */
+if (keypairs) {
+error_propagate(errp, local_err);
+goto out;
+}
+
+/* If the initial attempt to convert and process the options failed,
+ * we may be attempting to open an image file that has the rbd options
+ * specified in the older format consisting of all key/value pairs
+ * encoded in the filename. Go ahead and attempt to parse the
+ * filename, and see if we can pull out the required options. */
+r = qemu_rbd_attempt_legacy_options(options, &opts, &keypairs);
+if (r < 0) {
+/* Propagate the original error, not the legacy parsing fallback
+ * error, as the latter was just a best-effort attempt. */
+error_propagate(errp, local_err);
+goto out;
+}
+/* Take care whenever deciding to actually deprecate; once this ability
+ * is removed, we will not be able to open any images with
legacy-styled
+ * backing image strings. */
+error_report("RBD options encoded in the filename as keyvalue pairs "
+ "is deprecated");
}
/* Remove the processed options from the QDict (the visitor processes
--
2.17.1
[Qemu-devel] [PULL v2 4/5] block/rbd: add deprecation documentation for filename keyvalue pairs
Signed-off-by: Jeff Cody
Message-id:
647f5b5ab7efd8bf567a504c832b1d2d6f719b23.1536704901.git.jc...@redhat.com
Signed-off-by: Jeff Cody
---
qemu-deprecated.texi | 15 +++
1 file changed, 15 insertions(+)
diff --git a/qemu-deprecated.texi b/qemu-deprecated.texi
index a43fcf4835..2283fc52c3 100644
--- a/qemu-deprecated.texi
+++ b/qemu-deprecated.texi
@@ -128,6 +128,21 @@ used instead.
In order to prevent QEMU from automatically opening an image's backing
chain, use ``"backing": null'' instead.
+@subsubsection rbd keyvalue pair encoded filenames: "" (since 3.1.0)
+
+Options for ``rbd'' should be specified according to its runtime options,
+like other block drivers. Legacy parsing of keyvalue pair encoded
+filenames is useful to open images with the old format for backing files;
+These image files should be updated to use the current format.
+
+Example of legacy encoding:
+
+@code{json:@{"file.driver":"rbd", "file.filename":"rbd:rbd/name"@}}
+
+The above, converted to the current supported format:
+
+@code{json:@{"file.driver":"rbd", "file.pool":"rbd", "file.image":"name"@}}
+
@subsection vio-spapr-device device options
@subsubsection "irq": "" (since 3.0.0)
--
2.17.1
Re: [Qemu-devel] [PULL 0/4] Block patches
On Wed, Sep 12, 2018 at 09:18:49AM -0400, Jeff Cody wrote: > The following changes since commit 19b599f7664b2ebfd0f405fb79c14dd241557452: > > Merge remote-tracking branch 'remotes/armbru/tags/pull-error-2018-08-27-v2' > into staging (2018-08-27 16:44:20 +0100) > > are available in the Git repository at: > > git://github.com/codyprime/qemu-kvm-jtc.git tags/block-pull-request > > for you to fetch changes up to 8af2eb7b43a1b694fd6d1d090025027d6b72caac: > > block/rbd: add deprecation documentation for filename keyvalue pairs > (2018-09-12 08:51:45 -0400) > > > Block patches for RBD > ---- > > Jeff Cody (4): > block/rbd: pull out qemu_rbd_convert_options > block/rbd: Attempt to parse legacy filenames > block/rbd: add iotest for rbd legacy keyvalue filename parsing > block/rbd: add deprecation documentation for filename keyvalue pairs > > block/rbd.c| 89 -- > qemu-deprecated.texi | 15 +++ > tests/qemu-iotests/231 | 62 ++ > tests/qemu-iotests/231.out | 9 > tests/qemu-iotests/group | 1 + > 5 files changed, 162 insertions(+), 14 deletions(-) > create mode 100755 tests/qemu-iotests/231 > create mode 100644 tests/qemu-iotests/231.out > > -- > 2.17.1 > NACK. There is a minor update to one of the patches, and I'll toss another block patch in with a v2 (Richard Jones' curl patch).
[Qemu-devel] [PATCH 0/2] Maintainership changes
I'm not going to be involved with day-to-day qemu development, so this necessitates some changes. Jeff Cody (2): MAINTAINERS: Replace myself with John Snow for block jobs MAINTAINERS: Remove myself as block maintainer MAINTAINERS | 18 ++ 1 file changed, 2 insertions(+), 16 deletions(-) -- 2.17.1
[Qemu-devel] [PATCH 2/2] MAINTAINERS: Remove myself as block maintainer
I'll not be involved in day-to-day qemu development. Remove myself as maintainer from the remainder of the network block drivers (and vhdx), and revert them to the general block layer maintainership. Signed-off-by: Jeff Cody --- MAINTAINERS | 14 -- 1 file changed, 14 deletions(-) diff --git a/MAINTAINERS b/MAINTAINERS index e93f79672f..6ef6932628 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -1982,28 +1982,22 @@ F: block/vmdk.c RBD M: Josh Durgin -M: Jeff Cody L: qemu-bl...@nongnu.org S: Supported F: block/rbd.c -T: git git://github.com/codyprime/qemu-kvm-jtc.git block Sheepdog M: Hitoshi Mitake M: Liu Yuan -M: Jeff Cody L: qemu-bl...@nongnu.org L: sheep...@lists.wpkg.org S: Supported F: block/sheepdog.c -T: git git://github.com/codyprime/qemu-kvm-jtc.git block VHDX -M: Jeff Cody L: qemu-bl...@nongnu.org S: Supported F: block/vhdx* -T: git git://github.com/codyprime/qemu-kvm-jtc.git block VDI M: Stefan Weil @@ -2034,34 +2028,26 @@ F: docs/interop/nbd.txt T: git git://repo.or.cz/qemu/ericb.git nbd NFS -M: Jeff Cody M: Peter Lieven L: qemu-bl...@nongnu.org S: Maintained F: block/nfs.c -T: git git://github.com/codyprime/qemu-kvm-jtc.git block SSH M: Richard W.M. Jones -M: Jeff Cody L: qemu-bl...@nongnu.org S: Supported F: block/ssh.c -T: git git://github.com/codyprime/qemu-kvm-jtc.git block CURL -M: Jeff Cody L: qemu-bl...@nongnu.org S: Supported F: block/curl.c -T: git git://github.com/codyprime/qemu-kvm-jtc.git block GLUSTER -M: Jeff Cody L: qemu-bl...@nongnu.org S: Supported F: block/gluster.c -T: git git://github.com/codyprime/qemu-kvm-jtc.git block Null Block Driver M: Fam Zheng -- 2.17.1
[Qemu-devel] [PATCH 1/2] MAINTAINERS: Replace myself with John Snow for block jobs
I'll not be involved with day-to-day qemu development, and John Snow is a block jobs wizard. Have him take over block job maintainership duties. Signed-off-by: Jeff Cody --- MAINTAINERS | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/MAINTAINERS b/MAINTAINERS index aa1dd76dc2..e93f79672f 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -1448,7 +1448,7 @@ F: include/scsi/* F: scsi/* Block Jobs -M: Jeff Cody +M: John Snow L: qemu-bl...@nongnu.org S: Supported F: blockjob.c @@ -1461,7 +1461,7 @@ F: block/commit.c F: block/stream.c F: block/mirror.c F: qapi/job.json -T: git git://github.com/codyprime/qemu-kvm-jtc.git block +T: git git://github.com/jnsnow/qemu.git ide Block QAPI, monitor, command line M: Markus Armbruster -- 2.17.1
Re: [Qemu-devel] [PATCH 1/2] MAINTAINERS: Replace myself with John Snow for block jobs
On Tue, Sep 25, 2018 at 12:09:15AM -0400, Jeff Cody wrote: > I'll not be involved with day-to-day qemu development, and John > Snow is a block jobs wizard. Have him take over block job > maintainership duties. > > Signed-off-by: Jeff Cody > --- > MAINTAINERS | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/MAINTAINERS b/MAINTAINERS > index aa1dd76dc2..e93f79672f 100644 > --- a/MAINTAINERS > +++ b/MAINTAINERS > @@ -1448,7 +1448,7 @@ F: include/scsi/* > F: scsi/* > > Block Jobs > -M: Jeff Cody > +M: John Snow > L: qemu-bl...@nongnu.org > S: Supported > F: blockjob.c > @@ -1461,7 +1461,7 @@ F: block/commit.c > F: block/stream.c > F: block/mirror.c > F: qapi/job.json > -T: git git://github.com/codyprime/qemu-kvm-jtc.git block > +T: git git://github.com/jnsnow/qemu.git ide ^ This might need a v2 or touch-up whenever applied for the correct branch, if John does take over. > > Block QAPI, monitor, command line > M: Markus Armbruster > -- > 2.17.1 >
Re: [Qemu-devel] [PATCH 2/2] MAINTAINERS: Remove myself as block maintainer
On Tue, Sep 25, 2018 at 03:49:36PM +0800, Fam Zheng wrote: > On Tue, 09/25 09:37, Markus Armbruster wrote: > > Do we want to have a dedicated VHDX driver submaintainer again? Fam, > > you're maintaining VMDK, could you cover VHDX as well? > > I don't know a lot VHDX internals. Considering my capacity at the moment I'd > rather not take this one. > > Fam Anyone can feel free to email me at codypr...@gmail.com with any VHDX questions (I am subbed to the qemu mailing list there as well). I'll at least do my best to answer :)
[Qemu-devel] [PATCH v2 1/2] MAINTAINERS: Replace myself with John Snow for block jobs
I'll not be involved with day-to-day qemu development, and John Snow is a block jobs wizard. Have him take over block job maintainership duties. Signed-off-by: Jeff Cody --- MAINTAINERS | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/MAINTAINERS b/MAINTAINERS index ce7c351afa..0e22b795e6 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -1455,7 +1455,7 @@ F: include/scsi/* F: scsi/* Block Jobs -M: Jeff Cody +M: John Snow L: qemu-bl...@nongnu.org S: Supported F: blockjob.c @@ -1468,7 +1468,7 @@ F: block/commit.c F: block/stream.c F: block/mirror.c F: qapi/job.json -T: git git://github.com/codyprime/qemu-kvm-jtc.git block +T: git git://github.com/jnsnow/qemu.git jobs Block QAPI, monitor, command line M: Markus Armbruster -- 2.17.1
[Qemu-devel] [PATCH v2 2/2] MAINTAINERS: Remove myself as block maintainer
I'll not be involved in day-to-day qemu development. Remove myself as maintainer from the remainder of the network block drivers, and revert them to the general block layer maintainership. Move 'sheepdog' to the 'Odd Fixes' support level. For VHDX, added my personal email address as a maintainer, as I can answer questions or send the occassional bug fix. Leaving it as 'Supported', instead of 'Odd Fixes', because I think the rest of the block layer maintainers and developers will upkeep it as well, if needed. Signed-off-by: Jeff Cody --- MAINTAINERS | 17 ++--- 1 file changed, 2 insertions(+), 15 deletions(-) diff --git a/MAINTAINERS b/MAINTAINERS index 0e22b795e6..7147c44bbc 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -1988,28 +1988,23 @@ F: block/vmdk.c RBD M: Josh Durgin -M: Jeff Cody L: qemu-bl...@nongnu.org S: Supported F: block/rbd.c -T: git git://github.com/codyprime/qemu-kvm-jtc.git block Sheepdog M: Hitoshi Mitake M: Liu Yuan -M: Jeff Cody L: qemu-bl...@nongnu.org L: sheep...@lists.wpkg.org -S: Supported +S: Odd Fixes F: block/sheepdog.c -T: git git://github.com/codyprime/qemu-kvm-jtc.git block VHDX -M: Jeff Cody +M: Jeff Cody L: qemu-bl...@nongnu.org S: Supported F: block/vhdx* -T: git git://github.com/codyprime/qemu-kvm-jtc.git block VDI M: Stefan Weil @@ -2040,34 +2035,26 @@ F: docs/interop/nbd.txt T: git git://repo.or.cz/qemu/ericb.git nbd NFS -M: Jeff Cody M: Peter Lieven L: qemu-bl...@nongnu.org S: Maintained F: block/nfs.c -T: git git://github.com/codyprime/qemu-kvm-jtc.git block SSH M: Richard W.M. Jones -M: Jeff Cody L: qemu-bl...@nongnu.org S: Supported F: block/ssh.c -T: git git://github.com/codyprime/qemu-kvm-jtc.git block CURL -M: Jeff Cody L: qemu-bl...@nongnu.org S: Supported F: block/curl.c -T: git git://github.com/codyprime/qemu-kvm-jtc.git block GLUSTER -M: Jeff Cody L: qemu-bl...@nongnu.org S: Supported F: block/gluster.c -T: git git://github.com/codyprime/qemu-kvm-jtc.git block Null Block Driver M: Fam Zheng -- 2.17.1
[Qemu-devel] [PATCH v2 0/2] Maintainership changes
Changes from v1: - cc'ed Peter (meant to for v1, forgot) - added John's 'jobs' branch to his git url - kept myself as maintainer for vhdx - moved sheepdog to 'Odd Fixes' I'm not going to be involved with day-to-day qemu development, so this necessitates some changes. Jeff Cody (2): MAINTAINERS: Replace myself with John Snow for block jobs MAINTAINERS: Remove myself as block maintainer MAINTAINERS | 21 - 1 file changed, 4 insertions(+), 17 deletions(-) -- 2.17.1
Re: [Qemu-devel] [PATCH v3 01/20] blockjob: Track job ratelimits via bytes, not sectors
On Tue, Jun 27, 2017 at 02:24:39PM -0500, Eric Blake wrote:
> The user interface specifies job rate limits in bytes/second.
> It's pointless to have our internal representation track things
> in sectors/second, particularly since we want to move away from
> sector-based interfaces.
>
> Fix up a doc typo found while verifying that the ratelimit
> code handles the scaling difference.
>
> Repetition of expressions like 'n * BDRV_SECTOR_SIZE' will be
> cleaned up later when functions are converted to iterate over
> images by bytes rather than by sectors.
>
> Signed-off-by: Eric Blake
> Reviewed-by: John Snow
>
Reviewed-by: Jeff Cody
> ---
> v2: adjust commit message based on review; no code change
> ---
> include/qemu/ratelimit.h | 3 ++-
> block/backup.c | 5 +++--
> block/commit.c | 5 +++--
> block/mirror.c | 13 +++--
> block/stream.c | 5 +++--
> 5 files changed, 18 insertions(+), 13 deletions(-)
>
> diff --git a/include/qemu/ratelimit.h b/include/qemu/ratelimit.h
> index 8da1232..8dece48 100644
> --- a/include/qemu/ratelimit.h
> +++ b/include/qemu/ratelimit.h
> @@ -24,7 +24,8 @@ typedef struct {
>
> /** Calculate and return delay for next request in ns
> *
> - * Record that we sent @p n data units. If we may send more data units
> + * Record that we sent @n data units (where @n matches the scale chosen
> + * during ratelimit_set_speed). If we may send more data units
> * in the current time slice, return 0 (i.e. no delay). Otherwise
> * return the amount of time (in ns) until the start of the next time
> * slice that will permit sending the next chunk of data.
> diff --git a/block/backup.c b/block/backup.c
> index 5387fbd..9ca1d8e 100644
> --- a/block/backup.c
> +++ b/block/backup.c
> @@ -208,7 +208,7 @@ static void backup_set_speed(BlockJob *job, int64_t
> speed, Error **errp)
> error_setg(errp, QERR_INVALID_PARAMETER, "speed");
> return;
> }
> -ratelimit_set_speed(&s->limit, speed / BDRV_SECTOR_SIZE, SLICE_TIME);
> +ratelimit_set_speed(&s->limit, speed, SLICE_TIME);
> }
>
> static void backup_cleanup_sync_bitmap(BackupBlockJob *job, int ret)
> @@ -359,7 +359,8 @@ static bool coroutine_fn yield_and_check(BackupBlockJob
> *job)
> */
> if (job->common.speed) {
> uint64_t delay_ns = ratelimit_calculate_delay(&job->limit,
> - job->sectors_read);
> + job->sectors_read *
> + BDRV_SECTOR_SIZE);
> job->sectors_read = 0;
> block_job_sleep_ns(&job->common, QEMU_CLOCK_REALTIME, delay_ns);
> } else {
> diff --git a/block/commit.c b/block/commit.c
> index 524bd54..6993994 100644
> --- a/block/commit.c
> +++ b/block/commit.c
> @@ -209,7 +209,8 @@ static void coroutine_fn commit_run(void *opaque)
> s->common.offset += n * BDRV_SECTOR_SIZE;
>
> if (copy && s->common.speed) {
> -delay_ns = ratelimit_calculate_delay(&s->limit, n);
> +delay_ns = ratelimit_calculate_delay(&s->limit,
> + n * BDRV_SECTOR_SIZE);
> }
> }
>
> @@ -231,7 +232,7 @@ static void commit_set_speed(BlockJob *job, int64_t
> speed, Error **errp)
> error_setg(errp, QERR_INVALID_PARAMETER, "speed");
> return;
> }
> -ratelimit_set_speed(&s->limit, speed / BDRV_SECTOR_SIZE, SLICE_TIME);
> +ratelimit_set_speed(&s->limit, speed, SLICE_TIME);
> }
>
> static const BlockJobDriver commit_job_driver = {
> diff --git a/block/mirror.c b/block/mirror.c
> index 61a862d..eb27efc 100644
> --- a/block/mirror.c
> +++ b/block/mirror.c
> @@ -396,7 +396,8 @@ static uint64_t coroutine_fn
> mirror_iteration(MirrorBlockJob *s)
> bitmap_set(s->in_flight_bitmap, sector_num / sectors_per_chunk,
> nb_chunks);
> while (nb_chunks > 0 && sector_num < end) {
> int64_t ret;
> -int io_sectors, io_sectors_acct;
> +int io_sectors;
> +int64_t io_bytes_acct;
> BlockDriverState *file;
> enum MirrorMethod {
> MIRROR_METHOD_COPY,
> @@ -444,16 +445,16 @@ static uint64_t coroutine_fn
> mirror_iteration(MirrorBlockJob *s)
> switch (mirror_method) {
> case MIRROR_METHOD_COPY:
> io_sectors = mirror_do_read(s, sector_num, io_sectors);
> -io_sectors_acct
Re: [Qemu-devel] [PATCH v3 02/20] trace: Show blockjob actions via bytes, not sectors
On Tue, Jun 27, 2017 at 02:24:40PM -0500, Eric Blake wrote:
> Upcoming patches are going to switch to byte-based interfaces
> instead of sector-based. Even worse, trace_backup_do_cow_enter()
> had a weird mix of cluster and sector indices.
>
> The trace interface is low enough that there are no stability
> guarantees, and therefore nothing wrong with changing our units,
> even in cases like trace_backup_do_cow_skip() where we are not
> changing the trace output. So make the tracing uniformly use
> bytes.
>
> Signed-off-by: Eric Blake
> Reviewed-by: John Snow
>
Reviewed-by: Jeff Cody
> ---
> v2: improve commit message, no code change
> ---
> block/backup.c | 16 ++--
> block/commit.c | 3 ++-
> block/mirror.c | 26 +-
> block/stream.c | 3 ++-
> block/trace-events | 14 +++---
> 5 files changed, 38 insertions(+), 24 deletions(-)
>
> diff --git a/block/backup.c b/block/backup.c
> index 9ca1d8e..06431ac 100644
> --- a/block/backup.c
> +++ b/block/backup.c
> @@ -102,6 +102,7 @@ static int coroutine_fn backup_do_cow(BackupBlockJob *job,
> void *bounce_buffer = NULL;
> int ret = 0;
> int64_t sectors_per_cluster = cluster_size_sectors(job);
> +int64_t bytes_per_cluster = sectors_per_cluster * BDRV_SECTOR_SIZE;
> int64_t start, end;
> int n;
>
> @@ -110,18 +111,20 @@ static int coroutine_fn backup_do_cow(BackupBlockJob
> *job,
> start = sector_num / sectors_per_cluster;
> end = DIV_ROUND_UP(sector_num + nb_sectors, sectors_per_cluster);
>
> -trace_backup_do_cow_enter(job, start, sector_num, nb_sectors);
> +trace_backup_do_cow_enter(job, start * bytes_per_cluster,
> + sector_num * BDRV_SECTOR_SIZE,
> + nb_sectors * BDRV_SECTOR_SIZE);
>
> wait_for_overlapping_requests(job, start, end);
> cow_request_begin(&cow_request, job, start, end);
>
> for (; start < end; start++) {
> if (test_bit(start, job->done_bitmap)) {
> -trace_backup_do_cow_skip(job, start);
> +trace_backup_do_cow_skip(job, start * bytes_per_cluster);
> continue; /* already copied */
> }
>
> -trace_backup_do_cow_process(job, start);
> +trace_backup_do_cow_process(job, start * bytes_per_cluster);
>
> n = MIN(sectors_per_cluster,
> job->common.len / BDRV_SECTOR_SIZE -
> @@ -138,7 +141,7 @@ static int coroutine_fn backup_do_cow(BackupBlockJob *job,
> bounce_qiov.size, &bounce_qiov,
> is_write_notifier ? BDRV_REQ_NO_SERIALISING : 0);
> if (ret < 0) {
> -trace_backup_do_cow_read_fail(job, start, ret);
> +trace_backup_do_cow_read_fail(job, start * bytes_per_cluster,
> ret);
> if (error_is_read) {
> *error_is_read = true;
> }
> @@ -154,7 +157,7 @@ static int coroutine_fn backup_do_cow(BackupBlockJob *job,
> job->compress ? BDRV_REQ_WRITE_COMPRESSED :
> 0);
> }
> if (ret < 0) {
> -trace_backup_do_cow_write_fail(job, start, ret);
> +trace_backup_do_cow_write_fail(job, start * bytes_per_cluster,
> ret);
> if (error_is_read) {
> *error_is_read = false;
> }
> @@ -177,7 +180,8 @@ out:
>
> cow_request_end(&cow_request);
>
> -trace_backup_do_cow_return(job, sector_num, nb_sectors, ret);
> +trace_backup_do_cow_return(job, sector_num * BDRV_SECTOR_SIZE,
> + nb_sectors * BDRV_SECTOR_SIZE, ret);
>
> qemu_co_rwlock_unlock(&job->flush_rwlock);
>
> diff --git a/block/commit.c b/block/commit.c
> index 6993994..4cda7f2 100644
> --- a/block/commit.c
> +++ b/block/commit.c
> @@ -190,7 +190,8 @@ static void coroutine_fn commit_run(void *opaque)
>COMMIT_BUFFER_SIZE / BDRV_SECTOR_SIZE,
>&n);
> copy = (ret == 1);
> -trace_commit_one_iteration(s, sector_num, n, ret);
> +trace_commit_one_iteration(s, sector_num * BDRV_SECTOR_SIZE,
> + n * BDRV_SECTOR_SIZE, ret);
> if (copy) {
> ret = commit_populate(s->top, s->base, sector_num, n, buf);
> bytes_written += n * BDRV_SECTOR_SIZE;
> diff --git a/block/mirror.c b/block/mirror.c
> index eb27efc..b4dfe95 100644
> --- a/block/mirror.c
> +++ b/block/mirror.c
> @@ -103,7 +103,8 @@ static void
Re: [Qemu-devel] [PATCH v3 03/20] stream: Switch stream_populate() to byte-based
On Tue, Jun 27, 2017 at 02:24:41PM -0500, Eric Blake wrote:
> We are gradually converting to byte-based interfaces, as they are
> easier to reason about than sector-based. Start by converting an
> internal function (no semantic change).
>
> Signed-off-by: Eric Blake
> Reviewed-by: John Snow
>
Reviewed-by: Jeff Cody
> ---
> v2: no change
> ---
> block/stream.c | 11 ++-
> 1 file changed, 6 insertions(+), 5 deletions(-)
>
> diff --git a/block/stream.c b/block/stream.c
> index 6cb3939..746d525 100644
> --- a/block/stream.c
> +++ b/block/stream.c
> @@ -41,20 +41,20 @@ typedef struct StreamBlockJob {
> } StreamBlockJob;
>
> static int coroutine_fn stream_populate(BlockBackend *blk,
> -int64_t sector_num, int nb_sectors,
> +int64_t offset, uint64_t bytes,
> void *buf)
> {
> struct iovec iov = {
> .iov_base = buf,
> -.iov_len = nb_sectors * BDRV_SECTOR_SIZE,
> +.iov_len = bytes,
> };
> QEMUIOVector qiov;
>
> +assert(bytes < SIZE_MAX);
> qemu_iovec_init_external(&qiov, &iov, 1);
>
> /* Copy-on-read the unallocated clusters */
> -return blk_co_preadv(blk, sector_num * BDRV_SECTOR_SIZE, qiov.size,
> &qiov,
> - BDRV_REQ_COPY_ON_READ);
> +return blk_co_preadv(blk, offset, qiov.size, &qiov,
> BDRV_REQ_COPY_ON_READ);
> }
>
> typedef struct {
> @@ -171,7 +171,8 @@ static void coroutine_fn stream_run(void *opaque)
> trace_stream_one_iteration(s, sector_num * BDRV_SECTOR_SIZE,
> n * BDRV_SECTOR_SIZE, ret);
> if (copy) {
> -ret = stream_populate(blk, sector_num, n, buf);
> +ret = stream_populate(blk, sector_num * BDRV_SECTOR_SIZE,
> + n * BDRV_SECTOR_SIZE, buf);
> }
> if (ret < 0) {
> BlockErrorAction action =
> --
> 2.9.4
>
Re: [Qemu-devel] [PATCH v3 04/20] stream: Switch stream_run() to byte-based
On Tue, Jun 27, 2017 at 02:24:42PM -0500, Eric Blake wrote:
> We are gradually converting to byte-based interfaces, as they are
> easier to reason about than sector-based. Change the internal
> loop iteration of streaming to track by bytes instead of sectors
> (although we are still guaranteed that we iterate by steps that
> are sector-aligned).
>
> Signed-off-by: Eric Blake
> Reviewed-by: John Snow
>
Reviewed-by: Jeff Cody
> ---
> v2: no change
> ---
> block/stream.c | 24 ++--
> 1 file changed, 10 insertions(+), 14 deletions(-)
>
> diff --git a/block/stream.c b/block/stream.c
> index 746d525..2f9618b 100644
> --- a/block/stream.c
> +++ b/block/stream.c
> @@ -108,12 +108,11 @@ static void coroutine_fn stream_run(void *opaque)
> BlockBackend *blk = s->common.blk;
> BlockDriverState *bs = blk_bs(blk);
> BlockDriverState *base = s->base;
> -int64_t sector_num = 0;
> -int64_t end = -1;
> +int64_t offset = 0;
> uint64_t delay_ns = 0;
> int error = 0;
> int ret = 0;
> -int n = 0;
> +int n = 0; /* sectors */
> void *buf;
>
> if (!bs->backing) {
> @@ -126,7 +125,6 @@ static void coroutine_fn stream_run(void *opaque)
> goto out;
> }
>
> -end = s->common.len >> BDRV_SECTOR_BITS;
> buf = qemu_blockalign(bs, STREAM_BUFFER_SIZE);
>
> /* Turn on copy-on-read for the whole block device so that guest read
> @@ -138,7 +136,7 @@ static void coroutine_fn stream_run(void *opaque)
> bdrv_enable_copy_on_read(bs);
> }
>
> -for (sector_num = 0; sector_num < end; sector_num += n) {
> +for ( ; offset < s->common.len; offset += n * BDRV_SECTOR_SIZE) {
> bool copy;
>
> /* Note that even when no rate limit is applied we need to yield
> @@ -151,28 +149,26 @@ static void coroutine_fn stream_run(void *opaque)
>
> copy = false;
>
> -ret = bdrv_is_allocated(bs, sector_num,
> +ret = bdrv_is_allocated(bs, offset / BDRV_SECTOR_SIZE,
> STREAM_BUFFER_SIZE / BDRV_SECTOR_SIZE, &n);
> if (ret == 1) {
> /* Allocated in the top, no need to copy. */
> } else if (ret >= 0) {
> /* Copy if allocated in the intermediate images. Limit to the
> - * known-unallocated area [sector_num, sector_num+n). */
> + * known-unallocated area [offset, offset+n*BDRV_SECTOR_SIZE).
> */
> ret = bdrv_is_allocated_above(backing_bs(bs), base,
> - sector_num, n, &n);
> + offset / BDRV_SECTOR_SIZE, n, &n);
>
> /* Finish early if end of backing file has been reached */
> if (ret == 0 && n == 0) {
> -n = end - sector_num;
> +n = (s->common.len - offset) / BDRV_SECTOR_SIZE;
> }
>
> copy = (ret == 1);
> }
> -trace_stream_one_iteration(s, sector_num * BDRV_SECTOR_SIZE,
> - n * BDRV_SECTOR_SIZE, ret);
> +trace_stream_one_iteration(s, offset, n * BDRV_SECTOR_SIZE, ret);
> if (copy) {
> -ret = stream_populate(blk, sector_num * BDRV_SECTOR_SIZE,
> - n * BDRV_SECTOR_SIZE, buf);
> +ret = stream_populate(blk, offset, n * BDRV_SECTOR_SIZE, buf);
> }
> if (ret < 0) {
> BlockErrorAction action =
> @@ -211,7 +207,7 @@ out:
> /* Modify backing chain and close BDSes in main loop */
> data = g_malloc(sizeof(*data));
> data->ret = ret;
> -data->reached_end = sector_num == end;
> +data->reached_end = offset == s->common.len;
> block_job_defer_to_main_loop(&s->common, stream_complete, data);
> }
>
> --
> 2.9.4
>
Re: [Qemu-devel] [PATCH v3 05/20] commit: Switch commit_populate() to byte-based
On Tue, Jun 27, 2017 at 02:24:43PM -0500, Eric Blake wrote:
> We are gradually converting to byte-based interfaces, as they are
> easier to reason about than sector-based. Start by converting an
> internal function (no semantic change).
>
> Signed-off-by: Eric Blake
> Reviewed-by: John Snow
>
Reviewed-by: Jeff Cody
> ---
> v2: no change
> ---
> block/commit.c | 15 ---
> 1 file changed, 8 insertions(+), 7 deletions(-)
>
> diff --git a/block/commit.c b/block/commit.c
> index 4cda7f2..6f67d78 100644
> --- a/block/commit.c
> +++ b/block/commit.c
> @@ -47,26 +47,25 @@ typedef struct CommitBlockJob {
> } CommitBlockJob;
>
> static int coroutine_fn commit_populate(BlockBackend *bs, BlockBackend *base,
> -int64_t sector_num, int nb_sectors,
> +int64_t offset, uint64_t bytes,
> void *buf)
> {
> int ret = 0;
> QEMUIOVector qiov;
> struct iovec iov = {
> .iov_base = buf,
> -.iov_len = nb_sectors * BDRV_SECTOR_SIZE,
> +.iov_len = bytes,
> };
>
> +assert(bytes < SIZE_MAX);
> qemu_iovec_init_external(&qiov, &iov, 1);
>
> -ret = blk_co_preadv(bs, sector_num * BDRV_SECTOR_SIZE,
> -qiov.size, &qiov, 0);
> +ret = blk_co_preadv(bs, offset, qiov.size, &qiov, 0);
> if (ret < 0) {
> return ret;
> }
>
> -ret = blk_co_pwritev(base, sector_num * BDRV_SECTOR_SIZE,
> - qiov.size, &qiov, 0);
> +ret = blk_co_pwritev(base, offset, qiov.size, &qiov, 0);
> if (ret < 0) {
> return ret;
> }
> @@ -193,7 +192,9 @@ static void coroutine_fn commit_run(void *opaque)
> trace_commit_one_iteration(s, sector_num * BDRV_SECTOR_SIZE,
> n * BDRV_SECTOR_SIZE, ret);
> if (copy) {
> -ret = commit_populate(s->top, s->base, sector_num, n, buf);
> +ret = commit_populate(s->top, s->base,
> + sector_num * BDRV_SECTOR_SIZE,
> + n * BDRV_SECTOR_SIZE, buf);
> bytes_written += n * BDRV_SECTOR_SIZE;
> }
> if (ret < 0) {
> --
> 2.9.4
>
Re: [Qemu-devel] [PATCH v3 06/20] commit: Switch commit_run() to byte-based
On Tue, Jun 27, 2017 at 02:24:44PM -0500, Eric Blake wrote:
> We are gradually converting to byte-based interfaces, as they are
> easier to reason about than sector-based. Change the internal
> loop iteration of committing to track by bytes instead of sectors
> (although we are still guaranteed that we iterate by steps that
> are sector-aligned).
>
> Signed-off-by: Eric Blake
> Reviewed-by: John Snow
>
Reviewed-by: Jeff Cody
> ---
> v2: no change
> ---
> block/commit.c | 16 ++--
> 1 file changed, 6 insertions(+), 10 deletions(-)
>
> diff --git a/block/commit.c b/block/commit.c
> index 6f67d78..c3a7bca 100644
> --- a/block/commit.c
> +++ b/block/commit.c
> @@ -143,17 +143,16 @@ static void coroutine_fn commit_run(void *opaque)
> {
> CommitBlockJob *s = opaque;
> CommitCompleteData *data;
> -int64_t sector_num, end;
> +int64_t offset;
> uint64_t delay_ns = 0;
> int ret = 0;
> -int n = 0;
> +int n = 0; /* sectors */
> void *buf = NULL;
> int bytes_written = 0;
> int64_t base_len;
>
> ret = s->common.len = blk_getlength(s->top);
>
> -
> if (s->common.len < 0) {
> goto out;
> }
> @@ -170,10 +169,9 @@ static void coroutine_fn commit_run(void *opaque)
> }
> }
>
> -end = s->common.len >> BDRV_SECTOR_BITS;
> buf = blk_blockalign(s->top, COMMIT_BUFFER_SIZE);
>
> -for (sector_num = 0; sector_num < end; sector_num += n) {
> +for (offset = 0; offset < s->common.len; offset += n * BDRV_SECTOR_SIZE)
> {
> bool copy;
>
> /* Note that even when no rate limit is applied we need to yield
> @@ -185,15 +183,13 @@ static void coroutine_fn commit_run(void *opaque)
> }
> /* Copy if allocated above the base */
> ret = bdrv_is_allocated_above(blk_bs(s->top), blk_bs(s->base),
> - sector_num,
> + offset / BDRV_SECTOR_SIZE,
>COMMIT_BUFFER_SIZE / BDRV_SECTOR_SIZE,
>&n);
> copy = (ret == 1);
> -trace_commit_one_iteration(s, sector_num * BDRV_SECTOR_SIZE,
> - n * BDRV_SECTOR_SIZE, ret);
> +trace_commit_one_iteration(s, offset, n * BDRV_SECTOR_SIZE, ret);
> if (copy) {
> -ret = commit_populate(s->top, s->base,
> - sector_num * BDRV_SECTOR_SIZE,
> +ret = commit_populate(s->top, s->base, offset,
>n * BDRV_SECTOR_SIZE, buf);
> bytes_written += n * BDRV_SECTOR_SIZE;
> }
> --
> 2.9.4
>
Re: [Qemu-devel] [PATCH v3 07/20] mirror: Switch MirrorBlockJob to byte-based
On Tue, Jun 27, 2017 at 02:24:45PM -0500, Eric Blake wrote:
> We are gradually converting to byte-based interfaces, as they are
> easier to reason about than sector-based. Continue by converting an
> internal structure (no semantic change), and all references to the
> buffer size.
>
> [checkpatch has a false positive on use of MIN() in this patch]
>
> Signed-off-by: Eric Blake
> Reviewed-by: John Snow
>
Reviewed-by: Jeff Cody
> ---
> v2: no change
> ---
> block/mirror.c | 79
> --
> 1 file changed, 38 insertions(+), 41 deletions(-)
>
> diff --git a/block/mirror.c b/block/mirror.c
> index b4dfe95..9e28d59 100644
> --- a/block/mirror.c
> +++ b/block/mirror.c
> @@ -24,9 +24,8 @@
>
> #define SLICE_TIME1ULL /* ns */
> #define MAX_IN_FLIGHT 16
> -#define MAX_IO_SECTORS ((1 << 20) >> BDRV_SECTOR_BITS) /* 1 Mb */
> -#define DEFAULT_MIRROR_BUF_SIZE \
> -(MAX_IN_FLIGHT * MAX_IO_SECTORS * BDRV_SECTOR_SIZE)
> +#define MAX_IO_BYTES (1 << 20) /* 1 Mb */
> +#define DEFAULT_MIRROR_BUF_SIZE (MAX_IN_FLIGHT * MAX_IO_BYTES)
>
> /* The mirroring buffer is a list of granularity-sized chunks.
> * Free chunks are organized in a list.
> @@ -67,11 +66,11 @@ typedef struct MirrorBlockJob {
> uint64_t last_pause_ns;
> unsigned long *in_flight_bitmap;
> int in_flight;
> -int64_t sectors_in_flight;
> +int64_t bytes_in_flight;
> int ret;
> bool unmap;
> bool waiting_for_io;
> -int target_cluster_sectors;
> +int target_cluster_size;
> int max_iov;
> bool initial_zeroing_ongoing;
> } MirrorBlockJob;
> @@ -79,8 +78,8 @@ typedef struct MirrorBlockJob {
> typedef struct MirrorOp {
> MirrorBlockJob *s;
> QEMUIOVector qiov;
> -int64_t sector_num;
> -int nb_sectors;
> +int64_t offset;
> +uint64_t bytes;
> } MirrorOp;
>
> static BlockErrorAction mirror_error_action(MirrorBlockJob *s, bool read,
> @@ -101,13 +100,12 @@ static void mirror_iteration_done(MirrorOp *op, int ret)
> MirrorBlockJob *s = op->s;
> struct iovec *iov;
> int64_t chunk_num;
> -int i, nb_chunks, sectors_per_chunk;
> +int i, nb_chunks;
>
> -trace_mirror_iteration_done(s, op->sector_num * BDRV_SECTOR_SIZE,
> -op->nb_sectors * BDRV_SECTOR_SIZE, ret);
> +trace_mirror_iteration_done(s, op->offset, op->bytes, ret);
>
> s->in_flight--;
> -s->sectors_in_flight -= op->nb_sectors;
> +s->bytes_in_flight -= op->bytes;
> iov = op->qiov.iov;
> for (i = 0; i < op->qiov.niov; i++) {
> MirrorBuffer *buf = (MirrorBuffer *) iov[i].iov_base;
> @@ -115,16 +113,15 @@ static void mirror_iteration_done(MirrorOp *op, int ret)
> s->buf_free_count++;
> }
>
> -sectors_per_chunk = s->granularity >> BDRV_SECTOR_BITS;
> -chunk_num = op->sector_num / sectors_per_chunk;
> -nb_chunks = DIV_ROUND_UP(op->nb_sectors, sectors_per_chunk);
> +chunk_num = op->offset / s->granularity;
> +nb_chunks = DIV_ROUND_UP(op->bytes, s->granularity);
> bitmap_clear(s->in_flight_bitmap, chunk_num, nb_chunks);
> if (ret >= 0) {
> if (s->cow_bitmap) {
> bitmap_set(s->cow_bitmap, chunk_num, nb_chunks);
> }
> if (!s->initial_zeroing_ongoing) {
> -s->common.offset += (uint64_t)op->nb_sectors * BDRV_SECTOR_SIZE;
> +s->common.offset += op->bytes;
> }
> }
> qemu_iovec_destroy(&op->qiov);
> @@ -144,7 +141,8 @@ static void mirror_write_complete(void *opaque, int ret)
> if (ret < 0) {
> BlockErrorAction action;
>
> -bdrv_set_dirty_bitmap(s->dirty_bitmap, op->sector_num,
> op->nb_sectors);
> +bdrv_set_dirty_bitmap(s->dirty_bitmap, op->offset >>
> BDRV_SECTOR_BITS,
> + op->bytes >> BDRV_SECTOR_BITS);
> action = mirror_error_action(s, false, -ret);
> if (action == BLOCK_ERROR_ACTION_REPORT && s->ret >= 0) {
> s->ret = ret;
> @@ -163,7 +161,8 @@ static void mirror_read_complete(void *opaque, int ret)
> if (ret < 0) {
> BlockErrorAction action;
>
> -bdrv_set_dirty_bitmap(s->dirty_bitmap, op->sector_num,
> op->nb_sectors);
> +bdrv_set_dirty_bitmap(s->dirty_bitmap, op->offset >>
> BDRV_SECTOR_BITS,
> + op->bytes >> BDRV_SECTOR_BITS);
> act
Re: [Qemu-devel] [PATCH v3 08/20] mirror: Switch mirror_do_zero_or_discard() to byte-based
On Tue, Jun 27, 2017 at 02:24:46PM -0500, Eric Blake wrote:
> We are gradually converting to byte-based interfaces, as they are
> easier to reason about than sector-based. Convert another internal
> function (no semantic change).
>
> Signed-off-by: Eric Blake
> Reviewed-by: John Snow
>
Reviewed-by: Jeff Cody
> ---
> v2: no change
> ---
> block/mirror.c | 20 +++-
> 1 file changed, 11 insertions(+), 9 deletions(-)
>
> diff --git a/block/mirror.c b/block/mirror.c
> index 9e28d59..af27bcc 100644
> --- a/block/mirror.c
> +++ b/block/mirror.c
> @@ -305,8 +305,8 @@ static int mirror_do_read(MirrorBlockJob *s, int64_t
> sector_num,
> }
>
> static void mirror_do_zero_or_discard(MirrorBlockJob *s,
> - int64_t sector_num,
> - int nb_sectors,
> + int64_t offset,
> + uint64_t bytes,
>bool is_discard)
> {
> MirrorOp *op;
> @@ -315,16 +315,16 @@ static void mirror_do_zero_or_discard(MirrorBlockJob *s,
> * so the freeing in mirror_iteration_done is nop. */
> op = g_new0(MirrorOp, 1);
> op->s = s;
> -op->offset = sector_num * BDRV_SECTOR_SIZE;
> -op->bytes = nb_sectors * BDRV_SECTOR_SIZE;
> +op->offset = offset;
> +op->bytes = bytes;
>
> s->in_flight++;
> -s->bytes_in_flight += nb_sectors * BDRV_SECTOR_SIZE;
> +s->bytes_in_flight += bytes;
> if (is_discard) {
> -blk_aio_pdiscard(s->target, sector_num << BDRV_SECTOR_BITS,
> +blk_aio_pdiscard(s->target, offset,
> op->bytes, mirror_write_complete, op);
> } else {
> -blk_aio_pwrite_zeroes(s->target, sector_num * BDRV_SECTOR_SIZE,
> +blk_aio_pwrite_zeroes(s->target, offset,
>op->bytes, s->unmap ? BDRV_REQ_MAY_UNMAP : 0,
>mirror_write_complete, op);
> }
> @@ -453,7 +453,8 @@ static uint64_t coroutine_fn
> mirror_iteration(MirrorBlockJob *s)
> break;
> case MIRROR_METHOD_ZERO:
> case MIRROR_METHOD_DISCARD:
> -mirror_do_zero_or_discard(s, sector_num, io_sectors,
> +mirror_do_zero_or_discard(s, sector_num * BDRV_SECTOR_SIZE,
> + io_sectors * BDRV_SECTOR_SIZE,
>mirror_method ==
> MIRROR_METHOD_DISCARD);
> if (write_zeroes_ok) {
> io_bytes_acct = 0;
> @@ -657,7 +658,8 @@ static int coroutine_fn mirror_dirty_init(MirrorBlockJob
> *s)
> continue;
> }
>
> -mirror_do_zero_or_discard(s, sector_num, nb_sectors, false);
> +mirror_do_zero_or_discard(s, sector_num * BDRV_SECTOR_SIZE,
> + nb_sectors * BDRV_SECTOR_SIZE, false);
> sector_num += nb_sectors;
> }
>
> --
> 2.9.4
>
Re: [Qemu-devel] [PATCH v3 09/20] mirror: Update signature of mirror_clip_sectors()
On Tue, Jun 27, 2017 at 02:24:47PM -0500, Eric Blake wrote:
> Rather than having a void function that modifies its input
> in-place as the output, change the signature to reduce a layer
> of indirection and return the result.
>
> Suggested-by: John Snow
> Signed-off-by: Eric Blake
> Reviewed-by: John Snow
>
Reviewed-by: Jeff Cody
> ---
> v2: new patch
> ---
> block/mirror.c | 15 ---
> 1 file changed, 8 insertions(+), 7 deletions(-)
>
> diff --git a/block/mirror.c b/block/mirror.c
> index af27bcc..1a43304 100644
> --- a/block/mirror.c
> +++ b/block/mirror.c
> @@ -176,12 +176,12 @@ static void mirror_read_complete(void *opaque, int ret)
> aio_context_release(blk_get_aio_context(s->common.blk));
> }
>
> -static inline void mirror_clip_sectors(MirrorBlockJob *s,
> - int64_t sector_num,
> - int *nb_sectors)
> +static inline int mirror_clip_sectors(MirrorBlockJob *s,
> + int64_t sector_num,
> + int nb_sectors)
> {
> -*nb_sectors = MIN(*nb_sectors,
> - s->bdev_length / BDRV_SECTOR_SIZE - sector_num);
> +return MIN(nb_sectors,
> + s->bdev_length / BDRV_SECTOR_SIZE - sector_num);
> }
>
> /* Round sector_num and/or nb_sectors to target cluster if COW is needed, and
> @@ -216,7 +216,8 @@ static int mirror_cow_align(MirrorBlockJob *s,
> }
> /* Clipping may result in align_nb_sectors unaligned to chunk boundary,
> but
> * that doesn't matter because it's already the end of source image. */
> -mirror_clip_sectors(s, align_sector_num, &align_nb_sectors);
> +align_nb_sectors = mirror_clip_sectors(s, align_sector_num,
> + align_nb_sectors);
>
> ret = align_sector_num + align_nb_sectors - (*sector_num + *nb_sectors);
> *sector_num = align_sector_num;
> @@ -445,7 +446,7 @@ static uint64_t coroutine_fn
> mirror_iteration(MirrorBlockJob *s)
> return 0;
> }
>
> -mirror_clip_sectors(s, sector_num, &io_sectors);
> +io_sectors = mirror_clip_sectors(s, sector_num, io_sectors);
> switch (mirror_method) {
> case MIRROR_METHOD_COPY:
> io_sectors = mirror_do_read(s, sector_num, io_sectors);
> --
> 2.9.4
>
Re: [Qemu-devel] [PATCH v3 10/20] mirror: Switch mirror_cow_align() to byte-based
On Tue, Jun 27, 2017 at 02:24:48PM -0500, Eric Blake wrote:
> We are gradually converting to byte-based interfaces, as they are
> easier to reason about than sector-based. Convert another internal
> function (no semantic change), and add mirror_clip_bytes() as a
> counterpart to mirror_clip_sectors(). Some of the conversion is
> a bit tricky, requiring temporaries to convert between units; it
> will be cleared up in a following patch.
>
> Signed-off-by: Eric Blake
> Reviewed-by: John Snow
>
Reviewed-by: Jeff Cody
> ---
> v2: tweak mirror_clip_bytes() signature to match previous patch
> ---
> block/mirror.c | 64
> ++
> 1 file changed, 38 insertions(+), 26 deletions(-)
>
> diff --git a/block/mirror.c b/block/mirror.c
> index 1a43304..1a4602a 100644
> --- a/block/mirror.c
> +++ b/block/mirror.c
> @@ -176,6 +176,15 @@ static void mirror_read_complete(void *opaque, int ret)
> aio_context_release(blk_get_aio_context(s->common.blk));
> }
>
> +/* Clip bytes relative to offset to not exceed end-of-file */
> +static inline int64_t mirror_clip_bytes(MirrorBlockJob *s,
> +int64_t offset,
> +int64_t bytes)
> +{
> +return MIN(bytes, s->bdev_length - offset);
> +}
> +
> +/* Clip nb_sectors relative to sector_num to not exceed end-of-file */
> static inline int mirror_clip_sectors(MirrorBlockJob *s,
>int64_t sector_num,
>int nb_sectors)
> @@ -184,44 +193,39 @@ static inline int mirror_clip_sectors(MirrorBlockJob *s,
> s->bdev_length / BDRV_SECTOR_SIZE - sector_num);
> }
>
> -/* Round sector_num and/or nb_sectors to target cluster if COW is needed, and
> - * return the offset of the adjusted tail sector against original. */
> -static int mirror_cow_align(MirrorBlockJob *s,
> -int64_t *sector_num,
> -int *nb_sectors)
> +/* Round offset and/or bytes to target cluster if COW is needed, and
> + * return the offset of the adjusted tail against original. */
> +static int mirror_cow_align(MirrorBlockJob *s, int64_t *offset,
> +unsigned int *bytes)
> {
> bool need_cow;
> int ret = 0;
> -int chunk_sectors = s->granularity >> BDRV_SECTOR_BITS;
> -int64_t align_sector_num = *sector_num;
> -int align_nb_sectors = *nb_sectors;
> -int max_sectors = chunk_sectors * s->max_iov;
> +int64_t align_offset = *offset;
> +unsigned int align_bytes = *bytes;
> +int max_bytes = s->granularity * s->max_iov;
>
> -need_cow = !test_bit(*sector_num / chunk_sectors, s->cow_bitmap);
> -need_cow |= !test_bit((*sector_num + *nb_sectors - 1) / chunk_sectors,
> +need_cow = !test_bit(*offset / s->granularity, s->cow_bitmap);
> +need_cow |= !test_bit((*offset + *bytes - 1) / s->granularity,
>s->cow_bitmap);
> if (need_cow) {
> -bdrv_round_sectors_to_clusters(blk_bs(s->target), *sector_num,
> - *nb_sectors, &align_sector_num,
> - &align_nb_sectors);
> +bdrv_round_to_clusters(blk_bs(s->target), *offset, *bytes,
> + &align_offset, &align_bytes);
> }
>
> -if (align_nb_sectors > max_sectors) {
> -align_nb_sectors = max_sectors;
> +if (align_bytes > max_bytes) {
> +align_bytes = max_bytes;
> if (need_cow) {
> -align_nb_sectors = QEMU_ALIGN_DOWN(align_nb_sectors,
> - s->target_cluster_size >>
> - BDRV_SECTOR_BITS);
> +align_bytes = QEMU_ALIGN_DOWN(align_bytes,
> + s->target_cluster_size);
> }
> }
> -/* Clipping may result in align_nb_sectors unaligned to chunk boundary,
> but
> +/* Clipping may result in align_bytes unaligned to chunk boundary, but
> * that doesn't matter because it's already the end of source image. */
> -align_nb_sectors = mirror_clip_sectors(s, align_sector_num,
> - align_nb_sectors);
> +align_bytes = mirror_clip_bytes(s, align_offset, align_bytes);
>
> -ret = align_sector_num + align_nb_sectors - (*sector_num + *nb_sectors);
> -*sector_num = align_sector_num;
> -*nb_sectors = align_nb_sectors;
> +ret = align_offset + align_bytes - (*offs
Re: [Qemu-devel] [PATCH v3 11/20] mirror: Switch mirror_do_read() to byte-based
On Tue, Jun 27, 2017 at 02:24:49PM -0500, Eric Blake wrote:
> We are gradually converting to byte-based interfaces, as they are
> easier to reason about than sector-based. Convert another internal
> function (no semantic change).
>
> Signed-off-by: Eric Blake
> Reviewed-by: John Snow
>
Reviewed-by: Jeff Cody
> ---
> v2: rebase to earlier changes
> ---
> block/mirror.c | 75
> ++
> 1 file changed, 33 insertions(+), 42 deletions(-)
>
> diff --git a/block/mirror.c b/block/mirror.c
> index 1a4602a..81ff784 100644
> --- a/block/mirror.c
> +++ b/block/mirror.c
> @@ -196,7 +196,7 @@ static inline int mirror_clip_sectors(MirrorBlockJob *s,
> /* Round offset and/or bytes to target cluster if COW is needed, and
> * return the offset of the adjusted tail against original. */
> static int mirror_cow_align(MirrorBlockJob *s, int64_t *offset,
> -unsigned int *bytes)
> +uint64_t *bytes)
> {
> bool need_cow;
> int ret = 0;
> @@ -204,6 +204,7 @@ static int mirror_cow_align(MirrorBlockJob *s, int64_t
> *offset,
> unsigned int align_bytes = *bytes;
> int max_bytes = s->granularity * s->max_iov;
>
> +assert(*bytes < INT_MAX);
> need_cow = !test_bit(*offset / s->granularity, s->cow_bitmap);
> need_cow |= !test_bit((*offset + *bytes - 1) / s->granularity,
>s->cow_bitmap);
> @@ -239,59 +240,50 @@ static inline void mirror_wait_for_io(MirrorBlockJob *s)
> }
>
> /* Submit async read while handling COW.
> - * Returns: The number of sectors copied after and including sector_num,
> - * excluding any sectors copied prior to sector_num due to
> alignment.
> - * This will be nb_sectors if no alignment is necessary, or
> - * (new_end - sector_num) if tail is rounded up or down due to
> + * Returns: The number of bytes copied after and including offset,
> + * excluding any bytes copied prior to offset due to alignment.
> + * This will be @bytes if no alignment is necessary, or
> + * (new_end - offset) if tail is rounded up or down due to
> * alignment or buffer limit.
> */
> -static int mirror_do_read(MirrorBlockJob *s, int64_t sector_num,
> - int nb_sectors)
> +static uint64_t mirror_do_read(MirrorBlockJob *s, int64_t offset,
> + uint64_t bytes)
> {
> BlockBackend *source = s->common.blk;
> -int sectors_per_chunk, nb_chunks;
> -int ret;
> +int nb_chunks;
> +uint64_t ret;
> MirrorOp *op;
> -int max_sectors;
> +uint64_t max_bytes;
>
> -sectors_per_chunk = s->granularity >> BDRV_SECTOR_BITS;
> -max_sectors = sectors_per_chunk * s->max_iov;
> +max_bytes = s->granularity * s->max_iov;
>
> /* We can only handle as much as buf_size at a time. */
> -nb_sectors = MIN(s->buf_size >> BDRV_SECTOR_BITS, nb_sectors);
> -nb_sectors = MIN(max_sectors, nb_sectors);
> -assert(nb_sectors);
> -assert(nb_sectors < BDRV_REQUEST_MAX_SECTORS);
> -ret = nb_sectors;
> +bytes = MIN(s->buf_size, MIN(max_bytes, bytes));
> +assert(bytes);
> +assert(bytes < BDRV_REQUEST_MAX_BYTES);
> +ret = bytes;
>
> if (s->cow_bitmap) {
> -int64_t offset = sector_num * BDRV_SECTOR_SIZE;
> -unsigned int bytes = nb_sectors * BDRV_SECTOR_SIZE;
> -int gap;
> -
> -gap = mirror_cow_align(s, &offset, &bytes);
> -sector_num = offset / BDRV_SECTOR_SIZE;
> -nb_sectors = bytes / BDRV_SECTOR_SIZE;
> -ret += gap / BDRV_SECTOR_SIZE;
> +ret += mirror_cow_align(s, &offset, &bytes);
> }
> -assert(nb_sectors << BDRV_SECTOR_BITS <= s->buf_size);
> -/* The sector range must meet granularity because:
> +assert(bytes <= s->buf_size);
> +/* The range will be sector-aligned because:
> * 1) Caller passes in aligned values;
> - * 2) mirror_cow_align is used only when target cluster is larger. */
> -assert(!(sector_num % sectors_per_chunk));
> -nb_chunks = DIV_ROUND_UP(nb_sectors, sectors_per_chunk);
> + * 2) mirror_cow_align is used only when target cluster is larger.
> + * But it might not be cluster-aligned at end-of-file. */
> +assert(QEMU_IS_ALIGNED(bytes, BDRV_SECTOR_SIZE));
> +nb_chunks = DIV_ROUND_UP(bytes, s->granularity);
>
> while (s->buf_free_count < nb_chunks) {
> -trace_mirror_yield_in_flight(s, sector_num * BDRV_SECTOR_SIZE,
> -
Re: [Qemu-devel] [PATCH v3 12/20] mirror: Switch mirror_iteration() to byte-based
On Tue, Jun 27, 2017 at 02:24:50PM -0500, Eric Blake wrote:
> We are gradually converting to byte-based interfaces, as they are
> easier to reason about than sector-based. Change the internal
> loop iteration of mirroring to track by bytes instead of sectors
> (although we are still guaranteed that we iterate by steps that
> are both sector-aligned and multiples of the granularity). Drop
> the now-unused mirror_clip_sectors().
>
> Signed-off-by: Eric Blake
> Reviewed-by: John Snow
>
Reviewed-by: Jeff Cody
> ---
> v3: rebase to Paolo's thread-safety changes, R-b kept
> v2: straightforward rebase to earlier mirror_clip_bytes() change, R-b kept
> ---
> block/mirror.c | 105
> +
> 1 file changed, 46 insertions(+), 59 deletions(-)
>
> diff --git a/block/mirror.c b/block/mirror.c
> index 81ff784..0eb2af4 100644
> --- a/block/mirror.c
> +++ b/block/mirror.c
> @@ -184,15 +184,6 @@ static inline int64_t mirror_clip_bytes(MirrorBlockJob
> *s,
> return MIN(bytes, s->bdev_length - offset);
> }
>
> -/* Clip nb_sectors relative to sector_num to not exceed end-of-file */
> -static inline int mirror_clip_sectors(MirrorBlockJob *s,
> - int64_t sector_num,
> - int nb_sectors)
> -{
> -return MIN(nb_sectors,
> - s->bdev_length / BDRV_SECTOR_SIZE - sector_num);
> -}
> -
> /* Round offset and/or bytes to target cluster if COW is needed, and
> * return the offset of the adjusted tail against original. */
> static int mirror_cow_align(MirrorBlockJob *s, int64_t *offset,
> @@ -336,30 +327,28 @@ static void mirror_do_zero_or_discard(MirrorBlockJob *s,
> static uint64_t coroutine_fn mirror_iteration(MirrorBlockJob *s)
> {
> BlockDriverState *source = s->source;
> -int64_t sector_num, first_chunk;
> +int64_t offset, first_chunk;
> uint64_t delay_ns = 0;
> /* At least the first dirty chunk is mirrored in one iteration. */
> int nb_chunks = 1;
> -int64_t end = s->bdev_length / BDRV_SECTOR_SIZE;
> int sectors_per_chunk = s->granularity >> BDRV_SECTOR_BITS;
> bool write_zeroes_ok =
> bdrv_can_write_zeroes_with_unmap(blk_bs(s->target));
> int max_io_bytes = MAX(s->buf_size / MAX_IN_FLIGHT, MAX_IO_BYTES);
>
> bdrv_dirty_bitmap_lock(s->dirty_bitmap);
> -sector_num = bdrv_dirty_iter_next(s->dbi);
> -if (sector_num < 0) {
> +offset = bdrv_dirty_iter_next(s->dbi) * BDRV_SECTOR_SIZE;
> +if (offset < 0) {
> bdrv_set_dirty_iter(s->dbi, 0);
> -sector_num = bdrv_dirty_iter_next(s->dbi);
> +offset = bdrv_dirty_iter_next(s->dbi) * BDRV_SECTOR_SIZE;
> trace_mirror_restart_iter(s, bdrv_get_dirty_count(s->dirty_bitmap) *
>BDRV_SECTOR_SIZE);
> -assert(sector_num >= 0);
> +assert(offset >= 0);
> }
> bdrv_dirty_bitmap_unlock(s->dirty_bitmap);
>
> -first_chunk = sector_num / sectors_per_chunk;
> +first_chunk = offset / s->granularity;
> while (test_bit(first_chunk, s->in_flight_bitmap)) {
> -trace_mirror_yield_in_flight(s, sector_num * BDRV_SECTOR_SIZE,
> - s->in_flight);
> +trace_mirror_yield_in_flight(s, offset, s->in_flight);
> mirror_wait_for_io(s);
> }
>
> @@ -368,25 +357,26 @@ static uint64_t coroutine_fn
> mirror_iteration(MirrorBlockJob *s)
> /* Find the number of consective dirty chunks following the first dirty
> * one, and wait for in flight requests in them. */
> bdrv_dirty_bitmap_lock(s->dirty_bitmap);
> -while (nb_chunks * sectors_per_chunk < (s->buf_size >>
> BDRV_SECTOR_BITS)) {
> +while (nb_chunks * s->granularity < s->buf_size) {
> int64_t next_dirty;
> -int64_t next_sector = sector_num + nb_chunks * sectors_per_chunk;
> -int64_t next_chunk = next_sector / sectors_per_chunk;
> -if (next_sector >= end ||
> -!bdrv_get_dirty_locked(source, s->dirty_bitmap, next_sector)) {
> +int64_t next_offset = offset + nb_chunks * s->granularity;
> +int64_t next_chunk = next_offset / s->granularity;
> +if (next_offset >= s->bdev_length ||
> +!bdrv_get_dirty_locked(source, s->dirty_bitmap,
> + next_offset >> BDRV_SECTOR_BITS)) {
> break;
> }
> if (test_bit(next_chunk, s->in_flight_bitmap)) {
> break;
>
Re: [Qemu-devel] [Qemu-block] [PATCH v3 13/20] block: Drop unused bdrv_round_sectors_to_clusters()
On Tue, Jun 27, 2017 at 02:24:51PM -0500, Eric Blake wrote:
> Now that the last user [mirror_iteration()] has converted to using
> bytes, we no longer need a function to round sectors to clusters.
>
> Signed-off-by: Eric Blake
> Reviewed-by: John Snow
>
Reviewed-by: Jeff Cody
> ---
> v2: hoist to earlier series, no change
> ---
> include/block/block.h | 4
> block/io.c| 21 -
> 2 files changed, 25 deletions(-)
>
> diff --git a/include/block/block.h b/include/block/block.h
> index 3e91cac..5cdd690 100644
> --- a/include/block/block.h
> +++ b/include/block/block.h
> @@ -473,10 +473,6 @@ const char *bdrv_get_device_or_node_name(const
> BlockDriverState *bs);
> int bdrv_get_flags(BlockDriverState *bs);
> int bdrv_get_info(BlockDriverState *bs, BlockDriverInfo *bdi);
> ImageInfoSpecific *bdrv_get_specific_info(BlockDriverState *bs);
> -void bdrv_round_sectors_to_clusters(BlockDriverState *bs,
> -int64_t sector_num, int nb_sectors,
> -int64_t *cluster_sector_num,
> -int *cluster_nb_sectors);
> void bdrv_round_to_clusters(BlockDriverState *bs,
> int64_t offset, unsigned int bytes,
> int64_t *cluster_offset,
> diff --git a/block/io.c b/block/io.c
> index c72d701..d9fec1f 100644
> --- a/block/io.c
> +++ b/block/io.c
> @@ -419,27 +419,6 @@ static void mark_request_serialising(BdrvTrackedRequest
> *req, uint64_t align)
> }
>
> /**
> - * Round a region to cluster boundaries (sector-based)
> - */
> -void bdrv_round_sectors_to_clusters(BlockDriverState *bs,
> -int64_t sector_num, int nb_sectors,
> -int64_t *cluster_sector_num,
> -int *cluster_nb_sectors)
> -{
> -BlockDriverInfo bdi;
> -
> -if (bdrv_get_info(bs, &bdi) < 0 || bdi.cluster_size == 0) {
> -*cluster_sector_num = sector_num;
> -*cluster_nb_sectors = nb_sectors;
> -} else {
> -int64_t c = bdi.cluster_size / BDRV_SECTOR_SIZE;
> -*cluster_sector_num = QEMU_ALIGN_DOWN(sector_num, c);
> -*cluster_nb_sectors = QEMU_ALIGN_UP(sector_num - *cluster_sector_num
> +
> -nb_sectors, c);
> -}
> -}
> -
> -/**
> * Round a region to cluster boundaries
> */
> void bdrv_round_to_clusters(BlockDriverState *bs,
> --
> 2.9.4
>
>
Re: [Qemu-devel] [PATCH v3 14/20] backup: Switch BackupBlockJob to byte-based
On Tue, Jun 27, 2017 at 02:24:52PM -0500, Eric Blake wrote:
> We are gradually converting to byte-based interfaces, as they are
> easier to reason about than sector-based. Continue by converting an
> internal structure (no semantic change), and all references to
> tracking progress. Drop a redundant local variable bytes_per_cluster.
>
> Signed-off-by: Eric Blake
> Reviewed-by: John Snow
>
Reviewed-by: Jeff Cody
> ---
> v2: no change
> ---
> block/backup.c | 33 +++--
> 1 file changed, 15 insertions(+), 18 deletions(-)
>
> diff --git a/block/backup.c b/block/backup.c
> index 06431ac..4e64710 100644
> --- a/block/backup.c
> +++ b/block/backup.c
> @@ -39,7 +39,7 @@ typedef struct BackupBlockJob {
> BlockdevOnError on_source_error;
> BlockdevOnError on_target_error;
> CoRwlock flush_rwlock;
> -uint64_t sectors_read;
> +uint64_t bytes_read;
> unsigned long *done_bitmap;
> int64_t cluster_size;
> bool compress;
> @@ -102,16 +102,15 @@ static int coroutine_fn backup_do_cow(BackupBlockJob
> *job,
> void *bounce_buffer = NULL;
> int ret = 0;
> int64_t sectors_per_cluster = cluster_size_sectors(job);
> -int64_t bytes_per_cluster = sectors_per_cluster * BDRV_SECTOR_SIZE;
> -int64_t start, end;
> -int n;
> +int64_t start, end; /* clusters */
> +int n; /* bytes */
>
> qemu_co_rwlock_rdlock(&job->flush_rwlock);
>
> start = sector_num / sectors_per_cluster;
> end = DIV_ROUND_UP(sector_num + nb_sectors, sectors_per_cluster);
>
> -trace_backup_do_cow_enter(job, start * bytes_per_cluster,
> +trace_backup_do_cow_enter(job, start * job->cluster_size,
>sector_num * BDRV_SECTOR_SIZE,
>nb_sectors * BDRV_SECTOR_SIZE);
>
> @@ -120,28 +119,27 @@ static int coroutine_fn backup_do_cow(BackupBlockJob
> *job,
>
> for (; start < end; start++) {
> if (test_bit(start, job->done_bitmap)) {
> -trace_backup_do_cow_skip(job, start * bytes_per_cluster);
> +trace_backup_do_cow_skip(job, start * job->cluster_size);
> continue; /* already copied */
> }
>
> -trace_backup_do_cow_process(job, start * bytes_per_cluster);
> +trace_backup_do_cow_process(job, start * job->cluster_size);
>
> -n = MIN(sectors_per_cluster,
> -job->common.len / BDRV_SECTOR_SIZE -
> -start * sectors_per_cluster);
> +n = MIN(job->cluster_size,
> +job->common.len - start * job->cluster_size);
>
> if (!bounce_buffer) {
> bounce_buffer = blk_blockalign(blk, job->cluster_size);
> }
> iov.iov_base = bounce_buffer;
> -iov.iov_len = n * BDRV_SECTOR_SIZE;
> +iov.iov_len = n;
> qemu_iovec_init_external(&bounce_qiov, &iov, 1);
>
> ret = blk_co_preadv(blk, start * job->cluster_size,
> bounce_qiov.size, &bounce_qiov,
> is_write_notifier ? BDRV_REQ_NO_SERIALISING : 0);
> if (ret < 0) {
> -trace_backup_do_cow_read_fail(job, start * bytes_per_cluster,
> ret);
> +trace_backup_do_cow_read_fail(job, start * job->cluster_size,
> ret);
> if (error_is_read) {
> *error_is_read = true;
> }
> @@ -157,7 +155,7 @@ static int coroutine_fn backup_do_cow(BackupBlockJob *job,
> job->compress ? BDRV_REQ_WRITE_COMPRESSED :
> 0);
> }
> if (ret < 0) {
> -trace_backup_do_cow_write_fail(job, start * bytes_per_cluster,
> ret);
> +trace_backup_do_cow_write_fail(job, start * job->cluster_size,
> ret);
> if (error_is_read) {
> *error_is_read = false;
> }
> @@ -169,8 +167,8 @@ static int coroutine_fn backup_do_cow(BackupBlockJob *job,
> /* Publish progress, guest I/O counts as progress too. Note that the
> * offset field is an opaque progress value, it is not a disk offset.
> */
> -job->sectors_read += n;
> -job->common.offset += n * BDRV_SECTOR_SIZE;
> +job->bytes_read += n;
> +job->common.offset += n;
> }
>
> out:
> @@ -363,9 +361,8 @@ static bool coroutine_fn yield_and_check(BackupBlockJob
> *job)
> */
> if (job->common.speed) {
> uint64_t delay_ns = ratelimit_calculate_delay(&job->limit,
> - job->sectors_read *
> - BDRV_SECTOR_SIZE);
> -job->sectors_read = 0;
> + job->bytes_read);
> +job->bytes_read = 0;
> block_job_sleep_ns(&job->common, QEMU_CLOCK_REALTIME, delay_ns);
> } else {
> block_job_sleep_ns(&job->common, QEMU_CLOCK_REALTIME, 0);
> --
> 2.9.4
>
Re: [Qemu-devel] [PATCH v3 15/20] backup: Switch block_backup.h to byte-based
On Tue, Jun 27, 2017 at 02:24:53PM -0500, Eric Blake wrote:
> We are gradually converting to byte-based interfaces, as they are
> easier to reason about than sector-based. Continue by converting
> the public interface to backup jobs (no semantic change), including
> a change to CowRequest to track by bytes instead of cluster indices.
>
> Note that this does not change the difference between the public
> interface (starting point, and size of the subsequent range) and
> the internal interface (starting and end points).
>
> Signed-off-by: Eric Blake
> Reviewed-by: John Snow
>
Reviewed-by: Jeff Cody
> ---
> v2: change a couple more parameter names
> ---
> include/block/block_backup.h | 11 +--
> block/backup.c | 33 -
> block/replication.c | 12
> 3 files changed, 29 insertions(+), 27 deletions(-)
>
> diff --git a/include/block/block_backup.h b/include/block/block_backup.h
> index 8a75947..994a3bd 100644
> --- a/include/block/block_backup.h
> +++ b/include/block/block_backup.h
> @@ -21,17 +21,16 @@
> #include "block/block_int.h"
>
> typedef struct CowRequest {
> -int64_t start;
> -int64_t end;
> +int64_t start_byte;
> +int64_t end_byte;
> QLIST_ENTRY(CowRequest) list;
> CoQueue wait_queue; /* coroutines blocked on this request */
> } CowRequest;
>
> -void backup_wait_for_overlapping_requests(BlockJob *job, int64_t sector_num,
> - int nb_sectors);
> +void backup_wait_for_overlapping_requests(BlockJob *job, int64_t offset,
> + uint64_t bytes);
> void backup_cow_request_begin(CowRequest *req, BlockJob *job,
> - int64_t sector_num,
> - int nb_sectors);
> + int64_t offset, uint64_t bytes);
> void backup_cow_request_end(CowRequest *req);
>
> void backup_do_checkpoint(BlockJob *job, Error **errp);
> diff --git a/block/backup.c b/block/backup.c
> index 4e64710..cfbd921 100644
> --- a/block/backup.c
> +++ b/block/backup.c
> @@ -55,7 +55,7 @@ static inline int64_t cluster_size_sectors(BackupBlockJob
> *job)
>
> /* See if in-flight requests overlap and wait for them to complete */
> static void coroutine_fn wait_for_overlapping_requests(BackupBlockJob *job,
> - int64_t start,
> + int64_t offset,
> int64_t end)
> {
> CowRequest *req;
> @@ -64,7 +64,7 @@ static void coroutine_fn
> wait_for_overlapping_requests(BackupBlockJob *job,
> do {
> retry = false;
> QLIST_FOREACH(req, &job->inflight_reqs, list) {
> -if (end > req->start && start < req->end) {
> +if (end > req->start_byte && offset < req->end_byte) {
> qemu_co_queue_wait(&req->wait_queue, NULL);
> retry = true;
> break;
> @@ -75,10 +75,10 @@ static void coroutine_fn
> wait_for_overlapping_requests(BackupBlockJob *job,
>
> /* Keep track of an in-flight request */
> static void cow_request_begin(CowRequest *req, BackupBlockJob *job,
> - int64_t start, int64_t end)
> + int64_t offset, int64_t end)
> {
> -req->start = start;
> -req->end = end;
> +req->start_byte = offset;
> +req->end_byte = end;
> qemu_co_queue_init(&req->wait_queue);
> QLIST_INSERT_HEAD(&job->inflight_reqs, req, list);
> }
> @@ -114,8 +114,10 @@ static int coroutine_fn backup_do_cow(BackupBlockJob
> *job,
>sector_num * BDRV_SECTOR_SIZE,
>nb_sectors * BDRV_SECTOR_SIZE);
>
> -wait_for_overlapping_requests(job, start, end);
> -cow_request_begin(&cow_request, job, start, end);
> +wait_for_overlapping_requests(job, start * job->cluster_size,
> + end * job->cluster_size);
> +cow_request_begin(&cow_request, job, start * job->cluster_size,
> + end * job->cluster_size);
>
> for (; start < end; start++) {
> if (test_bit(start, job->done_bitmap)) {
> @@ -277,32 +279,29 @@ void backup_do_checkpoint(BlockJob *job, Error **errp)
> bitmap_zero(backup_job->done_bitmap, len);
> }
>
> -void backup_wait_for_overlapping_requests(BlockJob *job, int64_t sector_num,
> -
Re: [Qemu-devel] [PATCH v3 16/20] backup: Switch backup_do_cow() to byte-based
On Tue, Jun 27, 2017 at 02:24:54PM -0500, Eric Blake wrote:
> We are gradually converting to byte-based interfaces, as they are
> easier to reason about than sector-based. Convert another internal
> function (no semantic change).
>
> Signed-off-by: Eric Blake
> Reviewed-by: John Snow
>
Reviewed-by: Jeff Cody
> ---
> v2: no change
> ---
> block/backup.c | 62
> --
> 1 file changed, 26 insertions(+), 36 deletions(-)
>
> diff --git a/block/backup.c b/block/backup.c
> index cfbd921..c029d44 100644
> --- a/block/backup.c
> +++ b/block/backup.c
> @@ -91,7 +91,7 @@ static void cow_request_end(CowRequest *req)
> }
>
> static int coroutine_fn backup_do_cow(BackupBlockJob *job,
> - int64_t sector_num, int nb_sectors,
> + int64_t offset, uint64_t bytes,
>bool *error_is_read,
>bool is_write_notifier)
> {
> @@ -101,34 +101,28 @@ static int coroutine_fn backup_do_cow(BackupBlockJob
> *job,
> QEMUIOVector bounce_qiov;
> void *bounce_buffer = NULL;
> int ret = 0;
> -int64_t sectors_per_cluster = cluster_size_sectors(job);
> -int64_t start, end; /* clusters */
> +int64_t start, end; /* bytes */
> int n; /* bytes */
>
> qemu_co_rwlock_rdlock(&job->flush_rwlock);
>
> -start = sector_num / sectors_per_cluster;
> -end = DIV_ROUND_UP(sector_num + nb_sectors, sectors_per_cluster);
> +start = QEMU_ALIGN_DOWN(offset, job->cluster_size);
> +end = QEMU_ALIGN_UP(bytes + offset, job->cluster_size);
>
> -trace_backup_do_cow_enter(job, start * job->cluster_size,
> - sector_num * BDRV_SECTOR_SIZE,
> - nb_sectors * BDRV_SECTOR_SIZE);
> +trace_backup_do_cow_enter(job, start, offset, bytes);
>
> -wait_for_overlapping_requests(job, start * job->cluster_size,
> - end * job->cluster_size);
> -cow_request_begin(&cow_request, job, start * job->cluster_size,
> - end * job->cluster_size);
> +wait_for_overlapping_requests(job, start, end);
> +cow_request_begin(&cow_request, job, start, end);
>
> -for (; start < end; start++) {
> -if (test_bit(start, job->done_bitmap)) {
> -trace_backup_do_cow_skip(job, start * job->cluster_size);
> +for (; start < end; start += job->cluster_size) {
> +if (test_bit(start / job->cluster_size, job->done_bitmap)) {
> +trace_backup_do_cow_skip(job, start);
> continue; /* already copied */
> }
>
> -trace_backup_do_cow_process(job, start * job->cluster_size);
> +trace_backup_do_cow_process(job, start);
>
> -n = MIN(job->cluster_size,
> -job->common.len - start * job->cluster_size);
> +n = MIN(job->cluster_size, job->common.len - start);
>
> if (!bounce_buffer) {
> bounce_buffer = blk_blockalign(blk, job->cluster_size);
> @@ -137,11 +131,10 @@ static int coroutine_fn backup_do_cow(BackupBlockJob
> *job,
> iov.iov_len = n;
> qemu_iovec_init_external(&bounce_qiov, &iov, 1);
>
> -ret = blk_co_preadv(blk, start * job->cluster_size,
> -bounce_qiov.size, &bounce_qiov,
> +ret = blk_co_preadv(blk, start, bounce_qiov.size, &bounce_qiov,
> is_write_notifier ? BDRV_REQ_NO_SERIALISING : 0);
> if (ret < 0) {
> -trace_backup_do_cow_read_fail(job, start * job->cluster_size,
> ret);
> +trace_backup_do_cow_read_fail(job, start, ret);
> if (error_is_read) {
> *error_is_read = true;
> }
> @@ -149,22 +142,22 @@ static int coroutine_fn backup_do_cow(BackupBlockJob
> *job,
> }
>
> if (buffer_is_zero(iov.iov_base, iov.iov_len)) {
> -ret = blk_co_pwrite_zeroes(job->target, start *
> job->cluster_size,
> +ret = blk_co_pwrite_zeroes(job->target, start,
> bounce_qiov.size, BDRV_REQ_MAY_UNMAP);
> } else {
> -ret = blk_co_pwritev(job->target, start * job->cluster_size,
> +ret = blk_co_pwritev(job->target, start,
> bounce_qiov.size, &bounce_qiov,
> job->compress ? BDRV_REQ_WRITE_COMPRESSED :
Re: [Qemu-devel] [PATCH v3 17/20] backup: Switch backup_run() to byte-based
On Tue, Jun 27, 2017 at 02:24:55PM -0500, Eric Blake wrote:
> We are gradually converting to byte-based interfaces, as they are
> easier to reason about than sector-based. Change the internal
> loop iteration of backups to track by bytes instead of sectors
> (although we are still guaranteed that we iterate by steps that
> are cluster-aligned).
>
> Signed-off-by: Eric Blake
> Reviewed-by: John Snow
>
Reviewed-by: Jeff Cody
> ---
> v2: no change
> ---
> block/backup.c | 32 +++-
> 1 file changed, 15 insertions(+), 17 deletions(-)
>
> diff --git a/block/backup.c b/block/backup.c
> index c029d44..04def91 100644
> --- a/block/backup.c
> +++ b/block/backup.c
> @@ -370,11 +370,10 @@ static int coroutine_fn
> backup_run_incremental(BackupBlockJob *job)
> int ret = 0;
> int clusters_per_iter;
> uint32_t granularity;
> -int64_t sector;
> +int64_t offset;
> int64_t cluster;
> int64_t end;
> int64_t last_cluster = -1;
> -int64_t sectors_per_cluster = cluster_size_sectors(job);
> BdrvDirtyBitmapIter *dbi;
>
> granularity = bdrv_dirty_bitmap_granularity(job->sync_bitmap);
> @@ -382,8 +381,8 @@ static int coroutine_fn
> backup_run_incremental(BackupBlockJob *job)
> dbi = bdrv_dirty_iter_new(job->sync_bitmap, 0);
>
> /* Find the next dirty sector(s) */
> -while ((sector = bdrv_dirty_iter_next(dbi)) != -1) {
> -cluster = sector / sectors_per_cluster;
> +while ((offset = bdrv_dirty_iter_next(dbi) * BDRV_SECTOR_SIZE) >= 0) {
> +cluster = offset / job->cluster_size;
>
> /* Fake progress updates for any clusters we skipped */
> if (cluster != last_cluster + 1) {
> @@ -410,7 +409,8 @@ static int coroutine_fn
> backup_run_incremental(BackupBlockJob *job)
> /* If the bitmap granularity is smaller than the backup granularity,
> * we need to advance the iterator pointer to the next cluster. */
> if (granularity < job->cluster_size) {
> -bdrv_set_dirty_iter(dbi, cluster * sectors_per_cluster);
> +bdrv_set_dirty_iter(dbi,
> +cluster * job->cluster_size /
> BDRV_SECTOR_SIZE);
> }
>
> last_cluster = cluster - 1;
> @@ -432,17 +432,15 @@ static void coroutine_fn backup_run(void *opaque)
> BackupBlockJob *job = opaque;
> BackupCompleteData *data;
> BlockDriverState *bs = blk_bs(job->common.blk);
> -int64_t start, end;
> +int64_t offset;
> int64_t sectors_per_cluster = cluster_size_sectors(job);
> int ret = 0;
>
> QLIST_INIT(&job->inflight_reqs);
> qemu_co_rwlock_init(&job->flush_rwlock);
>
> -start = 0;
> -end = DIV_ROUND_UP(job->common.len, job->cluster_size);
> -
> -job->done_bitmap = bitmap_new(end);
> +job->done_bitmap = bitmap_new(DIV_ROUND_UP(job->common.len,
> + job->cluster_size));
>
> job->before_write.notify = backup_before_write_notify;
> bdrv_add_before_write_notifier(bs, &job->before_write);
> @@ -457,7 +455,8 @@ static void coroutine_fn backup_run(void *opaque)
> ret = backup_run_incremental(job);
> } else {
> /* Both FULL and TOP SYNC_MODE's require copying.. */
> -for (; start < end; start++) {
> +for (offset = 0; offset < job->common.len;
> + offset += job->cluster_size) {
> bool error_is_read;
> int alloced = 0;
>
> @@ -480,8 +479,8 @@ static void coroutine_fn backup_run(void *opaque)
> * needed but at some point that is always the case. */
> alloced =
> bdrv_is_allocated(bs,
> -start * sectors_per_cluster + i,
> -sectors_per_cluster - i, &n);
> + (offset >> BDRV_SECTOR_BITS) + i,
> + sectors_per_cluster - i, &n);
> i += n;
>
> if (alloced || n == 0) {
> @@ -499,9 +498,8 @@ static void coroutine_fn backup_run(void *opaque)
> if (alloced < 0) {
> ret = alloced;
> } else {
> -ret = backup_do_cow(job, start * job->cluster_size,
> -job->cluster_size, &error_is_read,
> -false);
> +ret = backup_do_cow(job, offset, job->cluster_size,
> +
Re: [Qemu-devel] [PATCH v3 18/20] block: Make bdrv_is_allocated() byte-based
On Tue, Jun 27, 2017 at 02:24:56PM -0500, Eric Blake wrote:
> We are gradually moving away from sector-based interfaces, towards
> byte-based. In the common case, allocation is unlikely to ever use
> values that are not naturally sector-aligned, but it is possible
> that byte-based values will let us be more precise about allocation
> at the end of an unaligned file that can do byte-based access.
>
> Changing the signature of the function to use int64_t *pnum ensures
> that the compiler enforces that all callers are updated. For now,
> the io.c layer still assert()s that all callers are sector-aligned
> on input and that *pnum is sector-aligned on return to the caller,
> but that can be relaxed when a later patch implements byte-based
> block status. Therefore, this code adds usages like
> DIV_ROUND_UP(,BDRV_SECTOR_SIZE) to callers that still want aligned
> values, where the call might reasonbly give non-aligned results
> in the future; on the other hand, no rounding is needed for callers
> that should just continue to work with byte alignment.
>
> For the most part this patch is just the addition of scaling at the
> callers followed by inverse scaling at bdrv_is_allocated(). But
> some code, particularly bdrv_commit(), gets a lot simpler because it
> no longer has to mess with sectors; also, it is now possible to pass
> NULL if the caller does not care how much of the image is allocated
> beyond the initial offset.
>
> For ease of review, bdrv_is_allocated_above() will be tackled
> separately.
>
> Signed-off-by: Eric Blake
> Reviewed-by: John Snow
>
Reviewed-by: Jeff Cody
> ---
> v2: rebase to earlier changes, tweak commit message
> ---
> include/block/block.h | 4 +--
> block/backup.c| 17 -
> block/commit.c| 21 +++-
> block/io.c| 49 +---
> block/stream.c| 5 ++--
> block/vvfat.c | 34 ++---
> migration/block.c | 9 ---
> qemu-img.c| 5 +++-
> qemu-io-cmds.c| 70
> +++
> 9 files changed, 114 insertions(+), 100 deletions(-)
>
> diff --git a/include/block/block.h b/include/block/block.h
> index 5cdd690..9b9d87b 100644
> --- a/include/block/block.h
> +++ b/include/block/block.h
> @@ -425,8 +425,8 @@ int64_t bdrv_get_block_status_above(BlockDriverState *bs,
> int64_t sector_num,
> int nb_sectors, int *pnum,
> BlockDriverState **file);
> -int bdrv_is_allocated(BlockDriverState *bs, int64_t sector_num, int
> nb_sectors,
> - int *pnum);
> +int bdrv_is_allocated(BlockDriverState *bs, int64_t offset, int64_t bytes,
> + int64_t *pnum);
> int bdrv_is_allocated_above(BlockDriverState *top, BlockDriverState *base,
> int64_t sector_num, int nb_sectors, int *pnum);
>
> diff --git a/block/backup.c b/block/backup.c
> index 04def91..b2048bf 100644
> --- a/block/backup.c
> +++ b/block/backup.c
> @@ -47,12 +47,6 @@ typedef struct BackupBlockJob {
> QLIST_HEAD(, CowRequest) inflight_reqs;
> } BackupBlockJob;
>
> -/* Size of a cluster in sectors, instead of bytes. */
> -static inline int64_t cluster_size_sectors(BackupBlockJob *job)
> -{
> - return job->cluster_size / BDRV_SECTOR_SIZE;
> -}
> -
> /* See if in-flight requests overlap and wait for them to complete */
> static void coroutine_fn wait_for_overlapping_requests(BackupBlockJob *job,
> int64_t offset,
> @@ -433,7 +427,6 @@ static void coroutine_fn backup_run(void *opaque)
> BackupCompleteData *data;
> BlockDriverState *bs = blk_bs(job->common.blk);
> int64_t offset;
> -int64_t sectors_per_cluster = cluster_size_sectors(job);
> int ret = 0;
>
> QLIST_INIT(&job->inflight_reqs);
> @@ -465,12 +458,13 @@ static void coroutine_fn backup_run(void *opaque)
> }
>
> if (job->sync_mode == MIRROR_SYNC_MODE_TOP) {
> -int i, n;
> +int i;
> +int64_t n;
>
> /* Check to see if these blocks are already in the
> * backing file. */
>
> -for (i = 0; i < sectors_per_cluster;) {
> +for (i = 0; i < job->cluster_size;) {
> /* bdrv_is_allocated() only returns true/false based
> * on the first set of sectors it comes across that
> * are are a
Re: [Qemu-devel] [Qemu-block] [PATCH v3 19/20] block: Minimize raw use of bds->total_sectors
On Tue, Jun 27, 2017 at 02:24:57PM -0500, Eric Blake wrote:
> bdrv_is_allocated_above() was relying on intermediate->total_sectors,
> which is a field that can have stale contents depending on the value
> of intermediate->has_variable_length. An audit shows that we are safe
> (we were first calling through bdrv_co_get_block_status() which in
> turn calls bdrv_nb_sectors() and therefore just refreshed the current
> length), but it's nicer to favor our accessor functions to avoid having
> to repeat such an audit, even if it means refresh_total_sectors() is
> called more frequently.
>
> Suggested-by: John Snow
> Signed-off-by: Eric Blake
>
Reviewed-by: Jeff Cody
> ---
> v2: new patch
> ---
> block/io.c | 12 +++-
> 1 file changed, 7 insertions(+), 5 deletions(-)
>
> diff --git a/block/io.c b/block/io.c
> index 0545180..5bbf153 100644
> --- a/block/io.c
> +++ b/block/io.c
> @@ -1924,6 +1924,7 @@ int bdrv_is_allocated_above(BlockDriverState *top,
> intermediate = top;
> while (intermediate && intermediate != base) {
> int64_t pnum_inter;
> +int64_t size_inter;
> int psectors_inter;
>
> ret = bdrv_is_allocated(intermediate, sector_num * BDRV_SECTOR_SIZE,
> @@ -1941,13 +1942,14 @@ int bdrv_is_allocated_above(BlockDriverState *top,
>
> /*
> * [sector_num, nb_sectors] is unallocated on top but intermediate
> - * might have
> - *
> - * [sector_num+x, nr_sectors] allocated.
> + * might have [sector_num+x, nb_sectors-x] allocated.
> */
> +size_inter = bdrv_nb_sectors(intermediate);
> +if (size_inter < 0) {
> +return size_inter;
> +}
> if (n > psectors_inter &&
> -(intermediate == top ||
> - sector_num + psectors_inter < intermediate->total_sectors)) {
> +(intermediate == top || sector_num + psectors_inter <
> size_inter)) {
> n = psectors_inter;
> }
>
> --
> 2.9.4
>
>
Re: [Qemu-devel] [PATCH v3 20/20] block: Make bdrv_is_allocated_above() byte-based
On Tue, Jun 27, 2017 at 02:24:58PM -0500, Eric Blake wrote:
> We are gradually moving away from sector-based interfaces, towards
> byte-based. In the common case, allocation is unlikely to ever use
> values that are not naturally sector-aligned, but it is possible
> that byte-based values will let us be more precise about allocation
> at the end of an unaligned file that can do byte-based access.
>
> Changing the signature of the function to use int64_t *pnum ensures
> that the compiler enforces that all callers are updated. For now,
> the io.c layer still assert()s that all callers are sector-aligned,
> but that can be relaxed when a later patch implements byte-based
> block status. Therefore, for the most part this patch is just the
> addition of scaling at the callers followed by inverse scaling at
> bdrv_is_allocated(). But some code, particularly stream_run(),
> gets a lot simpler because it no longer has to mess with sectors.
>
> For ease of review, bdrv_is_allocated() was tackled separately.
>
> Signed-off-by: Eric Blake
> Reviewed-by: John Snow
>
Reviewed-by: Jeff Cody
> ---
> v2: tweak function comments, favor bdrv_getlength() over ->total_sectors
> ---
> include/block/block.h | 2 +-
> block/commit.c| 20
> block/io.c| 42 --
> block/mirror.c| 5 -
> block/replication.c | 17 -
> block/stream.c| 21 +
> qemu-img.c| 10 +++---
> 7 files changed, 61 insertions(+), 56 deletions(-)
>
> diff --git a/include/block/block.h b/include/block/block.h
> index 9b9d87b..13022d5 100644
> --- a/include/block/block.h
> +++ b/include/block/block.h
> @@ -428,7 +428,7 @@ int64_t bdrv_get_block_status_above(BlockDriverState *bs,
> int bdrv_is_allocated(BlockDriverState *bs, int64_t offset, int64_t bytes,
>int64_t *pnum);
> int bdrv_is_allocated_above(BlockDriverState *top, BlockDriverState *base,
> -int64_t sector_num, int nb_sectors, int *pnum);
> +int64_t offset, int64_t bytes, int64_t *pnum);
>
> bool bdrv_is_read_only(BlockDriverState *bs);
> bool bdrv_is_writable(BlockDriverState *bs);
> diff --git a/block/commit.c b/block/commit.c
> index 241aa95..774a8a5 100644
> --- a/block/commit.c
> +++ b/block/commit.c
> @@ -146,7 +146,7 @@ static void coroutine_fn commit_run(void *opaque)
> int64_t offset;
> uint64_t delay_ns = 0;
> int ret = 0;
> -int n = 0; /* sectors */
> +int64_t n = 0; /* bytes */
> void *buf = NULL;
> int bytes_written = 0;
> int64_t base_len;
> @@ -171,7 +171,7 @@ static void coroutine_fn commit_run(void *opaque)
>
> buf = blk_blockalign(s->top, COMMIT_BUFFER_SIZE);
>
> -for (offset = 0; offset < s->common.len; offset += n * BDRV_SECTOR_SIZE)
> {
> +for (offset = 0; offset < s->common.len; offset += n) {
> bool copy;
>
> /* Note that even when no rate limit is applied we need to yield
> @@ -183,15 +183,12 @@ static void coroutine_fn commit_run(void *opaque)
> }
> /* Copy if allocated above the base */
> ret = bdrv_is_allocated_above(blk_bs(s->top), blk_bs(s->base),
> - offset / BDRV_SECTOR_SIZE,
> - COMMIT_BUFFER_SIZE / BDRV_SECTOR_SIZE,
> - &n);
> + offset, COMMIT_BUFFER_SIZE, &n);
> copy = (ret == 1);
> -trace_commit_one_iteration(s, offset, n * BDRV_SECTOR_SIZE, ret);
> +trace_commit_one_iteration(s, offset, n, ret);
> if (copy) {
> -ret = commit_populate(s->top, s->base, offset,
> - n * BDRV_SECTOR_SIZE, buf);
> -bytes_written += n * BDRV_SECTOR_SIZE;
> +ret = commit_populate(s->top, s->base, offset, n, buf);
> +bytes_written += n;
> }
> if (ret < 0) {
> BlockErrorAction action =
> @@ -204,11 +201,10 @@ static void coroutine_fn commit_run(void *opaque)
> }
> }
> /* Publish progress */
> -s->common.offset += n * BDRV_SECTOR_SIZE;
> +s->common.offset += n;
>
> if (copy && s->common.speed) {
> -delay_ns = ratelimit_calculate_delay(&s->limit,
> - n * BDRV_SECTOR_SIZE);
> +delay_ns = ratelimit_calculate_delay(&s->limit, n);
> }
> }
>
>
Re: [Qemu-devel] [Qemu-block] [PATCH v4 1/2] iotests: Use absolute paths for executables
On Fri, Jun 30, 2017 at 02:45:46PM -0500, Eric Blake wrote:
> On 06/30/2017 02:41 PM, Eric Blake wrote:
>
> > +++ 068.out.bad 2017-06-30 14:35:28.720241398 -0500
> > @@ -1,4 +1,5 @@
> > QA output created by 068
> > +realpath: '': No such file or directory
> >
> > The culprit? $QEMU_VXHS_PROG is empty for me, which means `set_prog_path
> > qnio_server` found nothing to use. You'll have to add in a safety valve
> > that only calls 'type' if operating on a non-empty path in the first place.
>
> I'm using this locally, in the meantime:
>
> diff --git i/tests/qemu-iotests/common.config
> w/tests/qemu-iotests/common.config
> index c1dc425..6f97331 100644
> --- i/tests/qemu-iotests/common.config
> +++ w/tests/qemu-iotests/common.config
> @@ -107,7 +107,9 @@ export QEMU_PROG=$(realpath -- "$(type -p
> "$QEMU_PROG")")
> export QEMU_IMG_PROG=$(realpath -- "$(type -p "$QEMU_IMG_PROG")")
> export QEMU_IO_PROG=$(realpath -- "$(type -p "$QEMU_IO_PROG")")
> export QEMU_NBD_PROG=$(realpath -- "$(type -p "$QEMU_NBD_PROG")")
> -export QEMU_VXHS_PROG=$(realpath -- "$(type -p "$QEMU_VXHS_PROG")")
> +if [ -n "$QEMU_VXHS_PROG" ]; then
> +export QEMU_VXHS_PROG=$(realpath -- "$(type -p "$QEMU_VXHS_PROG")")
> +fi
>
> _qemu_wrapper()
> {
>
>
> Is qnio_server easily available for Fedora?
>
Depends on your definition of "easily". It needs to be built from the
upstream project github [1]. Most developers will likely not have it
installed.
https://github.com/VeritasHyperScale/libqnio
Re: [Qemu-devel] [Qemu-block] [PATCH v2] tests: Avoid non-portable 'echo -ARG'
> -drive "$drv_bk" -drive "$drv_file" -drive "$drv_img" | grep -e "Cache" -e
> "[Cc]annot\|[Cc]ould not\|[Cc]an't"
>
> # Check cache.no-flush
>
> -echo -e "\n\ncache.no-flush=on on blk"
> +printf "\n\ncache.no-flush=on on blk\n"
> echo "$hmp_cmds" | run_qemu -drive "$drv_bkfile" -drive "$drv_bk" -drive
> "$drv_file" -drive "$drv_img",cache.no-flush=on | grep -e "Cache" -e
> "[Cc]annot\|[Cc]ould not\|[Cc]an't"
> -echo -e "\ncache.no-flush=on on file"
> +printf "\ncache.no-flush=on on file\n"
> echo "$hmp_cmds" | run_qemu -drive "$drv_bkfile" -drive "$drv_bk" -drive
> "$drv_file",cache.no-flush=on -drive "$drv_img" | grep -e "Cache" -e
> "[Cc]annot\|[Cc]ould not\|[Cc]an't"
> -echo -e "\ncache.no-flush=on on backing"
> +printf "\ncache.no-flush=on on backing\n"
> echo "$hmp_cmds" | run_qemu -drive "$drv_bkfile" -drive
> "$drv_bk",cache.no-flush=on -drive "$drv_file" -drive "$drv_img" | grep -e
> "Cache" -e "[Cc]annot\|[Cc]ould not\|[Cc]an't"
> -echo -e "\ncache.no-flush=on on backing-file"
> +printf "\ncache.no-flush=on on backing-file\n"
> echo "$hmp_cmds" | run_qemu -drive "$drv_bkfile",cache.no-flush=on
> -drive "$drv_bk" -drive "$drv_file" -drive "$drv_img" | grep -e "Cache" -e
> "[Cc]annot\|[Cc]ould not\|[Cc]an't"
> }
>
> diff --git a/tests/qemu-iotests/171 b/tests/qemu-iotests/171
> index 257be10..54298ad 100755
> --- a/tests/qemu-iotests/171
> +++ b/tests/qemu-iotests/171
> @@ -45,15 +45,15 @@ _supported_os Linux
>
> # Create JSON with options
> img_json() {
> -echo -n 'json:{"driver":"raw", '
> -echo -n "\"offset\":\"$img_offset\", "
> +printf 'json:{"driver":"raw", '
> +printf "\"offset\":\"$img_offset\", "
> if [ "$img_size" -ne -1 ] ; then
> -echo -n "\"size\":\"$img_size\", "
> +printf "\"size\":\"$img_size\", "
> fi
> -echo -n '"file": {'
> -echo -n'"driver":"file", '
> -echo -n"\"filename\":\"$TEST_IMG\" "
> -echo -n "} }"
> +printf '"file": {'
> +printf'"driver":"file", '
> +printf %s "\"filename\":\"$TEST_IMG\" "
> +printf "} }"
> }
>
> do_general_test() {
> diff --git a/tests/qemu-iotests/check b/tests/qemu-iotests/check
> index 4b1c674..0a13df9 100755
> --- a/tests/qemu-iotests/check
> +++ b/tests/qemu-iotests/check
> @@ -141,7 +141,7 @@ _wallclock()
> _timestamp()
> {
> now=`date "+%T"`
> -echo -n " [$now]"
> +printf " [$now]"
> }
>
> _wrapup()
> @@ -255,7 +255,7 @@ seq="check"
> for seq in $list
> do
> err=false
> -echo -n "$seq"
> +printf "$seq"
> if [ -n "$TESTS_REMAINING_LOG" ] ; then
> sed -e "s/$seq//" -e 's/ / /' -e 's/^ *//' $TESTS_REMAINING_LOG >
> $TESTS_REMAINING_LOG.tmp
> mv $TESTS_REMAINING_LOG.tmp $TESTS_REMAINING_LOG
> @@ -281,9 +281,9 @@ do
> rm -f $seq.out.bad
> lasttime=`sed -n -e "/^$seq /s/.* //p" <$TIMESTAMP_FILE`
> if [ "X$lasttime" != X ]; then
> -echo -n " ${lasttime}s ..."
> +printf " ${lasttime}s ..."
> else
> -echo -n ""# prettier output with timestamps.
> +printf ""# prettier output with timestamps.
> fi
> rm -f core $seq.notrun
>
> @@ -291,7 +291,7 @@ do
> echo "$seq" > "${TEST_DIR}"/check.sts
>
> start=`_wallclock`
> -$timestamp && echo -n "["`date "+%T"`"]"
> +$timestamp && printf "[$(date "+%T")]"
>
> if [ "$(head -n 1 "$source_iotests/$seq")" == "#!/usr/bin/env
> python" ]; then
> run_command="$PYTHON $seq"
> @@ -314,21 +314,21 @@ do
>
> if [ -f core ]
> then
> -echo -n " [dumped core]"
> +printf " [dumped core]"
> mv core $seq.core
> err=true
> fi
>
> if [ -f $seq.notrun ]
> then
> -$timestamp || echo -n " [not run] "
> -$timestamp && echo " [not run]" && echo -n "$seq -- "
> +$timestamp || printf " [not run] "
> +$timestamp && echo " [not run]" && printf "$seq -- "
> cat $seq.notrun
> notrun="$notrun $seq"
> else
> if [ $sts -ne 0 ]
> then
> -echo -n " [failed, exit status $sts]"
> +printf " [failed, exit status $sts]"
> err=true
> fi
>
> diff --git a/tests/rocker/all b/tests/rocker/all
> index d5ae963..3f9b786 100755
> --- a/tests/rocker/all
> +++ b/tests/rocker/all
> @@ -1,19 +1,19 @@
> -echo -n "Running port test... "
> +printf "Running port test... "
> ./port
> if [ $? -eq 0 ]; then echo "pass"; else echo "FAILED"; exit 1; fi
>
> -echo -n "Running bridge test..."
> +printf "Running bridge test..."
> ./bridge
> if [ $? -eq 0 ]; then echo "pass"; else echo "FAILED"; exit 1; fi
>
> -echo -n "Running bridge STP test..."
> +printf "Running bridge STP test..."
> ./bridge-stp
> if [ $? -eq 0 ]; then echo "pass"; else echo "FAILED"; exit 1; fi
>
> -echo -n "Running bridge VLAN test... "
> +printf "Running bridge VLAN test... "
> ./bridge-vlan
> if [ $? -eq 0 ]; then echo "pass"; else echo "FAILED"; exit 1; fi
>
> -echo -n "Running bridge VLAN STP test... "
> +printf "Running bridge VLAN STP test... "
> ./bridge-vlan-stp
> if [ $? -eq 0 ]; then echo "pass"; else echo "FAILED"; exit 1; fi
> diff --git a/tests/tcg/cris/Makefile b/tests/tcg/cris/Makefile
> index 6b3dba4..6888263 100644
> --- a/tests/tcg/cris/Makefile
> +++ b/tests/tcg/cris/Makefile
> @@ -150,17 +150,17 @@ check_addcv17.tst: crtv10.o sysv10.o
> build: $(CRT) $(SYS) $(TESTCASES)
>
> check: $(CRT) $(SYS) $(TESTCASES)
> - @echo -e "\nQEMU simulator."
> + @printf "\nQEMU simulator.\n"
> for case in $(TESTCASES); do \
> - echo -n "$$case "; \
> + printf "$$case "; \
> SIMARGS=; \
> case $$case in *v17*) SIMARGS="-cpu crisv17";; esac; \
> $(SIM) $$SIMARGS ./$$case; \
> done
> check-g: $(CRT) $(SYS) $(TESTCASES)
> - @echo -e "\nGDB simulator."
> + @printf "\nGDB simulator.\n"
> @for case in $(TESTCASES); do \
> - echo -n "$$case "; \
> + printf "$$case "; \
> $(SIMG) $$case; \
> done
>
> --
> 2.9.4
>
>
Reviewed-by: Jeff Cody
Re: [Qemu-devel] [qemu-web PATCH] Add a favicon.ico file
On Wed, Jul 26, 2017 at 04:54:48PM +0200, Paolo Bonzini wrote: > On 26/07/2017 16:47, Thomas Huth wrote: > > If you're willing to maintain that "mess", sure, please go ahead! But > > please remove my SoB, I don't want to be responsible for all those files ;-) > > Changed it to suggested-by then, and pushed. :> > > Please test! > I just tested in: chrome firefox konqueror internet explorer edge opera Looks as expected in those, at least.
Re: [Qemu-devel] [Qemu-block] [PATCH] qemu-iotests: add a "how to" to ./README
On Fri, Jul 21, 2017 at 10:34:16AM +0100, Stefan Hajnoczi wrote: > There is not much getting started documentation for qemu-iotests. This > patch explains how to create a new test and covers the overall testing > approach. > > Cc: Ishani Chugh > Signed-off-by: Stefan Hajnoczi > --- > tests/qemu-iotests/README | 83 > +++ > 1 file changed, 83 insertions(+) > > diff --git a/tests/qemu-iotests/README b/tests/qemu-iotests/README > index 6079b40..8259b9f 100644 > --- a/tests/qemu-iotests/README > +++ b/tests/qemu-iotests/README > @@ -14,8 +14,91 @@ Just run ./check to run all tests for the raw image > format, or ./check > -qcow2 to test the qcow2 image format. The output of ./check -h explains > additional options to test further image formats or I/O methods. > > +* Testing approach > + > +Each test is an executable file (usually a bash script) that is run by the > +./check test harness. Standard out and standard error are captured to an > +output file. If the output file differs from the "golden master" output file > +for the test then it fails. > + > +Tests are simply a sequence of commands that produce output and the test > itself > +does not judge whether it passed or failed. If you find yourself writing > +checks to determine success or failure then you should rethink the test and > +rely on output diffing instead. > + > +** Filtering volatile output > + > +When output contains absolute file paths, timestamps, process IDs, hostnames, > +or other volatile strings, the diff against golden master output will fail. > +Such output must be filtered to replace volatile strings with fixed > +placeholders. > + > +For example, the path to the temporary working directory changes between test > +runs so it must be filtered: > + > + sed -e "s#$TEST_DIR/#TEST_DIR/#g" > + > +Commonly needed filters are available in ./common.filter. > + > +** Python tests > + > +Most tests are implemented in bash but it is difficult to interact with the > QMP > +monitor. A Python module called 'iotests' is available for tests that > require > +JSON and interacting with QEMU. > + There are some that prefer the bash tests still, and we do have a 'standard' way to do so... so perhaps add as well: ** Bash tests If you wish to create a test in Bash that interacts with the QMP monitor, 'common.qemu' provides functions for interacting with multiple QEMU processes. > +* How to create a test > + > +1. Choose an unused test number > + > +Tests are identified by a unique number. Look for the highest test case > number > +by looking at the test files. Then search the qemu-devel@nongnu.org mailing > +list to check if anyone has already sent patches using the next available > +number. You may need to increment the number a few times to reach an unused > +number. > + > +2. Create the test file > + > +Copy an existing test (one that most closely resembles what you wish to test) > +to the new test number: > + > + cp 001 > + > +3. Assign groups to the test > + > +Add your test to the ./group file. This file is the index of tests and > assigns > +them to functional groups like "rw" for read-write tests. Most tests belong > to > +the "rw" and "auto" groups. "auto" means the test runs when ./check is > invoked > +without a -g argument. > + > +Consider adding your test to the "quick" group if it executes quickly (<1s). > +This group is run by "make check-block" and is often included as part of > build > +tests in continuous integration systems. > + > +4. Write the test > + > +Edit the test script. Look at existing tests for examples. > + > +5. Generate the golden master file > + > +Run your test with "./check ". You may need to pass additional > +options to use an image format or protocol. > + > +The test will fail because there is no golden master yet. Inspect the output > +that your test generated with "cat .out.bad". > + > +Verify that the output is as expected and contains no volatile strings like > +timestamps. You may need to add filters to your test to remove volatile > +strings. > + > +Once you are happy with the test output it can be used as the golden master > +with "mv .out.bad .out". Rerun the test to verify > +that it passes. > + > +Congratulations, you've created a new test! > + > * Feedback and patches > > Please send improvements to the test suite, general feedback or just > reports of failing tests cases to qemu-devel@nongnu.org with a CC: > to qemu-bl...@nongnu.org. > + > -- > 2.9.4 > >
Re: [Qemu-devel] [PATCH 1/2] iotests: Fix test 156
On Tue, Jul 25, 2017 at 05:56:43PM +0200, Max Reitz wrote:
> On one hand, the _make_test_img invocation for creating the target image
> was missing a -u because its backing file is not supposed to exist at
> that point.
>
> On the other hand, nobody noticed probably because the backing file is
> created later on and _cleanup failed to remove it: The quotation marks
> were misplaced so bash tried to deleted a file literally called
> "$TEST_IMG{,.target}..." instead of resolving the globs. Thus, the
> files stayed around after the first run and qemu-img create did not
> complain about a missing backing file on any run but the first.
>
> Signed-off-by: Max Reitz
> ---
> tests/qemu-iotests/156 | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/tests/qemu-iotests/156 b/tests/qemu-iotests/156
> index 2c4a06e..e75dc4d 100755
> --- a/tests/qemu-iotests/156
> +++ b/tests/qemu-iotests/156
> @@ -38,7 +38,7 @@ status=1# failure is the default!
> _cleanup()
> {
> _cleanup_qemu
> -rm -f "$TEST_IMG{,.target}{,.backing,.overlay}"
> +rm -f "$TEST_IMG"{,.target}{,.backing,.overlay}
> }
> trap "_cleanup; exit \$status" 0 1 2 3 15
>
> @@ -83,7 +83,7 @@ _send_qemu_cmd $QEMU_HANDLE \
> 'return'
>
> # Create target image
> -TEST_IMG="$TEST_IMG.target.overlay" _make_test_img -b "$TEST_IMG.target" 1M
> +TEST_IMG="$TEST_IMG.target.overlay" _make_test_img -u -b "$TEST_IMG.target"
> 1M
>
> # Mirror snapshot
> _send_qemu_cmd $QEMU_HANDLE \
> --
> 2.9.4
>
>
Reviewed-by: Jeff Cody
Re: [Qemu-devel] [PATCH 2/2] iotests: Redirect stderr to stdout in 186
On Tue, Jul 25, 2017 at 05:56:44PM +0200, Max Reitz wrote: > Without redirecting qemu's stderr to stdout, _filter_qemu will not apply > to warnings. This results in $QEMU_PROG not being replaced by QEMU_PROG > which is not great if your qemu executable is not called > qemu-system-x86_64 (e.g. qemu-system-i386). > > Signed-off-by: Max Reitz > --- > tests/qemu-iotests/186 | 2 +- > tests/qemu-iotests/186.out | 12 ++-- > 2 files changed, 7 insertions(+), 7 deletions(-) > > diff --git a/tests/qemu-iotests/186 b/tests/qemu-iotests/186 > index ab83ee4..2b9f618 100755 > --- a/tests/qemu-iotests/186 > +++ b/tests/qemu-iotests/186 > @@ -56,7 +56,7 @@ function do_run_qemu() > done > fi > echo quit > -) | $QEMU -S -nodefaults -display none -device virtio-scsi-pci -monitor > stdio "$@" > +) | $QEMU -S -nodefaults -display none -device virtio-scsi-pci -monitor > stdio "$@" 2>&1 > echo > } > > diff --git a/tests/qemu-iotests/186.out b/tests/qemu-iotests/186.out > index b8bf9a2..c8377fe 100644 > --- a/tests/qemu-iotests/186.out > +++ b/tests/qemu-iotests/186.out > @@ -442,28 +442,28 @@ ide0-cd0 (NODE_NAME): null-co:// (null-co, read-only) > Cache mode: writeback > (qemu) quit > > -qemu-system-x86_64: -drive if=scsi,driver=null-co: warning: bus=0,unit=0 is > deprecated with this machine type > Testing: -drive if=scsi,driver=null-co > QEMU X.Y.Z monitor - type 'help' for more information > -(qemu) info block > +(qemu) QEMU_PROG: -drive if=scsi,driver=null-co: warning: bus=0,unit=0 is > deprecated with this machine type > +info block > scsi0-hd0 (NODE_NAME): null-co:// (null-co) > Attached to: /machine/unattached/device[27]/scsi.0/legacy[0] > Cache mode: writeback > (qemu) quit > > -qemu-system-x86_64: -drive if=scsi,media=cdrom: warning: bus=0,unit=0 is > deprecated with this machine type > Testing: -drive if=scsi,media=cdrom > QEMU X.Y.Z monitor - type 'help' for more information > -(qemu) info block > +(qemu) QEMU_PROG: -drive if=scsi,media=cdrom: warning: bus=0,unit=0 is > deprecated with this machine type > +info block > scsi0-cd0: [not inserted] > Attached to: /machine/unattached/device[27]/scsi.0/legacy[0] > Removable device: not locked, tray closed > (qemu) quit > > -qemu-system-x86_64: -drive if=scsi,driver=null-co,media=cdrom: warning: > bus=0,unit=0 is deprecated with this machine type > Testing: -drive if=scsi,driver=null-co,media=cdrom > QEMU X.Y.Z monitor - type 'help' for more information > -(qemu) info block > +(qemu) QEMU_PROG: -drive if=scsi,driver=null-co,media=cdrom: warning: > bus=0,unit=0 is deprecated with this machine type > +info block > scsi0-cd0 (NODE_NAME): null-co:// (null-co, read-only) > Attached to: /machine/unattached/device[27]/scsi.0/legacy[0] > Removable device: not locked, tray closed > -- > 2.9.4 > > Reviewed-by: Jeff Cody
Re: [Qemu-devel] [PATCH for-2.10 2/7] qemu-iotests: Remove blkdebug.conf after tests
On Fri, Jul 28, 2017 at 03:14:47PM +0200, Kevin Wolf wrote:
> qemu-iotests 074 and 179 left a blkdebug.conf behind the scratch
> directory. Make sure to clean up after completing the tests.
>
> Signed-off-by: Kevin Wolf
> ---
> tests/qemu-iotests/074 | 1 +
> tests/qemu-iotests/179 | 1 +
> 2 files changed, 2 insertions(+)
>
> diff --git a/tests/qemu-iotests/074 b/tests/qemu-iotests/074
> index aba126cb69..b17866bd34 100755
> --- a/tests/qemu-iotests/074
> +++ b/tests/qemu-iotests/074
> @@ -32,6 +32,7 @@ _cleanup()
> echo "Cleanup"
> _cleanup_test_img
> rm "${TEST_IMG2}"
> +rm -f "$TEST_DIR/blkdebug.conf"
> }
> trap "_cleanup; exit \$status" 0 1 2 3 15
>
> diff --git a/tests/qemu-iotests/179 b/tests/qemu-iotests/179
> index 7bc8db8fe0..115944a753 100755
> --- a/tests/qemu-iotests/179
> +++ b/tests/qemu-iotests/179
> @@ -30,6 +30,7 @@ status=1# failure is the default!
> _cleanup()
> {
> _cleanup_test_img
> +rm -f "$TEST_DIR/blkdebug.conf"
> }
> trap "_cleanup; exit \$status" 0 1 2 3 15
>
> --
> 2.13.3
>
>
Reviewed-by: Jeff Cody
Re: [Qemu-devel] [PATCH for-2.10 4/7] qemu-iotests/153: Fix leaked scratch images
On Fri, Jul 28, 2017 at 03:14:49PM +0200, Kevin Wolf wrote:
> qemu-iotests 153 left t.qcow2.c behind in the scratch directory. Make
> sure to clean it up after completing the tests.
>
> Signed-off-by: Kevin Wolf
> ---
> tests/qemu-iotests/153 | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/tests/qemu-iotests/153 b/tests/qemu-iotests/153
> index 0b45d78ea3..fa25eb24bd 100755
> --- a/tests/qemu-iotests/153
> +++ b/tests/qemu-iotests/153
> @@ -35,6 +35,7 @@ _cleanup()
> rm -f "${TEST_IMG}.convert"
> rm -f "${TEST_IMG}.a"
> rm -f "${TEST_IMG}.b"
> +rm -f "${TEST_IMG}.c"
> rm -f "${TEST_IMG}.lnk"
> }
> trap "_cleanup; exit \$status" 0 1 2 3 15
> --
> 2.13.3
>
>
Reviewed-by: Jeff Cody
Re: [Qemu-devel] [PATCH for-2.10 3/7] qemu-iotests/141: Fix image cleanup
On Fri, Jul 28, 2017 at 03:14:48PM +0200, Kevin Wolf wrote:
> qemu-iotests 141 attempted to use brace expansion to remove all images
> with a single command. However, for this to work, the braces shouldn't
> be quoted.
>
> With this fix, the tests correctly cleans up its scratch images.
>
> Signed-off-by: Kevin Wolf
> ---
> tests/qemu-iotests/141 | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/tests/qemu-iotests/141 b/tests/qemu-iotests/141
> index 40a3405968..2f9d7b9bc2 100755
> --- a/tests/qemu-iotests/141
> +++ b/tests/qemu-iotests/141
> @@ -31,7 +31,7 @@ _cleanup()
> {
> _cleanup_qemu
> _cleanup_test_img
> -rm -f "$TEST_DIR/{b,m,o}.$IMGFMT"
> +rm -f "$TEST_DIR"/{b,m,o}.$IMGFMT
> }
> trap "_cleanup; exit \$status" 0 1 2 3 15
>
> --
> 2.13.3
>
>
Reviewed-by: Jeff Cody
Re: [Qemu-devel] [PATCH for-2.10 5/7] qemu-iotests/162: Fix leaked temporary files
On Fri, Jul 28, 2017 at 03:14:50PM +0200, Kevin Wolf wrote:
> qemu-iotests 162 left qemu-nbd.pid behind in the scratch directory, and
> potentially a file called '42' in the current directory. Make sure to
> clean it up after completing the tests.
>
> Signed-off-by: Kevin Wolf
> ---
> tests/qemu-iotests/162 | 7 +++
> 1 file changed, 7 insertions(+)
>
> diff --git a/tests/qemu-iotests/162 b/tests/qemu-iotests/162
> index cad2bd70ab..477a806360 100755
> --- a/tests/qemu-iotests/162
> +++ b/tests/qemu-iotests/162
> @@ -28,6 +28,13 @@ echo "QA output created by $seq"
> here="$PWD"
> status=1 # failure is the default!
>
> +_cleanup()
> +{
> +rm -f "${TEST_DIR}/qemu-nbd.pid"
> +rm -f 42
> +}
> +trap "_cleanup; exit \$status" 0 1 2 3 15
> +
> # get standard environment, filters and checks
> . ./common.rc
> . ./common.filter
> --
> 2.13.3
>
>
Reviewed-by: Jeff Cody
Re: [Qemu-devel] [PATCH for-2.10 6/7] qemu-iotests/063: Fix leaked image
On Fri, Jul 28, 2017 at 03:14:51PM +0200, Kevin Wolf wrote:
> qemu-iotests 063 left t.raw.raw1 behind in the scratch directory because
> it used the wrong suffix. Make sure to clean it up after completing the
> test.
>
> Signed-off-by: Kevin Wolf
> ---
> tests/qemu-iotests/063 | 4 +---
> 1 file changed, 1 insertion(+), 3 deletions(-)
>
> diff --git a/tests/qemu-iotests/063 b/tests/qemu-iotests/063
> index 352e78c778..e4f6ea9385 100755
> --- a/tests/qemu-iotests/063
> +++ b/tests/qemu-iotests/063
> @@ -31,7 +31,7 @@ status=1# failure is the default!
> _cleanup()
> {
> _cleanup_test_img
> - rm -f "$TEST_IMG.orig" "$TEST_IMG.raw" "$TEST_IMG.raw2"
> + rm -f "$TEST_IMG.orig" "$TEST_IMG.raw1" "$TEST_IMG.raw2"
> }
> trap "_cleanup; exit \$status" 0 1 2 3 15
>
> @@ -91,8 +91,6 @@ if $QEMU_IMG convert -f $IMGFMT -O $IMGFMT -n
> "$TEST_IMG.orig" "$TEST_IMG" >/dev
> exit 1
> fi
>
> -rm -f "$TEST_IMG.orig" "$TEST_IMG.raw" "$TEST_IMG.raw2"
> -
> echo "*** done"
> rm -f $seq.full
> status=0
> --
> 2.13.3
>
>
Reviewed-by: Jeff Cody
Re: [Qemu-devel] [PATCH for-2.10 1/7] qemu-iotests/041: Fix leaked scratch images
On Fri, Jul 28, 2017 at 03:14:46PM +0200, Kevin Wolf wrote: > qemu-iotests 041 left quorum_snapshot.img and target.img behind in the > scratch directory. Make sure to clean up after completing the tests. > > Signed-off-by: Kevin Wolf > --- > tests/qemu-iotests/041 | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > > diff --git a/tests/qemu-iotests/041 b/tests/qemu-iotests/041 > index 4cda540735..a860a31e9a 100755 > --- a/tests/qemu-iotests/041 > +++ b/tests/qemu-iotests/041 > @@ -418,6 +418,7 @@ new_state = "1" > def tearDown(self): > self.vm.shutdown() > os.remove(test_img) > +os.remove(target_img) > os.remove(backing_img) > os.remove(self.blkdebug_file) > > @@ -568,6 +569,7 @@ new_state = "1" > def tearDown(self): > self.vm.shutdown() > os.remove(test_img) > +os.remove(target_img) > os.remove(backing_img) > os.remove(self.blkdebug_file) > > @@ -821,7 +823,7 @@ class TestRepairQuorum(iotests.QMPTestCase): > > def tearDown(self): > self.vm.shutdown() > -for i in self.IMAGES + [ quorum_repair_img ]: > +for i in self.IMAGES + [ quorum_repair_img, quorum_snapshot_file ]: > # Do a try/except because the test may have deleted some images > try: > os.remove(i) > -- > 2.13.3 > > Reviewed-by: Jeff Cody
