From f87e08f9e24b43d123bbbafd3348bf0f3d67c31a Mon Sep 17 00:00:00 2001 From: "Daniel P. Berrange" Date: Thu, 18 Jan 2018 10:31:43 +0000 Subject: [PATCH 01/56] block: implement the bdrv_reopen_prepare helper for LUKS driver If the bdrv_reopen_prepare helper isn't provided, the qemu-img commit command fails to re-open the base layer after committing changes into it. Provide a no-op implementation for the LUKS driver, since there is not any custom work that needs doing to re-open it. Signed-off-by: Daniel P. Berrange Signed-off-by: Kevin Wolf --- block/crypto.c | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/block/crypto.c b/block/crypto.c index 17b5c0abad..e6095e7807 100644 --- a/block/crypto.c +++ b/block/crypto.c @@ -384,6 +384,12 @@ static void block_crypto_close(BlockDriverState *bs) qcrypto_block_free(crypto->block); } +static int block_crypto_reopen_prepare(BDRVReopenState *state, + BlockReopenQueue *queue, Error **errp) +{ + /* nothing needs checking */ + return 0; +} /* * 1 MB bounce buffer gives good performance / memory tradeoff @@ -621,6 +627,7 @@ BlockDriver bdrv_crypto_luks = { .bdrv_truncate = block_crypto_truncate, .create_opts = &block_crypto_create_opts_luks, + .bdrv_reopen_prepare = block_crypto_reopen_prepare, .bdrv_refresh_limits = block_crypto_refresh_limits, .bdrv_co_preadv = block_crypto_co_preadv, .bdrv_co_pwritev = block_crypto_co_pwritev, From 8b220eb7c8a1b1d5bc2522f394e16456bf20c91f Mon Sep 17 00:00:00 2001 From: Paolo Bonzini Date: Thu, 1 Mar 2018 17:36:14 +0100 Subject: [PATCH 02/56] qcow2: introduce qcow2_write_caches and qcow2_flush_caches They will be used to avoid recursively taking s->lock during bdrv_open or bdrv_check. Signed-off-by: Paolo Bonzini Message-Id: <1516279431-30424-7-git-send-email-pbonzini@redhat.com> Signed-off-by: Paolo Bonzini Signed-off-by: Kevin Wolf --- block/qcow2-refcount.c | 28 ++++++++++++++++++++++++++++ block/qcow2.c | 20 ++++---------------- block/qcow2.h | 2 ++ 3 files changed, 34 insertions(+), 16 deletions(-) diff --git a/block/qcow2-refcount.c b/block/qcow2-refcount.c index 126cca3276..2f7e710fa6 100644 --- a/block/qcow2-refcount.c +++ b/block/qcow2-refcount.c @@ -1171,7 +1171,35 @@ void qcow2_free_any_clusters(BlockDriverState *bs, uint64_t l2_entry, } } +int coroutine_fn qcow2_write_caches(BlockDriverState *bs) +{ + BDRVQcow2State *s = bs->opaque; + int ret; + ret = qcow2_cache_write(bs, s->l2_table_cache); + if (ret < 0) { + return ret; + } + + if (qcow2_need_accurate_refcounts(s)) { + ret = qcow2_cache_write(bs, s->refcount_block_cache); + if (ret < 0) { + return ret; + } + } + + return 0; +} + +int coroutine_fn qcow2_flush_caches(BlockDriverState *bs) +{ + int ret = qcow2_write_caches(bs); + if (ret < 0) { + return ret; + } + + return bdrv_flush(bs->file->bs); +} /*********************************************************/ /* snapshots and image creation */ diff --git a/block/qcow2.c b/block/qcow2.c index 071dc4d608..ee040a49cd 100644 --- a/block/qcow2.c +++ b/block/qcow2.c @@ -500,7 +500,7 @@ static int qcow2_mark_clean(BlockDriverState *bs) s->incompatible_features &= ~QCOW2_INCOMPAT_DIRTY; - ret = bdrv_flush(bs); + ret = qcow2_flush_caches(bs); if (ret < 0) { return ret; } @@ -530,7 +530,7 @@ int qcow2_mark_consistent(BlockDriverState *bs) BDRVQcow2State *s = bs->opaque; if (s->incompatible_features & QCOW2_INCOMPAT_CORRUPT) { - int ret = bdrv_flush(bs); + int ret = qcow2_flush_caches(bs); if (ret < 0) { return ret; } @@ -3647,22 +3647,10 @@ static coroutine_fn int qcow2_co_flush_to_os(BlockDriverState *bs) int ret; qemu_co_mutex_lock(&s->lock); - ret = qcow2_cache_write(bs, s->l2_table_cache); - if (ret < 0) { - qemu_co_mutex_unlock(&s->lock); - return ret; - } - - if (qcow2_need_accurate_refcounts(s)) { - ret = qcow2_cache_write(bs, s->refcount_block_cache); - if (ret < 0) { - qemu_co_mutex_unlock(&s->lock); - return ret; - } - } + ret = qcow2_write_caches(bs); qemu_co_mutex_unlock(&s->lock); - return 0; + return ret; } static BlockMeasureInfo *qcow2_measure(QemuOpts *opts, BlockDriverState *in_bs, diff --git a/block/qcow2.h b/block/qcow2.h index 1a84cc77b0..965846f7af 100644 --- a/block/qcow2.h +++ b/block/qcow2.h @@ -576,6 +576,8 @@ void qcow2_free_any_clusters(BlockDriverState *bs, uint64_t l2_entry, int qcow2_update_snapshot_refcount(BlockDriverState *bs, int64_t l1_table_offset, int l1_size, int addend); +int coroutine_fn qcow2_flush_caches(BlockDriverState *bs); +int coroutine_fn qcow2_write_caches(BlockDriverState *bs); int qcow2_check_refcounts(BlockDriverState *bs, BdrvCheckResult *res, BdrvCheckMode fix); From 1a0c2bfb039c371fe6628d58335705cc7011cecb Mon Sep 17 00:00:00 2001 From: Paolo Bonzini Date: Thu, 1 Mar 2018 17:36:15 +0100 Subject: [PATCH 03/56] qcow2: fix flushing after dirty bitmap metadata writes update_header_sync itself does not need to flush the caches to disk. The only paths that allocate clusters are: - bitmap_list_store with in_place=false, called by update_ext_header_and_dir - store_bitmap_data, called by store_bitmap - store_bitmap, called by qcow2_store_persistent_dirty_bitmaps and followed by update_ext_header_and_dir So in the end the central place where we need to flush the caches is update_ext_header_and_dir. Signed-off-by: Paolo Bonzini Signed-off-by: Kevin Wolf --- block/qcow2-bitmap.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c index 5127276f90..3010adb909 100644 --- a/block/qcow2-bitmap.c +++ b/block/qcow2-bitmap.c @@ -110,7 +110,7 @@ static int update_header_sync(BlockDriverState *bs) return ret; } - return bdrv_flush(bs); + return bdrv_flush(bs->file->bs); } static inline void bitmap_table_to_be(uint64_t *bitmap_table, size_t size) @@ -882,7 +882,7 @@ static int update_ext_header_and_dir(BlockDriverState *bs, return ret; } - ret = bdrv_flush(bs->file->bs); + ret = qcow2_flush_caches(bs); if (ret < 0) { goto fail; } From 1fafcd93681c5370ecf899e096a58642fa68a292 Mon Sep 17 00:00:00 2001 From: Paolo Bonzini Date: Thu, 1 Mar 2018 17:36:16 +0100 Subject: [PATCH 04/56] qcow2: make qcow2_do_open a coroutine_fn It is called from qcow2_invalidate_cache in coroutine context (incoming migration runs in a coroutine), so it's cleaner if metadata is always loaded from a coroutine. Signed-off-by: Paolo Bonzini Message-Id: <1516279431-30424-4-git-send-email-pbonzini@redhat.com> Signed-off-by: Paolo Bonzini Signed-off-by: Kevin Wolf --- block/qcow2.c | 46 +++++++++++++++++++++++++++++++++++++++++----- 1 file changed, 41 insertions(+), 5 deletions(-) diff --git a/block/qcow2.c b/block/qcow2.c index ee040a49cd..4ac1a45e61 100644 --- a/block/qcow2.c +++ b/block/qcow2.c @@ -1118,8 +1118,9 @@ static int qcow2_update_options(BlockDriverState *bs, QDict *options, return ret; } -static int qcow2_do_open(BlockDriverState *bs, QDict *options, int flags, - Error **errp) +/* Called with s->lock held. */ +static int coroutine_fn qcow2_do_open(BlockDriverState *bs, QDict *options, + int flags, Error **errp) { BDRVQcow2State *s = bs->opaque; unsigned int len, i; @@ -1498,8 +1499,6 @@ static int qcow2_do_open(BlockDriverState *bs, QDict *options, int flags, } } - /* Initialise locks */ - qemu_co_mutex_init(&s->lock); bs->supported_zero_flags = header.version >= 3 ? BDRV_REQ_MAY_UNMAP : 0; /* Repair image if dirty */ @@ -1545,16 +1544,53 @@ static int qcow2_do_open(BlockDriverState *bs, QDict *options, int flags, return ret; } +typedef struct QCow2OpenCo { + BlockDriverState *bs; + QDict *options; + int flags; + Error **errp; + int ret; +} QCow2OpenCo; + +static void coroutine_fn qcow2_open_entry(void *opaque) +{ + QCow2OpenCo *qoc = opaque; + BDRVQcow2State *s = qoc->bs->opaque; + + qemu_co_mutex_lock(&s->lock); + qoc->ret = qcow2_do_open(qoc->bs, qoc->options, qoc->flags, qoc->errp); + qemu_co_mutex_unlock(&s->lock); +} + static int qcow2_open(BlockDriverState *bs, QDict *options, int flags, Error **errp) { + BDRVQcow2State *s = bs->opaque; + QCow2OpenCo qoc = { + .bs = bs, + .options = options, + .flags = flags, + .errp = errp, + .ret = -EINPROGRESS + }; + bs->file = bdrv_open_child(NULL, options, "file", bs, &child_file, false, errp); if (!bs->file) { return -EINVAL; } - return qcow2_do_open(bs, options, flags, errp); + /* Initialise locks */ + qemu_co_mutex_init(&s->lock); + + if (qemu_in_coroutine()) { + /* From bdrv_co_create. */ + qcow2_open_entry(&qoc); + } else { + qemu_coroutine_enter(qemu_coroutine_create(qcow2_open_entry, &qoc)); + BDRV_POLL_WHILE(bs, qoc.ret == -EINPROGRESS); + } + return qoc.ret; } static void qcow2_refresh_limits(BlockDriverState *bs, Error **errp) From 9fb4dfc570ce3b013830321b8b91ae2ea991d768 Mon Sep 17 00:00:00 2001 From: Paolo Bonzini Date: Thu, 1 Mar 2018 17:36:17 +0100 Subject: [PATCH 05/56] qed: make bdrv_qed_do_open a coroutine_fn It is called from qed_invalidate_cache in coroutine context (incoming migration runs in a coroutine), so it's cleaner if metadata is always loaded from a coroutine. Signed-off-by: Paolo Bonzini Message-Id: <1516279431-30424-5-git-send-email-pbonzini@redhat.com> Signed-off-by: Paolo Bonzini Signed-off-by: Kevin Wolf --- block/qed.c | 40 +++++++++++++++++++++++++++++++++++++--- 1 file changed, 37 insertions(+), 3 deletions(-) diff --git a/block/qed.c b/block/qed.c index 72cf2f58ab..2996f14fb0 100644 --- a/block/qed.c +++ b/block/qed.c @@ -381,8 +381,9 @@ static void bdrv_qed_init_state(BlockDriverState *bs) qemu_co_queue_init(&s->allocating_write_reqs); } -static int bdrv_qed_do_open(BlockDriverState *bs, QDict *options, int flags, - Error **errp) +/* Called with table_lock held. */ +static int coroutine_fn bdrv_qed_do_open(BlockDriverState *bs, QDict *options, + int flags, Error **errp) { BDRVQEDState *s = bs->opaque; QEDHeader le_header; @@ -513,9 +514,35 @@ out: return ret; } +typedef struct QEDOpenCo { + BlockDriverState *bs; + QDict *options; + int flags; + Error **errp; + int ret; +} QEDOpenCo; + +static void coroutine_fn bdrv_qed_open_entry(void *opaque) +{ + QEDOpenCo *qoc = opaque; + BDRVQEDState *s = qoc->bs->opaque; + + qemu_co_mutex_lock(&s->table_lock); + qoc->ret = bdrv_qed_do_open(qoc->bs, qoc->options, qoc->flags, qoc->errp); + qemu_co_mutex_unlock(&s->table_lock); +} + static int bdrv_qed_open(BlockDriverState *bs, QDict *options, int flags, Error **errp) { + QEDOpenCo qoc = { + .bs = bs, + .options = options, + .flags = flags, + .errp = errp, + .ret = -EINPROGRESS + }; + bs->file = bdrv_open_child(NULL, options, "file", bs, &child_file, false, errp); if (!bs->file) { @@ -523,7 +550,14 @@ static int bdrv_qed_open(BlockDriverState *bs, QDict *options, int flags, } bdrv_qed_init_state(bs); - return bdrv_qed_do_open(bs, options, flags, errp); + if (qemu_in_coroutine()) { + bdrv_qed_open_entry(&qoc); + } else { + qemu_coroutine_enter(qemu_coroutine_create(bdrv_qed_open_entry, &qoc)); + BDRV_POLL_WHILE(bs, qoc.ret == -EINPROGRESS); + } + BDRV_POLL_WHILE(bs, qoc.ret == -EINPROGRESS); + return qoc.ret; } static void bdrv_qed_refresh_limits(BlockDriverState *bs, Error **errp) From 2b148f392b2bfeba76d3e6d9607c3bd072350e8c Mon Sep 17 00:00:00 2001 From: Paolo Bonzini Date: Thu, 1 Mar 2018 17:36:18 +0100 Subject: [PATCH 06/56] block: convert bdrv_invalidate_cache callback to coroutine_fn QED's bdrv_invalidate_cache implementation would like to reuse functions that acquire/release the metadata locks. Call it from coroutine context to simplify the logic. Signed-off-by: Paolo Bonzini Message-Id: <1516279431-30424-6-git-send-email-pbonzini@redhat.com> Signed-off-by: Paolo Bonzini Signed-off-by: Kevin Wolf --- block.c | 41 +++++++++++++++++++++++++++++++++++---- block/iscsi.c | 6 +++--- block/nfs.c | 6 +++--- block/qcow2.c | 7 +++++-- block/qed.c | 13 +++++-------- block/rbd.c | 6 +++--- include/block/block_int.h | 3 ++- 7 files changed, 58 insertions(+), 24 deletions(-) diff --git a/block.c b/block.c index 4f76714f6b..603befb325 100644 --- a/block.c +++ b/block.c @@ -4209,7 +4209,8 @@ void bdrv_init_with_whitelist(void) bdrv_init(); } -void bdrv_invalidate_cache(BlockDriverState *bs, Error **errp) +static void coroutine_fn bdrv_co_invalidate_cache(BlockDriverState *bs, + Error **errp) { BdrvChild *child, *parent; uint64_t perm, shared_perm; @@ -4225,7 +4226,7 @@ void bdrv_invalidate_cache(BlockDriverState *bs, Error **errp) } QLIST_FOREACH(child, &bs->children, next) { - bdrv_invalidate_cache(child->bs, &local_err); + bdrv_co_invalidate_cache(child->bs, &local_err); if (local_err) { error_propagate(errp, local_err); return; @@ -4255,8 +4256,8 @@ void bdrv_invalidate_cache(BlockDriverState *bs, Error **errp) } bdrv_set_perm(bs, perm, shared_perm); - if (bs->drv->bdrv_invalidate_cache) { - bs->drv->bdrv_invalidate_cache(bs, &local_err); + if (bs->drv->bdrv_co_invalidate_cache) { + bs->drv->bdrv_co_invalidate_cache(bs, &local_err); if (local_err) { bs->open_flags |= BDRV_O_INACTIVE; error_propagate(errp, local_err); @@ -4282,6 +4283,38 @@ void bdrv_invalidate_cache(BlockDriverState *bs, Error **errp) } } +typedef struct InvalidateCacheCo { + BlockDriverState *bs; + Error **errp; + bool done; +} InvalidateCacheCo; + +static void coroutine_fn bdrv_invalidate_cache_co_entry(void *opaque) +{ + InvalidateCacheCo *ico = opaque; + bdrv_co_invalidate_cache(ico->bs, ico->errp); + ico->done = true; +} + +void bdrv_invalidate_cache(BlockDriverState *bs, Error **errp) +{ + Coroutine *co; + InvalidateCacheCo ico = { + .bs = bs, + .done = false, + .errp = errp + }; + + if (qemu_in_coroutine()) { + /* Fast-path if already in coroutine context */ + bdrv_invalidate_cache_co_entry(&ico); + } else { + co = qemu_coroutine_create(bdrv_invalidate_cache_co_entry, &ico); + qemu_coroutine_enter(co); + BDRV_POLL_WHILE(bs, !ico.done); + } +} + void bdrv_invalidate_cache_all(Error **errp) { BlockDriverState *bs; diff --git a/block/iscsi.c b/block/iscsi.c index 8bf0e87244..a82170f16e 100644 --- a/block/iscsi.c +++ b/block/iscsi.c @@ -2177,8 +2177,8 @@ static int iscsi_get_info(BlockDriverState *bs, BlockDriverInfo *bdi) return 0; } -static void iscsi_invalidate_cache(BlockDriverState *bs, - Error **errp) +static void coroutine_fn iscsi_co_invalidate_cache(BlockDriverState *bs, + Error **errp) { IscsiLun *iscsilun = bs->opaque; iscsi_allocmap_invalidate(iscsilun); @@ -2209,7 +2209,7 @@ static BlockDriver bdrv_iscsi = { .create_opts = &iscsi_create_opts, .bdrv_reopen_prepare = iscsi_reopen_prepare, .bdrv_reopen_commit = iscsi_reopen_commit, - .bdrv_invalidate_cache = iscsi_invalidate_cache, + .bdrv_co_invalidate_cache = iscsi_co_invalidate_cache, .bdrv_getlength = iscsi_getlength, .bdrv_get_info = iscsi_get_info, diff --git a/block/nfs.c b/block/nfs.c index 1d82ff5042..7433d25856 100644 --- a/block/nfs.c +++ b/block/nfs.c @@ -876,8 +876,8 @@ static void nfs_refresh_filename(BlockDriverState *bs, QDict *options) } #ifdef LIBNFS_FEATURE_PAGECACHE -static void nfs_invalidate_cache(BlockDriverState *bs, - Error **errp) +static void coroutine_fn nfs_co_invalidate_cache(BlockDriverState *bs, + Error **errp) { NFSClient *client = bs->opaque; nfs_pagecache_invalidate(client->context, client->fh); @@ -910,7 +910,7 @@ static BlockDriver bdrv_nfs = { .bdrv_refresh_filename = nfs_refresh_filename, #ifdef LIBNFS_FEATURE_PAGECACHE - .bdrv_invalidate_cache = nfs_invalidate_cache, + .bdrv_co_invalidate_cache = nfs_co_invalidate_cache, #endif }; diff --git a/block/qcow2.c b/block/qcow2.c index 4ac1a45e61..ac8703fa47 100644 --- a/block/qcow2.c +++ b/block/qcow2.c @@ -2142,7 +2142,8 @@ static void qcow2_close(BlockDriverState *bs) qcow2_free_snapshots(bs); } -static void qcow2_invalidate_cache(BlockDriverState *bs, Error **errp) +static void coroutine_fn qcow2_co_invalidate_cache(BlockDriverState *bs, + Error **errp) { BDRVQcow2State *s = bs->opaque; int flags = s->flags; @@ -2165,7 +2166,9 @@ static void qcow2_invalidate_cache(BlockDriverState *bs, Error **errp) options = qdict_clone_shallow(bs->options); flags &= ~BDRV_O_INACTIVE; + qemu_co_mutex_lock(&s->lock); ret = qcow2_do_open(bs, options, flags, &local_err); + qemu_co_mutex_unlock(&s->lock); QDECREF(options); if (local_err) { error_propagate(errp, local_err); @@ -4406,7 +4409,7 @@ BlockDriver bdrv_qcow2 = { .bdrv_change_backing_file = qcow2_change_backing_file, .bdrv_refresh_limits = qcow2_refresh_limits, - .bdrv_invalidate_cache = qcow2_invalidate_cache, + .bdrv_co_invalidate_cache = qcow2_co_invalidate_cache, .bdrv_inactivate = qcow2_inactivate, .create_opts = &qcow2_create_opts, diff --git a/block/qed.c b/block/qed.c index 2996f14fb0..5a5414bfc3 100644 --- a/block/qed.c +++ b/block/qed.c @@ -1521,7 +1521,8 @@ static int bdrv_qed_change_backing_file(BlockDriverState *bs, return ret; } -static void bdrv_qed_invalidate_cache(BlockDriverState *bs, Error **errp) +static void coroutine_fn bdrv_qed_co_invalidate_cache(BlockDriverState *bs, + Error **errp) { BDRVQEDState *s = bs->opaque; Error *local_err = NULL; @@ -1530,13 +1531,9 @@ static void bdrv_qed_invalidate_cache(BlockDriverState *bs, Error **errp) bdrv_qed_close(bs); bdrv_qed_init_state(bs); - if (qemu_in_coroutine()) { - qemu_co_mutex_lock(&s->table_lock); - } + qemu_co_mutex_lock(&s->table_lock); ret = bdrv_qed_do_open(bs, NULL, bs->open_flags, &local_err); - if (qemu_in_coroutine()) { - qemu_co_mutex_unlock(&s->table_lock); - } + qemu_co_mutex_unlock(&s->table_lock); if (local_err) { error_propagate(errp, local_err); error_prepend(errp, "Could not reopen qed layer: "); @@ -1611,7 +1608,7 @@ static BlockDriver bdrv_qed = { .bdrv_get_info = bdrv_qed_get_info, .bdrv_refresh_limits = bdrv_qed_refresh_limits, .bdrv_change_backing_file = bdrv_qed_change_backing_file, - .bdrv_invalidate_cache = bdrv_qed_invalidate_cache, + .bdrv_co_invalidate_cache = bdrv_qed_co_invalidate_cache, .bdrv_check = bdrv_qed_check, .bdrv_detach_aio_context = bdrv_qed_detach_aio_context, .bdrv_attach_aio_context = bdrv_qed_attach_aio_context, diff --git a/block/rbd.c b/block/rbd.c index c7dd32e213..c1275c1ec9 100644 --- a/block/rbd.c +++ b/block/rbd.c @@ -1093,8 +1093,8 @@ static BlockAIOCB *qemu_rbd_aio_pdiscard(BlockDriverState *bs, #endif #ifdef LIBRBD_SUPPORTS_INVALIDATE -static void qemu_rbd_invalidate_cache(BlockDriverState *bs, - Error **errp) +static void coroutine_fn qemu_rbd_co_invalidate_cache(BlockDriverState *bs, + Error **errp) { BDRVRBDState *s = bs->opaque; int r = rbd_invalidate_cache(s->image); @@ -1160,7 +1160,7 @@ static BlockDriver bdrv_rbd = { .bdrv_snapshot_list = qemu_rbd_snap_list, .bdrv_snapshot_goto = qemu_rbd_snap_rollback, #ifdef LIBRBD_SUPPORTS_INVALIDATE - .bdrv_invalidate_cache = qemu_rbd_invalidate_cache, + .bdrv_co_invalidate_cache = qemu_rbd_co_invalidate_cache, #endif }; diff --git a/include/block/block_int.h b/include/block/block_int.h index 64a5700f2b..e391cae53b 100644 --- a/include/block/block_int.h +++ b/include/block/block_int.h @@ -224,7 +224,8 @@ struct BlockDriver { /* * Invalidate any cached meta-data. */ - void (*bdrv_invalidate_cache)(BlockDriverState *bs, Error **errp); + void coroutine_fn (*bdrv_co_invalidate_cache)(BlockDriverState *bs, + Error **errp); int (*bdrv_inactivate)(BlockDriverState *bs); /* From 2fd6163884d74e74dd1ef54ab229d15c61d0b4cb Mon Sep 17 00:00:00 2001 From: Paolo Bonzini Date: Thu, 1 Mar 2018 17:36:19 +0100 Subject: [PATCH 07/56] block: convert bdrv_check callback to coroutine_fn Suggested-by: Kevin Wolf Signed-off-by: Paolo Bonzini Message-Id: <1516279431-30424-8-git-send-email-pbonzini@redhat.com> Signed-off-by: Paolo Bonzini Signed-off-by: Kevin Wolf --- block.c | 43 ++++++++++++++++++++++++++++++++++++--- block/parallels.c | 17 ++++++++++------ block/qcow2.c | 23 +++++++++++++++++---- block/qed-check.c | 1 + block/qed-table.c | 26 ++++++++--------------- block/qed.c | 13 ++++++++---- block/vdi.c | 6 +++--- block/vhdx.c | 7 ++++--- block/vmdk.c | 7 ++++--- include/block/block_int.h | 5 +++-- 10 files changed, 103 insertions(+), 45 deletions(-) diff --git a/block.c b/block.c index 603befb325..8f1c43d037 100644 --- a/block.c +++ b/block.c @@ -3455,17 +3455,54 @@ static void bdrv_delete(BlockDriverState *bs) * free of errors) or -errno when an internal error occurred. The results of the * check are stored in res. */ -int bdrv_check(BlockDriverState *bs, BdrvCheckResult *res, BdrvCheckMode fix) +static int coroutine_fn bdrv_co_check(BlockDriverState *bs, + BdrvCheckResult *res, BdrvCheckMode fix) { if (bs->drv == NULL) { return -ENOMEDIUM; } - if (bs->drv->bdrv_check == NULL) { + if (bs->drv->bdrv_co_check == NULL) { return -ENOTSUP; } memset(res, 0, sizeof(*res)); - return bs->drv->bdrv_check(bs, res, fix); + return bs->drv->bdrv_co_check(bs, res, fix); +} + +typedef struct CheckCo { + BlockDriverState *bs; + BdrvCheckResult *res; + BdrvCheckMode fix; + int ret; +} CheckCo; + +static void bdrv_check_co_entry(void *opaque) +{ + CheckCo *cco = opaque; + cco->ret = bdrv_co_check(cco->bs, cco->res, cco->fix); +} + +int bdrv_check(BlockDriverState *bs, + BdrvCheckResult *res, BdrvCheckMode fix) +{ + Coroutine *co; + CheckCo cco = { + .bs = bs, + .res = res, + .ret = -EINPROGRESS, + .fix = fix, + }; + + if (qemu_in_coroutine()) { + /* Fast-path if already in coroutine context */ + bdrv_check_co_entry(&cco); + } else { + co = qemu_coroutine_create(bdrv_check_co_entry, &cco); + qemu_coroutine_enter(co); + BDRV_POLL_WHILE(bs, cco.ret == -EINPROGRESS); + } + + return cco.ret; } /* diff --git a/block/parallels.c b/block/parallels.c index 81085795c2..c13cb619e6 100644 --- a/block/parallels.c +++ b/block/parallels.c @@ -378,8 +378,9 @@ static coroutine_fn int parallels_co_readv(BlockDriverState *bs, } -static int parallels_check(BlockDriverState *bs, BdrvCheckResult *res, - BdrvCheckMode fix) +static int coroutine_fn parallels_co_check(BlockDriverState *bs, + BdrvCheckResult *res, + BdrvCheckMode fix) { BDRVParallelsState *s = bs->opaque; int64_t size, prev_off, high_off; @@ -394,6 +395,7 @@ static int parallels_check(BlockDriverState *bs, BdrvCheckResult *res, return size; } + qemu_co_mutex_lock(&s->lock); if (s->header_unclean) { fprintf(stderr, "%s image was not closed correctly\n", fix & BDRV_FIX_ERRORS ? "Repairing" : "ERROR"); @@ -442,11 +444,12 @@ static int parallels_check(BlockDriverState *bs, BdrvCheckResult *res, prev_off = off; } + ret = 0; if (flush_bat) { ret = bdrv_pwrite_sync(bs->file, 0, s->header, s->header_size); if (ret < 0) { res->check_errors++; - return ret; + goto out; } } @@ -465,13 +468,15 @@ static int parallels_check(BlockDriverState *bs, BdrvCheckResult *res, if (ret < 0) { error_report_err(local_err); res->check_errors++; - return ret; + goto out; } res->leaks_fixed += count; } } - return 0; +out: + qemu_co_mutex_unlock(&s->lock); + return ret; } @@ -799,7 +804,7 @@ static BlockDriver bdrv_parallels = { .bdrv_co_writev = parallels_co_writev, .supports_backing = true, .bdrv_co_create_opts = parallels_co_create_opts, - .bdrv_check = parallels_check, + .bdrv_co_check = parallels_co_check, .create_opts = ¶llels_create_opts, }; diff --git a/block/qcow2.c b/block/qcow2.c index ac8703fa47..9fb00f2373 100644 --- a/block/qcow2.c +++ b/block/qcow2.c @@ -541,8 +541,9 @@ int qcow2_mark_consistent(BlockDriverState *bs) return 0; } -static int qcow2_check(BlockDriverState *bs, BdrvCheckResult *result, - BdrvCheckMode fix) +static int coroutine_fn qcow2_co_check_locked(BlockDriverState *bs, + BdrvCheckResult *result, + BdrvCheckMode fix) { int ret = qcow2_check_refcounts(bs, result, fix); if (ret < 0) { @@ -559,6 +560,19 @@ static int qcow2_check(BlockDriverState *bs, BdrvCheckResult *result, return ret; } +static int coroutine_fn qcow2_co_check(BlockDriverState *bs, + BdrvCheckResult *result, + BdrvCheckMode fix) +{ + BDRVQcow2State *s = bs->opaque; + int ret; + + qemu_co_mutex_lock(&s->lock); + ret = qcow2_co_check_locked(bs, result, fix); + qemu_co_mutex_unlock(&s->lock); + return ret; +} + static int validate_table_offset(BlockDriverState *bs, uint64_t offset, uint64_t entries, size_t entry_len) { @@ -1506,7 +1520,8 @@ static int coroutine_fn qcow2_do_open(BlockDriverState *bs, QDict *options, (s->incompatible_features & QCOW2_INCOMPAT_DIRTY)) { BdrvCheckResult result = {0}; - ret = qcow2_check(bs, &result, BDRV_FIX_ERRORS | BDRV_FIX_LEAKS); + ret = qcow2_co_check_locked(bs, &result, + BDRV_FIX_ERRORS | BDRV_FIX_LEAKS); if (ret < 0 || result.check_errors) { if (ret >= 0) { ret = -EIO; @@ -4413,7 +4428,7 @@ BlockDriver bdrv_qcow2 = { .bdrv_inactivate = qcow2_inactivate, .create_opts = &qcow2_create_opts, - .bdrv_check = qcow2_check, + .bdrv_co_check = qcow2_co_check, .bdrv_amend_options = qcow2_amend_options, .bdrv_detach_aio_context = qcow2_detach_aio_context, diff --git a/block/qed-check.c b/block/qed-check.c index dcd4f036b8..0edac03159 100644 --- a/block/qed-check.c +++ b/block/qed-check.c @@ -217,6 +217,7 @@ static void qed_check_mark_clean(BDRVQEDState *s, BdrvCheckResult *result) qed_write_header_sync(s); } +/* Called with table_lock held. */ int qed_check(BDRVQEDState *s, BdrvCheckResult *result, bool fix) { QEDCheck check = { diff --git a/block/qed-table.c b/block/qed-table.c index eead8b0fc7..7df5680adb 100644 --- a/block/qed-table.c +++ b/block/qed-table.c @@ -18,7 +18,7 @@ #include "qed.h" #include "qemu/bswap.h" -/* Called either from qed_check or with table_lock held. */ +/* Called with table_lock held. */ static int qed_read_table(BDRVQEDState *s, uint64_t offset, QEDTable *table) { QEMUIOVector qiov; @@ -33,13 +33,9 @@ static int qed_read_table(BDRVQEDState *s, uint64_t offset, QEDTable *table) trace_qed_read_table(s, offset, table); - if (qemu_in_coroutine()) { - qemu_co_mutex_unlock(&s->table_lock); - } + qemu_co_mutex_unlock(&s->table_lock); ret = bdrv_preadv(s->bs->file, offset, &qiov); - if (qemu_in_coroutine()) { - qemu_co_mutex_lock(&s->table_lock); - } + qemu_co_mutex_lock(&s->table_lock); if (ret < 0) { goto out; } @@ -67,7 +63,7 @@ out: * @n: Number of elements * @flush: Whether or not to sync to disk * - * Called either from qed_check or with table_lock held. + * Called with table_lock held. */ static int qed_write_table(BDRVQEDState *s, uint64_t offset, QEDTable *table, unsigned int index, unsigned int n, bool flush) @@ -104,13 +100,9 @@ static int qed_write_table(BDRVQEDState *s, uint64_t offset, QEDTable *table, /* Adjust for offset into table */ offset += start * sizeof(uint64_t); - if (qemu_in_coroutine()) { - qemu_co_mutex_unlock(&s->table_lock); - } + qemu_co_mutex_unlock(&s->table_lock); ret = bdrv_pwritev(s->bs->file, offset, &qiov); - if (qemu_in_coroutine()) { - qemu_co_mutex_lock(&s->table_lock); - } + qemu_co_mutex_lock(&s->table_lock); trace_qed_write_table_cb(s, table, flush, ret); if (ret < 0) { goto out; @@ -134,7 +126,7 @@ int qed_read_l1_table_sync(BDRVQEDState *s) return qed_read_table(s, s->header.l1_table_offset, s->l1_table); } -/* Called either from qed_check or with table_lock held. */ +/* Called with table_lock held. */ int qed_write_l1_table(BDRVQEDState *s, unsigned int index, unsigned int n) { BLKDBG_EVENT(s->bs->file, BLKDBG_L1_UPDATE); @@ -148,7 +140,7 @@ int qed_write_l1_table_sync(BDRVQEDState *s, unsigned int index, return qed_write_l1_table(s, index, n); } -/* Called either from qed_check or with table_lock held. */ +/* Called with table_lock held. */ int qed_read_l2_table(BDRVQEDState *s, QEDRequest *request, uint64_t offset) { int ret; @@ -191,7 +183,7 @@ int qed_read_l2_table_sync(BDRVQEDState *s, QEDRequest *request, uint64_t offset return qed_read_l2_table(s, request, offset); } -/* Called either from qed_check or with table_lock held. */ +/* Called with table_lock held. */ int qed_write_l2_table(BDRVQEDState *s, QEDRequest *request, unsigned int index, unsigned int n, bool flush) { diff --git a/block/qed.c b/block/qed.c index 5a5414bfc3..5e6a6bfaa0 100644 --- a/block/qed.c +++ b/block/qed.c @@ -1544,12 +1544,17 @@ static void coroutine_fn bdrv_qed_co_invalidate_cache(BlockDriverState *bs, } } -static int bdrv_qed_check(BlockDriverState *bs, BdrvCheckResult *result, - BdrvCheckMode fix) +static int bdrv_qed_co_check(BlockDriverState *bs, BdrvCheckResult *result, + BdrvCheckMode fix) { BDRVQEDState *s = bs->opaque; + int ret; - return qed_check(s, result, !!fix); + qemu_co_mutex_lock(&s->table_lock); + ret = qed_check(s, result, !!fix); + qemu_co_mutex_unlock(&s->table_lock); + + return ret; } static QemuOptsList qed_create_opts = { @@ -1609,7 +1614,7 @@ static BlockDriver bdrv_qed = { .bdrv_refresh_limits = bdrv_qed_refresh_limits, .bdrv_change_backing_file = bdrv_qed_change_backing_file, .bdrv_co_invalidate_cache = bdrv_qed_co_invalidate_cache, - .bdrv_check = bdrv_qed_check, + .bdrv_co_check = bdrv_qed_co_check, .bdrv_detach_aio_context = bdrv_qed_detach_aio_context, .bdrv_attach_aio_context = bdrv_qed_attach_aio_context, .bdrv_co_drain_begin = bdrv_qed_co_drain_begin, diff --git a/block/vdi.c b/block/vdi.c index 68592cc58d..2b5ddd0666 100644 --- a/block/vdi.c +++ b/block/vdi.c @@ -263,8 +263,8 @@ static void vdi_header_print(VdiHeader *header) } #endif -static int vdi_check(BlockDriverState *bs, BdrvCheckResult *res, - BdrvCheckMode fix) +static int coroutine_fn vdi_co_check(BlockDriverState *bs, BdrvCheckResult *res, + BdrvCheckMode fix) { /* TODO: additional checks possible. */ BDRVVdiState *s = (BDRVVdiState *)bs->opaque; @@ -908,7 +908,7 @@ static BlockDriver bdrv_vdi = { .bdrv_get_info = vdi_get_info, .create_opts = &vdi_create_opts, - .bdrv_check = vdi_check, + .bdrv_co_check = vdi_co_check, }; static void bdrv_vdi_init(void) diff --git a/block/vhdx.c b/block/vhdx.c index 3fbff5048b..d82350d07c 100644 --- a/block/vhdx.c +++ b/block/vhdx.c @@ -1944,8 +1944,9 @@ exit: * r/w and any log has already been replayed, so there is nothing (currently) * for us to do here */ -static int vhdx_check(BlockDriverState *bs, BdrvCheckResult *result, - BdrvCheckMode fix) +static int coroutine_fn vhdx_co_check(BlockDriverState *bs, + BdrvCheckResult *result, + BdrvCheckMode fix) { BDRVVHDXState *s = bs->opaque; @@ -2006,7 +2007,7 @@ static BlockDriver bdrv_vhdx = { .bdrv_co_writev = vhdx_co_writev, .bdrv_co_create_opts = vhdx_co_create_opts, .bdrv_get_info = vhdx_get_info, - .bdrv_check = vhdx_check, + .bdrv_co_check = vhdx_co_check, .bdrv_has_zero_init = bdrv_has_zero_init_1, .create_opts = &vhdx_create_opts, diff --git a/block/vmdk.c b/block/vmdk.c index 67342ed69b..f94c49a9c0 100644 --- a/block/vmdk.c +++ b/block/vmdk.c @@ -2221,8 +2221,9 @@ static ImageInfo *vmdk_get_extent_info(VmdkExtent *extent) return info; } -static int vmdk_check(BlockDriverState *bs, BdrvCheckResult *result, - BdrvCheckMode fix) +static int coroutine_fn vmdk_co_check(BlockDriverState *bs, + BdrvCheckResult *result, + BdrvCheckMode fix) { BDRVVmdkState *s = bs->opaque; VmdkExtent *extent = NULL; @@ -2391,7 +2392,7 @@ static BlockDriver bdrv_vmdk = { .instance_size = sizeof(BDRVVmdkState), .bdrv_probe = vmdk_probe, .bdrv_open = vmdk_open, - .bdrv_check = vmdk_check, + .bdrv_co_check = vmdk_co_check, .bdrv_reopen_prepare = vmdk_reopen_prepare, .bdrv_child_perm = bdrv_format_default_perms, .bdrv_co_preadv = vmdk_co_preadv, diff --git a/include/block/block_int.h b/include/block/block_int.h index e391cae53b..a84cc04d55 100644 --- a/include/block/block_int.h +++ b/include/block/block_int.h @@ -307,8 +307,9 @@ struct BlockDriver { * Returns 0 for completed check, -errno for internal errors. * The check results are stored in result. */ - int (*bdrv_check)(BlockDriverState *bs, BdrvCheckResult *result, - BdrvCheckMode fix); + int coroutine_fn (*bdrv_co_check)(BlockDriverState *bs, + BdrvCheckResult *result, + BdrvCheckMode fix); int (*bdrv_amend_options)(BlockDriverState *bs, QemuOpts *opts, BlockDriverAmendStatusCB *status_cb, From 0cf0e5980bb95a914a40ba0eb8e69c3d75c513fb Mon Sep 17 00:00:00 2001 From: Alberto Garcia Date: Tue, 6 Mar 2018 18:14:06 +0200 Subject: [PATCH 08/56] qcow2: Generalize validate_table_offset() into qcow2_validate_table() This function checks that the offset and size of a table are valid. While the offset checks are fine, the size check is too generic, since it only verifies that the total size in bytes fits in a 64-bit integer. In practice all tables used in qcow2 have much smaller size limits, so the size needs to be checked again for each table using its actual limit. This patch generalizes this function by allowing the caller to specify the maximum size for that table. In addition to that it allows passing an Error variable. The function is also renamed and made public since we're going to use it in other parts of the code. Signed-off-by: Alberto Garcia Reviewed-by: Eric Blake Signed-off-by: Kevin Wolf --- block/qcow2.c | 77 +++++++++++++++----------------------- block/qcow2.h | 10 ++--- tests/qemu-iotests/080.out | 16 ++++---- 3 files changed, 43 insertions(+), 60 deletions(-) diff --git a/block/qcow2.c b/block/qcow2.c index 9fb00f2373..369e374a9b 100644 --- a/block/qcow2.c +++ b/block/qcow2.c @@ -573,26 +573,23 @@ static int coroutine_fn qcow2_co_check(BlockDriverState *bs, return ret; } -static int validate_table_offset(BlockDriverState *bs, uint64_t offset, - uint64_t entries, size_t entry_len) +int qcow2_validate_table(BlockDriverState *bs, uint64_t offset, + uint64_t entries, size_t entry_len, + int64_t max_size_bytes, const char *table_name, + Error **errp) { BDRVQcow2State *s = bs->opaque; - uint64_t size; + + if (entries > max_size_bytes / entry_len) { + error_setg(errp, "%s too large", table_name); + return -EFBIG; + } /* Use signed INT64_MAX as the maximum even for uint64_t header fields, * because values will be passed to qemu functions taking int64_t. */ - if (entries > INT64_MAX / entry_len) { - return -EINVAL; - } - - size = entries * entry_len; - - if (INT64_MAX - size < offset) { - return -EINVAL; - } - - /* Tables must be cluster aligned */ - if (offset_into_cluster(s, offset) != 0) { + if ((INT64_MAX - entries * entry_len < offset) || + (offset_into_cluster(s, offset) != 0)) { + error_setg(errp, "%s offset invalid", table_name); return -EINVAL; } @@ -1323,47 +1320,42 @@ static int coroutine_fn qcow2_do_open(BlockDriverState *bs, QDict *options, s->refcount_table_size = header.refcount_table_clusters << (s->cluster_bits - 3); - if (header.refcount_table_clusters > qcow2_max_refcount_clusters(s)) { - error_setg(errp, "Reference count table too large"); - ret = -EINVAL; - goto fail; - } - if (header.refcount_table_clusters == 0 && !(flags & BDRV_O_CHECK)) { error_setg(errp, "Image does not contain a reference count table"); ret = -EINVAL; goto fail; } - ret = validate_table_offset(bs, s->refcount_table_offset, - s->refcount_table_size, sizeof(uint64_t)); + ret = qcow2_validate_table(bs, s->refcount_table_offset, + header.refcount_table_clusters, + s->cluster_size, QCOW_MAX_REFTABLE_SIZE, + "Reference count table", errp); if (ret < 0) { - error_setg(errp, "Invalid reference count table offset"); goto fail; } - /* Snapshot table offset/length */ - if (header.nb_snapshots > QCOW_MAX_SNAPSHOTS) { - error_setg(errp, "Too many snapshots"); - ret = -EINVAL; - goto fail; - } - - ret = validate_table_offset(bs, header.snapshots_offset, - header.nb_snapshots, - sizeof(QCowSnapshotHeader)); + /* The total size in bytes of the snapshot table is checked in + * qcow2_read_snapshots() because the size of each snapshot is + * variable and we don't know it yet. + * Here we only check the offset and number of snapshots. */ + ret = qcow2_validate_table(bs, header.snapshots_offset, + header.nb_snapshots, + sizeof(QCowSnapshotHeader), + sizeof(QCowSnapshotHeader) * QCOW_MAX_SNAPSHOTS, + "Snapshot table", errp); if (ret < 0) { - error_setg(errp, "Invalid snapshot table offset"); goto fail; } /* read the level 1 table */ - if (header.l1_size > QCOW_MAX_L1_SIZE / sizeof(uint64_t)) { - error_setg(errp, "Active L1 table too large"); - ret = -EFBIG; + ret = qcow2_validate_table(bs, header.l1_table_offset, + header.l1_size, sizeof(uint64_t), + QCOW_MAX_L1_SIZE, "Active L1 table", errp); + if (ret < 0) { goto fail; } s->l1_size = header.l1_size; + s->l1_table_offset = header.l1_table_offset; l1_vm_state_index = size_to_l1(s, header.size); if (l1_vm_state_index > INT_MAX) { @@ -1381,15 +1373,6 @@ static int coroutine_fn qcow2_do_open(BlockDriverState *bs, QDict *options, goto fail; } - ret = validate_table_offset(bs, header.l1_table_offset, - header.l1_size, sizeof(uint64_t)); - if (ret < 0) { - error_setg(errp, "Invalid L1 table offset"); - goto fail; - } - s->l1_table_offset = header.l1_table_offset; - - if (s->l1_size > 0) { s->l1_table = qemu_try_blockalign(bs->file->bs, ROUND_UP(s->l1_size * sizeof(uint64_t), 512)); diff --git a/block/qcow2.h b/block/qcow2.h index 965846f7af..ccb92a9696 100644 --- a/block/qcow2.h +++ b/block/qcow2.h @@ -485,11 +485,6 @@ static inline int64_t qcow2_vm_state_offset(BDRVQcow2State *s) return (int64_t)s->l1_vm_state_index << (s->cluster_bits + s->l2_bits); } -static inline uint64_t qcow2_max_refcount_clusters(BDRVQcow2State *s) -{ - return QCOW_MAX_REFTABLE_SIZE >> s->cluster_bits; -} - static inline QCow2ClusterType qcow2_get_cluster_type(uint64_t l2_entry) { if (l2_entry & QCOW_OFLAG_COMPRESSED) { @@ -547,6 +542,11 @@ void qcow2_signal_corruption(BlockDriverState *bs, bool fatal, int64_t offset, int64_t size, const char *message_format, ...) GCC_FMT_ATTR(5, 6); +int qcow2_validate_table(BlockDriverState *bs, uint64_t offset, + uint64_t entries, size_t entry_len, + int64_t max_size_bytes, const char *table_name, + Error **errp); + /* qcow2-refcount.c functions */ int qcow2_refcount_init(BlockDriverState *bs); void qcow2_refcount_close(BlockDriverState *bs); diff --git a/tests/qemu-iotests/080.out b/tests/qemu-iotests/080.out index 6a7fda1356..4c7790c9ee 100644 --- a/tests/qemu-iotests/080.out +++ b/tests/qemu-iotests/080.out @@ -18,18 +18,18 @@ can't open device TEST_DIR/t.qcow2: Reference count table too large == Misaligned refcount table == Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=67108864 -can't open device TEST_DIR/t.qcow2: Invalid reference count table offset +can't open device TEST_DIR/t.qcow2: Reference count table offset invalid == Huge refcount offset == Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=67108864 -can't open device TEST_DIR/t.qcow2: Invalid reference count table offset +can't open device TEST_DIR/t.qcow2: Reference count table offset invalid == Invalid snapshot table == Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=67108864 -can't open device TEST_DIR/t.qcow2: Too many snapshots -can't open device TEST_DIR/t.qcow2: Too many snapshots -can't open device TEST_DIR/t.qcow2: Invalid snapshot table offset -can't open device TEST_DIR/t.qcow2: Invalid snapshot table offset +can't open device TEST_DIR/t.qcow2: Snapshot table too large +can't open device TEST_DIR/t.qcow2: Snapshot table too large +can't open device TEST_DIR/t.qcow2: Snapshot table offset invalid +can't open device TEST_DIR/t.qcow2: Snapshot table offset invalid == Hitting snapshot table size limit == Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=67108864 @@ -41,8 +41,8 @@ read 512/512 bytes at offset 0 Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=67108864 can't open device TEST_DIR/t.qcow2: Active L1 table too large can't open device TEST_DIR/t.qcow2: Active L1 table too large -can't open device TEST_DIR/t.qcow2: Invalid L1 table offset -can't open device TEST_DIR/t.qcow2: Invalid L1 table offset +can't open device TEST_DIR/t.qcow2: Active L1 table offset invalid +can't open device TEST_DIR/t.qcow2: Active L1 table offset invalid == Invalid L1 table (with internal snapshot in the image) == Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=67108864 From 314e8d3928aa82fa13673d47b5af7bdd9a6d82bf Mon Sep 17 00:00:00 2001 From: Alberto Garcia Date: Tue, 6 Mar 2018 18:14:07 +0200 Subject: [PATCH 09/56] qcow2: Check L1 table offset in qcow2_snapshot_load_tmp() This function checks that the size of a snapshot's L1 table is not too large, but it doesn't validate the offset. We now have a function to take care of this, so let's use it. Signed-off-by: Alberto Garcia Reviewed-by: Eric Blake Signed-off-by: Kevin Wolf --- block/qcow2-snapshot.c | 8 +++++--- tests/qemu-iotests/080 | 10 +++++++++- tests/qemu-iotests/080.out | 8 +++++++- 3 files changed, 21 insertions(+), 5 deletions(-) diff --git a/block/qcow2-snapshot.c b/block/qcow2-snapshot.c index cee25f582b..bf94f4f434 100644 --- a/block/qcow2-snapshot.c +++ b/block/qcow2-snapshot.c @@ -704,9 +704,11 @@ int qcow2_snapshot_load_tmp(BlockDriverState *bs, sn = &s->snapshots[snapshot_index]; /* Allocate and read in the snapshot's L1 table */ - if (sn->l1_size > QCOW_MAX_L1_SIZE / sizeof(uint64_t)) { - error_setg(errp, "Snapshot L1 table too large"); - return -EFBIG; + ret = qcow2_validate_table(bs, sn->l1_table_offset, sn->l1_size, + sizeof(uint64_t), QCOW_MAX_L1_SIZE, + "Snapshot L1 table", errp); + if (ret < 0) { + return ret; } new_l1_bytes = sn->l1_size * sizeof(uint64_t); new_l1_table = qemu_try_blockalign(bs->file->bs, diff --git a/tests/qemu-iotests/080 b/tests/qemu-iotests/080 index 1c2bd85742..6a10e7defa 100755 --- a/tests/qemu-iotests/080 +++ b/tests/qemu-iotests/080 @@ -171,7 +171,15 @@ poke_file "$TEST_IMG" "$offset_l2_table_0" "\x80\x00\x00\xff\xff\xff\x00\x00" { $QEMU_IMG snapshot -c test $TEST_IMG; } 2>&1 | _filter_qemu_io | _filter_testdir echo -echo "== Invalid snapshot L1 table ==" +echo "== Invalid snapshot L1 table offset ==" +_make_test_img 64M +{ $QEMU_IO -c "write 0 512" $TEST_IMG; } 2>&1 | _filter_qemu_io | _filter_testdir +{ $QEMU_IMG snapshot -c test $TEST_IMG; } 2>&1 | _filter_testdir +poke_file "$TEST_IMG" "$offset_snap1_l1_offset" "\x00\x00\x00\x00\x00\x40\x02\x00" +{ $QEMU_IMG convert -s test $TEST_IMG $TEST_IMG.snap; } 2>&1 | _filter_testdir + +echo +echo "== Invalid snapshot L1 table size ==" _make_test_img 64M { $QEMU_IO -c "write 0 512" $TEST_IMG; } 2>&1 | _filter_qemu_io | _filter_testdir { $QEMU_IMG snapshot -c test $TEST_IMG; } 2>&1 | _filter_testdir diff --git a/tests/qemu-iotests/080.out b/tests/qemu-iotests/080.out index 4c7790c9ee..f0d9038d55 100644 --- a/tests/qemu-iotests/080.out +++ b/tests/qemu-iotests/080.out @@ -59,7 +59,13 @@ wrote 512/512 bytes at offset 0 qemu-img: Could not create snapshot 'test': -27 (File too large) qemu-img: Could not create snapshot 'test': -11 (Resource temporarily unavailable) -== Invalid snapshot L1 table == +== Invalid snapshot L1 table offset == +Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=67108864 +wrote 512/512 bytes at offset 0 +512 bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) +qemu-img: Failed to load snapshot: Snapshot L1 table offset invalid + +== Invalid snapshot L1 table size == Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=67108864 wrote 512/512 bytes at offset 0 512 bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) From c9a442e45094e693da45f0e3d03746d68e4460ec Mon Sep 17 00:00:00 2001 From: Alberto Garcia Date: Tue, 6 Mar 2018 18:14:08 +0200 Subject: [PATCH 10/56] qcow2: Check L1 table parameters in qcow2_expand_zero_clusters() This function iterates over all snapshots of a qcow2 file in order to expand all zero clusters, but it does not validate the snapshots' L1 tables first. We now have a function to take care of this, so let's use it. We can also take the opportunity to replace the sector-based bdrv_read() with bdrv_pread(). Cc: Eric Blake Signed-off-by: Alberto Garcia Reviewed-by: Eric Blake Signed-off-by: Kevin Wolf --- block/qcow2-cluster.c | 24 +++++++++++++++++------- tests/qemu-iotests/080 | 2 ++ tests/qemu-iotests/080.out | 4 ++++ 3 files changed, 23 insertions(+), 7 deletions(-) diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c index 98908c4264..1aee726c6a 100644 --- a/block/qcow2-cluster.c +++ b/block/qcow2-cluster.c @@ -25,6 +25,7 @@ #include "qemu/osdep.h" #include +#include "qapi/error.h" #include "qemu-common.h" #include "block/block_int.h" #include "block/qcow2.h" @@ -2092,11 +2093,21 @@ int qcow2_expand_zero_clusters(BlockDriverState *bs, } for (i = 0; i < s->nb_snapshots; i++) { - int l1_sectors = DIV_ROUND_UP(s->snapshots[i].l1_size * - sizeof(uint64_t), BDRV_SECTOR_SIZE); + int l1_size2; + uint64_t *new_l1_table; + Error *local_err = NULL; - uint64_t *new_l1_table = - g_try_realloc(l1_table, l1_sectors * BDRV_SECTOR_SIZE); + ret = qcow2_validate_table(bs, s->snapshots[i].l1_table_offset, + s->snapshots[i].l1_size, sizeof(uint64_t), + QCOW_MAX_L1_SIZE, "Snapshot L1 table", + &local_err); + if (ret < 0) { + error_report_err(local_err); + goto fail; + } + + l1_size2 = s->snapshots[i].l1_size * sizeof(uint64_t); + new_l1_table = g_try_realloc(l1_table, l1_size2); if (!new_l1_table) { ret = -ENOMEM; @@ -2105,9 +2116,8 @@ int qcow2_expand_zero_clusters(BlockDriverState *bs, l1_table = new_l1_table; - ret = bdrv_read(bs->file, - s->snapshots[i].l1_table_offset / BDRV_SECTOR_SIZE, - (void *)l1_table, l1_sectors); + ret = bdrv_pread(bs->file, s->snapshots[i].l1_table_offset, + l1_table, l1_size2); if (ret < 0) { goto fail; } diff --git a/tests/qemu-iotests/080 b/tests/qemu-iotests/080 index 6a10e7defa..5622604f83 100755 --- a/tests/qemu-iotests/080 +++ b/tests/qemu-iotests/080 @@ -177,6 +177,7 @@ _make_test_img 64M { $QEMU_IMG snapshot -c test $TEST_IMG; } 2>&1 | _filter_testdir poke_file "$TEST_IMG" "$offset_snap1_l1_offset" "\x00\x00\x00\x00\x00\x40\x02\x00" { $QEMU_IMG convert -s test $TEST_IMG $TEST_IMG.snap; } 2>&1 | _filter_testdir +{ $QEMU_IMG amend -o compat=0.10 $TEST_IMG; } 2>&1 | _filter_testdir echo echo "== Invalid snapshot L1 table size ==" @@ -185,6 +186,7 @@ _make_test_img 64M { $QEMU_IMG snapshot -c test $TEST_IMG; } 2>&1 | _filter_testdir poke_file "$TEST_IMG" "$offset_snap1_l1_size" "\x10\x00\x00\x00" { $QEMU_IMG convert -s test $TEST_IMG $TEST_IMG.snap; } 2>&1 | _filter_testdir +{ $QEMU_IMG amend -o compat=0.10 $TEST_IMG; } 2>&1 | _filter_testdir # success, all done echo "*** done" diff --git a/tests/qemu-iotests/080.out b/tests/qemu-iotests/080.out index f0d9038d55..315edbc26f 100644 --- a/tests/qemu-iotests/080.out +++ b/tests/qemu-iotests/080.out @@ -64,10 +64,14 @@ Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=67108864 wrote 512/512 bytes at offset 0 512 bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) qemu-img: Failed to load snapshot: Snapshot L1 table offset invalid +qemu-img: Snapshot L1 table offset invalid +qemu-img: Error while amending options: Invalid argument == Invalid snapshot L1 table size == Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=67108864 wrote 512/512 bytes at offset 0 512 bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) qemu-img: Failed to load snapshot: Snapshot L1 table too large +qemu-img: Snapshot L1 table too large +qemu-img: Error while amending options: File too large *** done From c7a9d81d7061a7fb3b5b5726a6f139444e1ad5e6 Mon Sep 17 00:00:00 2001 From: Alberto Garcia Date: Tue, 6 Mar 2018 18:14:09 +0200 Subject: [PATCH 11/56] qcow2: Check snapshot L1 tables in qcow2_check_metadata_overlap() The inactive-l2 overlap check iterates uses the L1 tables from all snapshots, but it does not validate them first. We now have a function to take care of this, so let's use it. Signed-off-by: Alberto Garcia Reviewed-by: Eric Blake Signed-off-by: Kevin Wolf --- block/qcow2-refcount.c | 10 +++++++++- tests/qemu-iotests/080 | 4 ++++ tests/qemu-iotests/080.out | 4 ++++ 3 files changed, 17 insertions(+), 1 deletion(-) diff --git a/block/qcow2-refcount.c b/block/qcow2-refcount.c index 2f7e710fa6..b18ea0ca98 100644 --- a/block/qcow2-refcount.c +++ b/block/qcow2-refcount.c @@ -2642,9 +2642,17 @@ int qcow2_check_metadata_overlap(BlockDriverState *bs, int ign, int64_t offset, uint64_t l1_ofs = s->snapshots[i].l1_table_offset; uint32_t l1_sz = s->snapshots[i].l1_size; uint64_t l1_sz2 = l1_sz * sizeof(uint64_t); - uint64_t *l1 = g_try_malloc(l1_sz2); + uint64_t *l1; int ret; + ret = qcow2_validate_table(bs, l1_ofs, l1_sz, sizeof(uint64_t), + QCOW_MAX_L1_SIZE, "", NULL); + if (ret < 0) { + return ret; + } + + l1 = g_try_malloc(l1_sz2); + if (l1_sz2 && l1 == NULL) { return -ENOMEM; } diff --git a/tests/qemu-iotests/080 b/tests/qemu-iotests/080 index 5622604f83..018f815697 100755 --- a/tests/qemu-iotests/080 +++ b/tests/qemu-iotests/080 @@ -178,6 +178,8 @@ _make_test_img 64M poke_file "$TEST_IMG" "$offset_snap1_l1_offset" "\x00\x00\x00\x00\x00\x40\x02\x00" { $QEMU_IMG convert -s test $TEST_IMG $TEST_IMG.snap; } 2>&1 | _filter_testdir { $QEMU_IMG amend -o compat=0.10 $TEST_IMG; } 2>&1 | _filter_testdir +{ $QEMU_IO -c "open -o overlap-check.inactive-l2=on $TEST_IMG" \ + -c 'write 0 4k'; } 2>&1 | _filter_qemu_io | _filter_testdir echo echo "== Invalid snapshot L1 table size ==" @@ -187,6 +189,8 @@ _make_test_img 64M poke_file "$TEST_IMG" "$offset_snap1_l1_size" "\x10\x00\x00\x00" { $QEMU_IMG convert -s test $TEST_IMG $TEST_IMG.snap; } 2>&1 | _filter_testdir { $QEMU_IMG amend -o compat=0.10 $TEST_IMG; } 2>&1 | _filter_testdir +{ $QEMU_IO -c "open -o overlap-check.inactive-l2=on $TEST_IMG" \ + -c 'write 0 4k'; } 2>&1 | _filter_qemu_io | _filter_testdir # success, all done echo "*** done" diff --git a/tests/qemu-iotests/080.out b/tests/qemu-iotests/080.out index 315edbc26f..d622ac06c0 100644 --- a/tests/qemu-iotests/080.out +++ b/tests/qemu-iotests/080.out @@ -66,6 +66,8 @@ wrote 512/512 bytes at offset 0 qemu-img: Failed to load snapshot: Snapshot L1 table offset invalid qemu-img: Snapshot L1 table offset invalid qemu-img: Error while amending options: Invalid argument +Failed to flush the refcount block cache: Invalid argument +write failed: Invalid argument == Invalid snapshot L1 table size == Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=67108864 @@ -74,4 +76,6 @@ wrote 512/512 bytes at offset 0 qemu-img: Failed to load snapshot: Snapshot L1 table too large qemu-img: Snapshot L1 table too large qemu-img: Error while amending options: File too large +Failed to flush the refcount block cache: File too large +write failed: File too large *** done From a8475d7573c0598ba8f92f84534110218ee11e3d Mon Sep 17 00:00:00 2001 From: Alberto Garcia Date: Tue, 6 Mar 2018 18:14:10 +0200 Subject: [PATCH 12/56] qcow2: Check snapshot L1 table in qcow2_snapshot_goto() This function copies a snapshot's L1 table into the active one without validating it first. We now have a function to take care of this, so let's use it. Cc: Eric Blake Signed-off-by: Alberto Garcia Reviewed-by: Eric Blake Signed-off-by: Kevin Wolf --- block/qcow2-snapshot.c | 9 +++++++++ tests/qemu-iotests/080 | 2 ++ tests/qemu-iotests/080.out | 4 ++++ 3 files changed, 15 insertions(+) diff --git a/block/qcow2-snapshot.c b/block/qcow2-snapshot.c index bf94f4f434..0faf728dc4 100644 --- a/block/qcow2-snapshot.c +++ b/block/qcow2-snapshot.c @@ -465,6 +465,7 @@ int qcow2_snapshot_goto(BlockDriverState *bs, const char *snapshot_id) { BDRVQcow2State *s = bs->opaque; QCowSnapshot *sn; + Error *local_err = NULL; int i, snapshot_index; int cur_l1_bytes, sn_l1_bytes; int ret; @@ -477,6 +478,14 @@ int qcow2_snapshot_goto(BlockDriverState *bs, const char *snapshot_id) } sn = &s->snapshots[snapshot_index]; + ret = qcow2_validate_table(bs, sn->l1_table_offset, sn->l1_size, + sizeof(uint64_t), QCOW_MAX_L1_SIZE, + "Snapshot L1 table", &local_err); + if (ret < 0) { + error_report_err(local_err); + goto fail; + } + if (sn->disk_size != bs->total_sectors * BDRV_SECTOR_SIZE) { error_report("qcow2: Loading snapshots with different disk " "size is not implemented"); diff --git a/tests/qemu-iotests/080 b/tests/qemu-iotests/080 index 018f815697..538857310f 100755 --- a/tests/qemu-iotests/080 +++ b/tests/qemu-iotests/080 @@ -180,6 +180,7 @@ poke_file "$TEST_IMG" "$offset_snap1_l1_offset" "\x00\x00\x00\x00\x00\x40\x02\x0 { $QEMU_IMG amend -o compat=0.10 $TEST_IMG; } 2>&1 | _filter_testdir { $QEMU_IO -c "open -o overlap-check.inactive-l2=on $TEST_IMG" \ -c 'write 0 4k'; } 2>&1 | _filter_qemu_io | _filter_testdir +{ $QEMU_IMG snapshot -a test $TEST_IMG; } 2>&1 | _filter_testdir echo echo "== Invalid snapshot L1 table size ==" @@ -191,6 +192,7 @@ poke_file "$TEST_IMG" "$offset_snap1_l1_size" "\x10\x00\x00\x00" { $QEMU_IMG amend -o compat=0.10 $TEST_IMG; } 2>&1 | _filter_testdir { $QEMU_IO -c "open -o overlap-check.inactive-l2=on $TEST_IMG" \ -c 'write 0 4k'; } 2>&1 | _filter_qemu_io | _filter_testdir +{ $QEMU_IMG snapshot -a test $TEST_IMG; } 2>&1 | _filter_testdir # success, all done echo "*** done" diff --git a/tests/qemu-iotests/080.out b/tests/qemu-iotests/080.out index d622ac06c0..1525e1b087 100644 --- a/tests/qemu-iotests/080.out +++ b/tests/qemu-iotests/080.out @@ -68,6 +68,8 @@ qemu-img: Snapshot L1 table offset invalid qemu-img: Error while amending options: Invalid argument Failed to flush the refcount block cache: Invalid argument write failed: Invalid argument +qemu-img: Snapshot L1 table offset invalid +qemu-img: Could not apply snapshot 'test': Failed to load snapshot: Invalid argument == Invalid snapshot L1 table size == Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=67108864 @@ -78,4 +80,6 @@ qemu-img: Snapshot L1 table too large qemu-img: Error while amending options: File too large Failed to flush the refcount block cache: File too large write failed: File too large +qemu-img: Snapshot L1 table too large +qemu-img: Could not apply snapshot 'test': Failed to load snapshot: File too large *** done From db5794f1f17d1f8247c0ea8e6f0376a47b112466 Mon Sep 17 00:00:00 2001 From: Alberto Garcia Date: Tue, 6 Mar 2018 18:14:11 +0200 Subject: [PATCH 13/56] qcow2: Check snapshot L1 table in qcow2_snapshot_delete() This function deletes a snapshot from disk, removing its entry from the snapshot table, freeing its L1 table and decreasing the refcounts of all clusters. The L1 table offset and size are however not validated. If we use invalid values in this function we'll probably corrupt the image even more, so we should return an error instead. We now have a function to take care of this, so let's use it. Signed-off-by: Alberto Garcia Reviewed-by: Eric Blake Signed-off-by: Kevin Wolf --- block/qcow2-snapshot.c | 7 +++++++ tests/qemu-iotests/080 | 2 ++ tests/qemu-iotests/080.out | 2 ++ 3 files changed, 11 insertions(+) diff --git a/block/qcow2-snapshot.c b/block/qcow2-snapshot.c index 0faf728dc4..74293be470 100644 --- a/block/qcow2-snapshot.c +++ b/block/qcow2-snapshot.c @@ -611,6 +611,13 @@ int qcow2_snapshot_delete(BlockDriverState *bs, } sn = s->snapshots[snapshot_index]; + ret = qcow2_validate_table(bs, sn.l1_table_offset, sn.l1_size, + sizeof(uint64_t), QCOW_MAX_L1_SIZE, + "Snapshot L1 table", errp); + if (ret < 0) { + return ret; + } + /* Remove it from the snapshot list */ memmove(s->snapshots + snapshot_index, s->snapshots + snapshot_index + 1, diff --git a/tests/qemu-iotests/080 b/tests/qemu-iotests/080 index 538857310f..f8e7d6f4df 100755 --- a/tests/qemu-iotests/080 +++ b/tests/qemu-iotests/080 @@ -181,6 +181,7 @@ poke_file "$TEST_IMG" "$offset_snap1_l1_offset" "\x00\x00\x00\x00\x00\x40\x02\x0 { $QEMU_IO -c "open -o overlap-check.inactive-l2=on $TEST_IMG" \ -c 'write 0 4k'; } 2>&1 | _filter_qemu_io | _filter_testdir { $QEMU_IMG snapshot -a test $TEST_IMG; } 2>&1 | _filter_testdir +{ $QEMU_IMG snapshot -d test $TEST_IMG; } 2>&1 | _filter_testdir echo echo "== Invalid snapshot L1 table size ==" @@ -193,6 +194,7 @@ poke_file "$TEST_IMG" "$offset_snap1_l1_size" "\x10\x00\x00\x00" { $QEMU_IO -c "open -o overlap-check.inactive-l2=on $TEST_IMG" \ -c 'write 0 4k'; } 2>&1 | _filter_qemu_io | _filter_testdir { $QEMU_IMG snapshot -a test $TEST_IMG; } 2>&1 | _filter_testdir +{ $QEMU_IMG snapshot -d test $TEST_IMG; } 2>&1 | _filter_testdir # success, all done echo "*** done" diff --git a/tests/qemu-iotests/080.out b/tests/qemu-iotests/080.out index 1525e1b087..89bcd27172 100644 --- a/tests/qemu-iotests/080.out +++ b/tests/qemu-iotests/080.out @@ -70,6 +70,7 @@ Failed to flush the refcount block cache: Invalid argument write failed: Invalid argument qemu-img: Snapshot L1 table offset invalid qemu-img: Could not apply snapshot 'test': Failed to load snapshot: Invalid argument +qemu-img: Could not delete snapshot 'test': Snapshot L1 table offset invalid == Invalid snapshot L1 table size == Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=67108864 @@ -82,4 +83,5 @@ Failed to flush the refcount block cache: File too large write failed: File too large qemu-img: Snapshot L1 table too large qemu-img: Could not apply snapshot 'test': Failed to load snapshot: File too large +qemu-img: Could not delete snapshot 'test': Snapshot L1 table too large *** done From 0c2ada8136a993b1fae9cee2c26b357e9c56c016 Mon Sep 17 00:00:00 2001 From: Alberto Garcia Date: Tue, 6 Mar 2018 18:14:12 +0200 Subject: [PATCH 14/56] qcow2: Make qemu-img check detect corrupted L1 tables in snapshots 'qemu-img check' cannot detect if a snapshot's L1 table is corrupted. This patch checks the table's offset and size and reports corruption if the values are not valid. This patch doesn't add code to fix that corruption yet, only to detect and report it. Signed-off-by: Alberto Garcia Reviewed-by: Eric Blake Signed-off-by: Kevin Wolf --- block/qcow2-refcount.c | 14 ++++++++++++++ tests/qemu-iotests/080 | 2 ++ tests/qemu-iotests/080.out | 20 ++++++++++++++++++++ 3 files changed, 36 insertions(+) diff --git a/block/qcow2-refcount.c b/block/qcow2-refcount.c index b18ea0ca98..362deaf303 100644 --- a/block/qcow2-refcount.c +++ b/block/qcow2-refcount.c @@ -2047,6 +2047,20 @@ static int calculate_refcounts(BlockDriverState *bs, BdrvCheckResult *res, /* snapshots */ for (i = 0; i < s->nb_snapshots; i++) { sn = s->snapshots + i; + if (offset_into_cluster(s, sn->l1_table_offset)) { + fprintf(stderr, "ERROR snapshot %s (%s) l1_offset=%#" PRIx64 ": " + "L1 table is not cluster aligned; snapshot table entry " + "corrupted\n", sn->id_str, sn->name, sn->l1_table_offset); + res->corruptions++; + continue; + } + if (sn->l1_size > QCOW_MAX_L1_SIZE / sizeof(uint64_t)) { + fprintf(stderr, "ERROR snapshot %s (%s) l1_size=%#" PRIx32 ": " + "L1 table is too large; snapshot table entry corrupted\n", + sn->id_str, sn->name, sn->l1_size); + res->corruptions++; + continue; + } ret = check_refcounts_l1(bs, res, refcount_table, nb_clusters, sn->l1_table_offset, sn->l1_size, 0, fix); if (ret < 0) { diff --git a/tests/qemu-iotests/080 b/tests/qemu-iotests/080 index f8e7d6f4df..4dbe68e950 100755 --- a/tests/qemu-iotests/080 +++ b/tests/qemu-iotests/080 @@ -182,6 +182,7 @@ poke_file "$TEST_IMG" "$offset_snap1_l1_offset" "\x00\x00\x00\x00\x00\x40\x02\x0 -c 'write 0 4k'; } 2>&1 | _filter_qemu_io | _filter_testdir { $QEMU_IMG snapshot -a test $TEST_IMG; } 2>&1 | _filter_testdir { $QEMU_IMG snapshot -d test $TEST_IMG; } 2>&1 | _filter_testdir +_check_test_img echo echo "== Invalid snapshot L1 table size ==" @@ -195,6 +196,7 @@ poke_file "$TEST_IMG" "$offset_snap1_l1_size" "\x10\x00\x00\x00" -c 'write 0 4k'; } 2>&1 | _filter_qemu_io | _filter_testdir { $QEMU_IMG snapshot -a test $TEST_IMG; } 2>&1 | _filter_testdir { $QEMU_IMG snapshot -d test $TEST_IMG; } 2>&1 | _filter_testdir +_check_test_img # success, all done echo "*** done" diff --git a/tests/qemu-iotests/080.out b/tests/qemu-iotests/080.out index 89bcd27172..4e0f7f7b92 100644 --- a/tests/qemu-iotests/080.out +++ b/tests/qemu-iotests/080.out @@ -71,6 +71,16 @@ write failed: Invalid argument qemu-img: Snapshot L1 table offset invalid qemu-img: Could not apply snapshot 'test': Failed to load snapshot: Invalid argument qemu-img: Could not delete snapshot 'test': Snapshot L1 table offset invalid +ERROR snapshot 1 (test) l1_offset=0x400200: L1 table is not cluster aligned; snapshot table entry corrupted +Leaked cluster 4 refcount=2 reference=1 +Leaked cluster 5 refcount=2 reference=1 +Leaked cluster 6 refcount=1 reference=0 + +1 errors were found on the image. +Data may be corrupted, or further writes to the image may corrupt it. + +3 leaked clusters were found on the image. +This means waste of disk space, but no harm to data. == Invalid snapshot L1 table size == Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=67108864 @@ -84,4 +94,14 @@ write failed: File too large qemu-img: Snapshot L1 table too large qemu-img: Could not apply snapshot 'test': Failed to load snapshot: File too large qemu-img: Could not delete snapshot 'test': Snapshot L1 table too large +ERROR snapshot 1 (test) l1_size=0x10000000: L1 table is too large; snapshot table entry corrupted +Leaked cluster 4 refcount=2 reference=1 +Leaked cluster 5 refcount=2 reference=1 +Leaked cluster 6 refcount=1 reference=0 + +1 errors were found on the image. +Data may be corrupted, or further writes to the image may corrupt it. + +3 leaked clusters were found on the image. +This means waste of disk space, but no harm to data. *** done From 5361468974c72cf0b22bd0789e55a51fd9d75d50 Mon Sep 17 00:00:00 2001 From: Kevin Wolf Date: Fri, 24 Nov 2017 16:01:07 +0100 Subject: [PATCH 15/56] block/qapi: Introduce BlockdevCreateOptions This creates a BlockdevCreateOptions union type that will contain all of the options for image creation. We'll start out with an empty struct type BlockdevCreateNotSupported for all drivers. Signed-off-by: Kevin Wolf Reviewed-by: Eric Blake Reviewed-by: Max Reitz --- qapi/block-core.json | 62 ++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 62 insertions(+) diff --git a/qapi/block-core.json b/qapi/block-core.json index 00475f08d4..bb2db662f7 100644 --- a/qapi/block-core.json +++ b/qapi/block-core.json @@ -3358,6 +3358,68 @@ ## { 'command': 'blockdev-del', 'data': { 'node-name': 'str' } } +## +# @BlockdevCreateNotSupported: +# +# This is used for all drivers that don't support creating images. +# +# Since: 2.12 +## +{ 'struct': 'BlockdevCreateNotSupported', 'data': {}} + +## +# @BlockdevCreateOptions: +# +# Options for creating an image format on a given node. +# +# @driver block driver to create the image format +# +# Since: 2.12 +## +{ 'union': 'BlockdevCreateOptions', + 'base': { + 'driver': 'BlockdevDriver' }, + 'discriminator': 'driver', + 'data': { + 'blkdebug': 'BlockdevCreateNotSupported', + 'blkverify': 'BlockdevCreateNotSupported', + 'bochs': 'BlockdevCreateNotSupported', + 'cloop': 'BlockdevCreateNotSupported', + 'dmg': 'BlockdevCreateNotSupported', + 'file': 'BlockdevCreateNotSupported', + 'ftp': 'BlockdevCreateNotSupported', + 'ftps': 'BlockdevCreateNotSupported', + 'gluster': 'BlockdevCreateNotSupported', + 'host_cdrom': 'BlockdevCreateNotSupported', + 'host_device': 'BlockdevCreateNotSupported', + 'http': 'BlockdevCreateNotSupported', + 'https': 'BlockdevCreateNotSupported', + 'iscsi': 'BlockdevCreateNotSupported', + 'luks': 'BlockdevCreateNotSupported', + 'nbd': 'BlockdevCreateNotSupported', + 'nfs': 'BlockdevCreateNotSupported', + 'null-aio': 'BlockdevCreateNotSupported', + 'null-co': 'BlockdevCreateNotSupported', + 'nvme': 'BlockdevCreateNotSupported', + 'parallels': 'BlockdevCreateNotSupported', + 'qcow2': 'BlockdevCreateNotSupported', + 'qcow': 'BlockdevCreateNotSupported', + 'qed': 'BlockdevCreateNotSupported', + 'quorum': 'BlockdevCreateNotSupported', + 'raw': 'BlockdevCreateNotSupported', + 'rbd': 'BlockdevCreateNotSupported', + 'replication': 'BlockdevCreateNotSupported', + 'sheepdog': 'BlockdevCreateNotSupported', + 'ssh': 'BlockdevCreateNotSupported', + 'throttle': 'BlockdevCreateNotSupported', + 'vdi': 'BlockdevCreateNotSupported', + 'vhdx': 'BlockdevCreateNotSupported', + 'vmdk': 'BlockdevCreateNotSupported', + 'vpc': 'BlockdevCreateNotSupported', + 'vvfat': 'BlockdevCreateNotSupported', + 'vxhs': 'BlockdevCreateNotSupported' + } } + ## # @blockdev-open-tray: # From c2808abaf73585562ab8c3a552b270065236349a Mon Sep 17 00:00:00 2001 From: Kevin Wolf Date: Fri, 24 Nov 2017 16:01:07 +0100 Subject: [PATCH 16/56] block/qapi: Add qcow2 create options to schema Signed-off-by: Kevin Wolf Reviewed-by: Eric Blake Reviewed-by: Max Reitz --- qapi/block-core.json | 45 +++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 44 insertions(+), 1 deletion(-) diff --git a/qapi/block-core.json b/qapi/block-core.json index bb2db662f7..dfea7b0102 100644 --- a/qapi/block-core.json +++ b/qapi/block-core.json @@ -3358,6 +3358,49 @@ ## { 'command': 'blockdev-del', 'data': { 'node-name': 'str' } } +## +# @BlockdevQcow2Version: +# +# @v2: The original QCOW2 format as introduced in qemu 0.10 (version 2) +# @v3: The extended QCOW2 format as introduced in qemu 1.1 (version 3) +# +# Since: 2.12 +## +{ 'enum': 'BlockdevQcow2Version', + 'data': [ 'v2', 'v3' ] } + + +## +# @BlockdevCreateOptionsQcow2: +# +# Driver specific image creation options for qcow2. +# +# @file Node to create the image format on +# @size Size of the virtual disk in bytes +# @version Compatibility level (default: v3) +# @backing-file File name of the backing file if a backing file +# should be used +# @backing-fmt Name of the block driver to use for the backing file +# @encrypt Encryption options if the image should be encrypted +# @cluster-size qcow2 cluster size in bytes (default: 65536) +# @preallocation Preallocation mode for the new image (default: off) +# @lazy-refcounts True if refcounts may be updated lazily (default: off) +# @refcount-bits Width of reference counts in bits (default: 16) +# +# Since: 2.12 +## +{ 'struct': 'BlockdevCreateOptionsQcow2', + 'data': { 'file': 'BlockdevRef', + 'size': 'size', + '*version': 'BlockdevQcow2Version', + '*backing-file': 'str', + '*backing-fmt': 'BlockdevDriver', + '*encrypt': 'QCryptoBlockCreateOptions', + '*cluster-size': 'size', + '*preallocation': 'PreallocMode', + '*lazy-refcounts': 'bool', + '*refcount-bits': 'int' } } + ## # @BlockdevCreateNotSupported: # @@ -3402,7 +3445,7 @@ 'null-co': 'BlockdevCreateNotSupported', 'nvme': 'BlockdevCreateNotSupported', 'parallels': 'BlockdevCreateNotSupported', - 'qcow2': 'BlockdevCreateNotSupported', + 'qcow2': 'BlockdevCreateOptionsQcow2', 'qcow': 'BlockdevCreateNotSupported', 'qed': 'BlockdevCreateNotSupported', 'quorum': 'BlockdevCreateNotSupported', From 8ca5bfdc46031a10475fce428446d3e3c1b89e5b Mon Sep 17 00:00:00 2001 From: Kevin Wolf Date: Thu, 1 Mar 2018 17:45:23 +0100 Subject: [PATCH 17/56] qcow2: Rename qcow2_co_create2() to qcow2_co_create() The functions originally known as qcow2_create() and qcow2_create2() are now called qcow2_co_create_opts() and qcow2_co_create(), which matches the names of the BlockDriver callbacks that they will implement at the end of this patch series. Signed-off-by: Kevin Wolf Reviewed-by: Eric Blake Reviewed-by: Max Reitz --- block/qcow2.c | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/block/qcow2.c b/block/qcow2.c index 369e374a9b..b7354a27a1 100644 --- a/block/qcow2.c +++ b/block/qcow2.c @@ -2761,11 +2761,11 @@ static uint64_t qcow2_opt_get_refcount_bits_del(QemuOpts *opts, int version, } static int coroutine_fn -qcow2_co_create2(const char *filename, int64_t total_size, - const char *backing_file, const char *backing_format, - int flags, size_t cluster_size, PreallocMode prealloc, - QemuOpts *opts, int version, int refcount_order, - const char *encryptfmt, Error **errp) +qcow2_co_create(const char *filename, int64_t total_size, + const char *backing_file, const char *backing_format, + int flags, size_t cluster_size, PreallocMode prealloc, + QemuOpts *opts, int version, int refcount_order, + const char *encryptfmt, Error **errp) { QDict *options; @@ -3034,9 +3034,9 @@ static int coroutine_fn qcow2_co_create_opts(const char *filename, QemuOpts *opt refcount_order = ctz32(refcount_bits); - ret = qcow2_co_create2(filename, size, backing_file, backing_fmt, flags, - cluster_size, prealloc, opts, version, refcount_order, - encryptfmt, &local_err); + ret = qcow2_co_create(filename, size, backing_file, backing_fmt, flags, + cluster_size, prealloc, opts, version, refcount_order, + encryptfmt, &local_err); error_propagate(errp, local_err); finish: From cbf2b7c4045550353a08c0b182beef7a72e4f813 Mon Sep 17 00:00:00 2001 From: Kevin Wolf Date: Tue, 9 Jan 2018 17:35:26 +0100 Subject: [PATCH 18/56] qcow2: Let qcow2_create() handle protocol layer Currently, qcow2_create() only parses the QemuOpts and then calls qcow2_co_create() for the actual image creation, which includes both the creation of the actual file on the file system and writing a valid empty qcow2 image into that file. The plan is that qcow2_co_create() becomes the function that implements the functionality for a future 'blockdev-create' QMP command, which only creates the qcow2 layer on an already opened file node. This is a first step towards that goal: Let's move out anything that deals with the protocol layer from qcow2_co_create() into qcow2_create(). This means that qcow2_co_create() doesn't need a file name any more. Signed-off-by: Kevin Wolf Reviewed-by: Eric Blake Reviewed-by: Max Reitz --- block/qcow2.c | 64 ++++++++++++++++++++++++++++++--------------------- 1 file changed, 38 insertions(+), 26 deletions(-) diff --git a/block/qcow2.c b/block/qcow2.c index b7354a27a1..7a11874d22 100644 --- a/block/qcow2.c +++ b/block/qcow2.c @@ -2761,7 +2761,7 @@ static uint64_t qcow2_opt_get_refcount_bits_del(QemuOpts *opts, int version, } static int coroutine_fn -qcow2_co_create(const char *filename, int64_t total_size, +qcow2_co_create(BlockDriverState *bs, int64_t total_size, const char *backing_file, const char *backing_format, int flags, size_t cluster_size, PreallocMode prealloc, QemuOpts *opts, int version, int refcount_order, @@ -2787,28 +2787,11 @@ qcow2_co_create(const char *filename, int64_t total_size, Error *local_err = NULL; int ret; - if (prealloc == PREALLOC_MODE_FULL || prealloc == PREALLOC_MODE_FALLOC) { - int64_t prealloc_size = - qcow2_calc_prealloc_size(total_size, cluster_size, refcount_order); - qemu_opt_set_number(opts, BLOCK_OPT_SIZE, prealloc_size, &error_abort); - qemu_opt_set(opts, BLOCK_OPT_PREALLOC, PreallocMode_str(prealloc), - &error_abort); - } - - ret = bdrv_create_file(filename, opts, &local_err); + blk = blk_new(BLK_PERM_WRITE | BLK_PERM_RESIZE, BLK_PERM_ALL); + ret = blk_insert_bs(blk, bs, errp); if (ret < 0) { - error_propagate(errp, local_err); - return ret; + goto out; } - - blk = blk_new_open(filename, NULL, NULL, - BDRV_O_RDWR | BDRV_O_RESIZE | BDRV_O_PROTOCOL, - &local_err); - if (blk == NULL) { - error_propagate(errp, local_err); - return -EIO; - } - blk_set_allow_write_beyond_eof(blk, true); /* Write the header */ @@ -2863,7 +2846,8 @@ qcow2_co_create(const char *filename, int64_t total_size, */ options = qdict_new(); qdict_put_str(options, "driver", "qcow2"); - blk = blk_new_open(filename, NULL, options, + qdict_put_str(options, "file", bs->node_name); + blk = blk_new_open(NULL, NULL, options, BDRV_O_RDWR | BDRV_O_RESIZE | BDRV_O_NO_FLUSH, &local_err); if (blk == NULL) { @@ -2935,7 +2919,8 @@ qcow2_co_create(const char *filename, int64_t total_size, */ options = qdict_new(); qdict_put_str(options, "driver", "qcow2"); - blk = blk_new_open(filename, NULL, options, + qdict_put_str(options, "file", bs->node_name); + blk = blk_new_open(NULL, NULL, options, BDRV_O_RDWR | BDRV_O_NO_BACKING | BDRV_O_NO_IO, &local_err); if (blk == NULL) { @@ -2966,6 +2951,7 @@ static int coroutine_fn qcow2_co_create_opts(const char *filename, QemuOpts *opt uint64_t refcount_bits; int refcount_order; char *encryptfmt = NULL; + BlockDriverState *bs = NULL; Error *local_err = NULL; int ret; @@ -3034,12 +3020,38 @@ static int coroutine_fn qcow2_co_create_opts(const char *filename, QemuOpts *opt refcount_order = ctz32(refcount_bits); - ret = qcow2_co_create(filename, size, backing_file, backing_fmt, flags, + /* Create and open the file (protocol layer) */ + if (prealloc == PREALLOC_MODE_FULL || prealloc == PREALLOC_MODE_FALLOC) { + int64_t prealloc_size = + qcow2_calc_prealloc_size(size, cluster_size, refcount_order); + qemu_opt_set_number(opts, BLOCK_OPT_SIZE, prealloc_size, &error_abort); + qemu_opt_set(opts, BLOCK_OPT_PREALLOC, PreallocMode_str(prealloc), + &error_abort); + } + + ret = bdrv_create_file(filename, opts, errp); + if (ret < 0) { + goto finish; + } + + bs = bdrv_open(filename, NULL, NULL, + BDRV_O_RDWR | BDRV_O_RESIZE | BDRV_O_PROTOCOL, errp); + if (bs == NULL) { + ret = -EIO; + goto finish; + } + + /* Create the qcow2 image (format layer) */ + ret = qcow2_co_create(bs, size, backing_file, backing_fmt, flags, cluster_size, prealloc, opts, version, refcount_order, - encryptfmt, &local_err); - error_propagate(errp, local_err); + encryptfmt, errp); + if (ret < 0) { + goto finish; + } finish: + bdrv_unref(bs); + g_free(backing_file); g_free(backing_fmt); g_free(encryptfmt); From 29ca9e450dd0fd5961f5f3c12f8e983707ec71ed Mon Sep 17 00:00:00 2001 From: Kevin Wolf Date: Tue, 9 Jan 2018 19:44:33 +0100 Subject: [PATCH 19/56] qcow2: Pass BlockdevCreateOptions to qcow2_co_create() All of the simple options are now passed to qcow2_co_create() in a BlockdevCreateOptions object. Still missing: node-name and the encryption options. Signed-off-by: Kevin Wolf Reviewed-by: Max Reitz Reviewed-by: Eric Blake --- block/qcow2.c | 189 ++++++++++++++++++++++++++++++++++++++++---------- 1 file changed, 151 insertions(+), 38 deletions(-) diff --git a/block/qcow2.c b/block/qcow2.c index 7a11874d22..7679c28f57 100644 --- a/block/qcow2.c +++ b/block/qcow2.c @@ -2700,19 +2700,26 @@ static int64_t qcow2_calc_prealloc_size(int64_t total_size, return meta_size + aligned_total_size; } -static size_t qcow2_opt_get_cluster_size_del(QemuOpts *opts, Error **errp) +static bool validate_cluster_size(size_t cluster_size, Error **errp) { - size_t cluster_size; - int cluster_bits; - - cluster_size = qemu_opt_get_size_del(opts, BLOCK_OPT_CLUSTER_SIZE, - DEFAULT_CLUSTER_SIZE); - cluster_bits = ctz32(cluster_size); + int cluster_bits = ctz32(cluster_size); if (cluster_bits < MIN_CLUSTER_BITS || cluster_bits > MAX_CLUSTER_BITS || (1 << cluster_bits) != cluster_size) { error_setg(errp, "Cluster size must be a power of two between %d and " "%dk", 1 << MIN_CLUSTER_BITS, 1 << (MAX_CLUSTER_BITS - 10)); + return false; + } + return true; +} + +static size_t qcow2_opt_get_cluster_size_del(QemuOpts *opts, Error **errp) +{ + size_t cluster_size; + + cluster_size = qemu_opt_get_size_del(opts, BLOCK_OPT_CLUSTER_SIZE, + DEFAULT_CLUSTER_SIZE); + if (!validate_cluster_size(cluster_size, errp)) { return 0; } return cluster_size; @@ -2761,12 +2768,10 @@ static uint64_t qcow2_opt_get_refcount_bits_del(QemuOpts *opts, int version, } static int coroutine_fn -qcow2_co_create(BlockDriverState *bs, int64_t total_size, - const char *backing_file, const char *backing_format, - int flags, size_t cluster_size, PreallocMode prealloc, - QemuOpts *opts, int version, int refcount_order, - const char *encryptfmt, Error **errp) +qcow2_co_create(BlockDriverState *bs, BlockdevCreateOptions *create_options, + QemuOpts *opts, const char *encryptfmt, Error **errp) { + BlockdevCreateOptionsQcow2 *qcow2_opts; QDict *options; /* @@ -2783,10 +2788,92 @@ qcow2_co_create(BlockDriverState *bs, int64_t total_size, */ BlockBackend *blk; QCowHeader *header; + size_t cluster_size; + int version; + int refcount_order; uint64_t* refcount_table; Error *local_err = NULL; int ret; + /* Validate options and set default values */ + assert(create_options->driver == BLOCKDEV_DRIVER_QCOW2); + qcow2_opts = &create_options->u.qcow2; + + if (!QEMU_IS_ALIGNED(qcow2_opts->size, BDRV_SECTOR_SIZE)) { + error_setg(errp, "Image size must be a multiple of 512 bytes"); + ret = -EINVAL; + goto out; + } + + if (qcow2_opts->has_version) { + switch (qcow2_opts->version) { + case BLOCKDEV_QCOW2_VERSION_V2: + version = 2; + break; + case BLOCKDEV_QCOW2_VERSION_V3: + version = 3; + break; + default: + g_assert_not_reached(); + } + } else { + version = 3; + } + + if (qcow2_opts->has_cluster_size) { + cluster_size = qcow2_opts->cluster_size; + } else { + cluster_size = DEFAULT_CLUSTER_SIZE; + } + + if (!validate_cluster_size(cluster_size, errp)) { + return -EINVAL; + } + + if (!qcow2_opts->has_preallocation) { + qcow2_opts->preallocation = PREALLOC_MODE_OFF; + } + if (qcow2_opts->has_backing_file && + qcow2_opts->preallocation != PREALLOC_MODE_OFF) + { + error_setg(errp, "Backing file and preallocation cannot be used at " + "the same time"); + return -EINVAL; + } + if (qcow2_opts->has_backing_fmt && !qcow2_opts->has_backing_file) { + error_setg(errp, "Backing format cannot be used without backing file"); + return -EINVAL; + } + + if (!qcow2_opts->has_lazy_refcounts) { + qcow2_opts->lazy_refcounts = false; + } + if (version < 3 && qcow2_opts->lazy_refcounts) { + error_setg(errp, "Lazy refcounts only supported with compatibility " + "level 1.1 and above (use compat=1.1 or greater)"); + return -EINVAL; + } + + if (!qcow2_opts->has_refcount_bits) { + qcow2_opts->refcount_bits = 16; + } + if (qcow2_opts->refcount_bits > 64 || + !is_power_of_2(qcow2_opts->refcount_bits)) + { + error_setg(errp, "Refcount width must be a power of two and may not " + "exceed 64 bits"); + return -EINVAL; + } + if (version < 3 && qcow2_opts->refcount_bits != 16) { + error_setg(errp, "Different refcount widths than 16 bits require " + "compatibility level 1.1 or above (use compat=1.1 or " + "greater)"); + return -EINVAL; + } + refcount_order = ctz32(qcow2_opts->refcount_bits); + + + /* Create BlockBackend to write to the image */ blk = blk_new(BLK_PERM_WRITE | BLK_PERM_RESIZE, BLK_PERM_ALL); ret = blk_insert_bs(blk, bs, errp); if (ret < 0) { @@ -2813,7 +2900,7 @@ qcow2_co_create(BlockDriverState *bs, int64_t total_size, /* We'll update this to correct value later */ header->crypt_method = cpu_to_be32(QCOW_CRYPT_NONE); - if (flags & BLOCK_FLAG_LAZY_REFCOUNTS) { + if (qcow2_opts->lazy_refcounts) { header->compatible_features |= cpu_to_be64(QCOW2_COMPAT_LAZY_REFCOUNTS); } @@ -2875,18 +2962,26 @@ qcow2_co_create(BlockDriverState *bs, int64_t total_size, } /* Okay, now that we have a valid image, let's give it the right size */ - ret = blk_truncate(blk, total_size, PREALLOC_MODE_OFF, errp); + ret = blk_truncate(blk, qcow2_opts->size, PREALLOC_MODE_OFF, errp); if (ret < 0) { error_prepend(errp, "Could not resize image: "); goto out; } /* Want a backing file? There you go.*/ - if (backing_file) { - ret = bdrv_change_backing_file(blk_bs(blk), backing_file, backing_format); + if (qcow2_opts->has_backing_file) { + const char *backing_format = NULL; + + if (qcow2_opts->has_backing_fmt) { + backing_format = BlockdevDriver_str(qcow2_opts->backing_fmt); + } + + ret = bdrv_change_backing_file(blk_bs(blk), qcow2_opts->backing_file, + backing_format); if (ret < 0) { error_setg_errno(errp, -ret, "Could not assign backing file '%s' " - "with format '%s'", backing_file, backing_format); + "with format '%s'", qcow2_opts->backing_file, + backing_format); goto out; } } @@ -2900,8 +2995,8 @@ qcow2_co_create(BlockDriverState *bs, int64_t total_size, } /* And if we're supposed to preallocate metadata, do that now */ - if (prealloc != PREALLOC_MODE_OFF) { - ret = preallocate(blk_bs(blk), 0, total_size); + if (qcow2_opts->preallocation != PREALLOC_MODE_OFF) { + ret = preallocate(blk_bs(blk), 0, qcow2_opts->size); if (ret < 0) { error_setg_errno(errp, -ret, "Could not preallocate metadata"); goto out; @@ -2940,8 +3035,10 @@ out: static int coroutine_fn qcow2_co_create_opts(const char *filename, QemuOpts *opts, Error **errp) { + BlockdevCreateOptions create_options; char *backing_file = NULL; char *backing_fmt = NULL; + BlockdevDriver backing_drv; char *buf = NULL; uint64_t size = 0; int flags = 0; @@ -2949,7 +3046,6 @@ static int coroutine_fn qcow2_co_create_opts(const char *filename, QemuOpts *opt PreallocMode prealloc; int version; uint64_t refcount_bits; - int refcount_order; char *encryptfmt = NULL; BlockDriverState *bs = NULL; Error *local_err = NULL; @@ -2960,6 +3056,13 @@ static int coroutine_fn qcow2_co_create_opts(const char *filename, QemuOpts *opt BDRV_SECTOR_SIZE); backing_file = qemu_opt_get_del(opts, BLOCK_OPT_BACKING_FILE); backing_fmt = qemu_opt_get_del(opts, BLOCK_OPT_BACKING_FMT); + backing_drv = qapi_enum_parse(&BlockdevDriver_lookup, backing_fmt, + 0, &local_err); + if (local_err) { + error_propagate(errp, local_err); + ret = -EINVAL; + goto finish; + } encryptfmt = qemu_opt_get_del(opts, BLOCK_OPT_ENCRYPT_FORMAT); if (encryptfmt) { if (qemu_opt_get(opts, BLOCK_OPT_ENCRYPT)) { @@ -2997,20 +3100,6 @@ static int coroutine_fn qcow2_co_create_opts(const char *filename, QemuOpts *opt flags |= BLOCK_FLAG_LAZY_REFCOUNTS; } - if (backing_file && prealloc != PREALLOC_MODE_OFF) { - error_setg(errp, "Backing file and preallocation cannot be used at " - "the same time"); - ret = -EINVAL; - goto finish; - } - - if (version < 3 && (flags & BLOCK_FLAG_LAZY_REFCOUNTS)) { - error_setg(errp, "Lazy refcounts only supported with compatibility " - "level 1.1 and above (use compat=1.1 or greater)"); - ret = -EINVAL; - goto finish; - } - refcount_bits = qcow2_opt_get_refcount_bits_del(opts, version, &local_err); if (local_err) { error_propagate(errp, local_err); @@ -3018,10 +3107,10 @@ static int coroutine_fn qcow2_co_create_opts(const char *filename, QemuOpts *opt goto finish; } - refcount_order = ctz32(refcount_bits); /* Create and open the file (protocol layer) */ if (prealloc == PREALLOC_MODE_FULL || prealloc == PREALLOC_MODE_FALLOC) { + int refcount_order = ctz32(refcount_bits); int64_t prealloc_size = qcow2_calc_prealloc_size(size, cluster_size, refcount_order); qemu_opt_set_number(opts, BLOCK_OPT_SIZE, prealloc_size, &error_abort); @@ -3042,9 +3131,33 @@ static int coroutine_fn qcow2_co_create_opts(const char *filename, QemuOpts *opt } /* Create the qcow2 image (format layer) */ - ret = qcow2_co_create(bs, size, backing_file, backing_fmt, flags, - cluster_size, prealloc, opts, version, refcount_order, - encryptfmt, errp); + create_options = (BlockdevCreateOptions) { + .driver = BLOCKDEV_DRIVER_QCOW2, + .u.qcow2 = { + .file = &(BlockdevRef) { + .type = QTYPE_QSTRING, + .u.reference = bs->node_name, + }, + .size = size, + .has_version = true, + .version = version == 2 + ? BLOCKDEV_QCOW2_VERSION_V2 + : BLOCKDEV_QCOW2_VERSION_V3, + .has_backing_file = (backing_file != NULL), + .backing_file = backing_file, + .has_backing_fmt = (backing_fmt != NULL), + .backing_fmt = backing_drv, + .has_cluster_size = true, + .cluster_size = cluster_size, + .has_preallocation = true, + .preallocation = prealloc, + .has_lazy_refcounts = true, + .lazy_refcounts = (flags & BLOCK_FLAG_LAZY_REFCOUNTS), + .has_refcount_bits = true, + .refcount_bits = refcount_bits, + }, + }; + ret = qcow2_co_create(bs, &create_options, opts, encryptfmt, errp); if (ret < 0) { goto finish; } From e1d74bc6c61f9be8c1eb19f39d96b08e7332c864 Mon Sep 17 00:00:00 2001 From: Kevin Wolf Date: Wed, 10 Jan 2018 15:52:33 +0100 Subject: [PATCH 20/56] qcow2: Use BlockdevRef in qcow2_co_create() Instead of passing a separate BlockDriverState* into qcow2_co_create(), make use of the BlockdevRef that is included in BlockdevCreateOptions. Signed-off-by: Kevin Wolf Reviewed-by: Eric Blake Reviewed-by: Max Reitz --- block.c | 47 +++++++++++++++++++++++++++++++++++++++++++ block/qcow2.c | 39 ++++++++++++++++++++++------------- include/block/block.h | 1 + 3 files changed, 73 insertions(+), 14 deletions(-) diff --git a/block.c b/block.c index 8f1c43d037..4fc65f7621 100644 --- a/block.c +++ b/block.c @@ -34,6 +34,8 @@ #include "qapi/qmp/qdict.h" #include "qapi/qmp/qjson.h" #include "qapi/qmp/qstring.h" +#include "qapi/qobject-output-visitor.h" +#include "qapi/qapi-visit-block-core.h" #include "sysemu/block-backend.h" #include "sysemu/sysemu.h" #include "qemu/notify.h" @@ -2406,6 +2408,51 @@ BdrvChild *bdrv_open_child(const char *filename, return c; } +/* TODO Future callers may need to specify parent/child_role in order for + * option inheritance to work. Existing callers use it for the root node. */ +BlockDriverState *bdrv_open_blockdev_ref(BlockdevRef *ref, Error **errp) +{ + BlockDriverState *bs = NULL; + Error *local_err = NULL; + QObject *obj = NULL; + QDict *qdict = NULL; + const char *reference = NULL; + Visitor *v = NULL; + + if (ref->type == QTYPE_QSTRING) { + reference = ref->u.reference; + } else { + BlockdevOptions *options = &ref->u.definition; + assert(ref->type == QTYPE_QDICT); + + v = qobject_output_visitor_new(&obj); + visit_type_BlockdevOptions(v, NULL, &options, &local_err); + if (local_err) { + error_propagate(errp, local_err); + goto fail; + } + visit_complete(v, &obj); + + qdict = qobject_to_qdict(obj); + qdict_flatten(qdict); + + /* bdrv_open_inherit() defaults to the values in bdrv_flags (for + * compatibility with other callers) rather than what we want as the + * real defaults. Apply the defaults here instead. */ + qdict_set_default_str(qdict, BDRV_OPT_CACHE_DIRECT, "off"); + qdict_set_default_str(qdict, BDRV_OPT_CACHE_NO_FLUSH, "off"); + qdict_set_default_str(qdict, BDRV_OPT_READ_ONLY, "off"); + } + + bs = bdrv_open_inherit(NULL, reference, qdict, 0, NULL, NULL, errp); + obj = NULL; + +fail: + qobject_decref(obj); + visit_free(v); + return bs; +} + static BlockDriverState *bdrv_append_temp_snapshot(BlockDriverState *bs, int flags, QDict *snapshot_options, diff --git a/block/qcow2.c b/block/qcow2.c index 7679c28f57..b7df2d5cab 100644 --- a/block/qcow2.c +++ b/block/qcow2.c @@ -2768,8 +2768,8 @@ static uint64_t qcow2_opt_get_refcount_bits_del(QemuOpts *opts, int version, } static int coroutine_fn -qcow2_co_create(BlockDriverState *bs, BlockdevCreateOptions *create_options, - QemuOpts *opts, const char *encryptfmt, Error **errp) +qcow2_co_create(BlockdevCreateOptions *create_options, QemuOpts *opts, + const char *encryptfmt, Error **errp) { BlockdevCreateOptionsQcow2 *qcow2_opts; QDict *options; @@ -2786,7 +2786,8 @@ qcow2_co_create(BlockDriverState *bs, BlockdevCreateOptions *create_options, * 2 GB for 64k clusters, and we don't want to have a 2 GB initial file * size for any qcow2 image. */ - BlockBackend *blk; + BlockBackend *blk = NULL; + BlockDriverState *bs = NULL; QCowHeader *header; size_t cluster_size; int version; @@ -2795,10 +2796,15 @@ qcow2_co_create(BlockDriverState *bs, BlockdevCreateOptions *create_options, Error *local_err = NULL; int ret; - /* Validate options and set default values */ assert(create_options->driver == BLOCKDEV_DRIVER_QCOW2); qcow2_opts = &create_options->u.qcow2; + bs = bdrv_open_blockdev_ref(qcow2_opts->file, errp); + if (bs == NULL) { + return -EIO; + } + + /* Validate options and set default values */ if (!QEMU_IS_ALIGNED(qcow2_opts->size, BDRV_SECTOR_SIZE)) { error_setg(errp, "Image size must be a multiple of 512 bytes"); ret = -EINVAL; @@ -2827,7 +2833,8 @@ qcow2_co_create(BlockDriverState *bs, BlockdevCreateOptions *create_options, } if (!validate_cluster_size(cluster_size, errp)) { - return -EINVAL; + ret = -EINVAL; + goto out; } if (!qcow2_opts->has_preallocation) { @@ -2838,11 +2845,13 @@ qcow2_co_create(BlockDriverState *bs, BlockdevCreateOptions *create_options, { error_setg(errp, "Backing file and preallocation cannot be used at " "the same time"); - return -EINVAL; + ret = -EINVAL; + goto out; } if (qcow2_opts->has_backing_fmt && !qcow2_opts->has_backing_file) { error_setg(errp, "Backing format cannot be used without backing file"); - return -EINVAL; + ret = -EINVAL; + goto out; } if (!qcow2_opts->has_lazy_refcounts) { @@ -2851,7 +2860,8 @@ qcow2_co_create(BlockDriverState *bs, BlockdevCreateOptions *create_options, if (version < 3 && qcow2_opts->lazy_refcounts) { error_setg(errp, "Lazy refcounts only supported with compatibility " "level 1.1 and above (use compat=1.1 or greater)"); - return -EINVAL; + ret = -EINVAL; + goto out; } if (!qcow2_opts->has_refcount_bits) { @@ -2862,13 +2872,15 @@ qcow2_co_create(BlockDriverState *bs, BlockdevCreateOptions *create_options, { error_setg(errp, "Refcount width must be a power of two and may not " "exceed 64 bits"); - return -EINVAL; + ret = -EINVAL; + goto out; } if (version < 3 && qcow2_opts->refcount_bits != 16) { error_setg(errp, "Different refcount widths than 16 bits require " "compatibility level 1.1 or above (use compat=1.1 or " "greater)"); - return -EINVAL; + ret = -EINVAL; + goto out; } refcount_order = ctz32(qcow2_opts->refcount_bits); @@ -3026,9 +3038,8 @@ qcow2_co_create(BlockDriverState *bs, BlockdevCreateOptions *create_options, ret = 0; out: - if (blk) { - blk_unref(blk); - } + blk_unref(blk); + bdrv_unref(bs); return ret; } @@ -3157,7 +3168,7 @@ static int coroutine_fn qcow2_co_create_opts(const char *filename, QemuOpts *opt .refcount_bits = refcount_bits, }, }; - ret = qcow2_co_create(bs, &create_options, opts, encryptfmt, errp); + ret = qcow2_co_create(&create_options, opts, encryptfmt, errp); if (ret < 0) { goto finish; } diff --git a/include/block/block.h b/include/block/block.h index 8b6db952a2..7805187b30 100644 --- a/include/block/block.h +++ b/include/block/block.h @@ -246,6 +246,7 @@ BdrvChild *bdrv_open_child(const char *filename, BlockDriverState* parent, const BdrvChildRole *child_role, bool allow_none, Error **errp); +BlockDriverState *bdrv_open_blockdev_ref(BlockdevRef *ref, Error **errp); void bdrv_set_backing_hd(BlockDriverState *bs, BlockDriverState *backing_hd, Error **errp); int bdrv_open_backing_file(BlockDriverState *bs, QDict *parent_options, From 60900b7bd930e0b9acad118ead4fabf4610066a8 Mon Sep 17 00:00:00 2001 From: Kevin Wolf Date: Wed, 10 Jan 2018 17:55:16 +0100 Subject: [PATCH 21/56] qcow2: Use QCryptoBlockCreateOptions in qcow2_co_create() Instead of passing the encryption format name and the QemuOpts down, use the QCryptoBlockCreateOptions contained in BlockdevCreateOptions. Signed-off-by: Kevin Wolf Reviewed-by: Eric Blake Reviewed-by: Max Reitz --- block/qcow2.c | 62 +++++++++++++++++++++++++++++++++++++-------------- 1 file changed, 45 insertions(+), 17 deletions(-) diff --git a/block/qcow2.c b/block/qcow2.c index b7df2d5cab..e1821eb3c8 100644 --- a/block/qcow2.c +++ b/block/qcow2.c @@ -2449,13 +2449,10 @@ static int qcow2_crypt_method_from_format(const char *encryptfmt) } } -static int qcow2_set_up_encryption(BlockDriverState *bs, const char *encryptfmt, - QemuOpts *opts, Error **errp) +static QCryptoBlockCreateOptions * +qcow2_parse_encryption(const char *encryptfmt, QemuOpts *opts, Error **errp) { - BDRVQcow2State *s = bs->opaque; QCryptoBlockCreateOptions *cryptoopts = NULL; - QCryptoBlock *crypto = NULL; - int ret = -EINVAL; QDict *options, *encryptopts; int fmt; @@ -2478,10 +2475,31 @@ static int qcow2_set_up_encryption(BlockDriverState *bs, const char *encryptfmt, error_setg(errp, "Unknown encryption format '%s'", encryptfmt); break; } - if (!cryptoopts) { - ret = -EINVAL; - goto out; + + QDECREF(encryptopts); + return cryptoopts; +} + +static int qcow2_set_up_encryption(BlockDriverState *bs, + QCryptoBlockCreateOptions *cryptoopts, + Error **errp) +{ + BDRVQcow2State *s = bs->opaque; + QCryptoBlock *crypto = NULL; + int fmt, ret; + + switch (cryptoopts->format) { + case Q_CRYPTO_BLOCK_FORMAT_LUKS: + fmt = QCOW_CRYPT_LUKS; + break; + case Q_CRYPTO_BLOCK_FORMAT_QCOW: + fmt = QCOW_CRYPT_AES; + break; + default: + error_setg(errp, "Crypto format not supported in qcow2"); + return -EINVAL; } + s->crypt_method_header = fmt; crypto = qcrypto_block_create(cryptoopts, "encrypt.", @@ -2489,8 +2507,7 @@ static int qcow2_set_up_encryption(BlockDriverState *bs, const char *encryptfmt, qcow2_crypto_hdr_write_func, bs, errp); if (!crypto) { - ret = -EINVAL; - goto out; + return -EINVAL; } ret = qcow2_update_header(bs); @@ -2499,10 +2516,9 @@ static int qcow2_set_up_encryption(BlockDriverState *bs, const char *encryptfmt, goto out; } + ret = 0; out: - QDECREF(encryptopts); qcrypto_block_free(crypto); - qapi_free_QCryptoBlockCreateOptions(cryptoopts); return ret; } @@ -2768,8 +2784,7 @@ static uint64_t qcow2_opt_get_refcount_bits_del(QemuOpts *opts, int version, } static int coroutine_fn -qcow2_co_create(BlockdevCreateOptions *create_options, QemuOpts *opts, - const char *encryptfmt, Error **errp) +qcow2_co_create(BlockdevCreateOptions *create_options, Error **errp) { BlockdevCreateOptionsQcow2 *qcow2_opts; QDict *options; @@ -2999,8 +3014,8 @@ qcow2_co_create(BlockdevCreateOptions *create_options, QemuOpts *opts, } /* Want encryption? There you go. */ - if (encryptfmt) { - ret = qcow2_set_up_encryption(blk_bs(blk), encryptfmt, opts, errp); + if (qcow2_opts->has_encrypt) { + ret = qcow2_set_up_encryption(blk_bs(blk), qcow2_opts->encrypt, errp); if (ret < 0) { goto out; } @@ -3058,6 +3073,7 @@ static int coroutine_fn qcow2_co_create_opts(const char *filename, QemuOpts *opt int version; uint64_t refcount_bits; char *encryptfmt = NULL; + QCryptoBlockCreateOptions *cryptoopts = NULL; BlockDriverState *bs = NULL; Error *local_err = NULL; int ret; @@ -3074,6 +3090,7 @@ static int coroutine_fn qcow2_co_create_opts(const char *filename, QemuOpts *opt ret = -EINVAL; goto finish; } + encryptfmt = qemu_opt_get_del(opts, BLOCK_OPT_ENCRYPT_FORMAT); if (encryptfmt) { if (qemu_opt_get(opts, BLOCK_OPT_ENCRYPT)) { @@ -3085,6 +3102,14 @@ static int coroutine_fn qcow2_co_create_opts(const char *filename, QemuOpts *opt } else if (qemu_opt_get_bool_del(opts, BLOCK_OPT_ENCRYPT, false)) { encryptfmt = g_strdup("aes"); } + if (encryptfmt) { + cryptoopts = qcow2_parse_encryption(encryptfmt, opts, errp); + if (cryptoopts == NULL) { + ret = -EINVAL; + goto finish; + } + } + cluster_size = qcow2_opt_get_cluster_size_del(opts, &local_err); if (local_err) { error_propagate(errp, local_err); @@ -3158,6 +3183,8 @@ static int coroutine_fn qcow2_co_create_opts(const char *filename, QemuOpts *opt .backing_file = backing_file, .has_backing_fmt = (backing_fmt != NULL), .backing_fmt = backing_drv, + .has_encrypt = (encryptfmt != NULL), + .encrypt = cryptoopts, .has_cluster_size = true, .cluster_size = cluster_size, .has_preallocation = true, @@ -3168,7 +3195,7 @@ static int coroutine_fn qcow2_co_create_opts(const char *filename, QemuOpts *opt .refcount_bits = refcount_bits, }, }; - ret = qcow2_co_create(&create_options, opts, encryptfmt, errp); + ret = qcow2_co_create(&create_options, errp); if (ret < 0) { goto finish; } @@ -3176,6 +3203,7 @@ static int coroutine_fn qcow2_co_create_opts(const char *filename, QemuOpts *opt finish: bdrv_unref(bs); + qapi_free_QCryptoBlockCreateOptions(cryptoopts); g_free(backing_file); g_free(backing_fmt); g_free(encryptfmt); From e4b5dad8269b1b629c52be25e6da50644293abf7 Mon Sep 17 00:00:00 2001 From: Kevin Wolf Date: Thu, 11 Jan 2018 14:38:34 +0100 Subject: [PATCH 22/56] qcow2: Handle full/falloc preallocation in qcow2_co_create() Once qcow2_co_create() can be called directly on an already existing node, we must provide the 'full' and 'falloc' preallocation modes outside of creating the image on the protocol layer. Fortunately, we have preallocated truncate now which can provide this functionality. Signed-off-by: Kevin Wolf Reviewed-by: Eric Blake Reviewed-by: Max Reitz --- block/qcow2.c | 28 +++++++++++++++++++--------- 1 file changed, 19 insertions(+), 9 deletions(-) diff --git a/block/qcow2.c b/block/qcow2.c index e1821eb3c8..933c612754 100644 --- a/block/qcow2.c +++ b/block/qcow2.c @@ -2908,6 +2908,25 @@ qcow2_co_create(BlockdevCreateOptions *create_options, Error **errp) } blk_set_allow_write_beyond_eof(blk, true); + /* Clear the protocol layer and preallocate it if necessary */ + ret = blk_truncate(blk, 0, PREALLOC_MODE_OFF, errp); + if (ret < 0) { + goto out; + } + + if (qcow2_opts->preallocation == PREALLOC_MODE_FULL || + qcow2_opts->preallocation == PREALLOC_MODE_FALLOC) + { + int64_t prealloc_size = + qcow2_calc_prealloc_size(qcow2_opts->size, cluster_size, + refcount_order); + + ret = blk_truncate(blk, prealloc_size, qcow2_opts->preallocation, errp); + if (ret < 0) { + goto out; + } + } + /* Write the header */ QEMU_BUILD_BUG_ON((1 << MIN_CLUSTER_BITS) < sizeof(*header)); header = g_malloc0(cluster_size); @@ -3145,15 +3164,6 @@ static int coroutine_fn qcow2_co_create_opts(const char *filename, QemuOpts *opt /* Create and open the file (protocol layer) */ - if (prealloc == PREALLOC_MODE_FULL || prealloc == PREALLOC_MODE_FALLOC) { - int refcount_order = ctz32(refcount_bits); - int64_t prealloc_size = - qcow2_calc_prealloc_size(size, cluster_size, refcount_order); - qemu_opt_set_number(opts, BLOCK_OPT_SIZE, prealloc_size, &error_abort); - qemu_opt_set(opts, BLOCK_OPT_PREALLOC, PreallocMode_str(prealloc), - &error_abort); - } - ret = bdrv_create_file(filename, opts, errp); if (ret < 0) { goto finish; From 72215395b9e44f13be48e8576b90719738766edf Mon Sep 17 00:00:00 2001 From: Kevin Wolf Date: Thu, 11 Jan 2018 17:37:23 +0100 Subject: [PATCH 23/56] util: Add qemu_opts_to_qdict_filtered() This allows, given a QemuOpts for a QemuOptsList that was merged from multiple QemuOptsList, to only consider those options that exist in one specific list. Block drivers need this to separate format-layer create options from protocol-level options. Signed-off-by: Kevin Wolf Reviewed-by: Eric Blake Reviewed-by: Max Reitz --- include/qemu/option.h | 2 ++ util/qemu-option.c | 42 +++++++++++++++++++++++++++++++++++++----- 2 files changed, 39 insertions(+), 5 deletions(-) diff --git a/include/qemu/option.h b/include/qemu/option.h index b127fb6db6..306fdb5f7a 100644 --- a/include/qemu/option.h +++ b/include/qemu/option.h @@ -124,6 +124,8 @@ void qemu_opts_set_defaults(QemuOptsList *list, const char *params, int permit_abbrev); QemuOpts *qemu_opts_from_qdict(QemuOptsList *list, const QDict *qdict, Error **errp); +QDict *qemu_opts_to_qdict_filtered(QemuOpts *opts, QDict *qdict, + QemuOptsList *list, bool del); QDict *qemu_opts_to_qdict(QemuOpts *opts, QDict *qdict); void qemu_opts_absorb_qdict(QemuOpts *opts, QDict *qdict, Error **errp); diff --git a/util/qemu-option.c b/util/qemu-option.c index a401e936da..2b412eff5e 100644 --- a/util/qemu-option.c +++ b/util/qemu-option.c @@ -1007,14 +1007,23 @@ void qemu_opts_absorb_qdict(QemuOpts *opts, QDict *qdict, Error **errp) } /* - * Convert from QemuOpts to QDict. - * The QDict values are of type QString. + * Convert from QemuOpts to QDict. The QDict values are of type QString. + * + * If @list is given, only add those options to the QDict that are contained in + * the list. If @del is true, any options added to the QDict are removed from + * the QemuOpts, otherwise they remain there. + * + * If two options in @opts have the same name, they are processed in order + * so that the last one wins (consistent with the reverse iteration in + * qemu_opt_find()), but all of them are deleted if @del is true. + * * TODO We'll want to use types appropriate for opt->desc->type, but * this is enough for now. */ -QDict *qemu_opts_to_qdict(QemuOpts *opts, QDict *qdict) +QDict *qemu_opts_to_qdict_filtered(QemuOpts *opts, QDict *qdict, + QemuOptsList *list, bool del) { - QemuOpt *opt; + QemuOpt *opt, *next; if (!qdict) { qdict = qdict_new(); @@ -1022,12 +1031,35 @@ QDict *qemu_opts_to_qdict(QemuOpts *opts, QDict *qdict) if (opts->id) { qdict_put_str(qdict, "id", opts->id); } - QTAILQ_FOREACH(opt, &opts->head, next) { + QTAILQ_FOREACH_SAFE(opt, &opts->head, next, next) { + if (list) { + QemuOptDesc *desc; + bool found = false; + for (desc = list->desc; desc->name; desc++) { + if (!strcmp(desc->name, opt->name)) { + found = true; + break; + } + } + if (!found) { + continue; + } + } qdict_put_str(qdict, opt->name, opt->str); + if (del) { + qemu_opt_del(opt); + } } return qdict; } +/* Copy all options in a QemuOpts to the given QDict. See + * qemu_opts_to_qdict_filtered() for details. */ +QDict *qemu_opts_to_qdict(QemuOpts *opts, QDict *qdict) +{ + return qemu_opts_to_qdict_filtered(opts, qdict, NULL, false); +} + /* Validate parsed opts against descriptions where no * descriptions were provided in the QemuOptsList. */ From 575ef8bff62834f38aef353f96084850a4348078 Mon Sep 17 00:00:00 2001 From: Kevin Wolf Date: Mon, 19 Feb 2018 17:28:08 +0100 Subject: [PATCH 24/56] test-qemu-opts: Test qemu_opts_append() Basic test for merging two QemuOptsLists. Signed-off-by: Kevin Wolf Reviewed-by: Eric Blake Reviewed-by: Max Reitz --- tests/test-qemu-opts.c | 128 +++++++++++++++++++++++++++++++++++++++++ 1 file changed, 128 insertions(+) diff --git a/tests/test-qemu-opts.c b/tests/test-qemu-opts.c index 5d5a3daa7b..6c3183390b 100644 --- a/tests/test-qemu-opts.c +++ b/tests/test-qemu-opts.c @@ -23,6 +23,8 @@ static QemuOptsList opts_list_01 = { { .name = "str1", .type = QEMU_OPT_STRING, + .help = "Help texts are preserved in qemu_opts_append", + .def_value_str = "default", },{ .name = "str2", .type = QEMU_OPT_STRING, @@ -32,6 +34,7 @@ static QemuOptsList opts_list_01 = { },{ .name = "number1", .type = QEMU_OPT_NUMBER, + .help = "Having help texts only for some options is okay", },{ .name = "number2", .type = QEMU_OPT_NUMBER, @@ -743,6 +746,129 @@ static void test_opts_parse_size(void) qemu_opts_reset(&opts_list_02); } +static void append_verify_list_01(QemuOptDesc *desc, bool with_overlapping) +{ + int i = 0; + + if (with_overlapping) { + g_assert_cmpstr(desc[i].name, ==, "str1"); + g_assert_cmpint(desc[i].type, ==, QEMU_OPT_STRING); + g_assert_cmpstr(desc[i].help, ==, + "Help texts are preserved in qemu_opts_append"); + g_assert_cmpstr(desc[i].def_value_str, ==, "default"); + i++; + + g_assert_cmpstr(desc[i].name, ==, "str2"); + g_assert_cmpint(desc[i].type, ==, QEMU_OPT_STRING); + g_assert_cmpstr(desc[i].help, ==, NULL); + g_assert_cmpstr(desc[i].def_value_str, ==, NULL); + i++; + } + + g_assert_cmpstr(desc[i].name, ==, "str3"); + g_assert_cmpint(desc[i].type, ==, QEMU_OPT_STRING); + g_assert_cmpstr(desc[i].help, ==, NULL); + g_assert_cmpstr(desc[i].def_value_str, ==, NULL); + i++; + + g_assert_cmpstr(desc[i].name, ==, "number1"); + g_assert_cmpint(desc[i].type, ==, QEMU_OPT_NUMBER); + g_assert_cmpstr(desc[i].help, ==, + "Having help texts only for some options is okay"); + g_assert_cmpstr(desc[i].def_value_str, ==, NULL); + i++; + + g_assert_cmpstr(desc[i].name, ==, "number2"); + g_assert_cmpint(desc[i].type, ==, QEMU_OPT_NUMBER); + g_assert_cmpstr(desc[i].help, ==, NULL); + g_assert_cmpstr(desc[i].def_value_str, ==, NULL); + i++; + + g_assert_cmpstr(desc[i].name, ==, NULL); +} + +static void append_verify_list_02(QemuOptDesc *desc) +{ + int i = 0; + + g_assert_cmpstr(desc[i].name, ==, "str1"); + g_assert_cmpint(desc[i].type, ==, QEMU_OPT_STRING); + g_assert_cmpstr(desc[i].help, ==, NULL); + g_assert_cmpstr(desc[i].def_value_str, ==, NULL); + i++; + + g_assert_cmpstr(desc[i].name, ==, "str2"); + g_assert_cmpint(desc[i].type, ==, QEMU_OPT_STRING); + g_assert_cmpstr(desc[i].help, ==, NULL); + g_assert_cmpstr(desc[i].def_value_str, ==, NULL); + i++; + + g_assert_cmpstr(desc[i].name, ==, "bool1"); + g_assert_cmpint(desc[i].type, ==, QEMU_OPT_BOOL); + g_assert_cmpstr(desc[i].help, ==, NULL); + g_assert_cmpstr(desc[i].def_value_str, ==, NULL); + i++; + + g_assert_cmpstr(desc[i].name, ==, "bool2"); + g_assert_cmpint(desc[i].type, ==, QEMU_OPT_BOOL); + g_assert_cmpstr(desc[i].help, ==, NULL); + g_assert_cmpstr(desc[i].def_value_str, ==, NULL); + i++; + + g_assert_cmpstr(desc[i].name, ==, "size1"); + g_assert_cmpint(desc[i].type, ==, QEMU_OPT_SIZE); + g_assert_cmpstr(desc[i].help, ==, NULL); + g_assert_cmpstr(desc[i].def_value_str, ==, NULL); + i++; + + g_assert_cmpstr(desc[i].name, ==, "size2"); + g_assert_cmpint(desc[i].type, ==, QEMU_OPT_SIZE); + g_assert_cmpstr(desc[i].help, ==, NULL); + g_assert_cmpstr(desc[i].def_value_str, ==, NULL); + i++; + + g_assert_cmpstr(desc[i].name, ==, "size3"); + g_assert_cmpint(desc[i].type, ==, QEMU_OPT_SIZE); + g_assert_cmpstr(desc[i].help, ==, NULL); + g_assert_cmpstr(desc[i].def_value_str, ==, NULL); +} + +static void test_opts_append_to_null(void) +{ + QemuOptsList *merged; + + merged = qemu_opts_append(NULL, &opts_list_01); + g_assert(merged != &opts_list_01); + + g_assert_cmpstr(merged->name, ==, NULL); + g_assert_cmpstr(merged->implied_opt_name, ==, NULL); + g_assert_false(merged->merge_lists); + + append_verify_list_01(merged->desc, true); + + qemu_opts_free(merged); +} + +static void test_opts_append(void) +{ + QemuOptsList *first, *merged; + + first = qemu_opts_append(NULL, &opts_list_02); + merged = qemu_opts_append(first, &opts_list_01); + g_assert(first != &opts_list_02); + g_assert(merged != &opts_list_01); + + g_assert_cmpstr(merged->name, ==, NULL); + g_assert_cmpstr(merged->implied_opt_name, ==, NULL); + g_assert_false(merged->merge_lists); + + append_verify_list_02(&merged->desc[0]); + append_verify_list_01(&merged->desc[7], false); + + qemu_opts_free(merged); +} + + int main(int argc, char *argv[]) { register_opts(); @@ -761,6 +887,8 @@ int main(int argc, char *argv[]) g_test_add_func("/qemu-opts/opts_parse/bool", test_opts_parse_bool); g_test_add_func("/qemu-opts/opts_parse/number", test_opts_parse_number); g_test_add_func("/qemu-opts/opts_parse/size", test_opts_parse_size); + g_test_add_func("/qemu-opts/append_to_null", test_opts_append_to_null); + g_test_add_func("/qemu-opts/append", test_opts_append); g_test_run(); return 0; } From 37974a97d4d29229255b39ddd16c31743e1880ed Mon Sep 17 00:00:00 2001 From: Kevin Wolf Date: Mon, 19 Feb 2018 19:05:30 +0100 Subject: [PATCH 25/56] test-qemu-opts: Test qemu_opts_to_qdict_filtered() Signed-off-by: Kevin Wolf Reviewed-by: Eric Blake Reviewed-by: Max Reitz --- tests/test-qemu-opts.c | 125 +++++++++++++++++++++++++++++++++++++++++ 1 file changed, 125 insertions(+) diff --git a/tests/test-qemu-opts.c b/tests/test-qemu-opts.c index 6c3183390b..2c422abcd4 100644 --- a/tests/test-qemu-opts.c +++ b/tests/test-qemu-opts.c @@ -10,6 +10,7 @@ #include "qemu/osdep.h" #include "qemu/cutils.h" #include "qemu/option.h" +#include "qemu/option_int.h" #include "qapi/error.h" #include "qapi/qmp/qdict.h" #include "qapi/qmp/qstring.h" @@ -868,6 +869,127 @@ static void test_opts_append(void) qemu_opts_free(merged); } +static void test_opts_to_qdict_basic(void) +{ + QemuOpts *opts; + QDict *dict; + + opts = qemu_opts_parse(&opts_list_01, "str1=foo,str2=,str3=bar,number1=42", + false, &error_abort); + g_assert(opts != NULL); + + dict = qemu_opts_to_qdict(opts, NULL); + g_assert(dict != NULL); + + g_assert_cmpstr(qdict_get_str(dict, "str1"), ==, "foo"); + g_assert_cmpstr(qdict_get_str(dict, "str2"), ==, ""); + g_assert_cmpstr(qdict_get_str(dict, "str3"), ==, "bar"); + g_assert_cmpstr(qdict_get_str(dict, "number1"), ==, "42"); + g_assert_false(qdict_haskey(dict, "number2")); + + QDECREF(dict); + qemu_opts_del(opts); +} + +static void test_opts_to_qdict_filtered(void) +{ + QemuOptsList *first, *merged; + QemuOpts *opts; + QDict *dict; + + first = qemu_opts_append(NULL, &opts_list_02); + merged = qemu_opts_append(first, &opts_list_01); + + opts = qemu_opts_parse(merged, + "str1=foo,str2=,str3=bar,bool1=off,number1=42", + false, &error_abort); + g_assert(opts != NULL); + + /* Convert to QDict without deleting from opts */ + dict = qemu_opts_to_qdict_filtered(opts, NULL, &opts_list_01, false); + g_assert(dict != NULL); + g_assert_cmpstr(qdict_get_str(dict, "str1"), ==, "foo"); + g_assert_cmpstr(qdict_get_str(dict, "str2"), ==, ""); + g_assert_cmpstr(qdict_get_str(dict, "str3"), ==, "bar"); + g_assert_cmpstr(qdict_get_str(dict, "number1"), ==, "42"); + g_assert_false(qdict_haskey(dict, "number2")); + g_assert_false(qdict_haskey(dict, "bool1")); + QDECREF(dict); + + dict = qemu_opts_to_qdict_filtered(opts, NULL, &opts_list_02, false); + g_assert(dict != NULL); + g_assert_cmpstr(qdict_get_str(dict, "str1"), ==, "foo"); + g_assert_cmpstr(qdict_get_str(dict, "str2"), ==, ""); + g_assert_cmpstr(qdict_get_str(dict, "bool1"), ==, "off"); + g_assert_false(qdict_haskey(dict, "str3")); + g_assert_false(qdict_haskey(dict, "number1")); + g_assert_false(qdict_haskey(dict, "number2")); + QDECREF(dict); + + /* Now delete converted options from opts */ + dict = qemu_opts_to_qdict_filtered(opts, NULL, &opts_list_01, true); + g_assert(dict != NULL); + g_assert_cmpstr(qdict_get_str(dict, "str1"), ==, "foo"); + g_assert_cmpstr(qdict_get_str(dict, "str2"), ==, ""); + g_assert_cmpstr(qdict_get_str(dict, "str3"), ==, "bar"); + g_assert_cmpstr(qdict_get_str(dict, "number1"), ==, "42"); + g_assert_false(qdict_haskey(dict, "number2")); + g_assert_false(qdict_haskey(dict, "bool1")); + QDECREF(dict); + + dict = qemu_opts_to_qdict_filtered(opts, NULL, &opts_list_02, true); + g_assert(dict != NULL); + g_assert_cmpstr(qdict_get_str(dict, "bool1"), ==, "off"); + g_assert_false(qdict_haskey(dict, "str1")); + g_assert_false(qdict_haskey(dict, "str2")); + g_assert_false(qdict_haskey(dict, "str3")); + g_assert_false(qdict_haskey(dict, "number1")); + g_assert_false(qdict_haskey(dict, "number2")); + QDECREF(dict); + + g_assert_true(QTAILQ_EMPTY(&opts->head)); + + qemu_opts_del(opts); + qemu_opts_free(merged); +} + +static void test_opts_to_qdict_duplicates(void) +{ + QemuOpts *opts; + QemuOpt *opt; + QDict *dict; + + opts = qemu_opts_parse(&opts_list_03, "foo=a,foo=b", false, &error_abort); + g_assert(opts != NULL); + + /* Verify that opts has two options with the same name */ + opt = QTAILQ_FIRST(&opts->head); + g_assert_cmpstr(opt->name, ==, "foo"); + g_assert_cmpstr(opt->str , ==, "a"); + + opt = QTAILQ_NEXT(opt, next); + g_assert_cmpstr(opt->name, ==, "foo"); + g_assert_cmpstr(opt->str , ==, "b"); + + opt = QTAILQ_NEXT(opt, next); + g_assert(opt == NULL); + + /* In the conversion to QDict, the last one wins */ + dict = qemu_opts_to_qdict(opts, NULL); + g_assert(dict != NULL); + g_assert_cmpstr(qdict_get_str(dict, "foo"), ==, "b"); + QDECREF(dict); + + /* The last one still wins if entries are deleted, and both are deleted */ + dict = qemu_opts_to_qdict_filtered(opts, NULL, NULL, true); + g_assert(dict != NULL); + g_assert_cmpstr(qdict_get_str(dict, "foo"), ==, "b"); + QDECREF(dict); + + g_assert_true(QTAILQ_EMPTY(&opts->head)); + + qemu_opts_del(opts); +} int main(int argc, char *argv[]) { @@ -889,6 +1011,9 @@ int main(int argc, char *argv[]) g_test_add_func("/qemu-opts/opts_parse/size", test_opts_parse_size); g_test_add_func("/qemu-opts/append_to_null", test_opts_append_to_null); g_test_add_func("/qemu-opts/append", test_opts_append); + g_test_add_func("/qemu-opts/to_qdict/basic", test_opts_to_qdict_basic); + g_test_add_func("/qemu-opts/to_qdict/filtered", test_opts_to_qdict_filtered); + g_test_add_func("/qemu-opts/to_qdict/duplicates", test_opts_to_qdict_duplicates); g_test_run(); return 0; } From bcebf102ccc3c6db327f341adc379fdf0673ca6b Mon Sep 17 00:00:00 2001 From: Kevin Wolf Date: Thu, 1 Feb 2018 20:00:44 +0100 Subject: [PATCH 26/56] qdict: Introduce qdict_rename_keys() A few block drivers will need to rename .bdrv_create options for their QAPIfication, so let's have a helper function for that. Signed-off-by: Kevin Wolf Reviewed-by: Max Reitz Reviewed-by: Eric Blake --- include/qapi/qmp/qdict.h | 6 ++ qobject/qdict.c | 34 +++++++++++ tests/check-qdict.c | 129 +++++++++++++++++++++++++++++++++++++++ 3 files changed, 169 insertions(+) diff --git a/include/qapi/qmp/qdict.h b/include/qapi/qmp/qdict.h index ff6f7842c3..7c6d844549 100644 --- a/include/qapi/qmp/qdict.h +++ b/include/qapi/qmp/qdict.h @@ -81,4 +81,10 @@ QObject *qdict_crumple(const QDict *src, Error **errp); void qdict_join(QDict *dest, QDict *src, bool overwrite); +typedef struct QDictRenames { + const char *from; + const char *to; +} QDictRenames; +bool qdict_rename_keys(QDict *qdict, const QDictRenames *renames, Error **errp); + #endif /* QDICT_H */ diff --git a/qobject/qdict.c b/qobject/qdict.c index 23df84f9cd..229b8c840b 100644 --- a/qobject/qdict.c +++ b/qobject/qdict.c @@ -1072,3 +1072,37 @@ void qdict_join(QDict *dest, QDict *src, bool overwrite) entry = next; } } + +/** + * qdict_rename_keys(): Rename keys in qdict according to the replacements + * specified in the array renames. The array must be terminated by an entry + * with from = NULL. + * + * The renames are performed individually in the order of the array, so entries + * may be renamed multiple times and may or may not conflict depending on the + * order of the renames array. + * + * Returns true for success, false in error cases. + */ +bool qdict_rename_keys(QDict *qdict, const QDictRenames *renames, Error **errp) +{ + QObject *qobj; + + while (renames->from) { + if (qdict_haskey(qdict, renames->from)) { + if (qdict_haskey(qdict, renames->to)) { + error_setg(errp, "'%s' and its alias '%s' can't be used at the " + "same time", renames->to, renames->from); + return false; + } + + qobj = qdict_get(qdict, renames->from); + qobject_incref(qobj); + qdict_put_obj(qdict, renames->to, qobj); + qdict_del(qdict, renames->from); + } + + renames++; + } + return true; +} diff --git a/tests/check-qdict.c b/tests/check-qdict.c index ec628f3453..a3faea8bfc 100644 --- a/tests/check-qdict.c +++ b/tests/check-qdict.c @@ -665,6 +665,133 @@ static void qdict_crumple_test_empty(void) QDECREF(dst); } +static int qdict_count_entries(QDict *dict) +{ + const QDictEntry *e; + int count = 0; + + for (e = qdict_first(dict); e; e = qdict_next(dict, e)) { + count++; + } + + return count; +} + +static void qdict_rename_keys_test(void) +{ + QDict *dict = qdict_new(); + QDict *copy; + QDictRenames *renames; + Error *local_err = NULL; + + qdict_put_str(dict, "abc", "foo"); + qdict_put_str(dict, "abcdef", "bar"); + qdict_put_int(dict, "number", 42); + qdict_put_bool(dict, "flag", true); + qdict_put_null(dict, "nothing"); + + /* Empty rename list */ + renames = (QDictRenames[]) { + { NULL, "this can be anything" } + }; + copy = qdict_clone_shallow(dict); + qdict_rename_keys(copy, renames, &error_abort); + + g_assert_cmpstr(qdict_get_str(copy, "abc"), ==, "foo"); + g_assert_cmpstr(qdict_get_str(copy, "abcdef"), ==, "bar"); + g_assert_cmpint(qdict_get_int(copy, "number"), ==, 42); + g_assert_cmpint(qdict_get_bool(copy, "flag"), ==, true); + g_assert(qobject_type(qdict_get(copy, "nothing")) == QTYPE_QNULL); + g_assert_cmpint(qdict_count_entries(copy), ==, 5); + + QDECREF(copy); + + /* Simple rename of all entries */ + renames = (QDictRenames[]) { + { "abc", "str1" }, + { "abcdef", "str2" }, + { "number", "int" }, + { "flag", "bool" }, + { "nothing", "null" }, + { NULL , NULL } + }; + copy = qdict_clone_shallow(dict); + qdict_rename_keys(copy, renames, &error_abort); + + g_assert(!qdict_haskey(copy, "abc")); + g_assert(!qdict_haskey(copy, "abcdef")); + g_assert(!qdict_haskey(copy, "number")); + g_assert(!qdict_haskey(copy, "flag")); + g_assert(!qdict_haskey(copy, "nothing")); + + g_assert_cmpstr(qdict_get_str(copy, "str1"), ==, "foo"); + g_assert_cmpstr(qdict_get_str(copy, "str2"), ==, "bar"); + g_assert_cmpint(qdict_get_int(copy, "int"), ==, 42); + g_assert_cmpint(qdict_get_bool(copy, "bool"), ==, true); + g_assert(qobject_type(qdict_get(copy, "null")) == QTYPE_QNULL); + g_assert_cmpint(qdict_count_entries(copy), ==, 5); + + QDECREF(copy); + + /* Renames are processed top to bottom */ + renames = (QDictRenames[]) { + { "abc", "tmp" }, + { "abcdef", "abc" }, + { "number", "abcdef" }, + { "flag", "number" }, + { "nothing", "flag" }, + { "tmp", "nothing" }, + { NULL , NULL } + }; + copy = qdict_clone_shallow(dict); + qdict_rename_keys(copy, renames, &error_abort); + + g_assert_cmpstr(qdict_get_str(copy, "nothing"), ==, "foo"); + g_assert_cmpstr(qdict_get_str(copy, "abc"), ==, "bar"); + g_assert_cmpint(qdict_get_int(copy, "abcdef"), ==, 42); + g_assert_cmpint(qdict_get_bool(copy, "number"), ==, true); + g_assert(qobject_type(qdict_get(copy, "flag")) == QTYPE_QNULL); + g_assert(!qdict_haskey(copy, "tmp")); + g_assert_cmpint(qdict_count_entries(copy), ==, 5); + + QDECREF(copy); + + /* Conflicting rename */ + renames = (QDictRenames[]) { + { "abcdef", "abc" }, + { NULL , NULL } + }; + copy = qdict_clone_shallow(dict); + qdict_rename_keys(copy, renames, &local_err); + + g_assert(local_err != NULL); + error_free(local_err); + local_err = NULL; + + g_assert_cmpstr(qdict_get_str(copy, "abc"), ==, "foo"); + g_assert_cmpstr(qdict_get_str(copy, "abcdef"), ==, "bar"); + g_assert_cmpint(qdict_get_int(copy, "number"), ==, 42); + g_assert_cmpint(qdict_get_bool(copy, "flag"), ==, true); + g_assert(qobject_type(qdict_get(copy, "nothing")) == QTYPE_QNULL); + g_assert_cmpint(qdict_count_entries(copy), ==, 5); + + QDECREF(copy); + + /* Renames in an empty dict */ + renames = (QDictRenames[]) { + { "abcdef", "abc" }, + { NULL , NULL } + }; + + QDECREF(dict); + dict = qdict_new(); + + qdict_rename_keys(dict, renames, &error_abort); + g_assert(qdict_first(dict) == NULL); + + QDECREF(dict); +} + static void qdict_crumple_test_bad_inputs(void) { QDict *src; @@ -880,6 +1007,8 @@ int main(int argc, char **argv) g_test_add_func("/public/crumple/bad_inputs", qdict_crumple_test_bad_inputs); + g_test_add_func("/public/rename_keys", qdict_rename_keys_test); + /* The Big one */ if (g_test_slow()) { g_test_add_func("/stress/test", qdict_stress_test); From b76b4f604521e59f857d6177bc55f6f2e41fd392 Mon Sep 17 00:00:00 2001 From: Kevin Wolf Date: Thu, 11 Jan 2018 16:18:08 +0100 Subject: [PATCH 27/56] qcow2: Use visitor for options in qcow2_create() Instead of manually creating the BlockdevCreateOptions object, use a visitor to parse the given options into the QAPI object. This involves translation from the old command line syntax to the syntax mandated by the QAPI schema. Option names are still checked against qcow2_create_opts, so only the old option names are allowed on the command line, even if they are translated in qcow2_create(). In contrast, new option values are optionally recognised besides the old values: 'compat' accepts 'v2'/'v3' as an alias for '0.10'/'1.1', and 'encrypt.format' accepts 'qcow' as an alias for 'aes' now. Signed-off-by: Kevin Wolf Reviewed-by: Max Reitz Reviewed-by: Eric Blake --- block/qcow2.c | 226 ++++++++++++++----------------------- tests/qemu-iotests/049.out | 8 +- tests/qemu-iotests/112.out | 4 +- 3 files changed, 88 insertions(+), 150 deletions(-) diff --git a/block/qcow2.c b/block/qcow2.c index 933c612754..37b0e36c1e 100644 --- a/block/qcow2.c +++ b/block/qcow2.c @@ -37,7 +37,8 @@ #include "qemu/option_int.h" #include "qemu/cutils.h" #include "qemu/bswap.h" -#include "qapi/opts-visitor.h" +#include "qapi/qobject-input-visitor.h" +#include "qapi/qapi-visit-block-core.h" #include "block/crypto.h" /* @@ -2449,37 +2450,6 @@ static int qcow2_crypt_method_from_format(const char *encryptfmt) } } -static QCryptoBlockCreateOptions * -qcow2_parse_encryption(const char *encryptfmt, QemuOpts *opts, Error **errp) -{ - QCryptoBlockCreateOptions *cryptoopts = NULL; - QDict *options, *encryptopts; - int fmt; - - options = qemu_opts_to_qdict(opts, NULL); - qdict_extract_subqdict(options, &encryptopts, "encrypt."); - QDECREF(options); - - fmt = qcow2_crypt_method_from_format(encryptfmt); - - switch (fmt) { - case QCOW_CRYPT_LUKS: - cryptoopts = block_crypto_create_opts_init( - Q_CRYPTO_BLOCK_FORMAT_LUKS, encryptopts, errp); - break; - case QCOW_CRYPT_AES: - cryptoopts = block_crypto_create_opts_init( - Q_CRYPTO_BLOCK_FORMAT_QCOW, encryptopts, errp); - break; - default: - error_setg(errp, "Unknown encryption format '%s'", encryptfmt); - break; - } - - QDECREF(encryptopts); - return cryptoopts; -} - static int qcow2_set_up_encryption(BlockDriverState *bs, QCryptoBlockCreateOptions *cryptoopts, Error **errp) @@ -2874,7 +2844,7 @@ qcow2_co_create(BlockdevCreateOptions *create_options, Error **errp) } if (version < 3 && qcow2_opts->lazy_refcounts) { error_setg(errp, "Lazy refcounts only supported with compatibility " - "level 1.1 and above (use compat=1.1 or greater)"); + "level 1.1 and above (use version=v3 or greater)"); ret = -EINVAL; goto out; } @@ -2892,7 +2862,7 @@ qcow2_co_create(BlockdevCreateOptions *create_options, Error **errp) } if (version < 3 && qcow2_opts->refcount_bits != 16) { error_setg(errp, "Different refcount widths than 16 bits require " - "compatibility level 1.1 or above (use compat=1.1 or " + "compatibility level 1.1 or above (use version=v3 or " "greater)"); ret = -EINVAL; goto out; @@ -3080,89 +3050,61 @@ out: static int coroutine_fn qcow2_co_create_opts(const char *filename, QemuOpts *opts, Error **errp) { - BlockdevCreateOptions create_options; - char *backing_file = NULL; - char *backing_fmt = NULL; - BlockdevDriver backing_drv; - char *buf = NULL; - uint64_t size = 0; - int flags = 0; - size_t cluster_size = DEFAULT_CLUSTER_SIZE; - PreallocMode prealloc; - int version; - uint64_t refcount_bits; - char *encryptfmt = NULL; - QCryptoBlockCreateOptions *cryptoopts = NULL; + BlockdevCreateOptions *create_options = NULL; + QDict *qdict = NULL; + QObject *qobj; + Visitor *v; BlockDriverState *bs = NULL; Error *local_err = NULL; + const char *val; int ret; - /* Read out options */ - size = ROUND_UP(qemu_opt_get_size_del(opts, BLOCK_OPT_SIZE, 0), - BDRV_SECTOR_SIZE); - backing_file = qemu_opt_get_del(opts, BLOCK_OPT_BACKING_FILE); - backing_fmt = qemu_opt_get_del(opts, BLOCK_OPT_BACKING_FMT); - backing_drv = qapi_enum_parse(&BlockdevDriver_lookup, backing_fmt, - 0, &local_err); - if (local_err) { - error_propagate(errp, local_err); + /* Only the keyval visitor supports the dotted syntax needed for + * encryption, so go through a QDict before getting a QAPI type. Ignore + * options meant for the protocol layer so that the visitor doesn't + * complain. */ + qdict = qemu_opts_to_qdict_filtered(opts, NULL, bdrv_qcow2.create_opts, + true); + + /* Handle encryption options */ + val = qdict_get_try_str(qdict, BLOCK_OPT_ENCRYPT); + if (val && !strcmp(val, "on")) { + qdict_put_str(qdict, BLOCK_OPT_ENCRYPT, "qcow"); + } else if (val && !strcmp(val, "off")) { + qdict_del(qdict, BLOCK_OPT_ENCRYPT); + } + + val = qdict_get_try_str(qdict, BLOCK_OPT_ENCRYPT_FORMAT); + if (val && !strcmp(val, "aes")) { + qdict_put_str(qdict, BLOCK_OPT_ENCRYPT_FORMAT, "qcow"); + } + + /* Convert compat=0.10/1.1 into compat=v2/v3, to be renamed into + * version=v2/v3 below. */ + val = qdict_get_try_str(qdict, BLOCK_OPT_COMPAT_LEVEL); + if (val && !strcmp(val, "0.10")) { + qdict_put_str(qdict, BLOCK_OPT_COMPAT_LEVEL, "v2"); + } else if (val && !strcmp(val, "1.1")) { + qdict_put_str(qdict, BLOCK_OPT_COMPAT_LEVEL, "v3"); + } + + /* Change legacy command line options into QMP ones */ + static const QDictRenames opt_renames[] = { + { BLOCK_OPT_BACKING_FILE, "backing-file" }, + { BLOCK_OPT_BACKING_FMT, "backing-fmt" }, + { BLOCK_OPT_CLUSTER_SIZE, "cluster-size" }, + { BLOCK_OPT_LAZY_REFCOUNTS, "lazy-refcounts" }, + { BLOCK_OPT_REFCOUNT_BITS, "refcount-bits" }, + { BLOCK_OPT_ENCRYPT, BLOCK_OPT_ENCRYPT_FORMAT }, + { BLOCK_OPT_COMPAT_LEVEL, "version" }, + { NULL, NULL }, + }; + + if (!qdict_rename_keys(qdict, opt_renames, errp)) { ret = -EINVAL; goto finish; } - encryptfmt = qemu_opt_get_del(opts, BLOCK_OPT_ENCRYPT_FORMAT); - if (encryptfmt) { - if (qemu_opt_get(opts, BLOCK_OPT_ENCRYPT)) { - error_setg(errp, "Options " BLOCK_OPT_ENCRYPT " and " - BLOCK_OPT_ENCRYPT_FORMAT " are mutually exclusive"); - ret = -EINVAL; - goto finish; - } - } else if (qemu_opt_get_bool_del(opts, BLOCK_OPT_ENCRYPT, false)) { - encryptfmt = g_strdup("aes"); - } - if (encryptfmt) { - cryptoopts = qcow2_parse_encryption(encryptfmt, opts, errp); - if (cryptoopts == NULL) { - ret = -EINVAL; - goto finish; - } - } - - cluster_size = qcow2_opt_get_cluster_size_del(opts, &local_err); - if (local_err) { - error_propagate(errp, local_err); - ret = -EINVAL; - goto finish; - } - buf = qemu_opt_get_del(opts, BLOCK_OPT_PREALLOC); - prealloc = qapi_enum_parse(&PreallocMode_lookup, buf, - PREALLOC_MODE_OFF, &local_err); - if (local_err) { - error_propagate(errp, local_err); - ret = -EINVAL; - goto finish; - } - - version = qcow2_opt_get_version_del(opts, &local_err); - if (local_err) { - error_propagate(errp, local_err); - ret = -EINVAL; - goto finish; - } - - if (qemu_opt_get_bool_del(opts, BLOCK_OPT_LAZY_REFCOUNTS, false)) { - flags |= BLOCK_FLAG_LAZY_REFCOUNTS; - } - - refcount_bits = qcow2_opt_get_refcount_bits_del(opts, version, &local_err); - if (local_err) { - error_propagate(errp, local_err); - ret = -EINVAL; - goto finish; - } - - /* Create and open the file (protocol layer) */ ret = bdrv_create_file(filename, opts, errp); if (ret < 0) { @@ -3176,48 +3118,44 @@ static int coroutine_fn qcow2_co_create_opts(const char *filename, QemuOpts *opt goto finish; } + /* Set 'driver' and 'node' options */ + qdict_put_str(qdict, "driver", "qcow2"); + qdict_put_str(qdict, "file", bs->node_name); + + /* Now get the QAPI type BlockdevCreateOptions */ + qobj = qdict_crumple(qdict, errp); + QDECREF(qdict); + qdict = qobject_to_qdict(qobj); + if (qdict == NULL) { + ret = -EINVAL; + goto finish; + } + + v = qobject_input_visitor_new_keyval(QOBJECT(qdict)); + visit_type_BlockdevCreateOptions(v, NULL, &create_options, &local_err); + visit_free(v); + + if (local_err) { + error_propagate(errp, local_err); + ret = -EINVAL; + goto finish; + } + + /* Silently round up size */ + create_options->u.qcow2.size = ROUND_UP(create_options->u.qcow2.size, + BDRV_SECTOR_SIZE); + /* Create the qcow2 image (format layer) */ - create_options = (BlockdevCreateOptions) { - .driver = BLOCKDEV_DRIVER_QCOW2, - .u.qcow2 = { - .file = &(BlockdevRef) { - .type = QTYPE_QSTRING, - .u.reference = bs->node_name, - }, - .size = size, - .has_version = true, - .version = version == 2 - ? BLOCKDEV_QCOW2_VERSION_V2 - : BLOCKDEV_QCOW2_VERSION_V3, - .has_backing_file = (backing_file != NULL), - .backing_file = backing_file, - .has_backing_fmt = (backing_fmt != NULL), - .backing_fmt = backing_drv, - .has_encrypt = (encryptfmt != NULL), - .encrypt = cryptoopts, - .has_cluster_size = true, - .cluster_size = cluster_size, - .has_preallocation = true, - .preallocation = prealloc, - .has_lazy_refcounts = true, - .lazy_refcounts = (flags & BLOCK_FLAG_LAZY_REFCOUNTS), - .has_refcount_bits = true, - .refcount_bits = refcount_bits, - }, - }; - ret = qcow2_co_create(&create_options, errp); + ret = qcow2_co_create(create_options, errp); if (ret < 0) { goto finish; } + ret = 0; finish: + QDECREF(qdict); bdrv_unref(bs); - - qapi_free_QCryptoBlockCreateOptions(cryptoopts); - g_free(backing_file); - g_free(backing_fmt); - g_free(encryptfmt); - g_free(buf); + qapi_free_BlockdevCreateOptions(create_options); return ret; } diff --git a/tests/qemu-iotests/049.out b/tests/qemu-iotests/049.out index 003247023e..0871bff564 100644 --- a/tests/qemu-iotests/049.out +++ b/tests/qemu-iotests/049.out @@ -166,11 +166,11 @@ qemu-img create -f qcow2 -o compat=1.1 TEST_DIR/t.qcow2 64M Formatting 'TEST_DIR/t.qcow2', fmt=qcow2 size=67108864 compat=1.1 cluster_size=65536 lazy_refcounts=off refcount_bits=16 qemu-img create -f qcow2 -o compat=0.42 TEST_DIR/t.qcow2 64M -qemu-img: TEST_DIR/t.qcow2: Invalid compatibility level: '0.42' +qemu-img: TEST_DIR/t.qcow2: Invalid parameter '0.42' Formatting 'TEST_DIR/t.qcow2', fmt=qcow2 size=67108864 compat=0.42 cluster_size=65536 lazy_refcounts=off refcount_bits=16 qemu-img create -f qcow2 -o compat=foobar TEST_DIR/t.qcow2 64M -qemu-img: TEST_DIR/t.qcow2: Invalid compatibility level: 'foobar' +qemu-img: TEST_DIR/t.qcow2: Invalid parameter 'foobar' Formatting 'TEST_DIR/t.qcow2', fmt=qcow2 size=67108864 compat=foobar cluster_size=65536 lazy_refcounts=off refcount_bits=16 == Check preallocation option == @@ -182,7 +182,7 @@ qemu-img create -f qcow2 -o preallocation=metadata TEST_DIR/t.qcow2 64M Formatting 'TEST_DIR/t.qcow2', fmt=qcow2 size=67108864 cluster_size=65536 preallocation=metadata lazy_refcounts=off refcount_bits=16 qemu-img create -f qcow2 -o preallocation=1234 TEST_DIR/t.qcow2 64M -qemu-img: TEST_DIR/t.qcow2: invalid parameter value: 1234 +qemu-img: TEST_DIR/t.qcow2: Invalid parameter '1234' Formatting 'TEST_DIR/t.qcow2', fmt=qcow2 size=67108864 cluster_size=65536 preallocation=1234 lazy_refcounts=off refcount_bits=16 == Check encryption option == @@ -205,7 +205,7 @@ qemu-img create -f qcow2 -o compat=0.10,lazy_refcounts=off TEST_DIR/t.qcow2 64M Formatting 'TEST_DIR/t.qcow2', fmt=qcow2 size=67108864 compat=0.10 cluster_size=65536 lazy_refcounts=off refcount_bits=16 qemu-img create -f qcow2 -o compat=0.10,lazy_refcounts=on TEST_DIR/t.qcow2 64M -qemu-img: TEST_DIR/t.qcow2: Lazy refcounts only supported with compatibility level 1.1 and above (use compat=1.1 or greater) +qemu-img: TEST_DIR/t.qcow2: Lazy refcounts only supported with compatibility level 1.1 and above (use version=v3 or greater) Formatting 'TEST_DIR/t.qcow2', fmt=qcow2 size=67108864 compat=0.10 cluster_size=65536 lazy_refcounts=on refcount_bits=16 *** done diff --git a/tests/qemu-iotests/112.out b/tests/qemu-iotests/112.out index 81b04d1452..86f041075d 100644 --- a/tests/qemu-iotests/112.out +++ b/tests/qemu-iotests/112.out @@ -21,9 +21,9 @@ refcount bits: 16 Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=67108864 refcount bits: 16 -qemu-img: TEST_DIR/t.IMGFMT: Different refcount widths than 16 bits require compatibility level 1.1 or above (use or greater) +qemu-img: TEST_DIR/t.IMGFMT: Different refcount widths than 16 bits require compatibility level 1.1 or above (use version=v3 or greater) Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=67108864 -qemu-img: TEST_DIR/t.IMGFMT: Different refcount widths than 16 bits require compatibility level 1.1 or above (use or greater) +qemu-img: TEST_DIR/t.IMGFMT: Different refcount widths than 16 bits require compatibility level 1.1 or above (use version=v3 or greater) Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=67108864 === Snapshot limit on refcount_bits=1 === From e8eb8637786635e73df3da3d24aff0e01d48fa51 Mon Sep 17 00:00:00 2001 From: Kevin Wolf Date: Wed, 21 Feb 2018 11:47:43 +0100 Subject: [PATCH 28/56] block: Make bdrv_is_whitelisted() public We'll use a separate source file for image creation, and we need to check there whether the requested driver is whitelisted. Signed-off-by: Kevin Wolf Reviewed-by: Max Reitz Reviewed-by: Eric Blake --- block.c | 2 +- include/block/block.h | 1 + 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/block.c b/block.c index 4fc65f7621..00f94241fc 100644 --- a/block.c +++ b/block.c @@ -370,7 +370,7 @@ BlockDriver *bdrv_find_format(const char *format_name) return bdrv_do_find_format(format_name); } -static int bdrv_is_whitelisted(BlockDriver *drv, bool read_only) +int bdrv_is_whitelisted(BlockDriver *drv, bool read_only) { static const char *whitelist_rw[] = { CONFIG_BDRV_RW_WHITELIST diff --git a/include/block/block.h b/include/block/block.h index 7805187b30..cdec3639a3 100644 --- a/include/block/block.h +++ b/include/block/block.h @@ -226,6 +226,7 @@ char *bdrv_perm_names(uint64_t perm); void bdrv_init(void); void bdrv_init_with_whitelist(void); bool bdrv_uses_whitelist(void); +int bdrv_is_whitelisted(BlockDriver *drv, bool read_only); BlockDriver *bdrv_find_protocol(const char *filename, bool allow_protocol_prefix, Error **errp); From b0292b851b85ba81c0cfedf5b576c32189cfaa11 Mon Sep 17 00:00:00 2001 From: Kevin Wolf Date: Tue, 9 Jan 2018 16:50:57 +0100 Subject: [PATCH 29/56] block: x-blockdev-create QMP command This adds a synchronous x-blockdev-create QMP command that can create qcow2 images on a given node name. We don't want to block while creating an image, so this is not the final interface in all aspects, but BlockdevCreateOptionsQcow2 and .bdrv_co_create() are what they actually might look like in the end. In any case, this should be good enough to test whether we interpret BlockdevCreateOptions as we should. Signed-off-by: Kevin Wolf Reviewed-by: Max Reitz Reviewed-by: Eric Blake --- block/Makefile.objs | 2 +- block/create.c | 76 +++++++++++++++++++++++++++++++++++++++ block/qcow2.c | 1 + include/block/block_int.h | 5 ++- qapi/block-core.json | 12 +++++++ 5 files changed, 94 insertions(+), 2 deletions(-) create mode 100644 block/create.c diff --git a/block/Makefile.objs b/block/Makefile.objs index aede94f105..d644bac60a 100644 --- a/block/Makefile.objs +++ b/block/Makefile.objs @@ -9,7 +9,7 @@ block-obj-y += block-backend.o snapshot.o qapi.o block-obj-$(CONFIG_WIN32) += file-win32.o win32-aio.o block-obj-$(CONFIG_POSIX) += file-posix.o block-obj-$(CONFIG_LINUX_AIO) += linux-aio.o -block-obj-y += null.o mirror.o commit.o io.o +block-obj-y += null.o mirror.o commit.o io.o create.o block-obj-y += throttle-groups.o block-obj-$(CONFIG_LINUX) += nvme.o diff --git a/block/create.c b/block/create.c new file mode 100644 index 0000000000..8bd8a03719 --- /dev/null +++ b/block/create.c @@ -0,0 +1,76 @@ +/* + * Block layer code related to image creation + * + * Copyright (c) 2018 Kevin Wolf + * + * Permission is hereby granted, free of charge, to any person obtaining a copy + * of this software and associated documentation files (the "Software"), to deal + * in the Software without restriction, including without limitation the rights + * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell + * copies of the Software, and to permit persons to whom the Software is + * furnished to do so, subject to the following conditions: + * + * The above copyright notice and this permission notice shall be included in + * all copies or substantial portions of the Software. + * + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN + * THE SOFTWARE. + */ + +#include "qemu/osdep.h" +#include "block/block_int.h" +#include "qapi/qapi-commands-block-core.h" +#include "qapi/error.h" + +typedef struct BlockdevCreateCo { + BlockDriver *drv; + BlockdevCreateOptions *opts; + int ret; + Error **errp; +} BlockdevCreateCo; + +static void coroutine_fn bdrv_co_create_co_entry(void *opaque) +{ + BlockdevCreateCo *cco = opaque; + cco->ret = cco->drv->bdrv_co_create(cco->opts, cco->errp); +} + +void qmp_x_blockdev_create(BlockdevCreateOptions *options, Error **errp) +{ + const char *fmt = BlockdevDriver_str(options->driver); + BlockDriver *drv = bdrv_find_format(fmt); + Coroutine *co; + BlockdevCreateCo cco; + + /* If the driver is in the schema, we know that it exists. But it may not + * be whitelisted. */ + assert(drv); + if (bdrv_uses_whitelist() && !bdrv_is_whitelisted(drv, false)) { + error_setg(errp, "Driver is not whitelisted"); + return; + } + + /* Call callback if it exists */ + if (!drv->bdrv_co_create) { + error_setg(errp, "Driver does not support blockdev-create"); + return; + } + + cco = (BlockdevCreateCo) { + .drv = drv, + .opts = options, + .ret = -EINPROGRESS, + .errp = errp, + }; + + co = qemu_coroutine_create(bdrv_co_create_co_entry, &cco); + qemu_coroutine_enter(co); + while (cco.ret == -EINPROGRESS) { + aio_poll(qemu_get_aio_context(), true); + } +} diff --git a/block/qcow2.c b/block/qcow2.c index 37b0e36c1e..7472af6931 100644 --- a/block/qcow2.c +++ b/block/qcow2.c @@ -4490,6 +4490,7 @@ BlockDriver bdrv_qcow2 = { .bdrv_join_options = qcow2_join_options, .bdrv_child_perm = bdrv_format_default_perms, .bdrv_co_create_opts = qcow2_co_create_opts, + .bdrv_co_create = qcow2_co_create, .bdrv_has_zero_init = bdrv_has_zero_init_1, .bdrv_co_block_status = qcow2_co_block_status, diff --git a/include/block/block_int.h b/include/block/block_int.h index a84cc04d55..27e17addba 100644 --- a/include/block/block_int.h +++ b/include/block/block_int.h @@ -129,8 +129,11 @@ struct BlockDriver { int (*bdrv_file_open)(BlockDriverState *bs, QDict *options, int flags, Error **errp); void (*bdrv_close)(BlockDriverState *bs); - int coroutine_fn (*bdrv_co_create_opts)(const char *filename, QemuOpts *opts, + int coroutine_fn (*bdrv_co_create)(BlockdevCreateOptions *opts, Error **errp); + int coroutine_fn (*bdrv_co_create_opts)(const char *filename, + QemuOpts *opts, + Error **errp); int (*bdrv_make_empty)(BlockDriverState *bs); void (*bdrv_refresh_filename)(BlockDriverState *bs, QDict *options); diff --git a/qapi/block-core.json b/qapi/block-core.json index dfea7b0102..88d7a8678d 100644 --- a/qapi/block-core.json +++ b/qapi/block-core.json @@ -3463,6 +3463,18 @@ 'vxhs': 'BlockdevCreateNotSupported' } } +## +# @x-blockdev-create: +# +# Create an image format on a given node. +# TODO Replace with something asynchronous (block job?) +# +# Since: 2.12 +## +{ 'command': 'x-blockdev-create', + 'data': 'BlockdevCreateOptions', + 'boxed': true } + ## # @blockdev-open-tray: # From 927f11e1316c1cf96ab70d271f8b29d65dc75f7c Mon Sep 17 00:00:00 2001 From: Kevin Wolf Date: Tue, 16 Jan 2018 16:04:21 +0100 Subject: [PATCH 30/56] file-posix: Support .bdrv_co_create This adds the .bdrv_co_create driver callback to file, which enables image creation over QMP. Signed-off-by: Kevin Wolf Reviewed-by: Max Reitz Reviewed-by: Eric Blake --- block/file-posix.c | 79 +++++++++++++++++++++++++++++++------------- qapi/block-core.json | 20 ++++++++++- 2 files changed, 75 insertions(+), 24 deletions(-) diff --git a/block/file-posix.c b/block/file-posix.c index 7f2cc63c60..fbc21a9921 100644 --- a/block/file-posix.c +++ b/block/file-posix.c @@ -1982,34 +1982,25 @@ static int64_t raw_get_allocated_file_size(BlockDriverState *bs) return (int64_t)st.st_blocks * 512; } -static int coroutine_fn raw_co_create_opts(const char *filename, QemuOpts *opts, - Error **errp) +static int raw_co_create(BlockdevCreateOptions *options, Error **errp) { + BlockdevCreateOptionsFile *file_opts; int fd; int result = 0; - int64_t total_size = 0; - bool nocow = false; - PreallocMode prealloc; - char *buf = NULL; - Error *local_err = NULL; - strstart(filename, "file:", &filename); + /* Validate options and set default values */ + assert(options->driver == BLOCKDEV_DRIVER_FILE); + file_opts = &options->u.file; - /* Read out options */ - total_size = ROUND_UP(qemu_opt_get_size_del(opts, BLOCK_OPT_SIZE, 0), - BDRV_SECTOR_SIZE); - nocow = qemu_opt_get_bool(opts, BLOCK_OPT_NOCOW, false); - buf = qemu_opt_get_del(opts, BLOCK_OPT_PREALLOC); - prealloc = qapi_enum_parse(&PreallocMode_lookup, buf, - PREALLOC_MODE_OFF, &local_err); - g_free(buf); - if (local_err) { - error_propagate(errp, local_err); - result = -EINVAL; - goto out; + if (!file_opts->has_nocow) { + file_opts->nocow = false; + } + if (!file_opts->has_preallocation) { + file_opts->preallocation = PREALLOC_MODE_OFF; } - fd = qemu_open(filename, O_RDWR | O_CREAT | O_TRUNC | O_BINARY, + /* Create file */ + fd = qemu_open(file_opts->filename, O_RDWR | O_CREAT | O_TRUNC | O_BINARY, 0644); if (fd < 0) { result = -errno; @@ -2017,7 +2008,7 @@ static int coroutine_fn raw_co_create_opts(const char *filename, QemuOpts *opts, goto out; } - if (nocow) { + if (file_opts->nocow) { #ifdef __linux__ /* Set NOCOW flag to solve performance issue on fs like btrfs. * This is an optimisation. The FS_IOC_SETFLAGS ioctl return value @@ -2032,7 +2023,8 @@ static int coroutine_fn raw_co_create_opts(const char *filename, QemuOpts *opts, #endif } - result = raw_regular_truncate(fd, total_size, prealloc, errp); + result = raw_regular_truncate(fd, file_opts->size, file_opts->preallocation, + errp); if (result < 0) { goto out_close; } @@ -2046,6 +2038,46 @@ out: return result; } +static int coroutine_fn raw_co_create_opts(const char *filename, QemuOpts *opts, + Error **errp) +{ + BlockdevCreateOptions options; + int64_t total_size = 0; + bool nocow = false; + PreallocMode prealloc; + char *buf = NULL; + Error *local_err = NULL; + + /* Skip file: protocol prefix */ + strstart(filename, "file:", &filename); + + /* Read out options */ + total_size = ROUND_UP(qemu_opt_get_size_del(opts, BLOCK_OPT_SIZE, 0), + BDRV_SECTOR_SIZE); + nocow = qemu_opt_get_bool(opts, BLOCK_OPT_NOCOW, false); + buf = qemu_opt_get_del(opts, BLOCK_OPT_PREALLOC); + prealloc = qapi_enum_parse(&PreallocMode_lookup, buf, + PREALLOC_MODE_OFF, &local_err); + g_free(buf); + if (local_err) { + error_propagate(errp, local_err); + return -EINVAL; + } + + options = (BlockdevCreateOptions) { + .driver = BLOCKDEV_DRIVER_FILE, + .u.file = { + .filename = (char *) filename, + .size = total_size, + .has_preallocation = true, + .preallocation = prealloc, + .has_nocow = true, + .nocow = nocow, + }, + }; + return raw_co_create(&options, errp); +} + /* * Find allocation range in @bs around offset @start. * May change underlying file descriptor's file offset. @@ -2277,6 +2309,7 @@ BlockDriver bdrv_file = { .bdrv_reopen_commit = raw_reopen_commit, .bdrv_reopen_abort = raw_reopen_abort, .bdrv_close = raw_close, + .bdrv_co_create = raw_co_create, .bdrv_co_create_opts = raw_co_create_opts, .bdrv_has_zero_init = bdrv_has_zero_init_1, .bdrv_co_block_status = raw_co_block_status, diff --git a/qapi/block-core.json b/qapi/block-core.json index 88d7a8678d..41955b097f 100644 --- a/qapi/block-core.json +++ b/qapi/block-core.json @@ -3358,6 +3358,24 @@ ## { 'command': 'blockdev-del', 'data': { 'node-name': 'str' } } +## +# @BlockdevCreateOptionsFile: +# +# Driver specific image creation options for file. +# +# @filename Filename for the new image file +# @size Size of the virtual disk in bytes +# @preallocation Preallocation mode for the new image (default: off) +# @nocow Turn off copy-on-write (valid only on btrfs; default: off) +# +# Since: 2.12 +## +{ 'struct': 'BlockdevCreateOptionsFile', + 'data': { 'filename': 'str', + 'size': 'size', + '*preallocation': 'PreallocMode', + '*nocow': 'bool' } } + ## # @BlockdevQcow2Version: # @@ -3429,7 +3447,7 @@ 'bochs': 'BlockdevCreateNotSupported', 'cloop': 'BlockdevCreateNotSupported', 'dmg': 'BlockdevCreateNotSupported', - 'file': 'BlockdevCreateNotSupported', + 'file': 'BlockdevCreateOptionsFile', 'ftp': 'BlockdevCreateNotSupported', 'ftps': 'BlockdevCreateNotSupported', 'gluster': 'BlockdevCreateNotSupported', From 3766ef579ce0bf6c8fff632a7f793833f35d2658 Mon Sep 17 00:00:00 2001 From: Kevin Wolf Date: Mon, 5 Feb 2018 16:24:32 +0100 Subject: [PATCH 31/56] file-win32: Support .bdrv_co_create This adds the .bdrv_co_create driver callback to file-win32, which enables image creation over QMP. Signed-off-by: Kevin Wolf Reviewed-by: Max Reitz Reviewed-by: Eric Blake --- block/file-win32.c | 53 +++++++++++++++++++++++++++++++++++----------- 1 file changed, 41 insertions(+), 12 deletions(-) diff --git a/block/file-win32.c b/block/file-win32.c index 4a430d45f1..2e2f746bb1 100644 --- a/block/file-win32.c +++ b/block/file-win32.c @@ -553,10 +553,40 @@ static int64_t raw_get_allocated_file_size(BlockDriverState *bs) return st.st_size; } +static int raw_co_create(BlockdevCreateOptions *options, Error **errp) +{ + BlockdevCreateOptionsFile *file_opts; + int fd; + + assert(options->driver == BLOCKDEV_DRIVER_FILE); + file_opts = &options->u.file; + + if (file_opts->has_preallocation) { + error_setg(errp, "Preallocation is not supported on Windows"); + return -EINVAL; + } + if (file_opts->has_nocow) { + error_setg(errp, "nocow is not supported on Windows"); + return -EINVAL; + } + + fd = qemu_open(file_opts->filename, O_WRONLY | O_CREAT | O_TRUNC | O_BINARY, + 0644); + if (fd < 0) { + error_setg_errno(errp, errno, "Could not create file"); + return -EIO; + } + set_sparse(fd); + ftruncate(fd, file_opts->size); + qemu_close(fd); + + return 0; +} + static int coroutine_fn raw_co_create_opts(const char *filename, QemuOpts *opts, Error **errp) { - int fd; + BlockdevCreateOptions options; int64_t total_size = 0; strstart(filename, "file:", &filename); @@ -565,19 +595,18 @@ static int coroutine_fn raw_co_create_opts(const char *filename, QemuOpts *opts, total_size = ROUND_UP(qemu_opt_get_size_del(opts, BLOCK_OPT_SIZE, 0), BDRV_SECTOR_SIZE); - fd = qemu_open(filename, O_WRONLY | O_CREAT | O_TRUNC | O_BINARY, - 0644); - if (fd < 0) { - error_setg_errno(errp, errno, "Could not create file"); - return -EIO; - } - set_sparse(fd); - ftruncate(fd, total_size); - qemu_close(fd); - return 0; + options = (BlockdevCreateOptions) { + .driver = BLOCKDEV_DRIVER_FILE, + .u.file = { + .filename = (char *) filename, + .size = total_size, + .has_preallocation = false, + .has_nocow = false, + }, + }; + return raw_co_create(&options, errp); } - static QemuOptsList raw_create_opts = { .name = "raw-create-opts", .head = QTAILQ_HEAD_INITIALIZER(raw_create_opts.head), From ab8bda76a010261ac6df2fd6e0559c3589b6e847 Mon Sep 17 00:00:00 2001 From: Kevin Wolf Date: Wed, 31 Jan 2018 16:27:38 +0100 Subject: [PATCH 32/56] gluster: Support .bdrv_co_create This adds the .bdrv_co_create driver callback to gluster, which enables image creation over QMP. Signed-off-by: Kevin Wolf Reviewed-by: Max Reitz Reviewed-by: Eric Blake --- block/gluster.c | 135 +++++++++++++++++++++++++++++-------------- qapi/block-core.json | 18 +++++- 2 files changed, 108 insertions(+), 45 deletions(-) diff --git a/block/gluster.c b/block/gluster.c index 79b4cfdf74..63d3c37d4c 100644 --- a/block/gluster.c +++ b/block/gluster.c @@ -655,9 +655,11 @@ out: return -errno; } -static struct glfs *qemu_gluster_init(BlockdevOptionsGluster *gconf, - const char *filename, - QDict *options, Error **errp) +/* Converts options given in @filename and the @options QDict into the QAPI + * object @gconf. */ +static int qemu_gluster_parse(BlockdevOptionsGluster *gconf, + const char *filename, + QDict *options, Error **errp) { int ret; if (filename) { @@ -668,8 +670,7 @@ static struct glfs *qemu_gluster_init(BlockdevOptionsGluster *gconf, "[host[:port]]volume/path[?socket=...]" "[,file.debug=N]" "[,file.logfile=/path/filename.log]\n"); - errno = -ret; - return NULL; + return ret; } } else { ret = qemu_gluster_parse_json(gconf, options, errp); @@ -685,10 +686,23 @@ static struct glfs *qemu_gluster_init(BlockdevOptionsGluster *gconf, "file.server.1.transport=unix," "file.server.1.socket=/var/run/glusterd.socket ..." "\n"); - errno = -ret; - return NULL; + return ret; } + } + return 0; +} + +static struct glfs *qemu_gluster_init(BlockdevOptionsGluster *gconf, + const char *filename, + QDict *options, Error **errp) +{ + int ret; + + ret = qemu_gluster_parse(gconf, filename, options, errp); + if (ret < 0) { + errno = -ret; + return NULL; } return qemu_gluster_glfs_init(gconf, errp); @@ -1021,20 +1035,72 @@ static int qemu_gluster_do_truncate(struct glfs_fd *fd, int64_t offset, return 0; } +static int qemu_gluster_co_create(BlockdevCreateOptions *options, + Error **errp) +{ + BlockdevCreateOptionsGluster *opts = &options->u.gluster; + struct glfs *glfs; + struct glfs_fd *fd = NULL; + int ret = 0; + + assert(options->driver == BLOCKDEV_DRIVER_GLUSTER); + + glfs = qemu_gluster_glfs_init(opts->location, errp); + if (!glfs) { + ret = -errno; + goto out; + } + + fd = glfs_creat(glfs, opts->location->path, + O_WRONLY | O_CREAT | O_TRUNC | O_BINARY, S_IRUSR | S_IWUSR); + if (!fd) { + ret = -errno; + goto out; + } + + ret = qemu_gluster_do_truncate(fd, opts->size, opts->preallocation, errp); + +out: + if (fd) { + if (glfs_close(fd) != 0 && ret == 0) { + ret = -errno; + } + } + glfs_clear_preopened(glfs); + return ret; +} + static int coroutine_fn qemu_gluster_co_create_opts(const char *filename, QemuOpts *opts, Error **errp) { + BlockdevCreateOptions *options; + BlockdevCreateOptionsGluster *gopts; BlockdevOptionsGluster *gconf; - struct glfs *glfs; - struct glfs_fd *fd = NULL; - int ret = 0; - PreallocMode prealloc; - int64_t total_size = 0; char *tmp = NULL; Error *local_err = NULL; + int ret; + + options = g_new0(BlockdevCreateOptions, 1); + options->driver = BLOCKDEV_DRIVER_GLUSTER; + gopts = &options->u.gluster; gconf = g_new0(BlockdevOptionsGluster, 1); + gopts->location = gconf; + + gopts->size = ROUND_UP(qemu_opt_get_size_del(opts, BLOCK_OPT_SIZE, 0), + BDRV_SECTOR_SIZE); + + tmp = qemu_opt_get_del(opts, BLOCK_OPT_PREALLOC); + gopts->preallocation = qapi_enum_parse(&PreallocMode_lookup, tmp, + PREALLOC_MODE_OFF, &local_err); + g_free(tmp); + if (local_err) { + error_propagate(errp, local_err); + ret = -EINVAL; + goto fail; + } + gconf->debug = qemu_opt_get_number_del(opts, GLUSTER_OPT_DEBUG, GLUSTER_DEBUG_DEFAULT); if (gconf->debug < 0) { @@ -1050,42 +1116,19 @@ static int coroutine_fn qemu_gluster_co_create_opts(const char *filename, } gconf->has_logfile = true; - glfs = qemu_gluster_init(gconf, filename, NULL, errp); - if (!glfs) { - ret = -errno; - goto out; + ret = qemu_gluster_parse(gconf, filename, NULL, errp); + if (ret < 0) { + goto fail; } - total_size = ROUND_UP(qemu_opt_get_size_del(opts, BLOCK_OPT_SIZE, 0), - BDRV_SECTOR_SIZE); - - tmp = qemu_opt_get_del(opts, BLOCK_OPT_PREALLOC); - prealloc = qapi_enum_parse(&PreallocMode_lookup, tmp, PREALLOC_MODE_OFF, - &local_err); - g_free(tmp); - if (local_err) { - error_propagate(errp, local_err); - ret = -EINVAL; - goto out; + ret = qemu_gluster_co_create(options, errp); + if (ret < 0) { + goto fail; } - fd = glfs_creat(glfs, gconf->path, - O_WRONLY | O_CREAT | O_TRUNC | O_BINARY, S_IRUSR | S_IWUSR); - if (!fd) { - ret = -errno; - goto out; - } - - ret = qemu_gluster_do_truncate(fd, total_size, prealloc, errp); - -out: - if (fd) { - if (glfs_close(fd) != 0 && ret == 0) { - ret = -errno; - } - } - qapi_free_BlockdevOptionsGluster(gconf); - glfs_clear_preopened(glfs); + ret = 0; +fail: + qapi_free_BlockdevCreateOptions(options); return ret; } @@ -1436,6 +1479,7 @@ static BlockDriver bdrv_gluster = { .bdrv_reopen_commit = qemu_gluster_reopen_commit, .bdrv_reopen_abort = qemu_gluster_reopen_abort, .bdrv_close = qemu_gluster_close, + .bdrv_co_create = qemu_gluster_co_create, .bdrv_co_create_opts = qemu_gluster_co_create_opts, .bdrv_getlength = qemu_gluster_getlength, .bdrv_get_allocated_file_size = qemu_gluster_allocated_file_size, @@ -1464,6 +1508,7 @@ static BlockDriver bdrv_gluster_tcp = { .bdrv_reopen_commit = qemu_gluster_reopen_commit, .bdrv_reopen_abort = qemu_gluster_reopen_abort, .bdrv_close = qemu_gluster_close, + .bdrv_co_create = qemu_gluster_co_create, .bdrv_co_create_opts = qemu_gluster_co_create_opts, .bdrv_getlength = qemu_gluster_getlength, .bdrv_get_allocated_file_size = qemu_gluster_allocated_file_size, @@ -1492,6 +1537,7 @@ static BlockDriver bdrv_gluster_unix = { .bdrv_reopen_commit = qemu_gluster_reopen_commit, .bdrv_reopen_abort = qemu_gluster_reopen_abort, .bdrv_close = qemu_gluster_close, + .bdrv_co_create = qemu_gluster_co_create, .bdrv_co_create_opts = qemu_gluster_co_create_opts, .bdrv_getlength = qemu_gluster_getlength, .bdrv_get_allocated_file_size = qemu_gluster_allocated_file_size, @@ -1526,6 +1572,7 @@ static BlockDriver bdrv_gluster_rdma = { .bdrv_reopen_commit = qemu_gluster_reopen_commit, .bdrv_reopen_abort = qemu_gluster_reopen_abort, .bdrv_close = qemu_gluster_close, + .bdrv_co_create = qemu_gluster_co_create, .bdrv_co_create_opts = qemu_gluster_co_create_opts, .bdrv_getlength = qemu_gluster_getlength, .bdrv_get_allocated_file_size = qemu_gluster_allocated_file_size, diff --git a/qapi/block-core.json b/qapi/block-core.json index 41955b097f..9170fbf6e6 100644 --- a/qapi/block-core.json +++ b/qapi/block-core.json @@ -3376,6 +3376,22 @@ '*preallocation': 'PreallocMode', '*nocow': 'bool' } } +## +# @BlockdevCreateOptionsGluster: +# +# Driver specific image creation options for gluster. +# +# @location Where to store the new image file +# @size Size of the virtual disk in bytes +# @preallocation Preallocation mode for the new image (default: off) +# +# Since: 2.12 +## +{ 'struct': 'BlockdevCreateOptionsGluster', + 'data': { 'location': 'BlockdevOptionsGluster', + 'size': 'size', + '*preallocation': 'PreallocMode' } } + ## # @BlockdevQcow2Version: # @@ -3450,7 +3466,7 @@ 'file': 'BlockdevCreateOptionsFile', 'ftp': 'BlockdevCreateNotSupported', 'ftps': 'BlockdevCreateNotSupported', - 'gluster': 'BlockdevCreateNotSupported', + 'gluster': 'BlockdevCreateOptionsGluster', 'host_cdrom': 'BlockdevCreateNotSupported', 'host_device': 'BlockdevCreateNotSupported', 'http': 'BlockdevCreateNotSupported', From 71c87815f9e0386b6f3e22942adc956fd603c82f Mon Sep 17 00:00:00 2001 From: Kevin Wolf Date: Fri, 16 Feb 2018 19:14:55 +0100 Subject: [PATCH 33/56] rbd: Fix use after free in qemu_rbd_set_keypairs() error path If we want to include the invalid option name in the error message, we can't free the string earlier than that. Cc: qemu-stable@nongnu.org Signed-off-by: Kevin Wolf Reviewed-by: Max Reitz Reviewed-by: Eric Blake --- block/rbd.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/block/rbd.c b/block/rbd.c index c1275c1ec9..c1025c8493 100644 --- a/block/rbd.c +++ b/block/rbd.c @@ -268,13 +268,14 @@ static int qemu_rbd_set_keypairs(rados_t cluster, const char *keypairs_json, key = qstring_get_str(name); ret = rados_conf_set(cluster, key, qstring_get_str(value)); - QDECREF(name); QDECREF(value); if (ret < 0) { error_setg_errno(errp, -ret, "invalid conf option %s", key); + QDECREF(name); ret = -EINVAL; break; } + QDECREF(name); } QDECREF(keypairs); From 3d9136f972d3dc678fc46de8fc126ba00a46508e Mon Sep 17 00:00:00 2001 From: Kevin Wolf Date: Thu, 15 Feb 2018 19:13:47 +0100 Subject: [PATCH 34/56] rbd: Factor out qemu_rbd_connect() The code to establish an RBD connection is duplicated between open and create. In order to be able to share the code, factor out the code from qemu_rbd_open() as a first step. Signed-off-by: Kevin Wolf Reviewed-by: Max Reitz --- block/rbd.c | 100 +++++++++++++++++++++++++++++++--------------------- 1 file changed, 60 insertions(+), 40 deletions(-) diff --git a/block/rbd.c b/block/rbd.c index c1025c8493..99fcc7ecdf 100644 --- a/block/rbd.c +++ b/block/rbd.c @@ -546,32 +546,17 @@ out: return rados_str; } -static int qemu_rbd_open(BlockDriverState *bs, QDict *options, int flags, - Error **errp) +static int qemu_rbd_connect(rados_t *cluster, rados_ioctx_t *io_ctx, + char **s_snap, char **s_image_name, + QDict *options, bool cache, Error **errp) { - BDRVRBDState *s = bs->opaque; - const char *pool, *snap, *conf, *user, *image_name, *keypairs; - const char *secretid, *filename; QemuOpts *opts; - Error *local_err = NULL; char *mon_host = NULL; + const char *pool, *snap, *conf, *user, *image_name, *keypairs; + const char *secretid; + Error *local_err = NULL; int r; - /* If we are given a filename, parse the filename, with precedence given to - * filename encoded options */ - filename = qdict_get_try_str(options, "filename"); - if (filename) { - warn_report("'filename' option specified. " - "This is an unsupported option, and may be deprecated " - "in the future"); - qemu_rbd_parse_filename(filename, options, &local_err); - if (local_err) { - r = -EINVAL; - error_propagate(errp, local_err); - goto exit; - } - } - opts = qemu_opts_create(&runtime_opts, NULL, 0, &error_abort); qemu_opts_absorb_qdict(opts, options, &local_err); if (local_err) { @@ -602,35 +587,35 @@ static int qemu_rbd_open(BlockDriverState *bs, QDict *options, int flags, goto failed_opts; } - r = rados_create(&s->cluster, user); + r = rados_create(cluster, user); if (r < 0) { error_setg_errno(errp, -r, "error initializing"); goto failed_opts; } - s->snap = g_strdup(snap); - s->image_name = g_strdup(image_name); + *s_snap = g_strdup(snap); + *s_image_name = g_strdup(image_name); /* try default location when conf=NULL, but ignore failure */ - r = rados_conf_read_file(s->cluster, conf); + r = rados_conf_read_file(*cluster, conf); if (conf && r < 0) { error_setg_errno(errp, -r, "error reading conf file %s", conf); goto failed_shutdown; } - r = qemu_rbd_set_keypairs(s->cluster, keypairs, errp); + r = qemu_rbd_set_keypairs(*cluster, keypairs, errp); if (r < 0) { goto failed_shutdown; } if (mon_host) { - r = rados_conf_set(s->cluster, "mon_host", mon_host); + r = rados_conf_set(*cluster, "mon_host", mon_host); if (r < 0) { goto failed_shutdown; } } - if (qemu_rbd_set_auth(s->cluster, secretid, errp) < 0) { + if (qemu_rbd_set_auth(*cluster, secretid, errp) < 0) { r = -EIO; goto failed_shutdown; } @@ -642,24 +627,65 @@ static int qemu_rbd_open(BlockDriverState *bs, QDict *options, int flags, * librbd defaults to no caching. If write through caching cannot * be set up, fall back to no caching. */ - if (flags & BDRV_O_NOCACHE) { - rados_conf_set(s->cluster, "rbd_cache", "false"); + if (cache) { + rados_conf_set(*cluster, "rbd_cache", "true"); } else { - rados_conf_set(s->cluster, "rbd_cache", "true"); + rados_conf_set(*cluster, "rbd_cache", "false"); } - r = rados_connect(s->cluster); + r = rados_connect(*cluster); if (r < 0) { error_setg_errno(errp, -r, "error connecting"); goto failed_shutdown; } - r = rados_ioctx_create(s->cluster, pool, &s->io_ctx); + r = rados_ioctx_create(*cluster, pool, io_ctx); if (r < 0) { error_setg_errno(errp, -r, "error opening pool %s", pool); goto failed_shutdown; } + qemu_opts_del(opts); + return 0; + +failed_shutdown: + rados_shutdown(*cluster); + g_free(*s_snap); + g_free(*s_image_name); +failed_opts: + qemu_opts_del(opts); + g_free(mon_host); + return r; +} + +static int qemu_rbd_open(BlockDriverState *bs, QDict *options, int flags, + Error **errp) +{ + BDRVRBDState *s = bs->opaque; + Error *local_err = NULL; + const char *filename; + int r; + + /* If we are given a filename, parse the filename, with precedence given to + * filename encoded options */ + filename = qdict_get_try_str(options, "filename"); + if (filename) { + warn_report("'filename' option specified. " + "This is an unsupported option, and may be deprecated " + "in the future"); + qemu_rbd_parse_filename(filename, options, &local_err); + if (local_err) { + error_propagate(errp, local_err); + return -EINVAL; + } + } + + r = qemu_rbd_connect(&s->cluster, &s->io_ctx, &s->snap, &s->image_name, + options, !(flags & BDRV_O_NOCACHE), errp); + if (r < 0) { + return r; + } + /* rbd_open is always r/w */ r = rbd_open(s->io_ctx, s->image_name, &s->image, s->snap); if (r < 0) { @@ -684,19 +710,13 @@ static int qemu_rbd_open(BlockDriverState *bs, QDict *options, int flags, } } - qemu_opts_del(opts); return 0; failed_open: rados_ioctx_destroy(s->io_ctx); -failed_shutdown: - rados_shutdown(s->cluster); g_free(s->snap); g_free(s->image_name); -failed_opts: - qemu_opts_del(opts); - g_free(mon_host); -exit: + rados_shutdown(s->cluster); return r; } From 4ff4504974d0b41cd7e2b1a8aa346072eb219541 Mon Sep 17 00:00:00 2001 From: Kevin Wolf Date: Thu, 15 Feb 2018 20:31:04 +0100 Subject: [PATCH 35/56] rbd: Remove non-schema options from runtime_opts Instead of the QemuOpts in qemu_rbd_connect(), we want to use QAPI objects. As a preparation, fetch those options directly from the QDict that .bdrv_open() supports in the rbd driver and that are not in the schema. Signed-off-by: Kevin Wolf Reviewed-by: Max Reitz --- block/rbd.c | 55 +++++++++++++++++++++++------------------------------ 1 file changed, 24 insertions(+), 31 deletions(-) diff --git a/block/rbd.c b/block/rbd.c index 99fcc7ecdf..a979107f65 100644 --- a/block/rbd.c +++ b/block/rbd.c @@ -326,28 +326,6 @@ static QemuOptsList runtime_opts = { /* * server.* extracted manually, see qemu_rbd_mon_host() */ - { - .name = "password-secret", - .type = QEMU_OPT_STRING, - .help = "ID of secret providing the password", - }, - - /* - * Keys for qemu_rbd_parse_filename(), not in the QAPI schema - */ - { - /* - * HACK: name starts with '=' so that qemu_opts_parse() - * can't set it - */ - .name = "=keyvalue-pairs", - .type = QEMU_OPT_STRING, - .help = "Legacy rados key/value option parameters", - }, - { - .name = "filename", - .type = QEMU_OPT_STRING, - }, { /* end of list */ } }, }; @@ -548,12 +526,13 @@ out: static int qemu_rbd_connect(rados_t *cluster, rados_ioctx_t *io_ctx, char **s_snap, char **s_image_name, - QDict *options, bool cache, Error **errp) + QDict *options, bool cache, + const char *keypairs, const char *secretid, + Error **errp) { QemuOpts *opts; char *mon_host = NULL; - const char *pool, *snap, *conf, *user, *image_name, *keypairs; - const char *secretid; + const char *pool, *snap, *conf, *user, *image_name; Error *local_err = NULL; int r; @@ -572,14 +551,11 @@ static int qemu_rbd_connect(rados_t *cluster, rados_ioctx_t *io_ctx, goto failed_opts; } - secretid = qemu_opt_get(opts, "password-secret"); - pool = qemu_opt_get(opts, "pool"); conf = qemu_opt_get(opts, "conf"); snap = qemu_opt_get(opts, "snapshot"); user = qemu_opt_get(opts, "user"); image_name = qemu_opt_get(opts, "image"); - keypairs = qemu_opt_get(opts, "=keyvalue-pairs"); if (!pool || !image_name) { error_setg(errp, "Parameters 'pool' and 'image' are required"); @@ -664,6 +640,7 @@ static int qemu_rbd_open(BlockDriverState *bs, QDict *options, int flags, BDRVRBDState *s = bs->opaque; Error *local_err = NULL; const char *filename; + char *keypairs, *secretid; int r; /* If we are given a filename, parse the filename, with precedence given to @@ -674,16 +651,28 @@ static int qemu_rbd_open(BlockDriverState *bs, QDict *options, int flags, "This is an unsupported option, and may be deprecated " "in the future"); qemu_rbd_parse_filename(filename, options, &local_err); + qdict_del(options, "filename"); if (local_err) { error_propagate(errp, local_err); return -EINVAL; } } + keypairs = g_strdup(qdict_get_try_str(options, "=keyvalue-pairs")); + if (keypairs) { + qdict_del(options, "=keyvalue-pairs"); + } + + secretid = g_strdup(qdict_get_try_str(options, "password-secret")); + if (secretid) { + qdict_del(options, "password-secret"); + } + r = qemu_rbd_connect(&s->cluster, &s->io_ctx, &s->snap, &s->image_name, - options, !(flags & BDRV_O_NOCACHE), errp); + options, !(flags & BDRV_O_NOCACHE), keypairs, secretid, + errp); if (r < 0) { - return r; + goto out; } /* rbd_open is always r/w */ @@ -710,13 +699,17 @@ static int qemu_rbd_open(BlockDriverState *bs, QDict *options, int flags, } } - return 0; + r = 0; + goto out; failed_open: rados_ioctx_destroy(s->io_ctx); g_free(s->snap); g_free(s->image_name); rados_shutdown(s->cluster); +out: + g_free(keypairs); + g_free(secretid); return r; } From 4bfb274165ba013bacd2a40fd28c7c2e04a831f3 Mon Sep 17 00:00:00 2001 From: Kevin Wolf Date: Thu, 15 Feb 2018 20:58:24 +0100 Subject: [PATCH 36/56] rbd: Pass BlockdevOptionsRbd to qemu_rbd_connect() With the conversion to a QAPI options object, the function is now prepared to be used in a .bdrv_co_create implementation. Signed-off-by: Kevin Wolf Reviewed-by: Max Reitz --- block/rbd.c | 113 +++++++++++++++++++++++++--------------------------- 1 file changed, 54 insertions(+), 59 deletions(-) diff --git a/block/rbd.c b/block/rbd.c index a979107f65..999fea105f 100644 --- a/block/rbd.c +++ b/block/rbd.c @@ -24,6 +24,8 @@ #include "qapi/qmp/qdict.h" #include "qapi/qmp/qjson.h" #include "qapi/qmp/qlist.h" +#include "qapi/qobject-input-visitor.h" +#include "qapi/qapi-visit-block-core.h" /* * When specifying the image filename use: @@ -484,98 +486,71 @@ static void qemu_rbd_complete_aio(RADOSCB *rcb) qemu_aio_unref(acb); } -static char *qemu_rbd_mon_host(QDict *options, Error **errp) +static char *qemu_rbd_mon_host(BlockdevOptionsRbd *opts, Error **errp) { - const char **vals = g_new(const char *, qdict_size(options) + 1); - char keybuf[32]; + const char **vals; const char *host, *port; char *rados_str; - int i; + InetSocketAddressBaseList *p; + int i, cnt; - for (i = 0;; i++) { - sprintf(keybuf, "server.%d.host", i); - host = qdict_get_try_str(options, keybuf); - qdict_del(options, keybuf); - sprintf(keybuf, "server.%d.port", i); - port = qdict_get_try_str(options, keybuf); - qdict_del(options, keybuf); - if (!host && !port) { - break; - } - if (!host) { - error_setg(errp, "Parameter server.%d.host is missing", i); - rados_str = NULL; - goto out; - } + if (!opts->has_server) { + return NULL; + } + + for (cnt = 0, p = opts->server; p; p = p->next) { + cnt++; + } + + vals = g_new(const char *, cnt + 1); + + for (i = 0, p = opts->server; p; p = p->next, i++) { + host = p->value->host; + port = p->value->port; if (strchr(host, ':')) { - vals[i] = port ? g_strdup_printf("[%s]:%s", host, port) - : g_strdup_printf("[%s]", host); + vals[i] = g_strdup_printf("[%s]:%s", host, port); } else { - vals[i] = port ? g_strdup_printf("%s:%s", host, port) - : g_strdup(host); + vals[i] = g_strdup_printf("%s:%s", host, port); } } vals[i] = NULL; rados_str = i ? g_strjoinv(";", (char **)vals) : NULL; -out: g_strfreev((char **)vals); return rados_str; } static int qemu_rbd_connect(rados_t *cluster, rados_ioctx_t *io_ctx, char **s_snap, char **s_image_name, - QDict *options, bool cache, + BlockdevOptionsRbd *opts, bool cache, const char *keypairs, const char *secretid, Error **errp) { - QemuOpts *opts; char *mon_host = NULL; - const char *pool, *snap, *conf, *user, *image_name; Error *local_err = NULL; int r; - opts = qemu_opts_create(&runtime_opts, NULL, 0, &error_abort); - qemu_opts_absorb_qdict(opts, options, &local_err); + mon_host = qemu_rbd_mon_host(opts, &local_err); if (local_err) { error_propagate(errp, local_err); r = -EINVAL; goto failed_opts; } - mon_host = qemu_rbd_mon_host(options, &local_err); - if (local_err) { - error_propagate(errp, local_err); - r = -EINVAL; - goto failed_opts; - } - - pool = qemu_opt_get(opts, "pool"); - conf = qemu_opt_get(opts, "conf"); - snap = qemu_opt_get(opts, "snapshot"); - user = qemu_opt_get(opts, "user"); - image_name = qemu_opt_get(opts, "image"); - - if (!pool || !image_name) { - error_setg(errp, "Parameters 'pool' and 'image' are required"); - r = -EINVAL; - goto failed_opts; - } - - r = rados_create(cluster, user); + r = rados_create(cluster, opts->user); if (r < 0) { error_setg_errno(errp, -r, "error initializing"); goto failed_opts; } - *s_snap = g_strdup(snap); - *s_image_name = g_strdup(image_name); + *s_snap = g_strdup(opts->snapshot); + *s_image_name = g_strdup(opts->image); /* try default location when conf=NULL, but ignore failure */ - r = rados_conf_read_file(*cluster, conf); - if (conf && r < 0) { - error_setg_errno(errp, -r, "error reading conf file %s", conf); + r = rados_conf_read_file(*cluster, opts->conf); + if (opts->has_conf && r < 0) { + error_setg_errno(errp, -r, "error reading conf file %s", opts->conf); goto failed_shutdown; } @@ -615,13 +590,12 @@ static int qemu_rbd_connect(rados_t *cluster, rados_ioctx_t *io_ctx, goto failed_shutdown; } - r = rados_ioctx_create(*cluster, pool, io_ctx); + r = rados_ioctx_create(*cluster, opts->pool, io_ctx); if (r < 0) { - error_setg_errno(errp, -r, "error opening pool %s", pool); + error_setg_errno(errp, -r, "error opening pool %s", opts->pool); goto failed_shutdown; } - qemu_opts_del(opts); return 0; failed_shutdown: @@ -629,7 +603,6 @@ failed_shutdown: g_free(*s_snap); g_free(*s_image_name); failed_opts: - qemu_opts_del(opts); g_free(mon_host); return r; } @@ -638,6 +611,9 @@ static int qemu_rbd_open(BlockDriverState *bs, QDict *options, int flags, Error **errp) { BDRVRBDState *s = bs->opaque; + BlockdevOptionsRbd *opts = NULL; + Visitor *v; + QObject *crumpled = NULL; Error *local_err = NULL; const char *filename; char *keypairs, *secretid; @@ -668,8 +644,26 @@ static int qemu_rbd_open(BlockDriverState *bs, QDict *options, int flags, qdict_del(options, "password-secret"); } + /* Convert the remaining options into a QAPI object */ + crumpled = qdict_crumple(options, errp); + if (crumpled == NULL) { + r = -EINVAL; + goto out; + } + + v = qobject_input_visitor_new_keyval(crumpled); + visit_type_BlockdevOptionsRbd(v, NULL, &opts, &local_err); + visit_free(v); + qobject_decref(crumpled); + + if (local_err) { + error_propagate(errp, local_err); + r = -EINVAL; + goto out; + } + r = qemu_rbd_connect(&s->cluster, &s->io_ctx, &s->snap, &s->image_name, - options, !(flags & BDRV_O_NOCACHE), keypairs, secretid, + opts, !(flags & BDRV_O_NOCACHE), keypairs, secretid, errp); if (r < 0) { goto out; @@ -708,6 +702,7 @@ failed_open: g_free(s->image_name); rados_shutdown(s->cluster); out: + qapi_free_BlockdevOptionsRbd(opts); g_free(keypairs); g_free(secretid); return r; From 1bebea37b43223a45c39dcbc158f3cec6164fdbf Mon Sep 17 00:00:00 2001 From: Kevin Wolf Date: Wed, 31 Jan 2018 16:27:38 +0100 Subject: [PATCH 37/56] rbd: Support .bdrv_co_create This adds the .bdrv_co_create driver callback to rbd, which enables image creation over QMP. Signed-off-by: Kevin Wolf Reviewed-by: Max Reitz --- block/rbd.c | 194 +++++++++++++++++++++++++++---------------- qapi/block-core.json | 19 ++++- 2 files changed, 140 insertions(+), 73 deletions(-) diff --git a/block/rbd.c b/block/rbd.c index 999fea105f..1cd526bcea 100644 --- a/block/rbd.c +++ b/block/rbd.c @@ -332,40 +332,127 @@ static QemuOptsList runtime_opts = { }, }; +/* FIXME Deprecate and remove keypairs or make it available in QMP. + * password_secret should eventually be configurable in opts->location. Support + * for it in .bdrv_open will make it work here as well. */ +static int qemu_rbd_do_create(BlockdevCreateOptions *options, + const char *keypairs, const char *password_secret, + Error **errp) +{ + BlockdevCreateOptionsRbd *opts = &options->u.rbd; + rados_t cluster; + rados_ioctx_t io_ctx; + int obj_order = 0; + int ret; + + assert(options->driver == BLOCKDEV_DRIVER_RBD); + if (opts->location->has_snapshot) { + error_setg(errp, "Can't use snapshot name for image creation"); + return -EINVAL; + } + + /* TODO Remove the limitation */ + if (opts->location->has_server) { + error_setg(errp, "Can't specify server for image creation"); + return -EINVAL; + } + + if (opts->has_cluster_size) { + int64_t objsize = opts->cluster_size; + if ((objsize - 1) & objsize) { /* not a power of 2? */ + error_setg(errp, "obj size needs to be power of 2"); + return -EINVAL; + } + if (objsize < 4096) { + error_setg(errp, "obj size too small"); + return -EINVAL; + } + obj_order = ctz32(objsize); + } + + ret = rados_create(&cluster, opts->location->user); + if (ret < 0) { + error_setg_errno(errp, -ret, "error initializing"); + return ret; + } + + /* try default location when conf=NULL, but ignore failure */ + ret = rados_conf_read_file(cluster, opts->location->conf); + if (opts->location->conf && ret < 0) { + error_setg_errno(errp, -ret, "error reading conf file %s", + opts->location->conf); + ret = -EIO; + goto shutdown; + } + + ret = qemu_rbd_set_keypairs(cluster, keypairs, errp); + if (ret < 0) { + ret = -EIO; + goto shutdown; + } + + if (qemu_rbd_set_auth(cluster, password_secret, errp) < 0) { + ret = -EIO; + goto shutdown; + } + + ret = rados_connect(cluster); + if (ret < 0) { + error_setg_errno(errp, -ret, "error connecting"); + goto shutdown; + } + + ret = rados_ioctx_create(cluster, opts->location->pool, &io_ctx); + if (ret < 0) { + error_setg_errno(errp, -ret, "error opening pool %s", + opts->location->pool); + goto shutdown; + } + + ret = rbd_create(io_ctx, opts->location->image, opts->size, &obj_order); + if (ret < 0) { + error_setg_errno(errp, -ret, "error rbd create"); + } + + rados_ioctx_destroy(io_ctx); + + ret = 0; +shutdown: + rados_shutdown(cluster); + return ret; +} + +static int qemu_rbd_co_create(BlockdevCreateOptions *options, Error **errp) +{ + return qemu_rbd_do_create(options, NULL, NULL, errp); +} + static int coroutine_fn qemu_rbd_co_create_opts(const char *filename, QemuOpts *opts, Error **errp) { + BlockdevCreateOptions *create_options; + BlockdevCreateOptionsRbd *rbd_opts; + BlockdevOptionsRbd *loc; Error *local_err = NULL; - int64_t bytes = 0; - int64_t objsize; - int obj_order = 0; - const char *pool, *image_name, *conf, *user, *keypairs; - const char *secretid; - rados_t cluster; - rados_ioctx_t io_ctx; + const char *keypairs, *password_secret; QDict *options = NULL; int ret = 0; - secretid = qemu_opt_get(opts, "password-secret"); + create_options = g_new0(BlockdevCreateOptions, 1); + create_options->driver = BLOCKDEV_DRIVER_RBD; + rbd_opts = &create_options->u.rbd; + + rbd_opts->location = g_new0(BlockdevOptionsRbd, 1); + + password_secret = qemu_opt_get(opts, "password-secret"); /* Read out options */ - bytes = ROUND_UP(qemu_opt_get_size_del(opts, BLOCK_OPT_SIZE, 0), - BDRV_SECTOR_SIZE); - objsize = qemu_opt_get_size_del(opts, BLOCK_OPT_CLUSTER_SIZE, 0); - if (objsize) { - if ((objsize - 1) & objsize) { /* not a power of 2? */ - error_setg(errp, "obj size needs to be power of 2"); - ret = -EINVAL; - goto exit; - } - if (objsize < 4096) { - error_setg(errp, "obj size too small"); - ret = -EINVAL; - goto exit; - } - obj_order = ctz32(objsize); - } + rbd_opts->size = ROUND_UP(qemu_opt_get_size_del(opts, BLOCK_OPT_SIZE, 0), + BDRV_SECTOR_SIZE); + rbd_opts->cluster_size = qemu_opt_get_size_del(opts, + BLOCK_OPT_CLUSTER_SIZE, 0); + rbd_opts->has_cluster_size = (rbd_opts->cluster_size != 0); options = qdict_new(); qemu_rbd_parse_filename(filename, options, &local_err); @@ -381,61 +468,23 @@ static int coroutine_fn qemu_rbd_co_create_opts(const char *filename, * or blockdev_add, its members are typed according to the QAPI * schema, but when they come from -drive, they're all QString. */ - pool = qdict_get_try_str(options, "pool"); - conf = qdict_get_try_str(options, "conf"); - user = qdict_get_try_str(options, "user"); - image_name = qdict_get_try_str(options, "image"); - keypairs = qdict_get_try_str(options, "=keyvalue-pairs"); + loc = rbd_opts->location; + loc->pool = g_strdup(qdict_get_try_str(options, "pool")); + loc->conf = g_strdup(qdict_get_try_str(options, "conf")); + loc->has_conf = !!loc->conf; + loc->user = g_strdup(qdict_get_try_str(options, "user")); + loc->has_user = !!loc->user; + loc->image = g_strdup(qdict_get_try_str(options, "image")); + keypairs = qdict_get_try_str(options, "=keyvalue-pairs"); - ret = rados_create(&cluster, user); + ret = qemu_rbd_do_create(create_options, keypairs, password_secret, errp); if (ret < 0) { - error_setg_errno(errp, -ret, "error initializing"); goto exit; } - /* try default location when conf=NULL, but ignore failure */ - ret = rados_conf_read_file(cluster, conf); - if (conf && ret < 0) { - error_setg_errno(errp, -ret, "error reading conf file %s", conf); - ret = -EIO; - goto shutdown; - } - - ret = qemu_rbd_set_keypairs(cluster, keypairs, errp); - if (ret < 0) { - ret = -EIO; - goto shutdown; - } - - if (qemu_rbd_set_auth(cluster, secretid, errp) < 0) { - ret = -EIO; - goto shutdown; - } - - ret = rados_connect(cluster); - if (ret < 0) { - error_setg_errno(errp, -ret, "error connecting"); - goto shutdown; - } - - ret = rados_ioctx_create(cluster, pool, &io_ctx); - if (ret < 0) { - error_setg_errno(errp, -ret, "error opening pool %s", pool); - goto shutdown; - } - - ret = rbd_create(io_ctx, image_name, bytes, &obj_order); - if (ret < 0) { - error_setg_errno(errp, -ret, "error rbd create"); - } - - rados_ioctx_destroy(io_ctx); - -shutdown: - rados_shutdown(cluster); - exit: QDECREF(options); + qapi_free_BlockdevCreateOptions(create_options); return ret; } @@ -1143,6 +1192,7 @@ static BlockDriver bdrv_rbd = { .bdrv_file_open = qemu_rbd_open, .bdrv_close = qemu_rbd_close, .bdrv_reopen_prepare = qemu_rbd_reopen_prepare, + .bdrv_co_create = qemu_rbd_co_create, .bdrv_co_create_opts = qemu_rbd_co_create_opts, .bdrv_has_zero_init = bdrv_has_zero_init_1, .bdrv_get_info = qemu_rbd_getinfo, diff --git a/qapi/block-core.json b/qapi/block-core.json index 9170fbf6e6..d4351877fc 100644 --- a/qapi/block-core.json +++ b/qapi/block-core.json @@ -3435,6 +3435,23 @@ '*lazy-refcounts': 'bool', '*refcount-bits': 'int' } } +## +# @BlockdevCreateOptionsRbd: +# +# Driver specific image creation options for rbd/Ceph. +# +# @location Where to store the new image file. This location cannot +# point to a snapshot. +# @size Size of the virtual disk in bytes +# @cluster-size RBD object size +# +# Since: 2.12 +## +{ 'struct': 'BlockdevCreateOptionsRbd', + 'data': { 'location': 'BlockdevOptionsRbd', + 'size': 'size', + '*cluster-size' : 'size' } } + ## # @BlockdevCreateNotSupported: # @@ -3484,7 +3501,7 @@ 'qed': 'BlockdevCreateNotSupported', 'quorum': 'BlockdevCreateNotSupported', 'raw': 'BlockdevCreateNotSupported', - 'rbd': 'BlockdevCreateNotSupported', + 'rbd': 'BlockdevCreateOptionsRbd', 'replication': 'BlockdevCreateNotSupported', 'sheepdog': 'BlockdevCreateNotSupported', 'ssh': 'BlockdevCreateNotSupported', From d41a55885de18f4fe8dddb8ae88be80497f8eb05 Mon Sep 17 00:00:00 2001 From: Kevin Wolf Date: Fri, 16 Feb 2018 18:54:52 +0100 Subject: [PATCH 38/56] rbd: Assign s->snap/image_name in qemu_rbd_open() Now that the options are already available in qemu_rbd_open() and not only parsed in qemu_rbd_connect(), we can assign s->snap and s->image_name there instead of passing the fields by reference to qemu_rbd_connect(). Signed-off-by: Kevin Wolf Reviewed-by: Max Reitz --- block/rbd.c | 14 +++++--------- 1 file changed, 5 insertions(+), 9 deletions(-) diff --git a/block/rbd.c b/block/rbd.c index 1cd526bcea..2ac7ffca42 100644 --- a/block/rbd.c +++ b/block/rbd.c @@ -571,7 +571,6 @@ static char *qemu_rbd_mon_host(BlockdevOptionsRbd *opts, Error **errp) } static int qemu_rbd_connect(rados_t *cluster, rados_ioctx_t *io_ctx, - char **s_snap, char **s_image_name, BlockdevOptionsRbd *opts, bool cache, const char *keypairs, const char *secretid, Error **errp) @@ -593,9 +592,6 @@ static int qemu_rbd_connect(rados_t *cluster, rados_ioctx_t *io_ctx, goto failed_opts; } - *s_snap = g_strdup(opts->snapshot); - *s_image_name = g_strdup(opts->image); - /* try default location when conf=NULL, but ignore failure */ r = rados_conf_read_file(*cluster, opts->conf); if (opts->has_conf && r < 0) { @@ -649,8 +645,6 @@ static int qemu_rbd_connect(rados_t *cluster, rados_ioctx_t *io_ctx, failed_shutdown: rados_shutdown(*cluster); - g_free(*s_snap); - g_free(*s_image_name); failed_opts: g_free(mon_host); return r; @@ -711,13 +705,15 @@ static int qemu_rbd_open(BlockDriverState *bs, QDict *options, int flags, goto out; } - r = qemu_rbd_connect(&s->cluster, &s->io_ctx, &s->snap, &s->image_name, - opts, !(flags & BDRV_O_NOCACHE), keypairs, secretid, - errp); + r = qemu_rbd_connect(&s->cluster, &s->io_ctx, opts, + !(flags & BDRV_O_NOCACHE), keypairs, secretid, errp); if (r < 0) { goto out; } + s->snap = g_strdup(opts->snapshot); + s->image_name = g_strdup(opts->image); + /* rbd_open is always r/w */ r = rbd_open(s->io_ctx, s->image_name, &s->image, s->snap); if (r < 0) { From aa045c2d37c831541607c0a81212f9d3f2dacad9 Mon Sep 17 00:00:00 2001 From: Kevin Wolf Date: Fri, 16 Feb 2018 18:48:25 +0100 Subject: [PATCH 39/56] rbd: Use qemu_rbd_connect() in qemu_rbd_do_create() This is almost exactly the same code. The differences are that qemu_rbd_connect() supports BlockdevOptionsRbd.server and that the cache mode is set explicitly. Supporting 'server' is a welcome new feature for image creation. Caching is disabled by default, so leave it that way. Signed-off-by: Kevin Wolf Reviewed-by: Max Reitz --- block/rbd.c | 54 ++++++++++------------------------------------------- 1 file changed, 10 insertions(+), 44 deletions(-) diff --git a/block/rbd.c b/block/rbd.c index 2ac7ffca42..294ed07ac4 100644 --- a/block/rbd.c +++ b/block/rbd.c @@ -103,6 +103,11 @@ typedef struct BDRVRBDState { char *snap; } BDRVRBDState; +static int qemu_rbd_connect(rados_t *cluster, rados_ioctx_t *io_ctx, + BlockdevOptionsRbd *opts, bool cache, + const char *keypairs, const char *secretid, + Error **errp); + static char *qemu_rbd_next_tok(char *src, char delim, char **p) { char *end; @@ -351,12 +356,6 @@ static int qemu_rbd_do_create(BlockdevCreateOptions *options, return -EINVAL; } - /* TODO Remove the limitation */ - if (opts->location->has_server) { - error_setg(errp, "Can't specify server for image creation"); - return -EINVAL; - } - if (opts->has_cluster_size) { int64_t objsize = opts->cluster_size; if ((objsize - 1) & objsize) { /* not a power of 2? */ @@ -370,54 +369,21 @@ static int qemu_rbd_do_create(BlockdevCreateOptions *options, obj_order = ctz32(objsize); } - ret = rados_create(&cluster, opts->location->user); + ret = qemu_rbd_connect(&cluster, &io_ctx, opts->location, false, keypairs, + password_secret, errp); if (ret < 0) { - error_setg_errno(errp, -ret, "error initializing"); return ret; } - /* try default location when conf=NULL, but ignore failure */ - ret = rados_conf_read_file(cluster, opts->location->conf); - if (opts->location->conf && ret < 0) { - error_setg_errno(errp, -ret, "error reading conf file %s", - opts->location->conf); - ret = -EIO; - goto shutdown; - } - - ret = qemu_rbd_set_keypairs(cluster, keypairs, errp); - if (ret < 0) { - ret = -EIO; - goto shutdown; - } - - if (qemu_rbd_set_auth(cluster, password_secret, errp) < 0) { - ret = -EIO; - goto shutdown; - } - - ret = rados_connect(cluster); - if (ret < 0) { - error_setg_errno(errp, -ret, "error connecting"); - goto shutdown; - } - - ret = rados_ioctx_create(cluster, opts->location->pool, &io_ctx); - if (ret < 0) { - error_setg_errno(errp, -ret, "error opening pool %s", - opts->location->pool); - goto shutdown; - } - ret = rbd_create(io_ctx, opts->location->image, opts->size, &obj_order); if (ret < 0) { error_setg_errno(errp, -ret, "error rbd create"); + goto out; } - rados_ioctx_destroy(io_ctx); - ret = 0; -shutdown: +out: + rados_ioctx_destroy(io_ctx); rados_shutdown(cluster); return ret; } From c22a03454544c2a08f1107c5cc8481a5574533d5 Mon Sep 17 00:00:00 2001 From: Kevin Wolf Date: Wed, 31 Jan 2018 19:47:48 +0100 Subject: [PATCH 40/56] nfs: Use QAPI options in nfs_client_open() Using the QAPI visitor to turn all options into QAPI BlockdevOptionsNfs simplifies the code a lot. It will also be useful for implementing the QAPI based .bdrv_co_create callback. Signed-off-by: Kevin Wolf Reviewed-by: Max Reitz --- block/nfs.c | 176 ++++++++++++++++------------------------------------ 1 file changed, 53 insertions(+), 123 deletions(-) diff --git a/block/nfs.c b/block/nfs.c index 7433d25856..e402d643fe 100644 --- a/block/nfs.c +++ b/block/nfs.c @@ -367,49 +367,6 @@ static int coroutine_fn nfs_co_flush(BlockDriverState *bs) return task.ret; } -static QemuOptsList runtime_opts = { - .name = "nfs", - .head = QTAILQ_HEAD_INITIALIZER(runtime_opts.head), - .desc = { - { - .name = "path", - .type = QEMU_OPT_STRING, - .help = "Path of the image on the host", - }, - { - .name = "user", - .type = QEMU_OPT_NUMBER, - .help = "UID value to use when talking to the server", - }, - { - .name = "group", - .type = QEMU_OPT_NUMBER, - .help = "GID value to use when talking to the server", - }, - { - .name = "tcp-syn-count", - .type = QEMU_OPT_NUMBER, - .help = "Number of SYNs to send during the session establish", - }, - { - .name = "readahead-size", - .type = QEMU_OPT_NUMBER, - .help = "Set the readahead size in bytes", - }, - { - .name = "page-cache-size", - .type = QEMU_OPT_NUMBER, - .help = "Set the pagecache size in bytes", - }, - { - .name = "debug", - .type = QEMU_OPT_NUMBER, - .help = "Set the NFS debug level (max 2)", - }, - { /* end of list */ } - }, -}; - static void nfs_detach_aio_context(BlockDriverState *bs) { NFSClient *client = bs->opaque; @@ -452,71 +409,16 @@ static void nfs_file_close(BlockDriverState *bs) nfs_client_close(client); } -static NFSServer *nfs_config(QDict *options, Error **errp) -{ - NFSServer *server = NULL; - QDict *addr = NULL; - QObject *crumpled_addr = NULL; - Visitor *iv = NULL; - Error *local_error = NULL; - - qdict_extract_subqdict(options, &addr, "server."); - if (!qdict_size(addr)) { - error_setg(errp, "NFS server address missing"); - goto out; - } - - crumpled_addr = qdict_crumple(addr, errp); - if (!crumpled_addr) { - goto out; - } - - /* - * Caution: this works only because all scalar members of - * NFSServer are QString in @crumpled_addr. The visitor expects - * @crumpled_addr to be typed according to the QAPI schema. It - * is when @options come from -blockdev or blockdev_add. But when - * they come from -drive, they're all QString. - */ - iv = qobject_input_visitor_new(crumpled_addr); - visit_type_NFSServer(iv, NULL, &server, &local_error); - if (local_error) { - error_propagate(errp, local_error); - goto out; - } - -out: - QDECREF(addr); - qobject_decref(crumpled_addr); - visit_free(iv); - return server; -} - - -static int64_t nfs_client_open(NFSClient *client, QDict *options, +static int64_t nfs_client_open(NFSClient *client, BlockdevOptionsNfs *opts, int flags, int open_flags, Error **errp) { int64_t ret = -EINVAL; - QemuOpts *opts = NULL; - Error *local_err = NULL; struct stat st; char *file = NULL, *strp = NULL; qemu_mutex_init(&client->mutex); - opts = qemu_opts_create(&runtime_opts, NULL, 0, &error_abort); - qemu_opts_absorb_qdict(opts, options, &local_err); - if (local_err) { - error_propagate(errp, local_err); - ret = -EINVAL; - goto fail; - } - client->path = g_strdup(qemu_opt_get(opts, "path")); - if (!client->path) { - ret = -EINVAL; - error_setg(errp, "No path was specified"); - goto fail; - } + client->path = g_strdup(opts->path); strp = strrchr(client->path, '/'); if (strp == NULL) { @@ -526,12 +428,10 @@ static int64_t nfs_client_open(NFSClient *client, QDict *options, file = g_strdup(strp); *strp = 0; - /* Pop the config into our state object, Exit if invalid */ - client->server = nfs_config(options, errp); - if (!client->server) { - ret = -EINVAL; - goto fail; - } + /* Steal the NFSServer object from opts; set the original pointer to NULL + * to avoid use after free and double free. */ + client->server = opts->server; + opts->server = NULL; client->context = nfs_init_context(); if (client->context == NULL) { @@ -539,29 +439,29 @@ static int64_t nfs_client_open(NFSClient *client, QDict *options, goto fail; } - if (qemu_opt_get(opts, "user")) { - client->uid = qemu_opt_get_number(opts, "user", 0); + if (opts->has_user) { + client->uid = opts->user; nfs_set_uid(client->context, client->uid); } - if (qemu_opt_get(opts, "group")) { - client->gid = qemu_opt_get_number(opts, "group", 0); + if (opts->has_group) { + client->gid = opts->group; nfs_set_gid(client->context, client->gid); } - if (qemu_opt_get(opts, "tcp-syn-count")) { - client->tcp_syncnt = qemu_opt_get_number(opts, "tcp-syn-count", 0); + if (opts->has_tcp_syn_count) { + client->tcp_syncnt = opts->tcp_syn_count; nfs_set_tcp_syncnt(client->context, client->tcp_syncnt); } #ifdef LIBNFS_FEATURE_READAHEAD - if (qemu_opt_get(opts, "readahead-size")) { + if (opts->has_readahead_size) { if (open_flags & BDRV_O_NOCACHE) { error_setg(errp, "Cannot enable NFS readahead " "if cache.direct = on"); goto fail; } - client->readahead = qemu_opt_get_number(opts, "readahead-size", 0); + client->readahead = opts->readahead_size; if (client->readahead > QEMU_NFS_MAX_READAHEAD_SIZE) { warn_report("Truncating NFS readahead size to %d", QEMU_NFS_MAX_READAHEAD_SIZE); @@ -576,13 +476,13 @@ static int64_t nfs_client_open(NFSClient *client, QDict *options, #endif #ifdef LIBNFS_FEATURE_PAGECACHE - if (qemu_opt_get(opts, "page-cache-size")) { + if (opts->has_page_cache_size) { if (open_flags & BDRV_O_NOCACHE) { error_setg(errp, "Cannot enable NFS pagecache " "if cache.direct = on"); goto fail; } - client->pagecache = qemu_opt_get_number(opts, "page-cache-size", 0); + client->pagecache = opts->page_cache_size; if (client->pagecache > QEMU_NFS_MAX_PAGECACHE_SIZE) { warn_report("Truncating NFS pagecache size to %d pages", QEMU_NFS_MAX_PAGECACHE_SIZE); @@ -595,8 +495,8 @@ static int64_t nfs_client_open(NFSClient *client, QDict *options, #endif #ifdef LIBNFS_FEATURE_DEBUG - if (qemu_opt_get(opts, "debug")) { - client->debug = qemu_opt_get_number(opts, "debug", 0); + if (opts->has_debug) { + client->debug = opts->debug; /* limit the maximum debug level to avoid potential flooding * of our log files. */ if (client->debug > QEMU_NFS_MAX_DEBUG_LEVEL) { @@ -647,11 +547,41 @@ static int64_t nfs_client_open(NFSClient *client, QDict *options, fail: nfs_client_close(client); out: - qemu_opts_del(opts); g_free(file); return ret; } +static int64_t nfs_client_open_qdict(NFSClient *client, QDict *options, + int flags, int open_flags, Error **errp) +{ + BlockdevOptionsNfs *opts = NULL; + QObject *crumpled = NULL; + Visitor *v; + Error *local_err = NULL; + int ret; + + crumpled = qdict_crumple(options, errp); + if (crumpled == NULL) { + return -EINVAL; + } + + v = qobject_input_visitor_new_keyval(crumpled); + visit_type_BlockdevOptionsNfs(v, NULL, &opts, &local_err); + visit_free(v); + + if (local_err) { + error_propagate(errp, local_err); + ret = -EINVAL; + goto fail; + } + + ret = nfs_client_open(client, opts, flags, open_flags, errp); +fail: + qobject_decref(crumpled); + qapi_free_BlockdevOptionsNfs(opts); + return ret; +} + static int nfs_file_open(BlockDriverState *bs, QDict *options, int flags, Error **errp) { NFSClient *client = bs->opaque; @@ -659,9 +589,9 @@ static int nfs_file_open(BlockDriverState *bs, QDict *options, int flags, client->aio_context = bdrv_get_aio_context(bs); - ret = nfs_client_open(client, options, - (flags & BDRV_O_RDWR) ? O_RDWR : O_RDONLY, - bs->open_flags, errp); + ret = nfs_client_open_qdict(client, options, + (flags & BDRV_O_RDWR) ? O_RDWR : O_RDONLY, + bs->open_flags, errp); if (ret < 0) { return ret; } @@ -703,7 +633,7 @@ static int coroutine_fn nfs_file_co_create_opts(const char *url, QemuOpts *opts, goto out; } - ret = nfs_client_open(client, options, O_CREAT, 0, errp); + ret = nfs_client_open_qdict(client, options, O_CREAT, 0, errp); if (ret < 0) { goto out; } From a1a42af422d46812f1f0cebe6b230c20409a3731 Mon Sep 17 00:00:00 2001 From: Kevin Wolf Date: Wed, 31 Jan 2018 16:27:38 +0100 Subject: [PATCH 41/56] nfs: Support .bdrv_co_create This adds the .bdrv_co_create driver callback to nfs, which enables image creation over QMP. Signed-off-by: Kevin Wolf Reviewed-by: Max Reitz --- block/nfs.c | 76 ++++++++++++++++++++++++++++++++++---------- qapi/block-core.json | 16 +++++++++- 2 files changed, 75 insertions(+), 17 deletions(-) diff --git a/block/nfs.c b/block/nfs.c index e402d643fe..2577df4b26 100644 --- a/block/nfs.c +++ b/block/nfs.c @@ -551,33 +551,45 @@ out: return ret; } -static int64_t nfs_client_open_qdict(NFSClient *client, QDict *options, - int flags, int open_flags, Error **errp) +static BlockdevOptionsNfs *nfs_options_qdict_to_qapi(QDict *options, + Error **errp) { BlockdevOptionsNfs *opts = NULL; QObject *crumpled = NULL; Visitor *v; Error *local_err = NULL; - int ret; crumpled = qdict_crumple(options, errp); if (crumpled == NULL) { - return -EINVAL; + return NULL; } v = qobject_input_visitor_new_keyval(crumpled); visit_type_BlockdevOptionsNfs(v, NULL, &opts, &local_err); visit_free(v); + qobject_decref(crumpled); if (local_err) { - error_propagate(errp, local_err); + return NULL; + } + + return opts; +} + +static int64_t nfs_client_open_qdict(NFSClient *client, QDict *options, + int flags, int open_flags, Error **errp) +{ + BlockdevOptionsNfs *opts; + int ret; + + opts = nfs_options_qdict_to_qapi(options, errp); + if (opts == NULL) { ret = -EINVAL; goto fail; } ret = nfs_client_open(client, opts, flags, open_flags, errp); fail: - qobject_decref(crumpled); qapi_free_BlockdevOptionsNfs(opts); return ret; } @@ -614,18 +626,43 @@ static QemuOptsList nfs_create_opts = { } }; -static int coroutine_fn nfs_file_co_create_opts(const char *url, QemuOpts *opts, - Error **errp) +static int nfs_file_co_create(BlockdevCreateOptions *options, Error **errp) { - int64_t ret, total_size; + BlockdevCreateOptionsNfs *opts = &options->u.nfs; NFSClient *client = g_new0(NFSClient, 1); - QDict *options = NULL; + int ret; + + assert(options->driver == BLOCKDEV_DRIVER_NFS); client->aio_context = qemu_get_aio_context(); + ret = nfs_client_open(client, opts->location, O_CREAT, 0, errp); + if (ret < 0) { + goto out; + } + ret = nfs_ftruncate(client->context, client->fh, opts->size); + nfs_client_close(client); + +out: + g_free(client); + return ret; +} + +static int coroutine_fn nfs_file_co_create_opts(const char *url, QemuOpts *opts, + Error **errp) +{ + BlockdevCreateOptions *create_options; + BlockdevCreateOptionsNfs *nfs_opts; + QDict *options; + int ret; + + create_options = g_new0(BlockdevCreateOptions, 1); + create_options->driver = BLOCKDEV_DRIVER_NFS; + nfs_opts = &create_options->u.nfs; + /* Read out options */ - total_size = ROUND_UP(qemu_opt_get_size_del(opts, BLOCK_OPT_SIZE, 0), - BDRV_SECTOR_SIZE); + nfs_opts->size = ROUND_UP(qemu_opt_get_size_del(opts, BLOCK_OPT_SIZE, 0), + BDRV_SECTOR_SIZE); options = qdict_new(); ret = nfs_parse_uri(url, options, errp); @@ -633,15 +670,21 @@ static int coroutine_fn nfs_file_co_create_opts(const char *url, QemuOpts *opts, goto out; } - ret = nfs_client_open_qdict(client, options, O_CREAT, 0, errp); + nfs_opts->location = nfs_options_qdict_to_qapi(options, errp); + if (nfs_opts->location == NULL) { + ret = -EINVAL; + goto out; + } + + ret = nfs_file_co_create(create_options, errp); if (ret < 0) { goto out; } - ret = nfs_ftruncate(client->context, client->fh, total_size); - nfs_client_close(client); + + ret = 0; out: QDECREF(options); - g_free(client); + qapi_free_BlockdevCreateOptions(create_options); return ret; } @@ -828,6 +871,7 @@ static BlockDriver bdrv_nfs = { .bdrv_file_open = nfs_file_open, .bdrv_close = nfs_file_close, + .bdrv_co_create = nfs_file_co_create, .bdrv_co_create_opts = nfs_file_co_create_opts, .bdrv_reopen_prepare = nfs_reopen_prepare, diff --git a/qapi/block-core.json b/qapi/block-core.json index d4351877fc..39e53c7791 100644 --- a/qapi/block-core.json +++ b/qapi/block-core.json @@ -3392,6 +3392,20 @@ 'size': 'size', '*preallocation': 'PreallocMode' } } +## +# @BlockdevCreateOptionsNfs: +# +# Driver specific image creation options for NFS. +# +# @location Where to store the new image file +# @size Size of the virtual disk in bytes +# +# Since: 2.12 +## +{ 'struct': 'BlockdevCreateOptionsNfs', + 'data': { 'location': 'BlockdevOptionsNfs', + 'size': 'size' } } + ## # @BlockdevQcow2Version: # @@ -3491,7 +3505,7 @@ 'iscsi': 'BlockdevCreateNotSupported', 'luks': 'BlockdevCreateNotSupported', 'nbd': 'BlockdevCreateNotSupported', - 'nfs': 'BlockdevCreateNotSupported', + 'nfs': 'BlockdevCreateOptionsNfs', 'null-aio': 'BlockdevCreateNotSupported', 'null-co': 'BlockdevCreateNotSupported', 'nvme': 'BlockdevCreateNotSupported', From a595e4bcca4ad6c5544008931dc92bed1da3f89f Mon Sep 17 00:00:00 2001 From: Kevin Wolf Date: Thu, 1 Feb 2018 17:26:27 +0100 Subject: [PATCH 42/56] sheepdog: QAPIfy "redundancy" create option The "redundancy" option for Sheepdog image creation is currently a string that can encode one or two integers depending on its format, which at the same time implicitly selects a mode. This patch turns it into a QAPI union and converts the string into such a QAPI object before interpreting the values. Signed-off-by: Kevin Wolf Reviewed-by: Max Reitz --- block/sheepdog.c | 96 +++++++++++++++++++++++++++++++------------- qapi/block-core.json | 45 +++++++++++++++++++++ 2 files changed, 113 insertions(+), 28 deletions(-) diff --git a/block/sheepdog.c b/block/sheepdog.c index d8c10b7cac..3966cd229a 100644 --- a/block/sheepdog.c +++ b/block/sheepdog.c @@ -1882,6 +1882,48 @@ out_with_err_set: return ret; } +static int parse_redundancy(BDRVSheepdogState *s, SheepdogRedundancy *opt) +{ + struct SheepdogInode *inode = &s->inode; + + switch (opt->type) { + case SHEEPDOG_REDUNDANCY_TYPE_FULL: + if (opt->u.full.copies > SD_MAX_COPIES || opt->u.full.copies < 1) { + return -EINVAL; + } + inode->copy_policy = 0; + inode->nr_copies = opt->u.full.copies; + return 0; + + case SHEEPDOG_REDUNDANCY_TYPE_ERASURE_CODED: + { + int64_t copy = opt->u.erasure_coded.data_strips; + int64_t parity = opt->u.erasure_coded.parity_strips; + + if (copy != 2 && copy != 4 && copy != 8 && copy != 16) { + return -EINVAL; + } + + if (parity >= SD_EC_MAX_STRIP || parity < 1) { + return -EINVAL; + } + + /* + * 4 bits for parity and 4 bits for data. + * We have to compress upper data bits because it can't represent 16 + */ + inode->copy_policy = ((copy / 2) << 4) + parity; + inode->nr_copies = copy + parity; + return 0; + } + + default: + g_assert_not_reached(); + } + + return -EINVAL; +} + /* * Sheepdog support two kinds of redundancy, full replication and erasure * coding. @@ -1892,12 +1934,13 @@ out_with_err_set: * # create a erasure coded vdi with x data strips and y parity strips * -o redundancy=x:y (x must be one of {2,4,8,16} and 1 <= y < SD_EC_MAX_STRIP) */ -static int parse_redundancy(BDRVSheepdogState *s, const char *opt) +static int parse_redundancy_str(BDRVSheepdogState *s, const char *opt) { - struct SheepdogInode *inode = &s->inode; + struct SheepdogRedundancy redundancy; const char *n1, *n2; long copy, parity; char p[10]; + int ret; pstrcpy(p, sizeof(p), opt); n1 = strtok(p, ":"); @@ -1907,35 +1950,32 @@ static int parse_redundancy(BDRVSheepdogState *s, const char *opt) return -EINVAL; } - copy = strtol(n1, NULL, 10); - /* FIXME fix error checking by switching to qemu_strtol() */ - if (copy > SD_MAX_COPIES || copy < 1) { - return -EINVAL; + ret = qemu_strtol(n1, NULL, 10, ©); + if (ret < 0) { + return ret; } + if (!n2) { - inode->copy_policy = 0; - inode->nr_copies = copy; - return 0; + redundancy = (SheepdogRedundancy) { + .type = SHEEPDOG_REDUNDANCY_TYPE_FULL, + .u.full.copies = copy, + }; + } else { + ret = qemu_strtol(n2, NULL, 10, &parity); + if (ret < 0) { + return ret; + } + + redundancy = (SheepdogRedundancy) { + .type = SHEEPDOG_REDUNDANCY_TYPE_ERASURE_CODED, + .u.erasure_coded = { + .data_strips = copy, + .parity_strips = parity, + }, + }; } - if (copy != 2 && copy != 4 && copy != 8 && copy != 16) { - return -EINVAL; - } - - parity = strtol(n2, NULL, 10); - /* FIXME fix error checking by switching to qemu_strtol() */ - if (parity >= SD_EC_MAX_STRIP || parity < 1) { - return -EINVAL; - } - - /* - * 4 bits for parity and 4 bits for data. - * We have to compress upper data bits because it can't represent 16 - */ - inode->copy_policy = ((copy / 2) << 4) + parity; - inode->nr_copies = copy + parity; - - return 0; + return parse_redundancy(s, &redundancy); } static int parse_block_size_shift(BDRVSheepdogState *s, QemuOpts *opt) @@ -2007,7 +2047,7 @@ static int coroutine_fn sd_co_create_opts(const char *filename, QemuOpts *opts, g_free(buf); buf = qemu_opt_get_del(opts, BLOCK_OPT_REDUNDANCY); if (buf) { - ret = parse_redundancy(s, buf); + ret = parse_redundancy_str(s, buf); if (ret < 0) { error_setg(errp, "Invalid redundancy mode: '%s'", buf); goto out; diff --git a/qapi/block-core.json b/qapi/block-core.json index 39e53c7791..e590ab6c71 100644 --- a/qapi/block-core.json +++ b/qapi/block-core.json @@ -3466,6 +3466,51 @@ 'size': 'size', '*cluster-size' : 'size' } } +## +# @SheepdogRedundancyType: +# +# @full Create a fully replicated vdi with x copies +# @erasure-coded Create an erasure coded vdi with x data strips and +# y parity strips +# +# Since: 2.12 +## +{ 'enum': 'SheepdogRedundancyType', + 'data': [ 'full', 'erasure-coded' ] } + +## +# @SheepdogRedundancyFull: +# +# @copies Number of copies to use (between 1 and 31) +# +# Since: 2.12 +## +{ 'struct': 'SheepdogRedundancyFull', + 'data': { 'copies': 'int' }} + +## +# @SheepdogRedundancyErasureCoded: +# +# @data-strips Number of data strips to use (one of {2,4,8,16}) +# @parity-strips Number of parity strips to use (between 1 and 15) +# +# Since: 2.12 +## +{ 'struct': 'SheepdogRedundancyErasureCoded', + 'data': { 'data-strips': 'int', + 'parity-strips': 'int' }} + +## +# @SheepdogRedundancy: +# +# Since: 2.12 +## +{ 'union': 'SheepdogRedundancy', + 'base': { 'type': 'SheepdogRedundancyType' }, + 'discriminator': 'type', + 'data': { 'full': 'SheepdogRedundancyFull', + 'erasure-coded': 'SheepdogRedundancyErasureCoded' } } + ## # @BlockdevCreateNotSupported: # From 63fd65a0a5acca28740fc7ed138278d040ca6c9c Mon Sep 17 00:00:00 2001 From: Kevin Wolf Date: Thu, 1 Feb 2018 13:20:44 +0100 Subject: [PATCH 43/56] sheepdog: Support .bdrv_co_create This adds the .bdrv_co_create driver callback to sheepdog, which enables image creation over QMP. Signed-off-by: Kevin Wolf Reviewed-by: Max Reitz --- block/sheepdog.c | 243 ++++++++++++++++++++++++++++++------------- qapi/block-core.json | 24 ++++- 2 files changed, 192 insertions(+), 75 deletions(-) diff --git a/block/sheepdog.c b/block/sheepdog.c index 3966cd229a..8680b2926f 100644 --- a/block/sheepdog.c +++ b/block/sheepdog.c @@ -15,8 +15,10 @@ #include "qemu/osdep.h" #include "qapi/error.h" #include "qapi/qapi-visit-sockets.h" +#include "qapi/qapi-visit-block-core.h" #include "qapi/qmp/qdict.h" #include "qapi/qobject-input-visitor.h" +#include "qapi/qobject-output-visitor.h" #include "qemu/uri.h" #include "qemu/error-report.h" #include "qemu/option.h" @@ -533,23 +535,6 @@ static void sd_aio_setup(SheepdogAIOCB *acb, BDRVSheepdogState *s, qemu_co_mutex_unlock(&s->queue_lock); } -static SocketAddress *sd_socket_address(const char *path, - const char *host, const char *port) -{ - SocketAddress *addr = g_new0(SocketAddress, 1); - - if (path) { - addr->type = SOCKET_ADDRESS_TYPE_UNIX; - addr->u.q_unix.path = g_strdup(path); - } else { - addr->type = SOCKET_ADDRESS_TYPE_INET; - addr->u.inet.host = g_strdup(host ?: SD_DEFAULT_ADDR); - addr->u.inet.port = g_strdup(port ?: stringify(SD_DEFAULT_PORT)); - } - - return addr; -} - static SocketAddress *sd_server_config(QDict *options, Error **errp) { QDict *server = NULL; @@ -1882,6 +1867,44 @@ out_with_err_set: return ret; } +static int sd_create_prealloc(BlockdevOptionsSheepdog *location, int64_t size, + Error **errp) +{ + BlockDriverState *bs; + Visitor *v; + QObject *obj = NULL; + QDict *qdict; + Error *local_err = NULL; + int ret; + + v = qobject_output_visitor_new(&obj); + visit_type_BlockdevOptionsSheepdog(v, NULL, &location, &local_err); + visit_free(v); + + if (local_err) { + error_propagate(errp, local_err); + qobject_decref(obj); + return -EINVAL; + } + + qdict = qobject_to_qdict(obj); + qdict_flatten(qdict); + + qdict_put_str(qdict, "driver", "sheepdog"); + + bs = bdrv_open(NULL, NULL, qdict, BDRV_O_PROTOCOL | BDRV_O_RDWR, errp); + if (bs == NULL) { + ret = -EIO; + goto fail; + } + + ret = sd_prealloc(bs, 0, size, errp); +fail: + bdrv_unref(bs); + QDECREF(qdict); + return ret; +} + static int parse_redundancy(BDRVSheepdogState *s, SheepdogRedundancy *opt) { struct SheepdogInode *inode = &s->inode; @@ -1934,9 +1957,9 @@ static int parse_redundancy(BDRVSheepdogState *s, SheepdogRedundancy *opt) * # create a erasure coded vdi with x data strips and y parity strips * -o redundancy=x:y (x must be one of {2,4,8,16} and 1 <= y < SD_EC_MAX_STRIP) */ -static int parse_redundancy_str(BDRVSheepdogState *s, const char *opt) +static SheepdogRedundancy *parse_redundancy_str(const char *opt) { - struct SheepdogRedundancy redundancy; + SheepdogRedundancy *redundancy; const char *n1, *n2; long copy, parity; char p[10]; @@ -1947,26 +1970,27 @@ static int parse_redundancy_str(BDRVSheepdogState *s, const char *opt) n2 = strtok(NULL, ":"); if (!n1) { - return -EINVAL; + return NULL; } ret = qemu_strtol(n1, NULL, 10, ©); if (ret < 0) { - return ret; + return NULL; } + redundancy = g_new0(SheepdogRedundancy, 1); if (!n2) { - redundancy = (SheepdogRedundancy) { + *redundancy = (SheepdogRedundancy) { .type = SHEEPDOG_REDUNDANCY_TYPE_FULL, .u.full.copies = copy, }; } else { ret = qemu_strtol(n2, NULL, 10, &parity); if (ret < 0) { - return ret; + return NULL; } - redundancy = (SheepdogRedundancy) { + *redundancy = (SheepdogRedundancy) { .type = SHEEPDOG_REDUNDANCY_TYPE_ERASURE_CODED, .u.erasure_coded = { .data_strips = copy, @@ -1975,17 +1999,19 @@ static int parse_redundancy_str(BDRVSheepdogState *s, const char *opt) }; } - return parse_redundancy(s, &redundancy); + return redundancy; } -static int parse_block_size_shift(BDRVSheepdogState *s, QemuOpts *opt) +static int parse_block_size_shift(BDRVSheepdogState *s, + BlockdevCreateOptionsSheepdog *opts) { struct SheepdogInode *inode = &s->inode; uint64_t object_size; int obj_order; - object_size = qemu_opt_get_size_del(opt, BLOCK_OPT_OBJECT_SIZE, 0); - if (object_size) { + if (opts->has_object_size) { + object_size = opts->object_size; + if ((object_size - 1) & object_size) { /* not a power of 2? */ return -EINVAL; } @@ -1999,57 +2025,55 @@ static int parse_block_size_shift(BDRVSheepdogState *s, QemuOpts *opt) return 0; } -static int coroutine_fn sd_co_create_opts(const char *filename, QemuOpts *opts, - Error **errp) +static int sd_co_create(BlockdevCreateOptions *options, Error **errp) { - Error *err = NULL; + BlockdevCreateOptionsSheepdog *opts = &options->u.sheepdog; int ret = 0; uint32_t vid = 0; char *backing_file = NULL; char *buf = NULL; BDRVSheepdogState *s; - SheepdogConfig cfg; uint64_t max_vdi_size; bool prealloc = false; + assert(options->driver == BLOCKDEV_DRIVER_SHEEPDOG); + s = g_new0(BDRVSheepdogState, 1); - if (strstr(filename, "://")) { - sd_parse_uri(&cfg, filename, &err); - } else { - parse_vdiname(&cfg, filename, &err); - } - if (err) { - error_propagate(errp, err); + /* Steal SocketAddress from QAPI, set NULL to prevent double free */ + s->addr = opts->location->server; + opts->location->server = NULL; + + if (strlen(opts->location->vdi) >= sizeof(s->name)) { + error_setg(errp, "'vdi' string too long"); + ret = -EINVAL; goto out; } + pstrcpy(s->name, sizeof(s->name), opts->location->vdi); - buf = cfg.port ? g_strdup_printf("%d", cfg.port) : NULL; - s->addr = sd_socket_address(cfg.path, cfg.host, buf); - g_free(buf); - strcpy(s->name, cfg.vdi); - sd_config_done(&cfg); + s->inode.vdi_size = opts->size; + backing_file = opts->backing_file; - s->inode.vdi_size = ROUND_UP(qemu_opt_get_size_del(opts, BLOCK_OPT_SIZE, 0), - BDRV_SECTOR_SIZE); - backing_file = qemu_opt_get_del(opts, BLOCK_OPT_BACKING_FILE); - buf = qemu_opt_get_del(opts, BLOCK_OPT_PREALLOC); - if (!buf || !strcmp(buf, "off")) { + if (!opts->has_preallocation) { + opts->preallocation = PREALLOC_MODE_OFF; + } + switch (opts->preallocation) { + case PREALLOC_MODE_OFF: prealloc = false; - } else if (!strcmp(buf, "full")) { + break; + case PREALLOC_MODE_FULL: prealloc = true; - } else { - error_setg(errp, "Invalid preallocation mode: '%s'", buf); + break; + default: + error_setg(errp, "Preallocation mode not supported for Sheepdog"); ret = -EINVAL; goto out; } - g_free(buf); - buf = qemu_opt_get_del(opts, BLOCK_OPT_REDUNDANCY); - if (buf) { - ret = parse_redundancy_str(s, buf); + if (opts->has_redundancy) { + ret = parse_redundancy(s, opts->redundancy); if (ret < 0) { - error_setg(errp, "Invalid redundancy mode: '%s'", buf); + error_setg(errp, "Invalid redundancy mode"); goto out; } } @@ -2061,20 +2085,20 @@ static int coroutine_fn sd_co_create_opts(const char *filename, QemuOpts *opts, goto out; } - if (backing_file) { + if (opts->has_backing_file) { BlockBackend *blk; BDRVSheepdogState *base; BlockDriver *drv; /* Currently, only Sheepdog backing image is supported. */ - drv = bdrv_find_protocol(backing_file, true, NULL); + drv = bdrv_find_protocol(opts->backing_file, true, NULL); if (!drv || strcmp(drv->protocol_name, "sheepdog") != 0) { error_setg(errp, "backing_file must be a sheepdog image"); ret = -EINVAL; goto out; } - blk = blk_new_open(backing_file, NULL, NULL, + blk = blk_new_open(opts->backing_file, NULL, NULL, BDRV_O_PROTOCOL, errp); if (blk == NULL) { ret = -EIO; @@ -2142,28 +2166,96 @@ static int coroutine_fn sd_co_create_opts(const char *filename, QemuOpts *opts, } if (prealloc) { - BlockDriverState *bs; - QDict *opts; - - opts = qdict_new(); - qdict_put_str(opts, "driver", "sheepdog"); - bs = bdrv_open(filename, NULL, opts, BDRV_O_PROTOCOL | BDRV_O_RDWR, - errp); - if (!bs) { - goto out; - } - - ret = sd_prealloc(bs, 0, s->inode.vdi_size, errp); - - bdrv_unref(bs); + ret = sd_create_prealloc(opts->location, opts->size, errp); } out: g_free(backing_file); g_free(buf); + g_free(s->addr); g_free(s); return ret; } +static int coroutine_fn sd_co_create_opts(const char *filename, QemuOpts *opts, + Error **errp) +{ + BlockdevCreateOptions *create_options = NULL; + QDict *qdict, *location_qdict; + QObject *crumpled; + Visitor *v; + const char *redundancy; + Error *local_err = NULL; + int ret; + + redundancy = qemu_opt_get_del(opts, BLOCK_OPT_REDUNDANCY); + + qdict = qemu_opts_to_qdict(opts, NULL); + qdict_put_str(qdict, "driver", "sheepdog"); + + location_qdict = qdict_new(); + qdict_put(qdict, "location", location_qdict); + + sd_parse_filename(filename, location_qdict, &local_err); + if (local_err) { + error_propagate(errp, local_err); + ret = -EINVAL; + goto fail; + } + + qdict_flatten(qdict); + + /* Change legacy command line options into QMP ones */ + static const QDictRenames opt_renames[] = { + { BLOCK_OPT_BACKING_FILE, "backing-file" }, + { BLOCK_OPT_OBJECT_SIZE, "object-size" }, + { NULL, NULL }, + }; + + if (!qdict_rename_keys(qdict, opt_renames, errp)) { + ret = -EINVAL; + goto fail; + } + + /* Get the QAPI object */ + crumpled = qdict_crumple(qdict, errp); + if (crumpled == NULL) { + ret = -EINVAL; + goto fail; + } + + v = qobject_input_visitor_new_keyval(crumpled); + visit_type_BlockdevCreateOptions(v, NULL, &create_options, &local_err); + visit_free(v); + qobject_decref(crumpled); + + if (local_err) { + error_propagate(errp, local_err); + ret = -EINVAL; + goto fail; + } + + assert(create_options->driver == BLOCKDEV_DRIVER_SHEEPDOG); + create_options->u.sheepdog.size = + ROUND_UP(create_options->u.sheepdog.size, BDRV_SECTOR_SIZE); + + if (redundancy) { + create_options->u.sheepdog.has_redundancy = true; + create_options->u.sheepdog.redundancy = + parse_redundancy_str(redundancy); + if (create_options->u.sheepdog.redundancy == NULL) { + error_setg(errp, "Invalid redundancy mode"); + ret = -EINVAL; + goto fail; + } + } + + ret = sd_co_create(create_options, errp); +fail: + qapi_free_BlockdevCreateOptions(create_options); + QDECREF(qdict); + return ret; +} + static void sd_close(BlockDriverState *bs) { Error *local_err = NULL; @@ -3143,6 +3235,7 @@ static BlockDriver bdrv_sheepdog = { .bdrv_reopen_commit = sd_reopen_commit, .bdrv_reopen_abort = sd_reopen_abort, .bdrv_close = sd_close, + .bdrv_co_create = sd_co_create, .bdrv_co_create_opts = sd_co_create_opts, .bdrv_has_zero_init = bdrv_has_zero_init_1, .bdrv_getlength = sd_getlength, @@ -3179,6 +3272,7 @@ static BlockDriver bdrv_sheepdog_tcp = { .bdrv_reopen_commit = sd_reopen_commit, .bdrv_reopen_abort = sd_reopen_abort, .bdrv_close = sd_close, + .bdrv_co_create = sd_co_create, .bdrv_co_create_opts = sd_co_create_opts, .bdrv_has_zero_init = bdrv_has_zero_init_1, .bdrv_getlength = sd_getlength, @@ -3215,6 +3309,7 @@ static BlockDriver bdrv_sheepdog_unix = { .bdrv_reopen_commit = sd_reopen_commit, .bdrv_reopen_abort = sd_reopen_abort, .bdrv_close = sd_close, + .bdrv_co_create = sd_co_create, .bdrv_co_create_opts = sd_co_create_opts, .bdrv_has_zero_init = bdrv_has_zero_init_1, .bdrv_getlength = sd_getlength, diff --git a/qapi/block-core.json b/qapi/block-core.json index e590ab6c71..fd444421fc 100644 --- a/qapi/block-core.json +++ b/qapi/block-core.json @@ -3511,6 +3511,28 @@ 'data': { 'full': 'SheepdogRedundancyFull', 'erasure-coded': 'SheepdogRedundancyErasureCoded' } } +## +# @BlockdevCreateOptionsSheepdog: +# +# Driver specific image creation options for Sheepdog. +# +# @location Where to store the new image file +# @size Size of the virtual disk in bytes +# @backing-file File name of a base image +# @preallocation Preallocation mode (allowed values: off, full) +# @redundancy Redundancy of the image +# @object-size Object size of the image +# +# Since: 2.12 +## +{ 'struct': 'BlockdevCreateOptionsSheepdog', + 'data': { 'location': 'BlockdevOptionsSheepdog', + 'size': 'size', + '*backing-file': 'str', + '*preallocation': 'PreallocMode', + '*redundancy': 'SheepdogRedundancy', + '*object-size': 'size' } } + ## # @BlockdevCreateNotSupported: # @@ -3562,7 +3584,7 @@ 'raw': 'BlockdevCreateNotSupported', 'rbd': 'BlockdevCreateOptionsRbd', 'replication': 'BlockdevCreateNotSupported', - 'sheepdog': 'BlockdevCreateNotSupported', + 'sheepdog': 'BlockdevCreateOptionsSheepdog', 'ssh': 'BlockdevCreateNotSupported', 'throttle': 'BlockdevCreateNotSupported', 'vdi': 'BlockdevCreateNotSupported', From 16e4bdb1bff83c41b73ed9c915d6bfc31d3e7ac8 Mon Sep 17 00:00:00 2001 From: Kevin Wolf Date: Fri, 2 Feb 2018 16:12:18 +0100 Subject: [PATCH 44/56] ssh: Use QAPI BlockdevOptionsSsh object Create a BlockdevOptionsSsh object in connect_to_ssh() and take the options from there. 'host_key_check' is still processed separately because it's not in the schema yet. Signed-off-by: Kevin Wolf Reviewed-by: Max Reitz --- block/ssh.c | 135 ++++++++++++++++++++++++---------------------------- 1 file changed, 61 insertions(+), 74 deletions(-) diff --git a/block/ssh.c b/block/ssh.c index ff9929497d..8b646c0ede 100644 --- a/block/ssh.c +++ b/block/ssh.c @@ -35,6 +35,7 @@ #include "qemu/sockets.h" #include "qemu/uri.h" #include "qapi/qapi-visit-sockets.h" +#include "qapi/qapi-visit-block-core.h" #include "qapi/qmp/qdict.h" #include "qapi/qmp/qstring.h" #include "qapi/qobject-input-visitor.h" @@ -543,21 +544,6 @@ static QemuOptsList ssh_runtime_opts = { .type = QEMU_OPT_NUMBER, .help = "Port to connect to", }, - { - .name = "path", - .type = QEMU_OPT_STRING, - .help = "Path of the image on the host", - }, - { - .name = "user", - .type = QEMU_OPT_STRING, - .help = "User as which to connect", - }, - { - .name = "host_key_check", - .type = QEMU_OPT_STRING, - .help = "Defines how and what to check the host key against", - }, { /* end of list */ } }, }; @@ -582,23 +568,31 @@ static bool ssh_process_legacy_socket_options(QDict *output_opts, return true; } -static InetSocketAddress *ssh_config(QDict *options, Error **errp) +static BlockdevOptionsSsh *ssh_parse_options(QDict *options, Error **errp) { - InetSocketAddress *inet = NULL; - QDict *addr = NULL; - QObject *crumpled_addr = NULL; - Visitor *iv = NULL; - Error *local_error = NULL; + BlockdevOptionsSsh *result = NULL; + QemuOpts *opts = NULL; + Error *local_err = NULL; + QObject *crumpled; + const QDictEntry *e; + Visitor *v; - qdict_extract_subqdict(options, &addr, "server."); - if (!qdict_size(addr)) { - error_setg(errp, "SSH server address missing"); - goto out; + /* Translate legacy options */ + opts = qemu_opts_create(&ssh_runtime_opts, NULL, 0, &error_abort); + qemu_opts_absorb_qdict(opts, options, &local_err); + if (local_err) { + error_propagate(errp, local_err); + goto fail; } - crumpled_addr = qdict_crumple(addr, errp); - if (!crumpled_addr) { - goto out; + if (!ssh_process_legacy_socket_options(options, opts, errp)) { + goto fail; + } + + /* Create the QAPI object */ + crumpled = qdict_crumple(options, errp); + if (crumpled == NULL) { + goto fail; } /* @@ -609,51 +603,50 @@ static InetSocketAddress *ssh_config(QDict *options, Error **errp) * but when they come from -drive, they're all QString. The * visitor expects the former. */ - iv = qobject_input_visitor_new(crumpled_addr); - visit_type_InetSocketAddress(iv, NULL, &inet, &local_error); - if (local_error) { - error_propagate(errp, local_error); - goto out; + v = qobject_input_visitor_new(crumpled); + visit_type_BlockdevOptionsSsh(v, NULL, &result, &local_err); + visit_free(v); + qobject_decref(crumpled); + + if (local_err) { + error_propagate(errp, local_err); + goto fail; } -out: - QDECREF(addr); - qobject_decref(crumpled_addr); - visit_free(iv); - return inet; + /* Remove the processed options from the QDict (the visitor processes + * _all_ options in the QDict) */ + while ((e = qdict_first(options))) { + qdict_del(options, e->key); + } + +fail: + qemu_opts_del(opts); + return result; } static int connect_to_ssh(BDRVSSHState *s, QDict *options, int ssh_flags, int creat_mode, Error **errp) { + BlockdevOptionsSsh *opts; int r, ret; - QemuOpts *opts = NULL; - Error *local_err = NULL; - const char *user, *path, *host_key_check; + const char *user, *host_key_check; long port = 0; - opts = qemu_opts_create(&ssh_runtime_opts, NULL, 0, &error_abort); - qemu_opts_absorb_qdict(opts, options, &local_err); - if (local_err) { - ret = -EINVAL; - error_propagate(errp, local_err); - goto err; + host_key_check = qdict_get_try_str(options, "host_key_check"); + if (!host_key_check) { + host_key_check = "yes"; + } else { + qdict_del(options, "host_key_check"); } - if (!ssh_process_legacy_socket_options(options, opts, errp)) { - ret = -EINVAL; - goto err; + opts = ssh_parse_options(options, errp); + if (opts == NULL) { + return -EINVAL; } - path = qemu_opt_get(opts, "path"); - if (!path) { - ret = -EINVAL; - error_setg(errp, "No path was specified"); - goto err; - } - - user = qemu_opt_get(opts, "user"); - if (!user) { + if (opts->has_user) { + user = opts->user; + } else { user = g_get_user_name(); if (!user) { error_setg_errno(errp, errno, "Can't get user name"); @@ -662,17 +655,9 @@ static int connect_to_ssh(BDRVSSHState *s, QDict *options, } } - host_key_check = qemu_opt_get(opts, "host_key_check"); - if (!host_key_check) { - host_key_check = "yes"; - } - /* Pop the config into our state object, Exit if invalid */ - s->inet = ssh_config(options, errp); - if (!s->inet) { - ret = -EINVAL; - goto err; - } + s->inet = opts->server; + opts->server = NULL; if (qemu_strtol(s->inet->port, NULL, 10, &port) < 0) { error_setg(errp, "Use only numeric port value"); @@ -729,15 +714,17 @@ static int connect_to_ssh(BDRVSSHState *s, QDict *options, /* Open the remote file. */ DPRINTF("opening file %s flags=0x%x creat_mode=0%o", - path, ssh_flags, creat_mode); - s->sftp_handle = libssh2_sftp_open(s->sftp, path, ssh_flags, creat_mode); + opts->path, ssh_flags, creat_mode); + s->sftp_handle = libssh2_sftp_open(s->sftp, opts->path, ssh_flags, + creat_mode); if (!s->sftp_handle) { - session_error_setg(errp, s, "failed to open remote file '%s'", path); + session_error_setg(errp, s, "failed to open remote file '%s'", + opts->path); ret = -EINVAL; goto err; } - qemu_opts_del(opts); + qapi_free_BlockdevOptionsSsh(opts); r = libssh2_sftp_fstat(s->sftp_handle, &s->attrs); if (r < 0) { @@ -764,7 +751,7 @@ static int connect_to_ssh(BDRVSSHState *s, QDict *options, } s->session = NULL; - qemu_opts_del(opts); + qapi_free_BlockdevOptionsSsh(opts); return ret; } From ec2f54182c5690387ba72a4acfcbdd961fcdcfc6 Mon Sep 17 00:00:00 2001 From: Kevin Wolf Date: Mon, 5 Feb 2018 14:59:05 +0100 Subject: [PATCH 45/56] ssh: QAPIfy host-key-check option This makes the host-key-check option available in blockdev-add. Signed-off-by: Kevin Wolf Reviewed-by: Max Reitz --- block/ssh.c | 92 ++++++++++++++++++++++++++++---------------- qapi/block-core.json | 63 +++++++++++++++++++++++++++++- 2 files changed, 119 insertions(+), 36 deletions(-) diff --git a/block/ssh.c b/block/ssh.c index 8b646c0ede..30cdf9a99f 100644 --- a/block/ssh.c +++ b/block/ssh.c @@ -431,31 +431,35 @@ check_host_key_hash(BDRVSSHState *s, const char *hash, } static int check_host_key(BDRVSSHState *s, const char *host, int port, - const char *host_key_check, Error **errp) + SshHostKeyCheck *hkc, Error **errp) { - /* host_key_check=no */ - if (strcmp(host_key_check, "no") == 0) { + SshHostKeyCheckMode mode; + + if (hkc) { + mode = hkc->mode; + } else { + mode = SSH_HOST_KEY_CHECK_MODE_KNOWN_HOSTS; + } + + switch (mode) { + case SSH_HOST_KEY_CHECK_MODE_NONE: return 0; - } - - /* host_key_check=md5:xx:yy:zz:... */ - if (strncmp(host_key_check, "md5:", 4) == 0) { - return check_host_key_hash(s, &host_key_check[4], - LIBSSH2_HOSTKEY_HASH_MD5, 16, errp); - } - - /* host_key_check=sha1:xx:yy:zz:... */ - if (strncmp(host_key_check, "sha1:", 5) == 0) { - return check_host_key_hash(s, &host_key_check[5], - LIBSSH2_HOSTKEY_HASH_SHA1, 20, errp); - } - - /* host_key_check=yes */ - if (strcmp(host_key_check, "yes") == 0) { + case SSH_HOST_KEY_CHECK_MODE_HASH: + if (hkc->u.hash.type == SSH_HOST_KEY_CHECK_HASH_TYPE_MD5) { + return check_host_key_hash(s, hkc->u.hash.hash, + LIBSSH2_HOSTKEY_HASH_MD5, 16, errp); + } else if (hkc->u.hash.type == SSH_HOST_KEY_CHECK_HASH_TYPE_SHA1) { + return check_host_key_hash(s, hkc->u.hash.hash, + LIBSSH2_HOSTKEY_HASH_SHA1, 20, errp); + } + g_assert_not_reached(); + break; + case SSH_HOST_KEY_CHECK_MODE_KNOWN_HOSTS: return check_host_key_knownhosts(s, host, port, errp); + default: + g_assert_not_reached(); } - error_setg(errp, "unknown host_key_check setting (%s)", host_key_check); return -EINVAL; } @@ -544,16 +548,22 @@ static QemuOptsList ssh_runtime_opts = { .type = QEMU_OPT_NUMBER, .help = "Port to connect to", }, + { + .name = "host_key_check", + .type = QEMU_OPT_STRING, + .help = "Defines how and what to check the host key against", + }, { /* end of list */ } }, }; -static bool ssh_process_legacy_socket_options(QDict *output_opts, - QemuOpts *legacy_opts, - Error **errp) +static bool ssh_process_legacy_options(QDict *output_opts, + QemuOpts *legacy_opts, + Error **errp) { const char *host = qemu_opt_get(legacy_opts, "host"); const char *port = qemu_opt_get(legacy_opts, "port"); + const char *host_key_check = qemu_opt_get(legacy_opts, "host_key_check"); if (!host && port) { error_setg(errp, "port may not be used without host"); @@ -565,6 +575,28 @@ static bool ssh_process_legacy_socket_options(QDict *output_opts, qdict_put_str(output_opts, "server.port", port ?: stringify(22)); } + if (host_key_check) { + if (strcmp(host_key_check, "no") == 0) { + qdict_put_str(output_opts, "host-key-check.mode", "none"); + } else if (strncmp(host_key_check, "md5:", 4) == 0) { + qdict_put_str(output_opts, "host-key-check.mode", "hash"); + qdict_put_str(output_opts, "host-key-check.type", "md5"); + qdict_put_str(output_opts, "host-key-check.hash", + &host_key_check[4]); + } else if (strncmp(host_key_check, "sha1:", 5) == 0) { + qdict_put_str(output_opts, "host-key-check.mode", "hash"); + qdict_put_str(output_opts, "host-key-check.type", "sha1"); + qdict_put_str(output_opts, "host-key-check.hash", + &host_key_check[5]); + } else if (strcmp(host_key_check, "yes") == 0) { + qdict_put_str(output_opts, "host-key-check.mode", "known_hosts"); + } else { + error_setg(errp, "unknown host_key_check setting (%s)", + host_key_check); + return false; + } + } + return true; } @@ -585,7 +617,7 @@ static BlockdevOptionsSsh *ssh_parse_options(QDict *options, Error **errp) goto fail; } - if (!ssh_process_legacy_socket_options(options, opts, errp)) { + if (!ssh_process_legacy_options(options, opts, errp)) { goto fail; } @@ -629,16 +661,9 @@ static int connect_to_ssh(BDRVSSHState *s, QDict *options, { BlockdevOptionsSsh *opts; int r, ret; - const char *user, *host_key_check; + const char *user; long port = 0; - host_key_check = qdict_get_try_str(options, "host_key_check"); - if (!host_key_check) { - host_key_check = "yes"; - } else { - qdict_del(options, "host_key_check"); - } - opts = ssh_parse_options(options, errp); if (opts == NULL) { return -EINVAL; @@ -692,8 +717,7 @@ static int connect_to_ssh(BDRVSSHState *s, QDict *options, } /* Check the remote host's key against known_hosts. */ - ret = check_host_key(s, s->inet->host, port, host_key_check, - errp); + ret = check_host_key(s, s->inet->host, port, opts->host_key_check, errp); if (ret < 0) { goto err; } diff --git a/qapi/block-core.json b/qapi/block-core.json index fd444421fc..4814bb7db7 100644 --- a/qapi/block-core.json +++ b/qapi/block-core.json @@ -2552,6 +2552,63 @@ '*cache-clean-interval': 'int', '*encrypt': 'BlockdevQcow2Encryption' } } +## +# @SshHostKeyCheckMode: +# +# @none Don't check the host key at all +# @hash Compare the host key with a given hash +# @known_hosts Check the host key against the known_hosts file +# +# Since: 2.12 +## +{ 'enum': 'SshHostKeyCheckMode', + 'data': [ 'none', 'hash', 'known_hosts' ] } + +## +# @SshHostKeyCheckHashType: +# +# @md5 The given hash is an md5 hash +# @sha1 The given hash is an sha1 hash +# +# Since: 2.12 +## +{ 'enum': 'SshHostKeyCheckHashType', + 'data': [ 'md5', 'sha1' ] } + +## +# @SshHostKeyHash: +# +# @type The hash algorithm used for the hash +# @hash The expected hash value +# +# Since: 2.12 +## +{ 'struct': 'SshHostKeyHash', + 'data': { 'type': 'SshHostKeyCheckHashType', + 'hash': 'str' }} + +## +# @SshHostKeyDummy: +# +# For those union branches that don't need additional fields. +# +# Since: 2.12 +## +{ 'struct': 'SshHostKeyDummy', + 'data': {} } + +## +# @SshHostKeyCheck: +# +# Since: 2.12 +## +{ 'union': 'SshHostKeyCheck', + 'base': { 'mode': 'SshHostKeyCheckMode' }, + 'discriminator': 'mode', + 'data': { 'none': 'SshHostKeyDummy', + 'hash': 'SshHostKeyHash', + 'known_hosts': 'SshHostKeyDummy' } } + ## # @BlockdevOptionsSsh: # @@ -2562,14 +2619,16 @@ # @user: user as which to connect, defaults to current # local user name # -# TODO: Expose the host_key_check option in QMP +# @host-key-check: Defines how and what to check the host key against +# (default: known_hosts) # # Since: 2.9 ## { 'struct': 'BlockdevOptionsSsh', 'data': { 'server': 'InetSocketAddress', 'path': 'str', - '*user': 'str' } } + '*user': 'str', + '*host-key-check': 'SshHostKeyCheck' } } ## From 375f0b9266d72cab238dc079ca319417f9a92983 Mon Sep 17 00:00:00 2001 From: Kevin Wolf Date: Mon, 5 Feb 2018 15:28:14 +0100 Subject: [PATCH 46/56] ssh: Pass BlockdevOptionsSsh to connect_to_ssh() Move the parsing of the QDict options up to the callers, in preparation for the .bdrv_co_create implementation that directly gets a QAPI type. Signed-off-by: Kevin Wolf Reviewed-by: Max Reitz --- block/ssh.c | 34 +++++++++++++++++++++------------- 1 file changed, 21 insertions(+), 13 deletions(-) diff --git a/block/ssh.c b/block/ssh.c index 30cdf9a99f..80f59055cc 100644 --- a/block/ssh.c +++ b/block/ssh.c @@ -656,19 +656,13 @@ fail: return result; } -static int connect_to_ssh(BDRVSSHState *s, QDict *options, +static int connect_to_ssh(BDRVSSHState *s, BlockdevOptionsSsh *opts, int ssh_flags, int creat_mode, Error **errp) { - BlockdevOptionsSsh *opts; int r, ret; const char *user; long port = 0; - opts = ssh_parse_options(options, errp); - if (opts == NULL) { - return -EINVAL; - } - if (opts->has_user) { user = opts->user; } else { @@ -748,8 +742,6 @@ static int connect_to_ssh(BDRVSSHState *s, QDict *options, goto err; } - qapi_free_BlockdevOptionsSsh(opts); - r = libssh2_sftp_fstat(s->sftp_handle, &s->attrs); if (r < 0) { sftp_error_setg(errp, s, "failed to read file attributes"); @@ -775,8 +767,6 @@ static int connect_to_ssh(BDRVSSHState *s, QDict *options, } s->session = NULL; - qapi_free_BlockdevOptionsSsh(opts); - return ret; } @@ -784,6 +774,7 @@ static int ssh_file_open(BlockDriverState *bs, QDict *options, int bdrv_flags, Error **errp) { BDRVSSHState *s = bs->opaque; + BlockdevOptionsSsh *opts; int ret; int ssh_flags; @@ -794,8 +785,13 @@ static int ssh_file_open(BlockDriverState *bs, QDict *options, int bdrv_flags, ssh_flags |= LIBSSH2_FXF_WRITE; } + opts = ssh_parse_options(options, errp); + if (opts == NULL) { + return -EINVAL; + } + /* Start up SSH. */ - ret = connect_to_ssh(s, options, ssh_flags, 0, errp); + ret = connect_to_ssh(s, opts, ssh_flags, 0, errp); if (ret < 0) { goto err; } @@ -803,6 +799,8 @@ static int ssh_file_open(BlockDriverState *bs, QDict *options, int bdrv_flags, /* Go non-blocking. */ libssh2_session_set_blocking(s->session, 0); + qapi_free_BlockdevOptionsSsh(opts); + return 0; err: @@ -811,6 +809,8 @@ static int ssh_file_open(BlockDriverState *bs, QDict *options, int bdrv_flags, } s->sock = -1; + qapi_free_BlockdevOptionsSsh(opts); + return ret; } @@ -860,6 +860,7 @@ static int coroutine_fn ssh_co_create_opts(const char *filename, QemuOpts *opts, int r, ret; int64_t total_size = 0; QDict *uri_options = NULL; + BlockdevOptionsSsh *ssh_opts = NULL; BDRVSSHState s; ssh_state_init(&s); @@ -876,7 +877,13 @@ static int coroutine_fn ssh_co_create_opts(const char *filename, QemuOpts *opts, goto out; } - r = connect_to_ssh(&s, uri_options, + ssh_opts = ssh_parse_options(uri_options, errp); + if (ssh_opts == NULL) { + ret = -EINVAL; + goto out; + } + + r = connect_to_ssh(&s, ssh_opts, LIBSSH2_FXF_READ|LIBSSH2_FXF_WRITE| LIBSSH2_FXF_CREAT|LIBSSH2_FXF_TRUNC, 0644, errp); @@ -899,6 +906,7 @@ static int coroutine_fn ssh_co_create_opts(const char *filename, QemuOpts *opts, if (uri_options != NULL) { QDECREF(uri_options); } + qapi_free_BlockdevOptionsSsh(ssh_opts); return ret; } From 4906da7e4de3720995b644f9903879cfd5a51a40 Mon Sep 17 00:00:00 2001 From: Kevin Wolf Date: Mon, 5 Feb 2018 16:24:32 +0100 Subject: [PATCH 47/56] ssh: Support .bdrv_co_create This adds the .bdrv_co_create driver callback to ssh, which enables image creation over QMP. Signed-off-by: Kevin Wolf Reviewed-by: Max Reitz --- block/ssh.c | 91 +++++++++++++++++++++++++------------------- qapi/block-core.json | 16 +++++++- 2 files changed, 67 insertions(+), 40 deletions(-) diff --git a/block/ssh.c b/block/ssh.c index 80f59055cc..ab3acf0c22 100644 --- a/block/ssh.c +++ b/block/ssh.c @@ -854,59 +854,71 @@ static QemuOptsList ssh_create_opts = { } }; -static int coroutine_fn ssh_co_create_opts(const char *filename, QemuOpts *opts, - Error **errp) +static int ssh_co_create(BlockdevCreateOptions *options, Error **errp) { - int r, ret; - int64_t total_size = 0; - QDict *uri_options = NULL; - BlockdevOptionsSsh *ssh_opts = NULL; + BlockdevCreateOptionsSsh *opts = &options->u.ssh; BDRVSSHState s; + int ret; + + assert(options->driver == BLOCKDEV_DRIVER_SSH); ssh_state_init(&s); - /* Get desired file size. */ - total_size = ROUND_UP(qemu_opt_get_size_del(opts, BLOCK_OPT_SIZE, 0), - BDRV_SECTOR_SIZE); - DPRINTF("total_size=%" PRIi64, total_size); - - uri_options = qdict_new(); - r = parse_uri(filename, uri_options, errp); - if (r < 0) { - ret = r; - goto out; + ret = connect_to_ssh(&s, opts->location, + LIBSSH2_FXF_READ|LIBSSH2_FXF_WRITE| + LIBSSH2_FXF_CREAT|LIBSSH2_FXF_TRUNC, + 0644, errp); + if (ret < 0) { + goto fail; } - ssh_opts = ssh_parse_options(uri_options, errp); - if (ssh_opts == NULL) { - ret = -EINVAL; - goto out; - } - - r = connect_to_ssh(&s, ssh_opts, - LIBSSH2_FXF_READ|LIBSSH2_FXF_WRITE| - LIBSSH2_FXF_CREAT|LIBSSH2_FXF_TRUNC, - 0644, errp); - if (r < 0) { - ret = r; - goto out; - } - - if (total_size > 0) { - ret = ssh_grow_file(&s, total_size, errp); + if (opts->size > 0) { + ret = ssh_grow_file(&s, opts->size, errp); if (ret < 0) { - goto out; + goto fail; } } ret = 0; +fail: + ssh_state_free(&s); + return ret; +} + +static int coroutine_fn ssh_co_create_opts(const char *filename, QemuOpts *opts, + Error **errp) +{ + BlockdevCreateOptions *create_options; + BlockdevCreateOptionsSsh *ssh_opts; + int ret; + QDict *uri_options = NULL; + + create_options = g_new0(BlockdevCreateOptions, 1); + create_options->driver = BLOCKDEV_DRIVER_SSH; + ssh_opts = &create_options->u.ssh; + + /* Get desired file size. */ + ssh_opts->size = ROUND_UP(qemu_opt_get_size_del(opts, BLOCK_OPT_SIZE, 0), + BDRV_SECTOR_SIZE); + DPRINTF("total_size=%" PRIi64, ssh_opts->size); + + uri_options = qdict_new(); + ret = parse_uri(filename, uri_options, errp); + if (ret < 0) { + goto out; + } + + ssh_opts->location = ssh_parse_options(uri_options, errp); + if (ssh_opts->location == NULL) { + ret = -EINVAL; + goto out; + } + + ret = ssh_co_create(create_options, errp); out: - ssh_state_free(&s); - if (uri_options != NULL) { - QDECREF(uri_options); - } - qapi_free_BlockdevOptionsSsh(ssh_opts); + QDECREF(uri_options); + qapi_free_BlockdevCreateOptions(create_options); return ret; } @@ -1268,6 +1280,7 @@ static BlockDriver bdrv_ssh = { .instance_size = sizeof(BDRVSSHState), .bdrv_parse_filename = ssh_parse_filename, .bdrv_file_open = ssh_file_open, + .bdrv_co_create = ssh_co_create, .bdrv_co_create_opts = ssh_co_create_opts, .bdrv_close = ssh_close, .bdrv_has_zero_init = ssh_has_zero_init, diff --git a/qapi/block-core.json b/qapi/block-core.json index 4814bb7db7..524d51567a 100644 --- a/qapi/block-core.json +++ b/qapi/block-core.json @@ -3592,6 +3592,20 @@ '*redundancy': 'SheepdogRedundancy', '*object-size': 'size' } } +## +# @BlockdevCreateOptionsSsh: +# +# Driver specific image creation options for SSH. +# +# @location Where to store the new image file +# @size Size of the virtual disk in bytes +# +# Since: 2.12 +## +{ 'struct': 'BlockdevCreateOptionsSsh', + 'data': { 'location': 'BlockdevOptionsSsh', + 'size': 'size' } } + ## # @BlockdevCreateNotSupported: # @@ -3644,7 +3658,7 @@ 'rbd': 'BlockdevCreateOptionsRbd', 'replication': 'BlockdevCreateNotSupported', 'sheepdog': 'BlockdevCreateOptionsSheepdog', - 'ssh': 'BlockdevCreateNotSupported', + 'ssh': 'BlockdevCreateOptionsSsh', 'throttle': 'BlockdevCreateNotSupported', 'vdi': 'BlockdevCreateNotSupported', 'vhdx': 'BlockdevCreateNotSupported', From 89b259eeaa0f4ad3fa89cbe500da9335536e20ee Mon Sep 17 00:00:00 2001 From: Kevin Wolf Date: Wed, 7 Feb 2018 16:38:05 +0100 Subject: [PATCH 48/56] file-posix: Fix no-op bdrv_truncate() with falloc preallocation If bdrv_truncate() is called, but the requested size is the same as before, don't call posix_fallocate(), which returns -EINVAL for length zero and would therefore make bdrv_truncate() fail. The problem can be triggered by creating a zero-sized raw image with 'falloc' preallocation mode. Signed-off-by: Kevin Wolf Reviewed-by: Max Reitz Reviewed-by: Eric Blake --- block/file-posix.c | 14 +++++++++----- 1 file changed, 9 insertions(+), 5 deletions(-) diff --git a/block/file-posix.c b/block/file-posix.c index fbc21a9921..d7fb772c14 100644 --- a/block/file-posix.c +++ b/block/file-posix.c @@ -1686,11 +1686,15 @@ static int raw_regular_truncate(int fd, int64_t offset, PreallocMode prealloc, * file systems that do not support fallocate(), trying to check if a * block is allocated before allocating it, so don't do that here. */ - result = -posix_fallocate(fd, current_length, offset - current_length); - if (result != 0) { - /* posix_fallocate() doesn't set errno. */ - error_setg_errno(errp, -result, - "Could not preallocate new data"); + if (offset != current_length) { + result = -posix_fallocate(fd, current_length, offset - current_length); + if (result != 0) { + /* posix_fallocate() doesn't set errno. */ + error_setg_errno(errp, -result, + "Could not preallocate new data"); + } + } else { + result = 0; } goto out; #endif From cd8b7aaa073d83337dd100f6258a56fdd76d778d Mon Sep 17 00:00:00 2001 From: Kevin Wolf Date: Wed, 7 Feb 2018 17:58:47 +0100 Subject: [PATCH 49/56] block: Fail bdrv_truncate() with negative size Most callers have their own checks, but something like this should also be checked centrally. As it happens, x-blockdev-create can pass negative image sizes to format drivers (because there is no QAPI type that would reject negative numbers) and triggers the check added by this patch. Signed-off-by: Kevin Wolf Reviewed-by: Max Reitz Reviewed-by: Eric Blake --- block.c | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/block.c b/block.c index 00f94241fc..75a9fd49de 100644 --- a/block.c +++ b/block.c @@ -3719,6 +3719,11 @@ int bdrv_truncate(BdrvChild *child, int64_t offset, PreallocMode prealloc, error_setg(errp, "No medium inserted"); return -ENOMEDIUM; } + if (offset < 0) { + error_setg(errp, "Image size cannot be negative"); + return -EINVAL; + } + if (!drv->bdrv_truncate) { if (bs->file && drv->is_filter) { return bdrv_truncate(bs->file, offset, prealloc, errp); From 39218a771ae2761c59e2e8ae8728b434eaaa794f Mon Sep 17 00:00:00 2001 From: Kevin Wolf Date: Wed, 7 Feb 2018 16:42:44 +0100 Subject: [PATCH 50/56] qemu-iotests: Test qcow2 over file image creation with QMP Signed-off-by: Kevin Wolf Reviewed-by: Max Reitz --- tests/qemu-iotests/206 | 436 +++++++++++++++++++++++++++++++++++++ tests/qemu-iotests/206.out | 209 ++++++++++++++++++ tests/qemu-iotests/group | 1 + 3 files changed, 646 insertions(+) create mode 100755 tests/qemu-iotests/206 create mode 100644 tests/qemu-iotests/206.out diff --git a/tests/qemu-iotests/206 b/tests/qemu-iotests/206 new file mode 100755 index 0000000000..0a18b2b19a --- /dev/null +++ b/tests/qemu-iotests/206 @@ -0,0 +1,436 @@ +#!/bin/bash +# +# Test qcow2 and file image creation +# +# 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 . +# + +# creator +owner=kwolf@redhat.com + +seq=`basename $0` +echo "QA output created by $seq" + +here=`pwd` +status=1 # failure is the default! + +# get standard environment, filters and checks +. ./common.rc +. ./common.filter + +_supported_fmt qcow2 +_supported_proto file +_supported_os Linux + +function do_run_qemu() +{ + echo Testing: "$@" + $QEMU -nographic -qmp stdio -serial none "$@" + echo +} + +function run_qemu() +{ + do_run_qemu "$@" 2>&1 | _filter_testdir | _filter_qmp \ + | _filter_qemu | _filter_imgfmt \ + | _filter_actual_image_size +} + +echo +echo "=== Successful image creation (defaults) ===" +echo + +size=$((128 * 1024 * 1024)) + +run_qemu < Date: Thu, 8 Feb 2018 20:06:41 +0100 Subject: [PATCH 51/56] qemu-iotests: Test ssh image creation over QMP Signed-off-by: Kevin Wolf Reviewed-by: Max Reitz --- tests/qemu-iotests/207 | 261 +++++++++++++++++++++++++++++++++++++ tests/qemu-iotests/207.out | 75 +++++++++++ tests/qemu-iotests/group | 1 + 3 files changed, 337 insertions(+) create mode 100755 tests/qemu-iotests/207 create mode 100644 tests/qemu-iotests/207.out diff --git a/tests/qemu-iotests/207 b/tests/qemu-iotests/207 new file mode 100755 index 0000000000..f5c77852d1 --- /dev/null +++ b/tests/qemu-iotests/207 @@ -0,0 +1,261 @@ +#!/bin/bash +# +# Test ssh image creation +# +# 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 . +# + +# creator +owner=kwolf@redhat.com + +seq=`basename $0` +echo "QA output created by $seq" + +here=`pwd` +status=1 # failure is the default! + +# get standard environment, filters and checks +. ./common.rc +. ./common.filter + +_supported_fmt raw +_supported_proto ssh +_supported_os Linux + +function do_run_qemu() +{ + echo Testing: "$@" + $QEMU -nographic -qmp stdio -serial none "$@" + echo +} + +function run_qemu() +{ + do_run_qemu "$@" 2>&1 | _filter_testdir | _filter_qmp \ + | _filter_qemu | _filter_imgfmt \ + | _filter_actual_image_size +} + +echo +echo "=== Successful image creation (defaults) ===" +echo + +run_qemu </dev/null | grep -v "\\^#" | + cut -d" " -f3 | base64 -d | md5sum -b | cut -d" " -f1) + +run_qemu </dev/null | grep -v "\\^#" | + cut -d" " -f3 | base64 -d | sha1sum -b | cut -d" " -f1) + +run_qemu < Date: Mon, 25 Dec 2017 10:51:07 +0800 Subject: [PATCH 52/56] iotests: Test creating overlay when guest running Signed-off-by: Fam Zheng Message-id: 20171225025107.23985-1-famz@redhat.com Reviewed-by: Eric Blake Signed-off-by: Max Reitz --- tests/qemu-iotests/153 | 8 +++++--- tests/qemu-iotests/153.out | 7 ++++--- 2 files changed, 9 insertions(+), 6 deletions(-) diff --git a/tests/qemu-iotests/153 b/tests/qemu-iotests/153 index fa25eb24bd..adfd02695b 100755 --- a/tests/qemu-iotests/153 +++ b/tests/qemu-iotests/153 @@ -32,6 +32,7 @@ _cleanup() { _cleanup_test_img rm -f "${TEST_IMG}.base" + rm -f "${TEST_IMG}.overlay" rm -f "${TEST_IMG}.convert" rm -f "${TEST_IMG}.a" rm -f "${TEST_IMG}.b" @@ -177,8 +178,6 @@ rm -f "${TEST_IMG}.lnk" &>/dev/null ln -s ${TEST_IMG} "${TEST_IMG}.lnk" || echo "Failed to create link" _run_qemu_with_images "${TEST_IMG}.lnk" "${TEST_IMG}" -echo -echo "== Closing an image should unlock it ==" _launch_qemu _send_qemu_cmd $QEMU_HANDLE \ @@ -193,7 +192,10 @@ _send_qemu_cmd $QEMU_HANDLE \ _run_cmd $QEMU_IO "${TEST_IMG}" -c 'write 0 512' -echo "Closing drive" +echo "Creating overlay with qemu-img when the guest is running should be allowed" +_run_cmd $QEMU_IMG create -f $IMGFMT -b "${TEST_IMG}" "${TEST_IMG}.overlay" + +echo "== Closing an image should unlock it ==" _send_qemu_cmd $QEMU_HANDLE \ "{ 'execute': 'human-monitor-command', 'arguments': { 'command-line': 'drive_del d0' } }" \ diff --git a/tests/qemu-iotests/153.out b/tests/qemu-iotests/153.out index 5b917b177c..34309cfb20 100644 --- a/tests/qemu-iotests/153.out +++ b/tests/qemu-iotests/153.out @@ -372,15 +372,16 @@ Is another process using the image? == Symbolic link == QEMU_PROG: -drive if=none,file=TEST_DIR/t.qcow2: Failed to get "write" lock Is another process using the image? - -== Closing an image should unlock it == {"return": {}} Adding drive _qemu_io_wrapper TEST_DIR/t.qcow2 -c write 0 512 can't open device TEST_DIR/t.qcow2: Failed to get "write" lock Is another process using the image? -Closing drive +Creating overlay with qemu-img when the guest is running should be allowed + +_qemu_img_wrapper create -f qcow2 -b TEST_DIR/t.qcow2 TEST_DIR/t.qcow2.overlay +== Closing an image should unlock it == _qemu_io_wrapper TEST_DIR/t.qcow2 -c write 0 512 Adding two and closing one From 990dc39cfa9b72fbe743a850db5542870caf7e05 Mon Sep 17 00:00:00 2001 From: Eric Blake Date: Mon, 5 Mar 2018 10:18:24 -0600 Subject: [PATCH 53/56] iotests: Mark all tests executable The majority of our iotests have the executable bit set; fix the few outliers for consistency. Signed-off-by: Eric Blake Message-id: 20180305161824.7188-1-eblake@redhat.com Signed-off-by: Max Reitz --- tests/qemu-iotests/096 | 0 tests/qemu-iotests/124 | 0 tests/qemu-iotests/129 | 0 tests/qemu-iotests/132 | 0 tests/qemu-iotests/136 | 0 tests/qemu-iotests/139 | 0 tests/qemu-iotests/148 | 0 tests/qemu-iotests/152 | 0 tests/qemu-iotests/163 | 0 tests/qemu-iotests/205 | 0 10 files changed, 0 insertions(+), 0 deletions(-) mode change 100644 => 100755 tests/qemu-iotests/096 mode change 100644 => 100755 tests/qemu-iotests/124 mode change 100644 => 100755 tests/qemu-iotests/129 mode change 100644 => 100755 tests/qemu-iotests/132 mode change 100644 => 100755 tests/qemu-iotests/136 mode change 100644 => 100755 tests/qemu-iotests/139 mode change 100644 => 100755 tests/qemu-iotests/148 mode change 100644 => 100755 tests/qemu-iotests/152 mode change 100644 => 100755 tests/qemu-iotests/163 mode change 100644 => 100755 tests/qemu-iotests/205 diff --git a/tests/qemu-iotests/096 b/tests/qemu-iotests/096 old mode 100644 new mode 100755 diff --git a/tests/qemu-iotests/124 b/tests/qemu-iotests/124 old mode 100644 new mode 100755 diff --git a/tests/qemu-iotests/129 b/tests/qemu-iotests/129 old mode 100644 new mode 100755 diff --git a/tests/qemu-iotests/132 b/tests/qemu-iotests/132 old mode 100644 new mode 100755 diff --git a/tests/qemu-iotests/136 b/tests/qemu-iotests/136 old mode 100644 new mode 100755 diff --git a/tests/qemu-iotests/139 b/tests/qemu-iotests/139 old mode 100644 new mode 100755 diff --git a/tests/qemu-iotests/148 b/tests/qemu-iotests/148 old mode 100644 new mode 100755 diff --git a/tests/qemu-iotests/152 b/tests/qemu-iotests/152 old mode 100644 new mode 100755 diff --git a/tests/qemu-iotests/163 b/tests/qemu-iotests/163 old mode 100644 new mode 100755 diff --git a/tests/qemu-iotests/205 b/tests/qemu-iotests/205 old mode 100644 new mode 100755 From 0bfed484a51e602ec77361c79c1caede396fb242 Mon Sep 17 00:00:00 2001 From: Fam Zheng Date: Thu, 1 Mar 2018 09:14:13 +0800 Subject: [PATCH 54/56] iotests: Skip test for ENOMEM error The AFL image is to exercise the code validating image size, which doesn't work on 32 bit or when out of memory (there is a large allocation before the interesting point). So check that and skip the test, instead of faking the result. Signed-off-by: Fam Zheng Message-id: 20180301011413.11531-1-famz@redhat.com Reviewed-by: Eric Blake Signed-off-by: Max Reitz --- tests/qemu-iotests/059 | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/tests/qemu-iotests/059 b/tests/qemu-iotests/059 index 40f89eae18..530bbbe6ce 100755 --- a/tests/qemu-iotests/059 +++ b/tests/qemu-iotests/059 @@ -152,9 +152,8 @@ done echo echo "=== Testing afl image with a very large capacity ===" _use_sample_img afl9.vmdk.bz2 -# The sed makes this test pass on machines with little RAM -# (and also with 32 bit builds) -_img_info | sed -e 's/Cannot allocate memory/Invalid argument/' +_img_info | grep -q 'Cannot allocate memory' && _notrun "Insufficent memory, skipped test" +_img_info _cleanup_test_img # success, all done From 39eaefcedb9af9131543e6b35386627780ac3f41 Mon Sep 17 00:00:00 2001 From: Alberto Garcia Date: Tue, 6 Mar 2018 15:01:21 +0200 Subject: [PATCH 55/56] iotests: Tweak 030 in order to trigger a race condition with parallel jobs This patch tweaks TestParallelOps in iotest 030 so it allocates data in smaller regions (256KB/512KB instead of 512KB/1MB) and the block-stream job in test_stream_commit() only needs to copy data that is at the very end of the image. This way when the block-stream job is awakened it will finish right away without any chance of being stopped by block_job_sleep_ns(). This triggers the bug that was fixed by 3d5d319e1221082974711af1d09d82f and 1a63a907507fbbcfaee3f622907ec24 and is therefore a more useful test case for parallel block jobs. After this patch the aforementiond bug can also be reproduced with the test_stream_parallel() test case. Since with this change the stream job in test_stream_commit() finishes early, this patch introduces a similar test case where both jobs are slowed down so they can actually run in parallel. Signed-off-by: Alberto Garcia Cc: John Snow Message-id: 20180306130121.30243-1-berto@igalia.com Signed-off-by: Max Reitz --- tests/qemu-iotests/030 | 52 +++++++++++++++++++++++++++++++------- tests/qemu-iotests/030.out | 4 +-- 2 files changed, 45 insertions(+), 11 deletions(-) diff --git a/tests/qemu-iotests/030 b/tests/qemu-iotests/030 index 457984b8e9..b5f88959aa 100755 --- a/tests/qemu-iotests/030 +++ b/tests/qemu-iotests/030 @@ -156,7 +156,7 @@ class TestSingleDrive(iotests.QMPTestCase): class TestParallelOps(iotests.QMPTestCase): num_ops = 4 # Number of parallel block-stream operations num_imgs = num_ops * 2 + 1 - image_len = num_ops * 1024 * 1024 + image_len = num_ops * 512 * 1024 imgs = [] def setUp(self): @@ -176,14 +176,14 @@ class TestParallelOps(iotests.QMPTestCase): '-o', 'backing_file=%s' % self.imgs[i-1], self.imgs[i]) # Put data into the images we are copying data from - for i in range(self.num_imgs / 2): - img_index = i * 2 + 1 - # Alternate between 512k and 1M. + odd_img_indexes = [x for x in reversed(range(self.num_imgs)) if x % 2 == 1] + for i in range(len(odd_img_indexes)): + # Alternate between 256KB and 512KB. # This way jobs will not finish in the same order they were created - num_kb = 512 + 512 * (i % 2) + num_kb = 256 + 256 * (i % 2) qemu_io('-f', iotests.imgfmt, - '-c', 'write -P %d %d %d' % (i, i*1024*1024, num_kb * 1024), - self.imgs[img_index]) + '-c', 'write -P 0xFF %dk %dk' % (i * 512, num_kb), + self.imgs[odd_img_indexes[i]]) # Attach the drive to the VM self.vm = iotests.VM() @@ -318,12 +318,14 @@ class TestParallelOps(iotests.QMPTestCase): self.wait_until_completed(drive='commit-drive0') # Test a block-stream and a block-commit job in parallel - def test_stream_commit(self): + # Here the stream job is supposed to finish quickly in order to reproduce + # the scenario that triggers the bug fixed in 3d5d319e1221 and 1a63a907507 + def test_stream_commit_1(self): self.assertLessEqual(8, self.num_imgs) self.assert_no_active_block_jobs() # Stream from node0 into node2 - result = self.vm.qmp('block-stream', device='node2', job_id='node2') + result = self.vm.qmp('block-stream', device='node2', base_node='node0', job_id='node2') self.assert_qmp(result, 'return', {}) # Commit from the active layer into node3 @@ -348,6 +350,38 @@ class TestParallelOps(iotests.QMPTestCase): self.assert_no_active_block_jobs() + # This is similar to test_stream_commit_1 but both jobs are slowed + # down so they can run in parallel for a little while. + def test_stream_commit_2(self): + self.assertLessEqual(8, self.num_imgs) + self.assert_no_active_block_jobs() + + # Stream from node0 into node4 + result = self.vm.qmp('block-stream', device='node4', base_node='node0', job_id='node4', speed=1024*1024) + self.assert_qmp(result, 'return', {}) + + # Commit from the active layer into node5 + result = self.vm.qmp('block-commit', device='drive0', base=self.imgs[5], speed=1024*1024) + self.assert_qmp(result, 'return', {}) + + # Wait for all jobs to be finished. + pending_jobs = ['node4', 'drive0'] + while len(pending_jobs) > 0: + for event in self.vm.get_qmp_events(wait=True): + if event['event'] == 'BLOCK_JOB_COMPLETED': + node_name = self.dictpath(event, 'data/device') + self.assertTrue(node_name in pending_jobs) + self.assert_qmp_absent(event, 'data/error') + pending_jobs.remove(node_name) + if event['event'] == 'BLOCK_JOB_READY': + self.assert_qmp(event, 'data/device', 'drive0') + self.assert_qmp(event, 'data/type', 'commit') + self.assert_qmp_absent(event, 'data/error') + self.assertTrue('drive0' in pending_jobs) + self.vm.qmp('block-job-complete', device='drive0') + + self.assert_no_active_block_jobs() + # Test the base_node parameter def test_stream_base_node_name(self): self.assert_no_active_block_jobs() diff --git a/tests/qemu-iotests/030.out b/tests/qemu-iotests/030.out index 391c8573ca..42314e9c00 100644 --- a/tests/qemu-iotests/030.out +++ b/tests/qemu-iotests/030.out @@ -1,5 +1,5 @@ -....................... +........................ ---------------------------------------------------------------------- -Ran 23 tests +Ran 24 tests OK From 21794244d4e9c5f81132e4574e5bd10ef5066715 Mon Sep 17 00:00:00 2001 From: Stefan Hajnoczi Date: Mon, 5 Mar 2018 15:59:26 +0000 Subject: [PATCH 56/56] qemu-iotests: fix 203 migration completion race There is a race between the test's 'query-migrate' QMP command after the QMP 'STOP' event and completing the migration: The test case invokes 'query-migrate' upon receiving 'STOP'. At this point the migration thread may still be in the process of completing. Therefore 'query-migrate' can return 'status': 'active' for a brief window of time instead of 'status': 'completed'. This results in qemu-iotests 203 hanging. Solve the race by enabling the 'events' migration capability, which causes QEMU to emit migration-specific QMP events that do not suffer from this race condition. Wait for the QMP 'MIGRATION' event with 'status': 'completed'. Reported-by: Max Reitz Signed-off-by: Stefan Hajnoczi Message-id: 20180305155926.25858-1-stefanha@redhat.com Reviewed-by: Max Reitz Signed-off-by: Max Reitz --- tests/qemu-iotests/203 | 15 +++++++++++---- tests/qemu-iotests/203.out | 5 +++++ 2 files changed, 16 insertions(+), 4 deletions(-) diff --git a/tests/qemu-iotests/203 b/tests/qemu-iotests/203 index 2c811917d8..4874a1a0d8 100755 --- a/tests/qemu-iotests/203 +++ b/tests/qemu-iotests/203 @@ -49,11 +49,18 @@ with iotests.FilePath('disk0.img') as disk0_img_path, \ node_name='drive1-node', iothread='iothread0', force=True)) + iotests.log('Enabling migration QMP events...') + iotests.log(vm.qmp('migrate-set-capabilities', capabilities=[ + { + 'capability': 'events', + 'state': True + } + ])) + iotests.log('Starting migration...') iotests.log(vm.qmp('migrate', uri='exec:cat >/dev/null')) while True: - vm.get_qmp_event(wait=60.0) - result = vm.qmp('query-migrate') - status = result.get('return', {}).get('status', None) - if status == 'completed': + event = vm.event_wait('MIGRATION') + iotests.log(event, filters=[iotests.filter_qmp_event]) + if event['data']['status'] == 'completed': break diff --git a/tests/qemu-iotests/203.out b/tests/qemu-iotests/203.out index 3f1ff900e4..1a11f0975c 100644 --- a/tests/qemu-iotests/203.out +++ b/tests/qemu-iotests/203.out @@ -2,5 +2,10 @@ Launching VM... Setting IOThreads... {u'return': {}} {u'return': {}} +Enabling migration QMP events... +{u'return': {}} Starting migration... {u'return': {}} +{u'timestamp': {u'seconds': 'SECS', u'microseconds': 'USECS'}, u'data': {u'status': u'setup'}, u'event': u'MIGRATION'} +{u'timestamp': {u'seconds': 'SECS', u'microseconds': 'USECS'}, u'data': {u'status': u'active'}, u'event': u'MIGRATION'} +{u'timestamp': {u'seconds': 'SECS', u'microseconds': 'USECS'}, u'data': {u'status': u'completed'}, u'event': u'MIGRATION'}