* [PATCH v2 0/6] qemu-img: Add convert --bitmaps
@ 2020-04-21 21:20 Eric Blake
2020-04-21 21:20 ` [PATCH v2 1/6] docs: Sort sections on qemu-img subcommand parameters Eric Blake
` (6 more replies)
0 siblings, 7 replies; 23+ messages in thread
From: Eric Blake @ 2020-04-21 21:20 UTC (permalink / raw)
To: qemu-devel; +Cc: nsoffer, kwolf, jsnow, qemu-block
Without this series, the process for copying one qcow2 image to
another including all of its bitmaps involves running qemu and doing
the copying by hand with a series of QMP commands. This makes the
process a bit more convenient.
Series can also be downloaded at:
https://repo.or.cz/qemu/ericb.git/shortlog/refs/tags/qemu-img-bitmaps-v2
Since v1:
- patches 1, 3 are new
- patch 2 (old 1/3) moves more operations into new file
- patch 4 no longer a separate thread (was at v2 in isolation)
- patches 4, 6 rebased to take advantage of 3
- patch 5, 6 (old 2-3/3) have other minor tweaks
Eric Blake (6):
docs: Sort sections on qemu-img subcommand parameters
blockdev: Split off basic bitmap operations for qemu-img
qemu-img: Add bitmap sub-command
qcow2: Expose bitmaps' size during measure
qemu-img: Add convert --bitmaps option
iotests: Add test 291 to for qemu-img bitmap coverage
docs/tools/qemu-img.rst | 78 ++++++---
Makefile.objs | 2 +-
qapi/block-core.json | 15 +-
include/sysemu/blockdev.h | 14 ++
block/crypto.c | 2 +-
block/qcow2.c | 29 +++-
block/raw-format.c | 2 +-
blockbitmaps.c | 324 +++++++++++++++++++++++++++++++++++++
blockdev.c | 293 ---------------------------------
qemu-img.c | 281 +++++++++++++++++++++++++++++++-
MAINTAINERS | 1 +
qemu-img-cmds.hx | 11 +-
tests/qemu-iotests/190 | 15 +-
tests/qemu-iotests/190.out | 13 +-
tests/qemu-iotests/291 | 103 ++++++++++++
tests/qemu-iotests/291.out | 78 +++++++++
tests/qemu-iotests/group | 1 +
17 files changed, 927 insertions(+), 335 deletions(-)
create mode 100644 blockbitmaps.c
create mode 100755 tests/qemu-iotests/291
create mode 100644 tests/qemu-iotests/291.out
--
2.26.2
^ permalink raw reply [flat|nested] 23+ messages in thread
* [PATCH v2 1/6] docs: Sort sections on qemu-img subcommand parameters
2020-04-21 21:20 [PATCH v2 0/6] qemu-img: Add convert --bitmaps Eric Blake
@ 2020-04-21 21:20 ` Eric Blake
2020-04-30 12:50 ` Max Reitz
2020-04-21 21:20 ` [PATCH v2 2/6] blockdev: Split off basic bitmap operations for qemu-img Eric Blake
` (5 subsequent siblings)
6 siblings, 1 reply; 23+ messages in thread
From: Eric Blake @ 2020-04-21 21:20 UTC (permalink / raw)
To: qemu-devel; +Cc: nsoffer, kwolf, jsnow, qemu-block
We already list the subcommand summaries alphabetically, we should do
the same for the documentation related to subcommand-specific
parameters.
Signed-off-by: Eric Blake <eblake@redhat.com>
---
docs/tools/qemu-img.rst | 48 ++++++++++++++++++++---------------------
1 file changed, 24 insertions(+), 24 deletions(-)
diff --git a/docs/tools/qemu-img.rst b/docs/tools/qemu-img.rst
index 0080f83a76c9..7d08c48d308f 100644
--- a/docs/tools/qemu-img.rst
+++ b/docs/tools/qemu-img.rst
@@ -142,30 +142,6 @@ by the used format or see the format descriptions below for details.
the documentation of the emulator's ``-drive cache=...`` option for allowed
values.
-Parameters to snapshot subcommand:
-
-.. program:: qemu-img-snapshot
-
-.. option:: snapshot
-
- Is the name of the snapshot to create, apply or delete
-
-.. option:: -a
-
- Applies a snapshot (revert disk to saved state)
-
-.. option:: -c
-
- Creates a snapshot
-
-.. option:: -d
-
- Deletes a snapshot
-
-.. option:: -l
-
- Lists all snapshots in the given image
-
Parameters to compare subcommand:
.. program:: qemu-img-compare
@@ -245,6 +221,30 @@ Parameters to dd subcommand:
Sets the number of input blocks to skip
+Parameters to snapshot subcommand:
+
+.. program:: qemu-img-snapshot
+
+.. option:: snapshot
+
+ Is the name of the snapshot to create, apply or delete
+
+.. option:: -a
+
+ Applies a snapshot (revert disk to saved state)
+
+.. option:: -c
+
+ Creates a snapshot
+
+.. option:: -d
+
+ Deletes a snapshot
+
+.. option:: -l
+
+ Lists all snapshots in the given image
+
Command description:
.. program:: qemu-img-commands
--
2.26.2
^ permalink raw reply related [flat|nested] 23+ messages in thread
* [PATCH v2 2/6] blockdev: Split off basic bitmap operations for qemu-img
2020-04-21 21:20 [PATCH v2 0/6] qemu-img: Add convert --bitmaps Eric Blake
2020-04-21 21:20 ` [PATCH v2 1/6] docs: Sort sections on qemu-img subcommand parameters Eric Blake
@ 2020-04-21 21:20 ` Eric Blake
2020-04-30 13:59 ` Max Reitz
2020-04-21 21:20 ` [PATCH v2 3/6] qemu-img: Add bitmap sub-command Eric Blake
` (4 subsequent siblings)
6 siblings, 1 reply; 23+ messages in thread
From: Eric Blake @ 2020-04-21 21:20 UTC (permalink / raw)
To: qemu-devel
Cc: kwolf, qemu-block, Markus Armbruster, Max Reitz, nsoffer, jsnow
Upcoming patches want to add some basic bitmap manipulation abilities
to qemu-img. But blockdev.o is too heavyweight to link into qemu-img
(among other things, it would drag in block jobs and transaction
support - qemu-img does offline manipulation, where atomicity is less
important because there are no concurrent modifications to compete
with), so it's time to split off the bare bones of what we will need
into a new file blockbitmaps.o.
In addition to exposing 6 QMP commands for use by qemu-img (add,
remove, clear, enable, disable, merge), this also has to export three
previously-static functions for use by blockdev.c transactions.
Signed-off-by: Eric Blake <eblake@redhat.com>
---
Makefile.objs | 2 +-
include/sysemu/blockdev.h | 14 ++
blockbitmaps.c | 324 ++++++++++++++++++++++++++++++++++++++
blockdev.c | 293 ----------------------------------
MAINTAINERS | 1 +
5 files changed, 340 insertions(+), 294 deletions(-)
create mode 100644 blockbitmaps.c
diff --git a/Makefile.objs b/Makefile.objs
index a7c967633acf..44e30fa9a6e3 100644
--- a/Makefile.objs
+++ b/Makefile.objs
@@ -14,7 +14,7 @@ chardev-obj-y = chardev/
authz-obj-y = authz/
block-obj-y = nbd/
-block-obj-y += block.o blockjob.o job.o
+block-obj-y += block.o blockbitmaps.o blockjob.o job.o
block-obj-y += block/ scsi/
block-obj-y += qemu-io-cmds.o
block-obj-$(CONFIG_REPLICATION) += replication.o
diff --git a/include/sysemu/blockdev.h b/include/sysemu/blockdev.h
index a86d99b3d875..523b7493b1cd 100644
--- a/include/sysemu/blockdev.h
+++ b/include/sysemu/blockdev.h
@@ -57,4 +57,18 @@ QemuOpts *drive_add(BlockInterfaceType type, int index, const char *file,
DriveInfo *drive_new(QemuOpts *arg, BlockInterfaceType block_default_type,
Error **errp);
+BdrvDirtyBitmap *block_dirty_bitmap_lookup(const char *node,
+ const char *name,
+ BlockDriverState **pbs,
+ Error **errp);
+BdrvDirtyBitmap *do_block_dirty_bitmap_remove(const char *node,
+ const char *name, bool release,
+ BlockDriverState **bitmap_bs,
+ Error **errp);
+BdrvDirtyBitmap *do_block_dirty_bitmap_merge(const char *node,
+ const char *target,
+ BlockDirtyBitmapMergeSourceList *bitmaps,
+ HBitmap **backup, Error **errp);
+
+
#endif
diff --git a/blockbitmaps.c b/blockbitmaps.c
new file mode 100644
index 000000000000..fea80efcd03a
--- /dev/null
+++ b/blockbitmaps.c
@@ -0,0 +1,324 @@
+/*
+ * QEMU host block device bitmaps
+ *
+ * Copyright (c) 2003-2008 Fabrice Bellard
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or
+ * later. See the COPYING file in the top-level directory.
+ *
+ * This file incorporates work covered by the following copyright and
+ * permission notice:
+ *
+ * Copyright (c) 2003-2008 Fabrice Bellard
+ *
+ * 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 "sysemu/blockdev.h"
+#include "block/block.h"
+#include "block/block_int.h"
+#include "qapi/qapi-commands-block.h"
+#include "qapi/error.h"
+
+/**
+ * block_dirty_bitmap_lookup:
+ * Return a dirty bitmap (if present), after validating
+ * the node reference and bitmap names.
+ *
+ * @node: The name of the BDS node to search for bitmaps
+ * @name: The name of the bitmap to search for
+ * @pbs: Output pointer for BDS lookup, if desired. Can be NULL.
+ * @errp: Output pointer for error information. Can be NULL.
+ *
+ * @return: A bitmap object on success, or NULL on failure.
+ */
+BdrvDirtyBitmap *block_dirty_bitmap_lookup(const char *node,
+ const char *name,
+ BlockDriverState **pbs,
+ Error **errp)
+{
+ BlockDriverState *bs;
+ BdrvDirtyBitmap *bitmap;
+
+ if (!node) {
+ error_setg(errp, "Node cannot be NULL");
+ return NULL;
+ }
+ if (!name) {
+ error_setg(errp, "Bitmap name cannot be NULL");
+ return NULL;
+ }
+ bs = bdrv_lookup_bs(node, node, NULL);
+ if (!bs) {
+ error_setg(errp, "Node '%s' not found", node);
+ return NULL;
+ }
+
+ bitmap = bdrv_find_dirty_bitmap(bs, name);
+ if (!bitmap) {
+ error_setg(errp, "Dirty bitmap '%s' not found", name);
+ return NULL;
+ }
+
+ if (pbs) {
+ *pbs = bs;
+ }
+
+ return bitmap;
+}
+
+void qmp_block_dirty_bitmap_add(const char *node, const char *name,
+ bool has_granularity, uint32_t granularity,
+ bool has_persistent, bool persistent,
+ bool has_disabled, bool disabled,
+ Error **errp)
+{
+ BlockDriverState *bs;
+ BdrvDirtyBitmap *bitmap;
+ AioContext *aio_context;
+
+ if (!name || name[0] == '\0') {
+ error_setg(errp, "Bitmap name cannot be empty");
+ return;
+ }
+
+ bs = bdrv_lookup_bs(node, node, errp);
+ if (!bs) {
+ return;
+ }
+
+ aio_context = bdrv_get_aio_context(bs);
+ aio_context_acquire(aio_context);
+
+ if (has_granularity) {
+ if (granularity < 512 || !is_power_of_2(granularity)) {
+ error_setg(errp, "Granularity must be power of 2 "
+ "and at least 512");
+ goto out;
+ }
+ } else {
+ /* Default to cluster size, if available: */
+ granularity = bdrv_get_default_bitmap_granularity(bs);
+ }
+
+ if (!has_persistent) {
+ persistent = false;
+ }
+
+ if (!has_disabled) {
+ disabled = false;
+ }
+
+ if (persistent &&
+ !bdrv_can_store_new_dirty_bitmap(bs, name, granularity, errp))
+ {
+ goto out;
+ }
+
+ bitmap = bdrv_create_dirty_bitmap(bs, granularity, name, errp);
+ if (bitmap == NULL) {
+ goto out;
+ }
+
+ if (disabled) {
+ bdrv_disable_dirty_bitmap(bitmap);
+ }
+
+ bdrv_dirty_bitmap_set_persistence(bitmap, persistent);
+
+out:
+ aio_context_release(aio_context);
+}
+
+BdrvDirtyBitmap *do_block_dirty_bitmap_remove(const char *node,
+ const char *name, bool release,
+ BlockDriverState **bitmap_bs,
+ Error **errp)
+{
+ BlockDriverState *bs;
+ BdrvDirtyBitmap *bitmap;
+ AioContext *aio_context;
+
+ bitmap = block_dirty_bitmap_lookup(node, name, &bs, errp);
+ if (!bitmap || !bs) {
+ return NULL;
+ }
+
+ aio_context = bdrv_get_aio_context(bs);
+ aio_context_acquire(aio_context);
+
+ if (bdrv_dirty_bitmap_check(bitmap, BDRV_BITMAP_BUSY | BDRV_BITMAP_RO,
+ errp)) {
+ aio_context_release(aio_context);
+ return NULL;
+ }
+
+ if (bdrv_dirty_bitmap_get_persistence(bitmap) &&
+ bdrv_remove_persistent_dirty_bitmap(bs, name, errp) < 0)
+ {
+ aio_context_release(aio_context);
+ return NULL;
+ }
+
+ if (release) {
+ bdrv_release_dirty_bitmap(bitmap);
+ }
+
+ if (bitmap_bs) {
+ *bitmap_bs = bs;
+ }
+
+ aio_context_release(aio_context);
+ return release ? NULL : bitmap;
+}
+
+void qmp_block_dirty_bitmap_remove(const char *node, const char *name,
+ Error **errp)
+{
+ do_block_dirty_bitmap_remove(node, name, true, NULL, errp);
+}
+
+/**
+ * Completely clear a bitmap, for the purposes of synchronizing a bitmap
+ * immediately after a full backup operation.
+ */
+void qmp_block_dirty_bitmap_clear(const char *node, const char *name,
+ Error **errp)
+{
+ BdrvDirtyBitmap *bitmap;
+ BlockDriverState *bs;
+
+ bitmap = block_dirty_bitmap_lookup(node, name, &bs, errp);
+ if (!bitmap || !bs) {
+ return;
+ }
+
+ if (bdrv_dirty_bitmap_check(bitmap, BDRV_BITMAP_DEFAULT, errp)) {
+ return;
+ }
+
+ bdrv_clear_dirty_bitmap(bitmap, NULL);
+}
+
+void qmp_block_dirty_bitmap_enable(const char *node, const char *name,
+ Error **errp)
+{
+ BlockDriverState *bs;
+ BdrvDirtyBitmap *bitmap;
+
+ bitmap = block_dirty_bitmap_lookup(node, name, &bs, errp);
+ if (!bitmap) {
+ return;
+ }
+
+ if (bdrv_dirty_bitmap_check(bitmap, BDRV_BITMAP_ALLOW_RO, errp)) {
+ return;
+ }
+
+ bdrv_enable_dirty_bitmap(bitmap);
+}
+
+void qmp_block_dirty_bitmap_disable(const char *node, const char *name,
+ Error **errp)
+{
+ BlockDriverState *bs;
+ BdrvDirtyBitmap *bitmap;
+
+ bitmap = block_dirty_bitmap_lookup(node, name, &bs, errp);
+ if (!bitmap) {
+ return;
+ }
+
+ if (bdrv_dirty_bitmap_check(bitmap, BDRV_BITMAP_ALLOW_RO, errp)) {
+ return;
+ }
+
+ bdrv_disable_dirty_bitmap(bitmap);
+}
+
+BdrvDirtyBitmap *do_block_dirty_bitmap_merge(
+ const char *node, const char *target,
+ BlockDirtyBitmapMergeSourceList *bitmaps,
+ HBitmap **backup, Error **errp)
+{
+ BlockDriverState *bs;
+ BdrvDirtyBitmap *dst, *src, *anon;
+ BlockDirtyBitmapMergeSourceList *lst;
+ Error *local_err = NULL;
+
+ dst = block_dirty_bitmap_lookup(node, target, &bs, errp);
+ if (!dst) {
+ return NULL;
+ }
+
+ anon = bdrv_create_dirty_bitmap(bs, bdrv_dirty_bitmap_granularity(dst),
+ NULL, errp);
+ if (!anon) {
+ return NULL;
+ }
+
+ for (lst = bitmaps; lst; lst = lst->next) {
+ switch (lst->value->type) {
+ const char *name, *node;
+ case QTYPE_QSTRING:
+ name = lst->value->u.local;
+ src = bdrv_find_dirty_bitmap(bs, name);
+ if (!src) {
+ error_setg(errp, "Dirty bitmap '%s' not found", name);
+ dst = NULL;
+ goto out;
+ }
+ break;
+ case QTYPE_QDICT:
+ node = lst->value->u.external.node;
+ name = lst->value->u.external.name;
+ src = block_dirty_bitmap_lookup(node, name, NULL, errp);
+ if (!src) {
+ dst = NULL;
+ goto out;
+ }
+ break;
+ default:
+ abort();
+ }
+
+ bdrv_merge_dirty_bitmap(anon, src, NULL, &local_err);
+ if (local_err) {
+ error_propagate(errp, local_err);
+ dst = NULL;
+ goto out;
+ }
+ }
+
+ /* Merge into dst; dst is unchanged on failure. */
+ bdrv_merge_dirty_bitmap(dst, anon, backup, errp);
+
+ out:
+ bdrv_release_dirty_bitmap(anon);
+ return dst;
+}
+
+void qmp_block_dirty_bitmap_merge(const char *node, const char *target,
+ BlockDirtyBitmapMergeSourceList *bitmaps,
+ Error **errp)
+{
+ do_block_dirty_bitmap_merge(node, target, bitmaps, NULL, errp);
+}
diff --git a/blockdev.c b/blockdev.c
index 5faddaa7052f..d4efd4cbf2cb 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -1185,53 +1185,6 @@ out_aio_context:
return NULL;
}
-/**
- * block_dirty_bitmap_lookup:
- * Return a dirty bitmap (if present), after validating
- * the node reference and bitmap names.
- *
- * @node: The name of the BDS node to search for bitmaps
- * @name: The name of the bitmap to search for
- * @pbs: Output pointer for BDS lookup, if desired. Can be NULL.
- * @errp: Output pointer for error information. Can be NULL.
- *
- * @return: A bitmap object on success, or NULL on failure.
- */
-static BdrvDirtyBitmap *block_dirty_bitmap_lookup(const char *node,
- const char *name,
- BlockDriverState **pbs,
- Error **errp)
-{
- BlockDriverState *bs;
- BdrvDirtyBitmap *bitmap;
-
- if (!node) {
- error_setg(errp, "Node cannot be NULL");
- return NULL;
- }
- if (!name) {
- error_setg(errp, "Bitmap name cannot be NULL");
- return NULL;
- }
- bs = bdrv_lookup_bs(node, node, NULL);
- if (!bs) {
- error_setg(errp, "Node '%s' not found", node);
- return NULL;
- }
-
- bitmap = bdrv_find_dirty_bitmap(bs, name);
- if (!bitmap) {
- error_setg(errp, "Dirty bitmap '%s' not found", name);
- return NULL;
- }
-
- if (pbs) {
- *pbs = bs;
- }
-
- return bitmap;
-}
-
/* New and old BlockDriverState structs for atomic group operations */
typedef struct BlkActionState BlkActionState;
@@ -2171,11 +2124,6 @@ static void block_dirty_bitmap_disable_abort(BlkActionState *common)
}
}
-static BdrvDirtyBitmap *do_block_dirty_bitmap_merge(
- const char *node, const char *target,
- BlockDirtyBitmapMergeSourceList *bitmaps,
- HBitmap **backup, Error **errp);
-
static void block_dirty_bitmap_merge_prepare(BlkActionState *common,
Error **errp)
{
@@ -2194,10 +2142,6 @@ static void block_dirty_bitmap_merge_prepare(BlkActionState *common,
errp);
}
-static BdrvDirtyBitmap *do_block_dirty_bitmap_remove(
- const char *node, const char *name, bool release,
- BlockDriverState **bitmap_bs, Error **errp);
-
static void block_dirty_bitmap_remove_prepare(BlkActionState *common,
Error **errp)
{
@@ -2441,243 +2385,6 @@ void qmp_block_passwd(bool has_device, const char *device,
"Setting block passwords directly is no longer supported");
}
-void qmp_block_dirty_bitmap_add(const char *node, const char *name,
- bool has_granularity, uint32_t granularity,
- bool has_persistent, bool persistent,
- bool has_disabled, bool disabled,
- Error **errp)
-{
- BlockDriverState *bs;
- BdrvDirtyBitmap *bitmap;
- AioContext *aio_context;
-
- if (!name || name[0] == '\0') {
- error_setg(errp, "Bitmap name cannot be empty");
- return;
- }
-
- bs = bdrv_lookup_bs(node, node, errp);
- if (!bs) {
- return;
- }
-
- aio_context = bdrv_get_aio_context(bs);
- aio_context_acquire(aio_context);
-
- if (has_granularity) {
- if (granularity < 512 || !is_power_of_2(granularity)) {
- error_setg(errp, "Granularity must be power of 2 "
- "and at least 512");
- goto out;
- }
- } else {
- /* Default to cluster size, if available: */
- granularity = bdrv_get_default_bitmap_granularity(bs);
- }
-
- if (!has_persistent) {
- persistent = false;
- }
-
- if (!has_disabled) {
- disabled = false;
- }
-
- if (persistent &&
- !bdrv_can_store_new_dirty_bitmap(bs, name, granularity, errp))
- {
- goto out;
- }
-
- bitmap = bdrv_create_dirty_bitmap(bs, granularity, name, errp);
- if (bitmap == NULL) {
- goto out;
- }
-
- if (disabled) {
- bdrv_disable_dirty_bitmap(bitmap);
- }
-
- bdrv_dirty_bitmap_set_persistence(bitmap, persistent);
-
-out:
- aio_context_release(aio_context);
-}
-
-static BdrvDirtyBitmap *do_block_dirty_bitmap_remove(
- const char *node, const char *name, bool release,
- BlockDriverState **bitmap_bs, Error **errp)
-{
- BlockDriverState *bs;
- BdrvDirtyBitmap *bitmap;
- AioContext *aio_context;
-
- bitmap = block_dirty_bitmap_lookup(node, name, &bs, errp);
- if (!bitmap || !bs) {
- return NULL;
- }
-
- aio_context = bdrv_get_aio_context(bs);
- aio_context_acquire(aio_context);
-
- if (bdrv_dirty_bitmap_check(bitmap, BDRV_BITMAP_BUSY | BDRV_BITMAP_RO,
- errp)) {
- aio_context_release(aio_context);
- return NULL;
- }
-
- if (bdrv_dirty_bitmap_get_persistence(bitmap) &&
- bdrv_remove_persistent_dirty_bitmap(bs, name, errp) < 0)
- {
- aio_context_release(aio_context);
- return NULL;
- }
-
- if (release) {
- bdrv_release_dirty_bitmap(bitmap);
- }
-
- if (bitmap_bs) {
- *bitmap_bs = bs;
- }
-
- aio_context_release(aio_context);
- return release ? NULL : bitmap;
-}
-
-void qmp_block_dirty_bitmap_remove(const char *node, const char *name,
- Error **errp)
-{
- do_block_dirty_bitmap_remove(node, name, true, NULL, errp);
-}
-
-/**
- * Completely clear a bitmap, for the purposes of synchronizing a bitmap
- * immediately after a full backup operation.
- */
-void qmp_block_dirty_bitmap_clear(const char *node, const char *name,
- Error **errp)
-{
- BdrvDirtyBitmap *bitmap;
- BlockDriverState *bs;
-
- bitmap = block_dirty_bitmap_lookup(node, name, &bs, errp);
- if (!bitmap || !bs) {
- return;
- }
-
- if (bdrv_dirty_bitmap_check(bitmap, BDRV_BITMAP_DEFAULT, errp)) {
- return;
- }
-
- bdrv_clear_dirty_bitmap(bitmap, NULL);
-}
-
-void qmp_block_dirty_bitmap_enable(const char *node, const char *name,
- Error **errp)
-{
- BlockDriverState *bs;
- BdrvDirtyBitmap *bitmap;
-
- bitmap = block_dirty_bitmap_lookup(node, name, &bs, errp);
- if (!bitmap) {
- return;
- }
-
- if (bdrv_dirty_bitmap_check(bitmap, BDRV_BITMAP_ALLOW_RO, errp)) {
- return;
- }
-
- bdrv_enable_dirty_bitmap(bitmap);
-}
-
-void qmp_block_dirty_bitmap_disable(const char *node, const char *name,
- Error **errp)
-{
- BlockDriverState *bs;
- BdrvDirtyBitmap *bitmap;
-
- bitmap = block_dirty_bitmap_lookup(node, name, &bs, errp);
- if (!bitmap) {
- return;
- }
-
- if (bdrv_dirty_bitmap_check(bitmap, BDRV_BITMAP_ALLOW_RO, errp)) {
- return;
- }
-
- bdrv_disable_dirty_bitmap(bitmap);
-}
-
-static BdrvDirtyBitmap *do_block_dirty_bitmap_merge(
- const char *node, const char *target,
- BlockDirtyBitmapMergeSourceList *bitmaps,
- HBitmap **backup, Error **errp)
-{
- BlockDriverState *bs;
- BdrvDirtyBitmap *dst, *src, *anon;
- BlockDirtyBitmapMergeSourceList *lst;
- Error *local_err = NULL;
-
- dst = block_dirty_bitmap_lookup(node, target, &bs, errp);
- if (!dst) {
- return NULL;
- }
-
- anon = bdrv_create_dirty_bitmap(bs, bdrv_dirty_bitmap_granularity(dst),
- NULL, errp);
- if (!anon) {
- return NULL;
- }
-
- for (lst = bitmaps; lst; lst = lst->next) {
- switch (lst->value->type) {
- const char *name, *node;
- case QTYPE_QSTRING:
- name = lst->value->u.local;
- src = bdrv_find_dirty_bitmap(bs, name);
- if (!src) {
- error_setg(errp, "Dirty bitmap '%s' not found", name);
- dst = NULL;
- goto out;
- }
- break;
- case QTYPE_QDICT:
- node = lst->value->u.external.node;
- name = lst->value->u.external.name;
- src = block_dirty_bitmap_lookup(node, name, NULL, errp);
- if (!src) {
- dst = NULL;
- goto out;
- }
- break;
- default:
- abort();
- }
-
- bdrv_merge_dirty_bitmap(anon, src, NULL, &local_err);
- if (local_err) {
- error_propagate(errp, local_err);
- dst = NULL;
- goto out;
- }
- }
-
- /* Merge into dst; dst is unchanged on failure. */
- bdrv_merge_dirty_bitmap(dst, anon, backup, errp);
-
- out:
- bdrv_release_dirty_bitmap(anon);
- return dst;
-}
-
-void qmp_block_dirty_bitmap_merge(const char *node, const char *target,
- BlockDirtyBitmapMergeSourceList *bitmaps,
- Error **errp)
-{
- do_block_dirty_bitmap_merge(node, target, bitmaps, NULL, errp);
-}
-
BlockDirtyBitmapSha256 *qmp_x_debug_block_dirty_bitmap_sha256(const char *node,
const char *name,
Error **errp)
diff --git a/MAINTAINERS b/MAINTAINERS
index 8cbc1fac2bfc..769cd357d281 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -1989,6 +1989,7 @@ T: git https://github.com/jnsnow/qemu.git jobs
Block QAPI, monitor, command line
M: Markus Armbruster <armbru@redhat.com>
S: Supported
+F: blockbitmaps.c
F: blockdev.c
F: blockdev-hmp-cmds.c
F: block/qapi.c
--
2.26.2
^ permalink raw reply related [flat|nested] 23+ messages in thread
* [PATCH v2 3/6] qemu-img: Add bitmap sub-command
2020-04-21 21:20 [PATCH v2 0/6] qemu-img: Add convert --bitmaps Eric Blake
2020-04-21 21:20 ` [PATCH v2 1/6] docs: Sort sections on qemu-img subcommand parameters Eric Blake
2020-04-21 21:20 ` [PATCH v2 2/6] blockdev: Split off basic bitmap operations for qemu-img Eric Blake
@ 2020-04-21 21:20 ` Eric Blake
2020-04-30 14:55 ` Max Reitz
2020-04-21 21:20 ` [PATCH v2 4/6] qcow2: Expose bitmaps' size during measure Eric Blake
` (3 subsequent siblings)
6 siblings, 1 reply; 23+ messages in thread
From: Eric Blake @ 2020-04-21 21:20 UTC (permalink / raw)
To: qemu-devel; +Cc: nsoffer, kwolf, jsnow, qemu-block, Max Reitz
Include actions for --add, --remove, --clear, --enable, --disable, and
--merge (note that --clear is a bit of fluff, because the same can be
accomplished by removing a bitmap and then adding a new one in its
place, but it matches what QMP commands exist). Listing is omitted,
because it does not require a bitmap name and because it was already
possible with 'qemu-img info'. Merge can work either from another
bitmap in the same image, or from a bitmap in a distinct image.
While this supports --image-opts for the file being modified, I did
not think it worth the extra complexity to support that for the source
file in a cross-file bitmap merge. Likewise, I chose to have --merge
only take a single source rather than following the QMP support for
multiple merges in one go; in part to simplify the command line, and
in part because an offline image can achieve the same effect by
multiple qemu-img bitmap --merge calls. We can enhance that if needed
in the future (the same way that 'qemu-img convert' has a mode that
concatenates multiple sources into one destination).
Upcoming patches will add iotest coverage of these commands while
also testing other features.
Signed-off-by: Eric Blake <eblake@redhat.com>
---
docs/tools/qemu-img.rst | 24 +++++
qemu-img.c | 198 ++++++++++++++++++++++++++++++++++++++++
qemu-img-cmds.hx | 7 ++
3 files changed, 229 insertions(+)
diff --git a/docs/tools/qemu-img.rst b/docs/tools/qemu-img.rst
index 7d08c48d308f..4f3b0e2c9ace 100644
--- a/docs/tools/qemu-img.rst
+++ b/docs/tools/qemu-img.rst
@@ -281,6 +281,30 @@ Command description:
For write tests, by default a buffer filled with zeros is written. This can be
overridden with a pattern byte specified by *PATTERN*.
+.. option:: bitmap {--add [-g GRANULARITY] [--disabled] | --remove | --clear | --enable | --disable | --merge SOURCE_BITMAP [-b SOURCE_FILE [-F SOURCE_FMT]]} [--object OBJECTDEF] [--image-opts] [-f FMT] FILENAME BITMAP
+
+ Perform a modification of the persistent bitmap *BITMAP* in the disk
+ image *FILENAME*. The various modifications are:
+
+ ``--add`` to create *BITMAP*, with additional options ``-g`` to
+ specify a non-default *GRANULARITY*, or whether the bitmap should be
+ ``--disabled`` instead of enabled.
+
+ ``--remove`` to remove *BITMAP*.
+
+ ``--clear`` to clear *BITMAP*.
+
+ ``--enable`` to change *BITMAP* to start recording future edits.
+
+ ``--disable`` to change *BITMAP* to stop recording future edits.
+
+ ``--merge`` to merge the contents of *SOURCE_BITMAP* into *BITMAP*.
+ This defaults to requiring a source bitmap from the same *FILENAME*,
+ but can also be used for cross-image merge by supplying ``-b`` to
+ specify a different *SOURCE_FILE*.
+
+ To see what bitmaps are present in an image, use ``qemu-img info``.
+
.. option:: check [--object OBJECTDEF] [--image-opts] [-q] [-f FMT] [--output=OFMT] [-r [leaks | all]] [-T SRC_CACHE] [-U] FILENAME
Perform a consistency check on the disk image *FILENAME*. The command can
diff --git a/qemu-img.c b/qemu-img.c
index 821cbf610e5f..02ebd870faa1 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -28,6 +28,7 @@
#include "qemu-common.h"
#include "qemu-version.h"
#include "qapi/error.h"
+#include "qapi/qapi-commands-block-core.h"
#include "qapi/qapi-visit-block-core.h"
#include "qapi/qobject-output-visitor.h"
#include "qapi/qmp/qjson.h"
@@ -71,6 +72,12 @@ enum {
OPTION_SHRINK = 266,
OPTION_SALVAGE = 267,
OPTION_TARGET_IS_ZERO = 268,
+ OPTION_ADD = 269,
+ OPTION_REMOVE = 270,
+ OPTION_CLEAR = 271,
+ OPTION_ENABLE = 272,
+ OPTION_DISABLE = 273,
+ OPTION_MERGE = 274,
};
typedef enum OutputFormat {
@@ -4438,6 +4445,197 @@ out:
return 0;
}
+static int img_bitmap(int argc, char **argv)
+{
+ Error *err = NULL;
+ int c, ret = -1;
+ QemuOpts *opts = NULL;
+ const char *fmt = NULL, *src_fmt = NULL, *src_filename = NULL;
+ const char *filename, *bitmap;
+ BlockBackend *blk = NULL, *src = NULL;
+ BlockDriverState *bs = NULL, *src_bs = NULL;
+ bool image_opts = false;
+ unsigned long granularity = 0;
+ bool add = false, remove = false, clear = false;
+ bool enable = false, disable = false, add_disabled = false;
+ const char *merge = NULL;
+
+ for (;;) {
+ static const struct option long_options[] = {
+ {"help", no_argument, 0, 'h'},
+ {"object", required_argument, 0, OPTION_OBJECT},
+ {"image-opts", no_argument, 0, OPTION_IMAGE_OPTS},
+ {"add", no_argument, 0, OPTION_ADD},
+ {"remove", no_argument, 0, OPTION_REMOVE},
+ {"clear", no_argument, 0, OPTION_CLEAR},
+ {"enable", no_argument, 0, OPTION_ENABLE},
+ {"disable", no_argument, 0, OPTION_DISABLE},
+ {"disabled", no_argument, 0, OPTION_DISABLE},
+ {"merge", required_argument, 0, OPTION_MERGE},
+ {"granularity", required_argument, 0, 'g'},
+ {"source-file", required_argument, 0, 'b'},
+ {"source-format", required_argument, 0, 'F'},
+ {0, 0, 0, 0}
+ };
+ c = getopt_long(argc, argv, ":b:f:F:g:h", long_options, NULL);
+ if (c == -1) {
+ break;
+ }
+
+ switch (c) {
+ case ':':
+ missing_argument(argv[optind - 1]);
+ break;
+ case '?':
+ unrecognized_option(argv[optind - 1]);
+ break;
+ case 'h':
+ help();
+ break;
+ case 'b':
+ src_filename = optarg;
+ break;
+ case 'f':
+ fmt = optarg;
+ break;
+ case 'F':
+ src_fmt = optarg;
+ break;
+ case 'g':
+ if (qemu_strtosz(optarg, NULL, &granularity)) {
+ error_report("Invalid granularity specified");
+ return 1;
+ }
+ break;
+ case OPTION_ADD:
+ add = true;
+ break;
+ case OPTION_REMOVE:
+ remove = true;
+ break;
+ case OPTION_CLEAR:
+ clear = true;
+ break;
+ case OPTION_ENABLE:
+ enable = true;
+ break;
+ case OPTION_DISABLE:
+ disable = true;
+ break;
+ case OPTION_MERGE:
+ merge = optarg;
+ break;
+ case OPTION_OBJECT:
+ opts = qemu_opts_parse_noisily(&qemu_object_opts,
+ optarg, true);
+ if (!opts) {
+ goto out;
+ }
+ break;
+ case OPTION_IMAGE_OPTS:
+ image_opts = true;
+ break;
+ }
+ }
+
+ if (qemu_opts_foreach(&qemu_object_opts,
+ user_creatable_add_opts_foreach,
+ qemu_img_object_print_help, &error_fatal)) {
+ goto out;
+ }
+
+ if (add && disable) {
+ disable = false;
+ add_disabled = true;
+ }
+ if (add + remove + clear + enable + disable + !!merge != 1) {
+ error_report("Need exactly one mode of --add, --remove, --clear, "
+ "--enable, --disable, or --merge");
+ goto out;
+ }
+ if (granularity && !add) {
+ error_report("granularity only supported with --add");
+ goto out;
+ }
+ if (src_fmt && !src_filename) {
+ error_report("-F only supported with -b");
+ goto out;
+ }
+ if (src_filename && !merge) {
+ error_report("alternate source file only supported with --merge");
+ goto out;
+ }
+
+ if (optind != argc - 2) {
+ error_report("Expecting filename and bitmap name");
+ goto out;
+ }
+
+ filename = argv[optind];
+ bitmap = argv[optind + 1];
+
+ blk = img_open(image_opts, filename, fmt, BDRV_O_RDWR, false, false,
+ false);
+ if (!blk) {
+ goto out;
+ }
+ bs = blk_bs(blk);
+
+ if (add) {
+ qmp_block_dirty_bitmap_add(bs->node_name, bitmap,
+ !!granularity, granularity, true, true,
+ true, add_disabled, &err);
+ } else if (remove) {
+ qmp_block_dirty_bitmap_remove(bs->node_name, bitmap, &err);
+ } else if (clear) {
+ qmp_block_dirty_bitmap_clear(bs->node_name, bitmap, &err);
+ } else if (enable) {
+ qmp_block_dirty_bitmap_enable(bs->node_name, bitmap, &err);
+ } else if (disable) {
+ qmp_block_dirty_bitmap_disable(bs->node_name, bitmap, &err);
+ } else if (merge) {
+ BlockDirtyBitmapMergeSource *merge_src;
+ BlockDirtyBitmapMergeSourceList *list;
+
+ if (src_filename) {
+ src = img_open(NULL, src_filename, src_fmt, 0, false, false,
+ false);
+ if (!src) {
+ goto out;
+ }
+ src_bs = blk_bs(src);
+ } else {
+ src_bs = bs;
+ }
+
+ merge_src = g_new0(BlockDirtyBitmapMergeSource, 1);
+ merge_src->type = QTYPE_QDICT;
+ merge_src->u.external.node = g_strdup(src_bs->node_name);
+ merge_src->u.external.name = g_strdup(merge);
+ list = g_new0(BlockDirtyBitmapMergeSourceList, 1);
+ list->value = merge_src;
+ qmp_block_dirty_bitmap_merge(bs->node_name, bitmap, list, &err);
+ qapi_free_BlockDirtyBitmapMergeSourceList(list);
+ }
+
+ if (err) {
+ error_reportf_err(err, "Bitmap %s operation failed", bitmap);
+ ret = -1;
+ goto out;
+ }
+
+ ret = 0;
+
+ out:
+ blk_unref(src);
+ blk_unref(blk);
+ qemu_opts_del(opts);
+ if (ret) {
+ return 1;
+ }
+ return 0;
+}
+
#define C_BS 01
#define C_COUNT 02
#define C_IF 04
diff --git a/qemu-img-cmds.hx b/qemu-img-cmds.hx
index c9c54de1df40..bf0035e226c8 100644
--- a/qemu-img-cmds.hx
+++ b/qemu-img-cmds.hx
@@ -20,6 +20,13 @@ DEF("bench", img_bench,
SRST
.. option:: bench [-c COUNT] [-d DEPTH] [-f FMT] [--flush-interval=FLUSH_INTERVAL] [-i AIO] [-n] [--no-drain] [-o OFFSET] [--pattern=PATTERN] [-q] [-s BUFFER_SIZE] [-S STEP_SIZE] [-t CACHE] [-w] [-U] FILENAME
ERST
+
+DEF("bitmap", img_bitmap,
+ "bitmap {--add [-g granularity] [--disabled] | --remove | --clear | --enable | --disable | --merge source_bitmap [-b source_file [-F source_fmt]]} [--object objectdef] [--image-opts] [-f fmt] filename bitmap")
+SRST
+.. option:: bitmap {--add [-g GRANULARITY] [--disabled] | --remove | --clear | --enable | --disable | --merge SOURCE_BITMAP [-b SOURCE_FILE [-F SOURCE_FMT]]} [--object OBJECTDEF] [--image-opts] [-f FMT] FILENAME BITMAP
+ERST
+
DEF("check", img_check,
"check [--object objectdef] [--image-opts] [-q] [-f fmt] [--output=ofmt] [-r [leaks | all]] [-T src_cache] [-U] filename")
SRST
--
2.26.2
^ permalink raw reply related [flat|nested] 23+ messages in thread
* [PATCH v2 4/6] qcow2: Expose bitmaps' size during measure
2020-04-21 21:20 [PATCH v2 0/6] qemu-img: Add convert --bitmaps Eric Blake
` (2 preceding siblings ...)
2020-04-21 21:20 ` [PATCH v2 3/6] qemu-img: Add bitmap sub-command Eric Blake
@ 2020-04-21 21:20 ` Eric Blake
2020-05-04 11:36 ` Max Reitz
2020-04-21 21:20 ` [PATCH v2 5/6] qemu-img: Add convert --bitmaps option Eric Blake
` (2 subsequent siblings)
6 siblings, 1 reply; 23+ messages in thread
From: Eric Blake @ 2020-04-21 21:20 UTC (permalink / raw)
To: qemu-devel
Cc: kwolf, qemu-block, Markus Armbruster, Max Reitz, nsoffer, jsnow
It's useful to know how much space can be occupied by qcow2 persistent
bitmaps, even though such metadata is unrelated to the guest-visible
data. Report this value as an additional field. Update iotest 190 to
cover it and a portion of the just-added qemu-img bitmap command.
The addition of a new field demonstrates why we should always
zero-initialize qapi C structs; while the qcow2 driver still fully
populates all fields, the raw and crypto drivers had to be tweaked.
See also: https://bugzilla.redhat.com/1779904
Reported-by: Nir Soffer <nsoffer@redhat.com>
Signed-off-by: Eric Blake <eblake@redhat.com>
---
qapi/block-core.json | 15 ++++++++++-----
block/crypto.c | 2 +-
block/qcow2.c | 29 ++++++++++++++++++++++++++++-
block/raw-format.c | 2 +-
qemu-img.c | 3 +++
tests/qemu-iotests/190 | 15 +++++++++++++--
tests/qemu-iotests/190.out | 13 ++++++++++++-
7 files changed, 68 insertions(+), 11 deletions(-)
diff --git a/qapi/block-core.json b/qapi/block-core.json
index 943df1926a91..b47c6d69ba27 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -633,18 +633,23 @@
# efficiently so file size may be smaller than virtual disk size.
#
# The values are upper bounds that are guaranteed to fit the new image file.
-# Subsequent modification, such as internal snapshot or bitmap creation, may
-# require additional space and is not covered here.
+# Subsequent modification, such as internal snapshot or further bitmap
+# creation, may require additional space and is not covered here.
#
-# @required: Size required for a new image file, in bytes.
+# @required: Size required for a new image file, in bytes, when copying just
+# guest-visible contents.
#
# @fully-allocated: Image file size, in bytes, once data has been written
-# to all sectors.
+# to all sectors, when copying just guest-visible contents.
+#
+# @bitmaps: Additional size required for bitmap metadata not directly used
+# for guest contents, when that metadata can be copied in addition
+# to guest contents. (since 5.1)
#
# Since: 2.10
##
{ 'struct': 'BlockMeasureInfo',
- 'data': {'required': 'int', 'fully-allocated': 'int'} }
+ 'data': {'required': 'int', 'fully-allocated': 'int', '*bitmaps': 'int'} }
##
# @query-block:
diff --git a/block/crypto.c b/block/crypto.c
index d577f89659fa..4e0f3ec97f0e 100644
--- a/block/crypto.c
+++ b/block/crypto.c
@@ -535,7 +535,7 @@ static BlockMeasureInfo *block_crypto_measure(QemuOpts *opts,
* Unallocated blocks are still encrypted so allocation status makes no
* difference to the file size.
*/
- info = g_new(BlockMeasureInfo, 1);
+ info = g_new0(BlockMeasureInfo, 1);
info->fully_allocated = luks_payload_size + size;
info->required = luks_payload_size + size;
return info;
diff --git a/block/qcow2.c b/block/qcow2.c
index b524b0c53f84..9fd650928016 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -4657,6 +4657,7 @@ static BlockMeasureInfo *qcow2_measure(QemuOpts *opts, BlockDriverState *in_bs,
PreallocMode prealloc;
bool has_backing_file;
bool has_luks;
+ uint64_t bitmaps_size = 0; /* size occupied by bitmaps in in_bs */
/* Parse image creation options */
cluster_size = qcow2_opt_get_cluster_size_del(opts, &local_err);
@@ -4732,6 +4733,8 @@ static BlockMeasureInfo *qcow2_measure(QemuOpts *opts, BlockDriverState *in_bs,
/* Account for input image */
if (in_bs) {
+ BdrvDirtyBitmap *bm;
+ size_t bitmap_overhead = 0;
int64_t ssize = bdrv_getlength(in_bs);
if (ssize < 0) {
error_setg_errno(&local_err, -ssize,
@@ -4739,6 +4742,28 @@ static BlockMeasureInfo *qcow2_measure(QemuOpts *opts, BlockDriverState *in_bs,
goto err;
}
+ FOR_EACH_DIRTY_BITMAP(in_bs, bm) {
+ if (bdrv_dirty_bitmap_get_persistence(bm)) {
+ const char *name = bdrv_dirty_bitmap_name(bm);
+ uint32_t granularity = bdrv_dirty_bitmap_granularity(bm);
+ uint64_t bmbits = DIV_ROUND_UP(bdrv_dirty_bitmap_size(bm),
+ granularity);
+ uint64_t bmclusters = DIV_ROUND_UP(DIV_ROUND_UP(bmbits,
+ CHAR_BIT),
+ cluster_size);
+
+ /* Assume the entire bitmap is allocated */
+ bitmaps_size += bmclusters * cluster_size;
+ /* Also reserve space for the bitmap table entries */
+ bitmaps_size += ROUND_UP(bmclusters * sizeof(uint64_t),
+ cluster_size);
+ /* Guess at contribution to bitmap directory size */
+ bitmap_overhead += ROUND_UP(strlen(name) + 24,
+ sizeof(uint64_t));
+ }
+ }
+ bitmaps_size += ROUND_UP(bitmap_overhead, cluster_size);
+
virtual_size = ROUND_UP(ssize, cluster_size);
if (has_backing_file) {
@@ -4785,7 +4810,7 @@ static BlockMeasureInfo *qcow2_measure(QemuOpts *opts, BlockDriverState *in_bs,
required = virtual_size;
}
- info = g_new(BlockMeasureInfo, 1);
+ info = g_new0(BlockMeasureInfo, 1);
info->fully_allocated =
qcow2_calc_prealloc_size(virtual_size, cluster_size,
ctz32(refcount_bits)) + luks_payload_size;
@@ -4795,6 +4820,8 @@ static BlockMeasureInfo *qcow2_measure(QemuOpts *opts, BlockDriverState *in_bs,
* still counted.
*/
info->required = info->fully_allocated - virtual_size + required;
+ info->has_bitmaps = !!bitmaps_size;
+ info->bitmaps = bitmaps_size;
return info;
err:
diff --git a/block/raw-format.c b/block/raw-format.c
index 93b25e1b6b0b..4bb54f4ac6c5 100644
--- a/block/raw-format.c
+++ b/block/raw-format.c
@@ -346,7 +346,7 @@ static BlockMeasureInfo *raw_measure(QemuOpts *opts, BlockDriverState *in_bs,
BDRV_SECTOR_SIZE);
}
- info = g_new(BlockMeasureInfo, 1);
+ info = g_new0(BlockMeasureInfo, 1);
info->required = required;
/* Unallocated sectors count towards the file size in raw images */
diff --git a/qemu-img.c b/qemu-img.c
index 02ebd870faa1..e1127273f21e 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -5207,6 +5207,9 @@ static int img_measure(int argc, char **argv)
if (output_format == OFORMAT_HUMAN) {
printf("required size: %" PRIu64 "\n", info->required);
printf("fully allocated size: %" PRIu64 "\n", info->fully_allocated);
+ if (info->has_bitmaps) {
+ printf("bitmaps size: %" PRIu64 "\n", info->bitmaps);
+ }
} else {
dump_json_block_measure_info(info);
}
diff --git a/tests/qemu-iotests/190 b/tests/qemu-iotests/190
index 6d41650438e1..cae643149a01 100755
--- a/tests/qemu-iotests/190
+++ b/tests/qemu-iotests/190
@@ -2,7 +2,7 @@
#
# qemu-img measure sub-command tests on huge qcow2 files
#
-# Copyright (C) 2017 Red Hat, Inc.
+# Copyright (C) 2017-2020 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
@@ -42,7 +42,7 @@ trap "_cleanup; exit \$status" 0 1 2 3 15
_supported_fmt qcow2
_supported_proto file
-echo "== Huge file =="
+echo "== Huge file without bitmaps =="
echo
_make_test_img -o 'cluster_size=2M' 2T
@@ -51,6 +51,17 @@ $QEMU_IMG measure -O raw -f qcow2 "$TEST_IMG"
$QEMU_IMG measure -O qcow2 -o cluster_size=64k -f qcow2 "$TEST_IMG"
$QEMU_IMG measure -O qcow2 -o cluster_size=2M -f qcow2 "$TEST_IMG"
+echo
+echo "== Huge file with bitmaps =="
+echo
+
+$QEMU_IMG bitmap --add --granularity 512 -f qcow2 "$TEST_IMG" b1
+$QEMU_IMG bitmap --add -g 2M -f qcow2 "$TEST_IMG" b2
+
+$QEMU_IMG measure -O raw -f qcow2 "$TEST_IMG"
+$QEMU_IMG measure -O qcow2 -o cluster_size=64k -f qcow2 "$TEST_IMG"
+$QEMU_IMG measure -O qcow2 -o cluster_size=2M -f qcow2 "$TEST_IMG"
+
# success, all done
echo "*** done"
rm -f $seq.full
diff --git a/tests/qemu-iotests/190.out b/tests/qemu-iotests/190.out
index d001942002db..11962f972429 100644
--- a/tests/qemu-iotests/190.out
+++ b/tests/qemu-iotests/190.out
@@ -1,5 +1,5 @@
QA output created by 190
-== Huge file ==
+== Huge file without bitmaps ==
Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=2199023255552
required size: 2199023255552
@@ -8,4 +8,15 @@ required size: 335806464
fully allocated size: 2199359062016
required size: 18874368
fully allocated size: 2199042129920
+
+== Huge file with bitmaps ==
+
+required size: 2199023255552
+fully allocated size: 2199023255552
+required size: 335806464
+fully allocated size: 2199359062016
+bitmaps size: 537198592
+required size: 18874368
+fully allocated size: 2199042129920
+bitmaps size: 545259520
*** done
--
2.26.2
^ permalink raw reply related [flat|nested] 23+ messages in thread
* [PATCH v2 5/6] qemu-img: Add convert --bitmaps option
2020-04-21 21:20 [PATCH v2 0/6] qemu-img: Add convert --bitmaps Eric Blake
` (3 preceding siblings ...)
2020-04-21 21:20 ` [PATCH v2 4/6] qcow2: Expose bitmaps' size during measure Eric Blake
@ 2020-04-21 21:20 ` Eric Blake
2020-05-04 12:14 ` Max Reitz
2020-04-21 21:20 ` [PATCH v2 6/6] iotests: Add test 291 to for qemu-img bitmap coverage Eric Blake
2020-04-21 22:22 ` [PATCH v2 0/6] qemu-img: Add convert --bitmaps no-reply
6 siblings, 1 reply; 23+ messages in thread
From: Eric Blake @ 2020-04-21 21:20 UTC (permalink / raw)
To: qemu-devel; +Cc: nsoffer, kwolf, jsnow, qemu-block, Max Reitz
Make it easier to copy all the persistent bitmaps of a source image
along with the contents, by adding a boolean flag for use with
qemu-img convert. This is basically shorthand, as the same effect
could be accomplished with a series of 'qemu-img bitmap --add' and
'qemu-img bitmap --merge -b source' commands, or by QMP commands.
See also https://bugzilla.redhat.com/show_bug.cgi?id=1779893
Signed-off-by: Eric Blake <eblake@redhat.com>
---
docs/tools/qemu-img.rst | 6 +++-
qemu-img.c | 80 +++++++++++++++++++++++++++++++++++++++--
qemu-img-cmds.hx | 4 +--
3 files changed, 84 insertions(+), 6 deletions(-)
diff --git a/docs/tools/qemu-img.rst b/docs/tools/qemu-img.rst
index 4f3b0e2c9ace..430fb5b46e43 100644
--- a/docs/tools/qemu-img.rst
+++ b/docs/tools/qemu-img.rst
@@ -162,6 +162,10 @@ Parameters to convert subcommand:
.. program:: qemu-img-convert
+.. option:: --bitmaps
+
+ Additionally copy all bitmaps
+
.. option:: -n
Skip the creation of the target volume
@@ -397,7 +401,7 @@ Command description:
4
Error on reading data
-.. option:: convert [--object OBJECTDEF] [--image-opts] [--target-image-opts] [--target-is-zero] [-U] [-C] [-c] [-p] [-q] [-n] [-f FMT] [-t CACHE] [-T SRC_CACHE] [-O OUTPUT_FMT] [-B BACKING_FILE] [-o OPTIONS] [-l SNAPSHOT_PARAM] [-S SPARSE_SIZE] [-m NUM_COROUTINES] [-W] FILENAME [FILENAME2 [...]] OUTPUT_FILENAME
+.. option:: convert [--object OBJECTDEF] [--image-opts] [--target-image-opts] [--target-is-zero] [--bitmaps] [-U] [-C] [-c] [-p] [-q] [-n] [-f FMT] [-t CACHE] [-T SRC_CACHE] [-O OUTPUT_FMT] [-B BACKING_FILE] [-o OPTIONS] [-l SNAPSHOT_PARAM] [-S SPARSE_SIZE] [-m NUM_COROUTINES] [-W] FILENAME [FILENAME2 [...]] OUTPUT_FILENAME
Convert the disk image *FILENAME* or a snapshot *SNAPSHOT_PARAM*
to disk image *OUTPUT_FILENAME* using format *OUTPUT_FMT*. It can
diff --git a/qemu-img.c b/qemu-img.c
index e1127273f21e..6cfc1f52ef98 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -78,6 +78,7 @@ enum {
OPTION_ENABLE = 272,
OPTION_DISABLE = 273,
OPTION_MERGE = 274,
+ OPTION_BITMAPS = 275,
};
typedef enum OutputFormat {
@@ -183,6 +184,7 @@ static void QEMU_NORETURN help(void)
" hiding corruption that has already occurred.\n"
"\n"
"Parameters to convert subcommand:\n"
+ " '--bitmaps' copies all persistent bitmaps to destination\n"
" '-m' specifies how many coroutines work in parallel during the convert\n"
" process (defaults to 8)\n"
" '-W' allow to write to the target out of order rather than sequential\n"
@@ -2061,6 +2063,47 @@ static int convert_do_copy(ImgConvertState *s)
return s->ret;
}
+static int convert_copy_bitmaps(BlockDriverState *src, BlockDriverState *dst)
+{
+ BdrvDirtyBitmap *bm;
+ Error *err = NULL;
+ BlockDirtyBitmapMergeSource *merge;
+ BlockDirtyBitmapMergeSourceList *list;
+
+ FOR_EACH_DIRTY_BITMAP(src, bm) {
+ const char *name;
+
+ if (!bdrv_dirty_bitmap_get_persistence(bm)) {
+ continue;
+ }
+ name = bdrv_dirty_bitmap_name(bm);
+ qmp_block_dirty_bitmap_add(dst->node_name, name,
+ true, bdrv_dirty_bitmap_granularity(bm),
+ true, true,
+ true, !bdrv_dirty_bitmap_enabled(bm),
+ &err);
+ if (err) {
+ error_reportf_err(err, "Failed to create bitmap %s: ", name);
+ return -1;
+ }
+
+ merge = g_new0(BlockDirtyBitmapMergeSource, 1);
+ merge->type = QTYPE_QDICT;
+ merge->u.external.node = g_strdup(src->node_name);
+ merge->u.external.name = g_strdup(name);
+ list = g_new0(BlockDirtyBitmapMergeSourceList, 1);
+ list->value = merge;
+ qmp_block_dirty_bitmap_merge(dst->node_name, name, list, &err);
+ qapi_free_BlockDirtyBitmapMergeSourceList(list);
+ if (err) {
+ error_reportf_err(err, "Failed to populate bitmap %s: ", name);
+ return -1;
+ }
+ }
+
+ return 0;
+}
+
#define MAX_BUF_SECTORS 32768
static int img_convert(int argc, char **argv)
@@ -2082,6 +2125,8 @@ static int img_convert(int argc, char **argv)
int64_t ret = -EINVAL;
bool force_share = false;
bool explict_min_sparse = false;
+ bool bitmaps = false;
+ size_t nbitmaps = 0;
ImgConvertState s = (ImgConvertState) {
/* Need at least 4k of zeros for sparse detection */
@@ -2101,6 +2146,7 @@ static int img_convert(int argc, char **argv)
{"target-image-opts", no_argument, 0, OPTION_TARGET_IMAGE_OPTS},
{"salvage", no_argument, 0, OPTION_SALVAGE},
{"target-is-zero", no_argument, 0, OPTION_TARGET_IS_ZERO},
+ {"bitmaps", no_argument, 0, OPTION_BITMAPS},
{0, 0, 0, 0}
};
c = getopt_long(argc, argv, ":hf:O:B:Cco:l:S:pt:T:qnm:WU",
@@ -2232,6 +2278,9 @@ static int img_convert(int argc, char **argv)
*/
s.has_zero_init = true;
break;
+ case OPTION_BITMAPS:
+ bitmaps = true;
+ break;
}
}
@@ -2293,7 +2342,6 @@ static int img_convert(int argc, char **argv)
goto fail_getopt;
}
-
/* ret is still -EINVAL until here */
ret = bdrv_parse_cache_mode(src_cache, &src_flags, &src_writethrough);
if (ret < 0) {
@@ -2453,6 +2501,28 @@ static int img_convert(int argc, char **argv)
}
}
+ /* Determine how many bitmaps need copying */
+ if (bitmaps) {
+ BdrvDirtyBitmap *bm;
+
+ if (s.src_num > 1) {
+ error_report("Copying bitmaps only possible with single source");
+ ret = -1;
+ goto out;
+ }
+ FOR_EACH_DIRTY_BITMAP(blk_bs(s.src[0]), bm) {
+ if (bdrv_dirty_bitmap_get_persistence(bm)) {
+ nbitmaps++;
+ }
+ }
+ if (nbitmaps > 0 && drv && !drv->bdrv_co_can_store_new_dirty_bitmap) {
+ error_report("Format driver '%s' does not support bitmaps",
+ out_fmt);
+ ret = -1;
+ goto out;
+ }
+ }
+
/*
* The later open call will need any decryption secrets, and
* bdrv_create() will purge "opts", so extract them now before
@@ -2461,9 +2531,7 @@ static int img_convert(int argc, char **argv)
if (!skip_create) {
open_opts = qdict_new();
qemu_opt_foreach(opts, img_add_key_secrets, open_opts, &error_abort);
- }
- if (!skip_create) {
/* Create the new image */
ret = bdrv_create(drv, out_filename, opts, &local_err);
if (ret < 0) {
@@ -2560,6 +2628,12 @@ static int img_convert(int argc, char **argv)
}
ret = convert_do_copy(&s);
+
+ /* Now copy the bitmaps */
+ if (nbitmaps > 0 && ret == 0) {
+ ret = convert_copy_bitmaps(blk_bs(s.src[0]), out_bs);
+ }
+
out:
if (!ret) {
qemu_progress_print(100, 0);
diff --git a/qemu-img-cmds.hx b/qemu-img-cmds.hx
index bf0035e226c8..cf574792bd99 100644
--- a/qemu-img-cmds.hx
+++ b/qemu-img-cmds.hx
@@ -46,9 +46,9 @@ SRST
ERST
DEF("convert", img_convert,
- "convert [--object objectdef] [--image-opts] [--target-image-opts] [--target-is-zero] [-U] [-C] [-c] [-p] [-q] [-n] [-f fmt] [-t cache] [-T src_cache] [-O output_fmt] [-B backing_file] [-o options] [-l snapshot_param] [-S sparse_size] [-m num_coroutines] [-W] [--salvage] filename [filename2 [...]] output_filename")
+ "convert [--object objectdef] [--image-opts] [--target-image-opts] [--target-is-zero] [--bitmaps] [-U] [-C] [-c] [-p] [-q] [-n] [-f fmt] [-t cache] [-T src_cache] [-O output_fmt] [-B backing_file] [-o options] [-l snapshot_param] [-S sparse_size] [-m num_coroutines] [-W] [--salvage] filename [filename2 [...]] output_filename")
SRST
-.. option:: convert [--object OBJECTDEF] [--image-opts] [--target-image-opts] [--target-is-zero] [-U] [-C] [-c] [-p] [-q] [-n] [-f FMT] [-t CACHE] [-T SRC_CACHE] [-O OUTPUT_FMT] [-B BACKING_FILE] [-o OPTIONS] [-l SNAPSHOT_PARAM] [-S SPARSE_SIZE] [-m NUM_COROUTINES] [-W] [--salvage] FILENAME [FILENAME2 [...]] OUTPUT_FILENAME
+.. option:: convert [--object OBJECTDEF] [--image-opts] [--target-image-opts] [--target-is-zero] [--bitmaps] [-U] [-C] [-c] [-p] [-q] [-n] [-f FMT] [-t CACHE] [-T SRC_CACHE] [-O OUTPUT_FMT] [-B BACKING_FILE] [-o OPTIONS] [-l SNAPSHOT_PARAM] [-S SPARSE_SIZE] [-m NUM_COROUTINES] [-W] [--salvage] FILENAME [FILENAME2 [...]] OUTPUT_FILENAME
ERST
DEF("create", img_create,
--
2.26.2
^ permalink raw reply related [flat|nested] 23+ messages in thread
* [PATCH v2 6/6] iotests: Add test 291 to for qemu-img bitmap coverage
2020-04-21 21:20 [PATCH v2 0/6] qemu-img: Add convert --bitmaps Eric Blake
` (4 preceding siblings ...)
2020-04-21 21:20 ` [PATCH v2 5/6] qemu-img: Add convert --bitmaps option Eric Blake
@ 2020-04-21 21:20 ` Eric Blake
2020-05-04 13:05 ` Max Reitz
2020-04-21 22:22 ` [PATCH v2 0/6] qemu-img: Add convert --bitmaps no-reply
6 siblings, 1 reply; 23+ messages in thread
From: Eric Blake @ 2020-04-21 21:20 UTC (permalink / raw)
To: qemu-devel; +Cc: nsoffer, kwolf, jsnow, qemu-block, Max Reitz
Add a new test covering the 'qemu-img bitmap' subcommand, as well as
'qemu-img convert --bitmaps', both added in recent patches.
Signed-off-by: Eric Blake <eblake@redhat.com>
---
tests/qemu-iotests/291 | 103 +++++++++++++++++++++++++++++++++++++
tests/qemu-iotests/291.out | 78 ++++++++++++++++++++++++++++
tests/qemu-iotests/group | 1 +
3 files changed, 182 insertions(+)
create mode 100755 tests/qemu-iotests/291
create mode 100644 tests/qemu-iotests/291.out
diff --git a/tests/qemu-iotests/291 b/tests/qemu-iotests/291
new file mode 100755
index 000000000000..77713c0cfea7
--- /dev/null
+++ b/tests/qemu-iotests/291
@@ -0,0 +1,103 @@
+#!/usr/bin/env bash
+#
+# Test qemu-img bitmap handling
+#
+# Copyright (C) 2018-2020 Red Hat, Inc.
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 2 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program. If not, see <http://www.gnu.org/licenses/>.
+#
+
+seq="$(basename $0)"
+echo "QA output created by $seq"
+
+status=1 # failure is the default!
+
+_cleanup()
+{
+ _cleanup_test_img
+ nbd_server_stop
+}
+trap "_cleanup; exit \$status" 0 1 2 3 15
+
+# get standard environment, filters and checks
+. ./common.rc
+. ./common.filter
+. ./common.nbd
+
+_supported_fmt qcow2
+_supported_proto file
+_supported_os Linux
+_require_command QEMU_NBD
+
+echo
+echo "=== Initial image setup ==="
+echo
+
+# Create backing image with one bitmap
+TEST_IMG="$TEST_IMG.base" _make_test_img 10M
+$QEMU_IMG bitmap --add -f $IMGFMT "$TEST_IMG.base" b0
+$QEMU_IO -c 'w 3M 1M' -f $IMGFMT "$TEST_IMG.base" | _filter_qemu_io
+# Create initial image and populate two bitmaps: one active, one inactive.
+_make_test_img -b "$TEST_IMG.base" -F $IMGFMT 10M
+$QEMU_IO -c 'w 0 1M' -f $IMGFMT "$TEST_IMG" | _filter_qemu_io
+$QEMU_IMG bitmap --add -g 512k -f $IMGFMT "$TEST_IMG" b1
+$QEMU_IMG bitmap --add --disabled -f $IMGFMT "$TEST_IMG" b2
+$QEMU_IO -c 'w 3M 1M' -f $IMGFMT "$TEST_IMG" | _filter_qemu_io
+$QEMU_IMG bitmap --clear -f $IMGFMT "$TEST_IMG" b1
+$QEMU_IO -c 'w 1M 1M' -f $IMGFMT "$TEST_IMG" | _filter_qemu_io
+$QEMU_IMG bitmap --disable -f $IMGFMT "$TEST_IMG" b1
+$QEMU_IMG bitmap --enable -f $IMGFMT "$TEST_IMG" b2
+$QEMU_IO -c 'w 2M 1M' -f $IMGFMT "$TEST_IMG" | _filter_qemu_io
+
+echo
+echo "=== Bitmap preservation not possible to non-qcow2 ==="
+echo
+
+mv "$TEST_IMG" "$TEST_IMG.orig"
+$QEMU_IMG convert --bitmaps -O raw "$TEST_IMG.orig" "$TEST_IMG"
+
+echo
+echo "=== Convert with bitmap preservation ==="
+echo
+
+# Only bitmaps from the active layer are copied
+$QEMU_IMG convert --bitmaps -O qcow2 "$TEST_IMG.orig" "$TEST_IMG"
+$QEMU_IMG info "$TEST_IMG" | _filter_img_info --format-specific
+# But we can also merge in bitmaps from other layers
+$QEMU_IMG bitmap --add --disabled -f $IMGFMT "$TEST_IMG" b0
+$QEMU_IMG bitmap --add -f $IMGFMT "$TEST_IMG" tmp
+$QEMU_IMG bitmap --merge b0 -b "$TEST_IMG.base" -F $IMGFMT "$TEST_IMG" tmp
+$QEMU_IMG bitmap --merge tmp "$TEST_IMG" b0
+$QEMU_IMG bitmap --remove -f $IMGFMT "$TEST_IMG" tmp
+$QEMU_IMG info "$TEST_IMG" | _filter_img_info --format-specific
+
+echo
+echo "=== Check bitmap contents ==="
+echo
+
+IMG="driver=nbd,server.type=unix,server.path=$nbd_unix_socket"
+nbd_server_start_unix_socket -r -f qcow2 -B b0 "$TEST_IMG"
+$QEMU_IMG map --output=json --image-opts \
+ "$IMG,x-dirty-bitmap=qemu:dirty-bitmap:b0" | _filter_qemu_img_map
+nbd_server_start_unix_socket -r -f qcow2 -B b1 "$TEST_IMG"
+$QEMU_IMG map --output=json --image-opts \
+ "$IMG,x-dirty-bitmap=qemu:dirty-bitmap:b1" | _filter_qemu_img_map
+nbd_server_start_unix_socket -r -f qcow2 -B b2 "$TEST_IMG"
+$QEMU_IMG map --output=json --image-opts \
+ "$IMG,x-dirty-bitmap=qemu:dirty-bitmap:b2" | _filter_qemu_img_map
+
+# success, all done
+echo '*** done'
+rm -f $seq.full
+status=0
diff --git a/tests/qemu-iotests/291.out b/tests/qemu-iotests/291.out
new file mode 100644
index 000000000000..d716c0c7cc0b
--- /dev/null
+++ b/tests/qemu-iotests/291.out
@@ -0,0 +1,78 @@
+QA output created by 291
+
+=== Initial image setup ===
+
+Formatting 'TEST_DIR/t.IMGFMT.base', fmt=IMGFMT size=10485760
+wrote 1048576/1048576 bytes at offset 3145728
+1 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=10485760 backing_file=TEST_DIR/t.IMGFMT.base backing_fmt=IMGFMT
+wrote 1048576/1048576 bytes at offset 0
+1 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+wrote 1048576/1048576 bytes at offset 3145728
+1 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+wrote 1048576/1048576 bytes at offset 1048576
+1 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+wrote 1048576/1048576 bytes at offset 2097152
+1 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+
+=== Bitmap preservation not possible to non-qcow2 ===
+
+qemu-img: Format driver 'raw' does not support bitmaps
+
+=== Convert with bitmap preservation ===
+
+image: TEST_DIR/t.IMGFMT
+file format: IMGFMT
+virtual size: 10 MiB (10485760 bytes)
+disk size: 4.39 MiB
+Format specific information:
+ compat: 1.1
+ lazy refcounts: false
+ bitmaps:
+ [0]:
+ flags:
+ name: b1
+ granularity: 524288
+ [1]:
+ flags:
+ [0]: auto
+ name: b2
+ granularity: 65536
+ refcount bits: 16
+ corrupt: false
+image: TEST_DIR/t.IMGFMT
+file format: IMGFMT
+virtual size: 10 MiB (10485760 bytes)
+disk size: 4.49 MiB
+Format specific information:
+ compat: 1.1
+ lazy refcounts: false
+ bitmaps:
+ [0]:
+ flags:
+ name: b1
+ granularity: 524288
+ [1]:
+ flags:
+ [0]: auto
+ name: b2
+ granularity: 65536
+ [2]:
+ flags:
+ name: b0
+ granularity: 65536
+ refcount bits: 16
+ corrupt: false
+
+=== Check bitmap contents ===
+
+[{ "start": 0, "length": 3145728, "depth": 0, "zero": false, "data": true, "offset": OFFSET},
+{ "start": 3145728, "length": 1048576, "depth": 0, "zero": false, "data": false},
+{ "start": 4194304, "length": 6291456, "depth": 0, "zero": false, "data": true, "offset": OFFSET}]
+[{ "start": 0, "length": 1048576, "depth": 0, "zero": false, "data": true, "offset": OFFSET},
+{ "start": 1048576, "length": 1048576, "depth": 0, "zero": false, "data": false},
+{ "start": 2097152, "length": 8388608, "depth": 0, "zero": false, "data": true, "offset": OFFSET}]
+[{ "start": 0, "length": 2097152, "depth": 0, "zero": false, "data": true, "offset": OFFSET},
+{ "start": 2097152, "length": 1048576, "depth": 0, "zero": false, "data": false},
+{ "start": 3145728, "length": 7340032, "depth": 0, "zero": false, "data": true, "offset": OFFSET}]
+*** done
diff --git a/tests/qemu-iotests/group b/tests/qemu-iotests/group
index 435dccd5af90..8e9b9513a091 100644
--- a/tests/qemu-iotests/group
+++ b/tests/qemu-iotests/group
@@ -297,3 +297,4 @@
288 quick
289 rw quick
290 rw auto quick
+291 rw quick
--
2.26.2
^ permalink raw reply related [flat|nested] 23+ messages in thread
* Re: [PATCH v2 0/6] qemu-img: Add convert --bitmaps
2020-04-21 21:20 [PATCH v2 0/6] qemu-img: Add convert --bitmaps Eric Blake
` (5 preceding siblings ...)
2020-04-21 21:20 ` [PATCH v2 6/6] iotests: Add test 291 to for qemu-img bitmap coverage Eric Blake
@ 2020-04-21 22:22 ` no-reply
2020-04-21 22:49 ` [PATCH] fixup! qemu-img: Add bitmap sub-command Eric Blake
6 siblings, 1 reply; 23+ messages in thread
From: no-reply @ 2020-04-21 22:22 UTC (permalink / raw)
To: eblake; +Cc: nsoffer, kwolf, jsnow, qemu-devel, qemu-block
Patchew URL: https://patchew.org/QEMU/20200421212019.170707-1-eblake@redhat.com/
Hi,
This series failed the docker-mingw@fedora build test. Please find the testing commands and
their output below. If you have Docker installed, you can probably reproduce it
locally.
=== TEST SCRIPT BEGIN ===
#! /bin/bash
export ARCH=x86_64
make docker-image-fedora V=1 NETWORK=1
time make docker-test-mingw@fedora J=14 NETWORK=1
=== TEST SCRIPT END ===
LINK qemu-edid.exe
LINK qemu-ga.exe
/tmp/qemu-test/src/qemu-img.c: In function 'img_bitmap':
/tmp/qemu-test/src/qemu-img.c:4579:44: error: passing argument 3 of 'qemu_strtosz' from incompatible pointer type [-Werror=incompatible-pointer-types]
if (qemu_strtosz(optarg, NULL, &granularity)) {
^~~~~~~~~~~~
In file included from /tmp/qemu-test/src/qemu-img.c:37:
---
int qemu_strtosz(const char *nptr, const char **end, uint64_t *result);
~~~~~~~~~~^~~~~~
cc1: all warnings being treated as errors
make: *** [/tmp/qemu-test/src/rules.mak:69: qemu-img.o] Error 1
make: *** Waiting for unfinished jobs....
Traceback (most recent call last):
File "./tests/docker/docker.py", line 664, in <module>
---
raise CalledProcessError(retcode, cmd)
subprocess.CalledProcessError: Command '['sudo', '-n', 'docker', 'run', '--label', 'com.qemu.instance.uuid=e06abfa47fc2454097264ac821c5dceb', '-u', '1001', '--security-opt', 'seccomp=unconfined', '--rm', '-e', 'TARGET_LIST=', '-e', 'EXTRA_CONFIGURE_OPTS=', '-e', 'V=', '-e', 'J=14', '-e', 'DEBUG=', '-e', 'SHOW_ENV=', '-e', 'CCACHE_DIR=/var/tmp/ccache', '-v', '/home/patchew/.cache/qemu-docker-ccache:/var/tmp/ccache:z', '-v', '/var/tmp/patchew-tester-tmp-d9esijcl/src/docker-src.2020-04-21-18.18.59.19445:/var/tmp/qemu:z,ro', 'qemu:fedora', '/var/tmp/qemu/run', 'test-mingw']' returned non-zero exit status 2.
filter=--filter=label=com.qemu.instance.uuid=e06abfa47fc2454097264ac821c5dceb
make[1]: *** [docker-run] Error 1
make[1]: Leaving directory `/var/tmp/patchew-tester-tmp-d9esijcl/src'
make: *** [docker-run-test-mingw@fedora] Error 2
real 3m19.767s
user 0m8.957s
The full log is available at
http://patchew.org/logs/20200421212019.170707-1-eblake@redhat.com/testing.docker-mingw@fedora/?type=message.
---
Email generated automatically by Patchew [https://patchew.org/].
Please send your feedback to patchew-devel@redhat.com
^ permalink raw reply [flat|nested] 23+ messages in thread
* [PATCH] fixup! qemu-img: Add bitmap sub-command
2020-04-21 22:22 ` [PATCH v2 0/6] qemu-img: Add convert --bitmaps no-reply
@ 2020-04-21 22:49 ` Eric Blake
0 siblings, 0 replies; 23+ messages in thread
From: Eric Blake @ 2020-04-21 22:49 UTC (permalink / raw)
To: qemu-devel; +Cc: nsoffer, kwolf, jsnow, qemu-block, Max Reitz
Signed-off-by: Eric Blake <eblake@redhat.com>
---
Squash this into patch 3/6 to fix docker-test-mingw@fedora
qemu-img.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/qemu-img.c b/qemu-img.c
index 6cfc1f52ef98..cc87eaf12778 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -4529,7 +4529,7 @@ static int img_bitmap(int argc, char **argv)
BlockBackend *blk = NULL, *src = NULL;
BlockDriverState *bs = NULL, *src_bs = NULL;
bool image_opts = false;
- unsigned long granularity = 0;
+ uint64_t granularity = 0;
bool add = false, remove = false, clear = false;
bool enable = false, disable = false, add_disabled = false;
const char *merge = NULL;
--
2.26.2
^ permalink raw reply related [flat|nested] 23+ messages in thread
* Re: [PATCH v2 1/6] docs: Sort sections on qemu-img subcommand parameters
2020-04-21 21:20 ` [PATCH v2 1/6] docs: Sort sections on qemu-img subcommand parameters Eric Blake
@ 2020-04-30 12:50 ` Max Reitz
0 siblings, 0 replies; 23+ messages in thread
From: Max Reitz @ 2020-04-30 12:50 UTC (permalink / raw)
To: Eric Blake, qemu-devel; +Cc: kwolf, qemu-block
[-- Attachment #1.1: Type: text/plain, Size: 430 bytes --]
On 21.04.20 23:20, Eric Blake wrote:
> We already list the subcommand summaries alphabetically, we should do
> the same for the documentation related to subcommand-specific
> parameters.
>
> Signed-off-by: Eric Blake <eblake@redhat.com>
> ---
> docs/tools/qemu-img.rst | 48 ++++++++++++++++++++---------------------
> 1 file changed, 24 insertions(+), 24 deletions(-)
Reviewed-by: Max Reitz <mreitz@redhat.com>
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v2 2/6] blockdev: Split off basic bitmap operations for qemu-img
2020-04-21 21:20 ` [PATCH v2 2/6] blockdev: Split off basic bitmap operations for qemu-img Eric Blake
@ 2020-04-30 13:59 ` Max Reitz
2020-04-30 14:50 ` Eric Blake
0 siblings, 1 reply; 23+ messages in thread
From: Max Reitz @ 2020-04-30 13:59 UTC (permalink / raw)
To: Eric Blake, qemu-devel; +Cc: kwolf, Markus Armbruster, qemu-block
[-- Attachment #1.1: Type: text/plain, Size: 3152 bytes --]
On 21.04.20 23:20, Eric Blake wrote:
> Upcoming patches want to add some basic bitmap manipulation abilities
> to qemu-img. But blockdev.o is too heavyweight to link into qemu-img
> (among other things, it would drag in block jobs and transaction
> support - qemu-img does offline manipulation, where atomicity is less
> important because there are no concurrent modifications to compete
> with), so it's time to split off the bare bones of what we will need
> into a new file blockbitmaps.o.
>
> In addition to exposing 6 QMP commands for use by qemu-img (add,
> remove, clear, enable, disable, merge), this also has to export three
> previously-static functions for use by blockdev.c transactions.
>
> Signed-off-by: Eric Blake <eblake@redhat.com>
> ---
> Makefile.objs | 2 +-
> include/sysemu/blockdev.h | 14 ++
> blockbitmaps.c | 324 ++++++++++++++++++++++++++++++++++++++
Hm. Can we get a better name? blockdev-bitmaps.c, for example?
> blockdev.c | 293 ----------------------------------
> MAINTAINERS | 1 +
> 5 files changed, 340 insertions(+), 294 deletions(-)
> create mode 100644 blockbitmaps.c
[...]
> diff --git a/include/sysemu/blockdev.h b/include/sysemu/blockdev.h
> index a86d99b3d875..523b7493b1cd 100644
> --- a/include/sysemu/blockdev.h
> +++ b/include/sysemu/blockdev.h
> @@ -57,4 +57,18 @@ QemuOpts *drive_add(BlockInterfaceType type, int index, const char *file,
> DriveInfo *drive_new(QemuOpts *arg, BlockInterfaceType block_default_type,
> Error **errp);
>
> +BdrvDirtyBitmap *block_dirty_bitmap_lookup(const char *node,
> + const char *name,
> + BlockDriverState **pbs,
> + Error **errp);
> +BdrvDirtyBitmap *do_block_dirty_bitmap_remove(const char *node,
> + const char *name, bool release,
> + BlockDriverState **bitmap_bs,
> + Error **errp);
> +BdrvDirtyBitmap *do_block_dirty_bitmap_merge(const char *node,
> + const char *target,
> + BlockDirtyBitmapMergeSourceList *bitmaps,
> + HBitmap **backup, Error **errp);
Putting do_* functions into a normal header seems a bit weird. Would
these fit better into block_int.h?
> +
> #endif
[...]
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 8cbc1fac2bfc..769cd357d281 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -1989,6 +1989,7 @@ T: git https://github.com/jnsnow/qemu.git jobs
> Block QAPI, monitor, command line
> M: Markus Armbruster <armbru@redhat.com>
> S: Supported
> +F: blockbitmaps.c
The natural choice for something split off of blockdev.c, but I wonder
if the Dirty Bitmaps section wouldn’t be a better fit.
Max
> F: blockdev.c
> F: blockdev-hmp-cmds.c
> F: block/qapi.c
>
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v2 2/6] blockdev: Split off basic bitmap operations for qemu-img
2020-04-30 13:59 ` Max Reitz
@ 2020-04-30 14:50 ` Eric Blake
2020-05-08 11:37 ` Kevin Wolf
0 siblings, 1 reply; 23+ messages in thread
From: Eric Blake @ 2020-04-30 14:50 UTC (permalink / raw)
To: Max Reitz, qemu-devel; +Cc: kwolf, Markus Armbruster, qemu-block
On 4/30/20 8:59 AM, Max Reitz wrote:
> On 21.04.20 23:20, Eric Blake wrote:
>> Upcoming patches want to add some basic bitmap manipulation abilities
>> to qemu-img. But blockdev.o is too heavyweight to link into qemu-img
>> (among other things, it would drag in block jobs and transaction
>> support - qemu-img does offline manipulation, where atomicity is less
>> important because there are no concurrent modifications to compete
>> with), so it's time to split off the bare bones of what we will need
>> into a new file blockbitmaps.o.
>>
>> In addition to exposing 6 QMP commands for use by qemu-img (add,
>> remove, clear, enable, disable, merge), this also has to export three
>> previously-static functions for use by blockdev.c transactions.
>>
>> Signed-off-by: Eric Blake <eblake@redhat.com>
>> ---
>> Makefile.objs | 2 +-
>> include/sysemu/blockdev.h | 14 ++
>> blockbitmaps.c | 324 ++++++++++++++++++++++++++++++++++++++
>
> Hm. Can we get a better name? blockdev-bitmaps.c, for example?
Sure, I'm open to bike-shed suggestions. I'd also _really_ love to make
the new file NOT live in the top-level, but that's a harder task that
I'm not sure how to do (it's easy to tweak Makefile.objs for another
file in the same directory, but harder to see through the magic to
figure out how to relocate things).
>
>> blockdev.c | 293 ----------------------------------
>> MAINTAINERS | 1 +
>> 5 files changed, 340 insertions(+), 294 deletions(-)
>> create mode 100644 blockbitmaps.c
>
> [...]
>
>> diff --git a/include/sysemu/blockdev.h b/include/sysemu/blockdev.h
>> index a86d99b3d875..523b7493b1cd 100644
>> --- a/include/sysemu/blockdev.h
>> +++ b/include/sysemu/blockdev.h
>> @@ -57,4 +57,18 @@ QemuOpts *drive_add(BlockInterfaceType type, int index, const char *file,
>> DriveInfo *drive_new(QemuOpts *arg, BlockInterfaceType block_default_type,
>> Error **errp);
>>
>> +BdrvDirtyBitmap *block_dirty_bitmap_lookup(const char *node,
>> + const char *name,
>> + BlockDriverState **pbs,
>> + Error **errp);
>> +BdrvDirtyBitmap *do_block_dirty_bitmap_remove(const char *node,
>> + const char *name, bool release,
>> + BlockDriverState **bitmap_bs,
>> + Error **errp);
>> +BdrvDirtyBitmap *do_block_dirty_bitmap_merge(const char *node,
>> + const char *target,
>> + BlockDirtyBitmapMergeSourceList *bitmaps,
>> + HBitmap **backup, Error **errp);
>
> Putting do_* functions into a normal header seems a bit weird. Would
> these fit better into block_int.h?
I don't care which header gets them, as long as things compile.
block_int.h seems reasonable, so I can try that. I also wonder if I
should rename them at all (at which point, maybe I split this into two
patches - one doing the rename, the other doing the file motion).
>> +++ b/MAINTAINERS
>> @@ -1989,6 +1989,7 @@ T: git https://github.com/jnsnow/qemu.git jobs
>> Block QAPI, monitor, command line
>> M: Markus Armbruster <armbru@redhat.com>
>> S: Supported
>> +F: blockbitmaps.c
>
> The natural choice for something split off of blockdev.c, but I wonder
> if the Dirty Bitmaps section wouldn’t be a better fit.
Or maybe even both?
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v2 3/6] qemu-img: Add bitmap sub-command
2020-04-21 21:20 ` [PATCH v2 3/6] qemu-img: Add bitmap sub-command Eric Blake
@ 2020-04-30 14:55 ` Max Reitz
2020-04-30 15:21 ` Eric Blake
0 siblings, 1 reply; 23+ messages in thread
From: Max Reitz @ 2020-04-30 14:55 UTC (permalink / raw)
To: Eric Blake, qemu-devel; +Cc: kwolf, qemu-block
[-- Attachment #1.1: Type: text/plain, Size: 13345 bytes --]
On 21.04.20 23:20, Eric Blake wrote:
> Include actions for --add, --remove, --clear, --enable, --disable, and
> --merge (note that --clear is a bit of fluff, because the same can be
> accomplished by removing a bitmap and then adding a new one in its
> place,
Well, ideally, all of qemu-img is just fluff because “the same can be
accomplished by launching qemu and issuing the equivalent QMP commands”. :)
> but it matches what QMP commands exist). Listing is omitted,
> because it does not require a bitmap name and because it was already
> possible with 'qemu-img info'.
Fair enough, although it can be said that qemu-img info’s output is
qcow2-specific. It might be nice to have some definitely
format-independent output. (But we don’t have persistent bitmaps in
anything but qcow2 yet (or do we in NBD?), so I don’t expect anyone to
care much.)
> Merge can work either from another
> bitmap in the same image, or from a bitmap in a distinct image.
>
> While this supports --image-opts for the file being modified, I did
> not think it worth the extra complexity to support that for the source
> file in a cross-file bitmap merge. Likewise, I chose to have --merge
> only take a single source rather than following the QMP support for
> multiple merges in one go; in part to simplify the command line, and
> in part because an offline image can achieve the same effect by
> multiple qemu-img bitmap --merge calls. We can enhance that if needed
> in the future (the same way that 'qemu-img convert' has a mode that
> concatenates multiple sources into one destination).
>
> Upcoming patches will add iotest coverage of these commands while
> also testing other features.
>
> Signed-off-by: Eric Blake <eblake@redhat.com>
> ---
> docs/tools/qemu-img.rst | 24 +++++
> qemu-img.c | 198 ++++++++++++++++++++++++++++++++++++++++
> qemu-img-cmds.hx | 7 ++
> 3 files changed, 229 insertions(+)
>
> diff --git a/docs/tools/qemu-img.rst b/docs/tools/qemu-img.rst
> index 7d08c48d308f..4f3b0e2c9ace 100644
> --- a/docs/tools/qemu-img.rst
> +++ b/docs/tools/qemu-img.rst
> @@ -281,6 +281,30 @@ Command description:
> For write tests, by default a buffer filled with zeros is written. This can be
> overridden with a pattern byte specified by *PATTERN*.
>
> +.. option:: bitmap {--add [-g GRANULARITY] [--disabled] | --remove | --clear | --enable | --disable | --merge SOURCE_BITMAP [-b SOURCE_FILE [-F SOURCE_FMT]]} [--object OBJECTDEF] [--image-opts] [-f FMT] FILENAME BITMAP
So I can do multiple operations in one roll, but they all use the same
BITMAP? Sounds a bit weird. It actually took me a while to understands
this, because I thought for sure that each command would take a bitmap
name. (And was ready to complain that it looked like they don’t, but,
well, that’s because they don’t.)
Although I suppose some practical example like
$ qemu-img bitmap --add --merge sbmap --disable foo.qcow2 nbmap
does make sense.[1]
Would
$ qemu-img bitmap --add nbmap --merge nbmap sbmap --enable nbmap \
foo.qcow2
make more sense? Doesn’t really look like it, because at that point you
just have to ask why not just call qemu-img bitmap multiple times.
*shrug*
[1] However, that leads me to ask: Why does --add need a --disabled
option? Isn’t that equivalent to --add --disable?
> +
> + Perform a modification of the persistent bitmap *BITMAP* in the disk
> + image *FILENAME*. The various modifications are:
> +
> + ``--add`` to create *BITMAP*, with additional options ``-g`` to
> + specify a non-default *GRANULARITY*, or whether the bitmap should be
> + ``--disabled`` instead of enabled.
> +
> + ``--remove`` to remove *BITMAP*.
> +
> + ``--clear`` to clear *BITMAP*.
> +
> + ``--enable`` to change *BITMAP* to start recording future edits.
> +
> + ``--disable`` to change *BITMAP* to stop recording future edits.
> +
> + ``--merge`` to merge the contents of *SOURCE_BITMAP* into *BITMAP*.
> + This defaults to requiring a source bitmap from the same *FILENAME*,
> + but can also be used for cross-image merge by supplying ``-b`` to
> + specify a different *SOURCE_FILE*.
> +
> + To see what bitmaps are present in an image, use ``qemu-img info``.
> +
> .. option:: check [--object OBJECTDEF] [--image-opts] [-q] [-f FMT] [--output=OFMT] [-r [leaks | all]] [-T SRC_CACHE] [-U] FILENAME
>
> Perform a consistency check on the disk image *FILENAME*. The command can
> diff --git a/qemu-img.c b/qemu-img.c
> index 821cbf610e5f..02ebd870faa1 100644
> --- a/qemu-img.c
> +++ b/qemu-img.c
> @@ -28,6 +28,7 @@
> #include "qemu-common.h"
> #include "qemu-version.h"
> #include "qapi/error.h"
> +#include "qapi/qapi-commands-block-core.h"
> #include "qapi/qapi-visit-block-core.h"
> #include "qapi/qobject-output-visitor.h"
> #include "qapi/qmp/qjson.h"
> @@ -71,6 +72,12 @@ enum {
> OPTION_SHRINK = 266,
> OPTION_SALVAGE = 267,
> OPTION_TARGET_IS_ZERO = 268,
> + OPTION_ADD = 269,
> + OPTION_REMOVE = 270,
> + OPTION_CLEAR = 271,
> + OPTION_ENABLE = 272,
> + OPTION_DISABLE = 273,
> + OPTION_MERGE = 274,
> };
>
> typedef enum OutputFormat {
> @@ -4438,6 +4445,197 @@ out:
> return 0;
> }
>
> +static int img_bitmap(int argc, char **argv)
> +{
> + Error *err = NULL;
> + int c, ret = -1;
> + QemuOpts *opts = NULL;
> + const char *fmt = NULL, *src_fmt = NULL, *src_filename = NULL;
> + const char *filename, *bitmap;
> + BlockBackend *blk = NULL, *src = NULL;
> + BlockDriverState *bs = NULL, *src_bs = NULL;
> + bool image_opts = false;
> + unsigned long granularity = 0;
> + bool add = false, remove = false, clear = false;
> + bool enable = false, disable = false, add_disabled = false;
> + const char *merge = NULL;
So actually we can’t do one operation multiple times in one roll.
> +
> + for (;;) {
> + static const struct option long_options[] = {
> + {"help", no_argument, 0, 'h'},
> + {"object", required_argument, 0, OPTION_OBJECT},
> + {"image-opts", no_argument, 0, OPTION_IMAGE_OPTS},
> + {"add", no_argument, 0, OPTION_ADD},
> + {"remove", no_argument, 0, OPTION_REMOVE},
> + {"clear", no_argument, 0, OPTION_CLEAR},
> + {"enable", no_argument, 0, OPTION_ENABLE},
> + {"disable", no_argument, 0, OPTION_DISABLE},
> + {"disabled", no_argument, 0, OPTION_DISABLE},
So if --disable and --disabled are exactly the same, I really don’t know
why --disabled even exists.
> + {"merge", required_argument, 0, OPTION_MERGE},
> + {"granularity", required_argument, 0, 'g'},
> + {"source-file", required_argument, 0, 'b'},
> + {"source-format", required_argument, 0, 'F'},
> + {0, 0, 0, 0}
> + };
> + c = getopt_long(argc, argv, ":b:f:F:g:h", long_options, NULL);
> + if (c == -1) {
> + break;
> + }
> +
> + switch (c) {
> + case ':':
> + missing_argument(argv[optind - 1]);
> + break;
> + case '?':
> + unrecognized_option(argv[optind - 1]);
> + break;
> + case 'h':
> + help();
> + break;
> + case 'b':
> + src_filename = optarg;
> + break;
> + case 'f':
> + fmt = optarg;
> + break;
> + case 'F':
> + src_fmt = optarg;
> + break;
> + case 'g':
> + if (qemu_strtosz(optarg, NULL, &granularity)) {
> + error_report("Invalid granularity specified");
> + return 1;
> + }
> + break;
> + case OPTION_ADD:
> + add = true;
> + break;
> + case OPTION_REMOVE:
> + remove = true;
> + break;
> + case OPTION_CLEAR:
> + clear = true;
> + break;
> + case OPTION_ENABLE:
> + enable = true;
> + break;
> + case OPTION_DISABLE:
> + disable = true;
> + break;
> + case OPTION_MERGE:
> + merge = optarg;
> + break;
> + case OPTION_OBJECT:
> + opts = qemu_opts_parse_noisily(&qemu_object_opts,
> + optarg, true);
> + if (!opts) {
> + goto out;
> + }
> + break;
> + case OPTION_IMAGE_OPTS:
> + image_opts = true;
> + break;
> + }
> + }
> +
> + if (qemu_opts_foreach(&qemu_object_opts,
> + user_creatable_add_opts_foreach,
> + qemu_img_object_print_help, &error_fatal)) {
> + goto out;
> + }
> +
> + if (add && disable) {
> + disable = false;
> + add_disabled = true;
> + }
> + if (add + remove + clear + enable + disable + !!merge != 1) {
> + error_report("Need exactly one mode of --add, --remove, --clear, "
> + "--enable, --disable, or --merge");
Aha. So you can actually only do a single operation.
That means the doc shouldn’t use {}, in my opinion, because that to me
means repetition (thanks to EBNF). It definitely served to confuse me
greatly until this point.
I also don’t see why we would disallow multiple operations in one go.
The --add --merge combination seems useful to me, and I don’t see a
problem in implementing it.
I don’t know why we don’t just create a list of operations to execute,
based on the order given in the argument list, and then execute them in
order. That would even allow multiple --merge operations.
If we don’t want that (because we don’t want argument order to matter),
we could still allow all operations to be done at least once and always
execute them in the same order, e.g.:
(1) add
(2) clear
(3) merge
(4) disable
(5) enable
(6) remove
> + goto out;
> + }
> + if (granularity && !add) {
> + error_report("granularity only supported with --add");
> + goto out;
> + }
> + if (src_fmt && !src_filename) {
> + error_report("-F only supported with -b");
> + goto out;
> + }
> + if (src_filename && !merge) {
> + error_report("alternate source file only supported with --merge");
“alternate” sounds strange. Why not be more precise and call it a
“Merge bitmap source file” or “File to source merge bitmap from”?
> + goto out;
> + }
> +
> + if (optind != argc - 2) {
> + error_report("Expecting filename and bitmap name");
> + goto out;
> + }
> +
> + filename = argv[optind];
> + bitmap = argv[optind + 1];
> +
> + blk = img_open(image_opts, filename, fmt, BDRV_O_RDWR, false, false,
> + false);
> + if (!blk) {
> + goto out;
> + }
> + bs = blk_bs(blk);
> +
> + if (add) {
> + qmp_block_dirty_bitmap_add(bs->node_name, bitmap,
> + !!granularity, granularity, true, true,
> + true, add_disabled, &err);
> + } else if (remove) {
> + qmp_block_dirty_bitmap_remove(bs->node_name, bitmap, &err);
> + } else if (clear) {
> + qmp_block_dirty_bitmap_clear(bs->node_name, bitmap, &err);
> + } else if (enable) {
> + qmp_block_dirty_bitmap_enable(bs->node_name, bitmap, &err);
> + } else if (disable) {
> + qmp_block_dirty_bitmap_disable(bs->node_name, bitmap, &err);
> + } else if (merge) {
> + BlockDirtyBitmapMergeSource *merge_src;
> + BlockDirtyBitmapMergeSourceList *list;
> +
> + if (src_filename) {
> + src = img_open(NULL, src_filename, src_fmt, 0, false, false,
> + false);
> + if (!src) {
> + goto out;
> + }
> + src_bs = blk_bs(src);
> + } else {
> + src_bs = bs;
> + }
> +
> + merge_src = g_new0(BlockDirtyBitmapMergeSource, 1);
> + merge_src->type = QTYPE_QDICT;
> + merge_src->u.external.node = g_strdup(src_bs->node_name);
> + merge_src->u.external.name = g_strdup(merge);
> + list = g_new0(BlockDirtyBitmapMergeSourceList, 1);
> + list->value = merge_src;
> + qmp_block_dirty_bitmap_merge(bs->node_name, bitmap, list, &err);
> + qapi_free_BlockDirtyBitmapMergeSourceList(list);
> + }
> +
> + if (err) {
> + error_reportf_err(err, "Bitmap %s operation failed", bitmap);
Wouldn’t “Operation on bitmap %s failed” work better?
Max
> + ret = -1;
> + goto out;
> + }
> +
> + ret = 0;
> +
> + out:
> + blk_unref(src);
> + blk_unref(blk);
> + qemu_opts_del(opts);
> + if (ret) {
> + return 1;
> + }
> + return 0;
> +}
> +
> #define C_BS 01
> #define C_COUNT 02
> #define C_IF 04
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v2 3/6] qemu-img: Add bitmap sub-command
2020-04-30 14:55 ` Max Reitz
@ 2020-04-30 15:21 ` Eric Blake
2020-05-04 10:01 ` Max Reitz
0 siblings, 1 reply; 23+ messages in thread
From: Eric Blake @ 2020-04-30 15:21 UTC (permalink / raw)
To: Max Reitz, qemu-devel; +Cc: kwolf, qemu-block
On 4/30/20 9:55 AM, Max Reitz wrote:
> On 21.04.20 23:20, Eric Blake wrote:
>> Include actions for --add, --remove, --clear, --enable, --disable, and
>> --merge (note that --clear is a bit of fluff, because the same can be
>> accomplished by removing a bitmap and then adding a new one in its
>> place,
>
> Well, ideally, all of qemu-img is just fluff because “the same can be
> accomplished by launching qemu and issuing the equivalent QMP commands”. :)
>
>> but it matches what QMP commands exist). Listing is omitted,
>> because it does not require a bitmap name and because it was already
>> possible with 'qemu-img info'.
>
> Fair enough, although it can be said that qemu-img info’s output is
> qcow2-specific. It might be nice to have some definitely
> format-independent output. (But we don’t have persistent bitmaps in
> anything but qcow2 yet (or do we in NBD?), so I don’t expect anyone to
> care much.)
We can add a list subcommand later if it is still desired. I agree that
a tabular format:
name enabled granularity
bitmap1 false 65536
bitmap2 true 512
in isolation is easier to read than:
bitmaps:
[0]:
flags:
name: bitmap1
granularity: 65536
[1]:
flags:
[0]: auto
name: bitmap2
granularity: 512
embedded inside even more information.
>
>> Merge can work either from another
>> bitmap in the same image, or from a bitmap in a distinct image.
>>
>> While this supports --image-opts for the file being modified, I did
>> not think it worth the extra complexity to support that for the source
>> file in a cross-file bitmap merge. Likewise, I chose to have --merge
>> only take a single source rather than following the QMP support for
>> multiple merges in one go; in part to simplify the command line, and
>> in part because an offline image can achieve the same effect by
>> multiple qemu-img bitmap --merge calls. We can enhance that if needed
>> in the future (the same way that 'qemu-img convert' has a mode that
>> concatenates multiple sources into one destination).
>>
>> Upcoming patches will add iotest coverage of these commands while
>> also testing other features.
>>
>> Signed-off-by: Eric Blake <eblake@redhat.com>
>> ---
>> docs/tools/qemu-img.rst | 24 +++++
>> qemu-img.c | 198 ++++++++++++++++++++++++++++++++++++++++
>> qemu-img-cmds.hx | 7 ++
>> 3 files changed, 229 insertions(+)
>>
>> diff --git a/docs/tools/qemu-img.rst b/docs/tools/qemu-img.rst
>> index 7d08c48d308f..4f3b0e2c9ace 100644
>> --- a/docs/tools/qemu-img.rst
>> +++ b/docs/tools/qemu-img.rst
>> @@ -281,6 +281,30 @@ Command description:
>> For write tests, by default a buffer filled with zeros is written. This can be
>> overridden with a pattern byte specified by *PATTERN*.
>>
>> +.. option:: bitmap {--add [-g GRANULARITY] [--disabled] | --remove | --clear | --enable | --disable | --merge SOURCE_BITMAP [-b SOURCE_FILE [-F SOURCE_FMT]]} [--object OBJECTDEF] [--image-opts] [-f FMT] FILENAME BITMAP
>
> So I can do multiple operations in one roll, but they all use the same
> BITMAP? Sounds a bit weird. It actually took me a while to understands
> this, because I thought for sure that each command would take a bitmap
> name. (And was ready to complain that it looked like they don’t, but,
> well, that’s because they don’t.)
All of the operations take one bitmap name (the final BITMAP).
Additionally, the --merge operation takes a second bitmap name
(SOURCE_BITMAP). None of the other operations need a second bitmap
name, so only --merge requires an option argument. As written, the { a
| b | c } implies that operations are mutually exclusive: you can only
request one operation per qemu-img invocation.
>
> Although I suppose some practical example like
>
> $ qemu-img bitmap --add --merge sbmap --disable foo.qcow2 nbmap
>
> does make sense.[1]
>
>
> Would
>
> $ qemu-img bitmap --add nbmap --merge nbmap sbmap --enable nbmap \
> foo.qcow2
>
> make more sense?
That would be more transactional, and more effort to implement. My
argument is that you should instead write:
$ qemu-img bitmap --add foo.qcow2 nbmap
$ qemu-img bitmap --merge sbmap foo.qcow2 nbmap
$ qemu-img bitmap --enable foo.qcow2 nbmap
where I only have to implement one operation per qemu-img.
> Doesn’t really look like it, because at that point you
> just have to ask why not just call qemu-img bitmap multiple times.
>
> *shrug*
>
>
> [1] However, that leads me to ask: Why does --add need a --disabled
> option? Isn’t that equivalent to --add --disable?
The QMP command for add has an optional 'disabled' parameter, which I
exposed here. Alternatively, we could indeed NOT expose that parameter
through qemu-img, but require two separate qemu-img bitmap commands to
add and then disable things. Atomicity is not a concern here like it
was in QMP. Removing that sugar does simplify things slightly.
>> +static int img_bitmap(int argc, char **argv)
>> +{
>> + Error *err = NULL;
>> + int c, ret = -1;
>> + QemuOpts *opts = NULL;
>> + const char *fmt = NULL, *src_fmt = NULL, *src_filename = NULL;
>> + const char *filename, *bitmap;
>> + BlockBackend *blk = NULL, *src = NULL;
>> + BlockDriverState *bs = NULL, *src_bs = NULL;
>> + bool image_opts = false;
>> + unsigned long granularity = 0;
>> + bool add = false, remove = false, clear = false;
>> + bool enable = false, disable = false, add_disabled = false;
>> + const char *merge = NULL;
>
> So actually we can’t do one operation multiple times in one roll.
Correct. At least, not as I wrote it.
>
>> +
>> + for (;;) {
>> + static const struct option long_options[] = {
>> + {"help", no_argument, 0, 'h'},
>> + {"object", required_argument, 0, OPTION_OBJECT},
>> + {"image-opts", no_argument, 0, OPTION_IMAGE_OPTS},
>> + {"add", no_argument, 0, OPTION_ADD},
>> + {"remove", no_argument, 0, OPTION_REMOVE},
>> + {"clear", no_argument, 0, OPTION_CLEAR},
>> + {"enable", no_argument, 0, OPTION_ENABLE},
>> + {"disable", no_argument, 0, OPTION_DISABLE},
>> + {"disabled", no_argument, 0, OPTION_DISABLE},
>
> So if --disable and --disabled are exactly the same, I really don’t know
> why --disabled even exists.
Logically, '--add --disabled' matches the name of the QMP command with
its optional 'disabled' parameter, while --disable matches the name of
the QMP command. We don't have to have the alias; and in fact, if you
agree that supporting '--add --disabled' is too much sugar (since we
don't care about atomicity the way QMP did), then life gets simpler to
drop --disabled altogether.
>> + if (add && disable) {
>> + disable = false;
>> + add_disabled = true;
>> + }
>> + if (add + remove + clear + enable + disable + !!merge != 1) {
>> + error_report("Need exactly one mode of --add, --remove, --clear, "
>> + "--enable, --disable, or --merge");
>
> Aha. So you can actually only do a single operation.
>
> That means the doc shouldn’t use {}, in my opinion, because that to me
> means repetition (thanks to EBNF). It definitely served to confuse me
> greatly until this point.
In command line syntax, I'm most used to seeing repetition as '...',
optional as [], and mutually-exclusive choice as {|}. Yes, that's
different than EBNF.
>
> I also don’t see why we would disallow multiple operations in one go.
> The --add --merge combination seems useful to me, and I don’t see a
> problem in implementing it.
>
> I don’t know why we don’t just create a list of operations to execute,
> based on the order given in the argument list, and then execute them in
> order. That would even allow multiple --merge operations.
If I understand, you're asking why we can't do:
qemu-img bitmap foo.qcow2 --add b1 --merge sb b1
in one operation.
That changes the syntax entirely, compared to what I implemented. You'd
have to have an argument to every option, AND figure out how to specify
TWO arguments to the --merge option. Might be doable, but seems hairy.
>
> If we don’t want that (because we don’t want argument order to matter),
> we could still allow all operations to be done at least once and always
> execute them in the same order, e.g.:
> (1) add
> (2) clear
> (3) merge
> (4) disable
> (5) enable
> (6) remove
I still find it simpler to do exactly one operation per invocation.
>
>> + goto out;
>> + }
>> + if (granularity && !add) {
>> + error_report("granularity only supported with --add");
>> + goto out;
>> + }
>> + if (src_fmt && !src_filename) {
>> + error_report("-F only supported with -b");
>> + goto out;
>> + }
>> + if (src_filename && !merge) {
>> + error_report("alternate source file only supported with --merge");
>
> “alternate” sounds strange. Why not be more precise and call it a
> “Merge bitmap source file” or “File to source merge bitmap from”?
"Merge bitmap source file" sounds fine.
>> +
>> + if (err) {
>> + error_reportf_err(err, "Bitmap %s operation failed", bitmap);
>
> Wouldn’t “Operation on bitmap %s failed” work better?
Yes.
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v2 3/6] qemu-img: Add bitmap sub-command
2020-04-30 15:21 ` Eric Blake
@ 2020-05-04 10:01 ` Max Reitz
2020-05-04 13:28 ` Eric Blake
0 siblings, 1 reply; 23+ messages in thread
From: Max Reitz @ 2020-05-04 10:01 UTC (permalink / raw)
To: Eric Blake, qemu-devel; +Cc: kwolf, qemu-block
[-- Attachment #1.1: Type: text/plain, Size: 9796 bytes --]
On 30.04.20 17:21, Eric Blake wrote:
> On 4/30/20 9:55 AM, Max Reitz wrote:
>> On 21.04.20 23:20, Eric Blake wrote:
>>> Include actions for --add, --remove, --clear, --enable, --disable, and
>>> --merge (note that --clear is a bit of fluff, because the same can be
>>> accomplished by removing a bitmap and then adding a new one in its
>>> place,
>>
>> Well, ideally, all of qemu-img is just fluff because “the same can be
>> accomplished by launching qemu and issuing the equivalent QMP
>> commands”. :)
>>
>>> but it matches what QMP commands exist). Listing is omitted,
>>> because it does not require a bitmap name and because it was already
>>> possible with 'qemu-img info'.
>>
>> Fair enough, although it can be said that qemu-img info’s output is
>> qcow2-specific. It might be nice to have some definitely
>> format-independent output. (But we don’t have persistent bitmaps in
>> anything but qcow2 yet (or do we in NBD?), so I don’t expect anyone to
>> care much.)
>
> We can add a list subcommand later if it is still desired. I agree that
> a tabular format:
>
> name enabled granularity
> bitmap1 false 65536
> bitmap2 true 512
>
> in isolation is easier to read than:
>
> bitmaps:
> [0]:
> flags:
> name: bitmap1
> granularity: 65536
> [1]:
> flags:
> [0]: auto
> name: bitmap2
> granularity: 512
>
> embedded inside even more information.
>
>>
>>> Merge can work either from another
>>> bitmap in the same image, or from a bitmap in a distinct image.
>>>
>>> While this supports --image-opts for the file being modified, I did
>>> not think it worth the extra complexity to support that for the source
>>> file in a cross-file bitmap merge. Likewise, I chose to have --merge
>>> only take a single source rather than following the QMP support for
>>> multiple merges in one go; in part to simplify the command line, and
>>> in part because an offline image can achieve the same effect by
>>> multiple qemu-img bitmap --merge calls. We can enhance that if needed
>>> in the future (the same way that 'qemu-img convert' has a mode that
>>> concatenates multiple sources into one destination).
>>>
>>> Upcoming patches will add iotest coverage of these commands while
>>> also testing other features.
>>>
>>> Signed-off-by: Eric Blake <eblake@redhat.com>
>>> ---
>>> docs/tools/qemu-img.rst | 24 +++++
>>> qemu-img.c | 198 ++++++++++++++++++++++++++++++++++++++++
>>> qemu-img-cmds.hx | 7 ++
>>> 3 files changed, 229 insertions(+)
>>>
>>> diff --git a/docs/tools/qemu-img.rst b/docs/tools/qemu-img.rst
>>> index 7d08c48d308f..4f3b0e2c9ace 100644
>>> --- a/docs/tools/qemu-img.rst
>>> +++ b/docs/tools/qemu-img.rst
>>> @@ -281,6 +281,30 @@ Command description:
>>> For write tests, by default a buffer filled with zeros is
>>> written. This can be
>>> overridden with a pattern byte specified by *PATTERN*.
>>>
>>> +.. option:: bitmap {--add [-g GRANULARITY] [--disabled] | --remove |
>>> --clear | --enable | --disable | --merge SOURCE_BITMAP [-b
>>> SOURCE_FILE [-F SOURCE_FMT]]} [--object OBJECTDEF] [--image-opts] [-f
>>> FMT] FILENAME BITMAP
>>
>> So I can do multiple operations in one roll, but they all use the same
>> BITMAP? Sounds a bit weird. It actually took me a while to understands
>> this, because I thought for sure that each command would take a bitmap
>> name. (And was ready to complain that it looked like they don’t, but,
>> well, that’s because they don’t.)
>
> All of the operations take one bitmap name (the final BITMAP).
> Additionally, the --merge operation takes a second bitmap name
> (SOURCE_BITMAP). None of the other operations need a second bitmap
> name, so only --merge requires an option argument. As written, the { a
> | b | c } implies that operations are mutually exclusive: you can only
> request one operation per qemu-img invocation.
Well, as I found out later it’s supposed to imply that. I always expect
{} to mean repetition.
>> Although I suppose some practical example like
>>
>> $ qemu-img bitmap --add --merge sbmap --disable foo.qcow2 nbmap
>>
>> does make sense.[1]
>>
>>
>> Would
>>
>> $ qemu-img bitmap --add nbmap --merge nbmap sbmap --enable nbmap \
>> foo.qcow2
>>
>> make more sense?
>
> That would be more transactional, and more effort to implement.
As a user, I wouldn’t expect it to be a transaction, but just executed
in order.
> My argument is that you should instead write:
>
> $ qemu-img bitmap --add foo.qcow2 nbmap
> $ qemu-img bitmap --merge sbmap foo.qcow2 nbmap
> $ qemu-img bitmap --enable foo.qcow2 nbmap
>
> where I only have to implement one operation per qemu-img.
I don’t know about the “have to”, because from an algorithm standpoint,
doing so would be trivial. (That is, collecting the operations into a
list, along with their specific arguments, and then execute the list in
a loop.)
I can see that doing this in C is more difficult than it would be in
nicer languages, and so not actually trivial.
But allowing all switches at most once without mutual exclusion still
seems simple. The question is mostly whether the implementation would
match what we can expect users to expect.
[...]
>> So if --disable and --disabled are exactly the same, I really don’t know
>> why --disabled even exists.
>
> Logically, '--add --disabled' matches the name of the QMP command with
> its optional 'disabled' parameter, while --disable matches the name of
> the QMP command. We don't have to have the alias; and in fact, if you
> agree that supporting '--add --disabled' is too much sugar (since we
> don't care about atomicity the way QMP did), then life gets simpler to
> drop --disabled altogether.
I find it makes the interface unnecessarily complex, as does requiring
mutual exclusion.
If we want mutual exclusion, I can see that a separate --disabled for
--add makes sense.
>>> + if (add && disable) {
>>> + disable = false;
>>> + add_disabled = true;
>>> + }
>>> + if (add + remove + clear + enable + disable + !!merge != 1) {
>>> + error_report("Need exactly one mode of --add, --remove,
>>> --clear, "
>>> + "--enable, --disable, or --merge");
>>
>> Aha. So you can actually only do a single operation.
>>
>> That means the doc shouldn’t use {}, in my opinion, because that to me
>> means repetition (thanks to EBNF). It definitely served to confuse me
>> greatly until this point.
>
> In command line syntax, I'm most used to seeing repetition as '...',
> optional as [], and mutually-exclusive choice as {|}. Yes, that's
> different than EBNF.
It’s confusing is what it is, and unnecessarily so. The | already
signifies exclusion: Say there are -a and -b, if they’re not mutually
exclusive, then the doc describe them as “[-a] [-b]”; if they are, it’d
be “-a | -b”. Maybe “(-a | -b)”.
I can’t remember having seen {|} for mutual exclusivity before, but then
again, it doesn’t happen often, I suppose. git for one thing seems to
use (|).
(Regex also uses {} for repetition, even if in a different way from EBNF.)
I’ve never seen “...” used for switches, only for free-form arguments
like filenames. (Because normally, a switch can be specified only once,
or it wouldn’t be a “switch”.)
>> I also don’t see why we would disallow multiple operations in one go.
>> The --add --merge combination seems useful to me, and I don’t see a
>> problem in implementing it.
>>
>> I don’t know why we don’t just create a list of operations to execute,
>> based on the order given in the argument list, and then execute them in
>> order. That would even allow multiple --merge operations.
>
> If I understand, you're asking why we can't do:
>
> qemu-img bitmap foo.qcow2 --add b1 --merge sb b1
>
> in one operation.
>
> That changes the syntax entirely, compared to what I implemented. You'd
> have to have an argument to every option, AND figure out how to specify
> TWO arguments to the --merge option. Might be doable, but seems hairy.
Just “qemu-img bitmap --add --merge sb foo.qcow2 b1” would be enough.
>> If we don’t want that (because we don’t want argument order to matter),
>> we could still allow all operations to be done at least once and always
>> execute them in the same order, e.g.:
>> (1) add
>> (2) clear
>> (3) merge
>> (4) disable
>> (5) enable
>> (6) remove
>
> I still find it simpler to do exactly one operation per invocation.
I feel like that’s mostly because of our coding style allowing an “else
if” to end and start a block on the same line.
(That is, I feel like if we allowed multiple operations in a single go,
the only difference would be that we wouldn’t have to check for mutual
exclusivity, and that all “} else if (cond) {”s would have to be turned
into “} if (cond) {”s. And reordered.)
It’s possible that I mostly feel compelled to vote for non-exclusivity
because it would be clear then how to document the interface (i.e.,
“[--add] [--clear] [...]”) and I wouldn’t have to explain my utter
confusion at the sight of {}.
Max
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v2 4/6] qcow2: Expose bitmaps' size during measure
2020-04-21 21:20 ` [PATCH v2 4/6] qcow2: Expose bitmaps' size during measure Eric Blake
@ 2020-05-04 11:36 ` Max Reitz
2020-05-04 13:44 ` Eric Blake
0 siblings, 1 reply; 23+ messages in thread
From: Max Reitz @ 2020-05-04 11:36 UTC (permalink / raw)
To: Eric Blake, qemu-devel; +Cc: kwolf, Markus Armbruster, qemu-block
[-- Attachment #1.1: Type: text/plain, Size: 6625 bytes --]
On 21.04.20 23:20, Eric Blake wrote:
> It's useful to know how much space can be occupied by qcow2 persistent
> bitmaps, even though such metadata is unrelated to the guest-visible
> data. Report this value as an additional field. Update iotest 190 to
> cover it and a portion of the just-added qemu-img bitmap command.
>
> The addition of a new field demonstrates why we should always
> zero-initialize qapi C structs; while the qcow2 driver still fully
> populates all fields, the raw and crypto drivers had to be tweaked.
>
> See also: https://bugzilla.redhat.com/1779904
>
> Reported-by: Nir Soffer <nsoffer@redhat.com>
> Signed-off-by: Eric Blake <eblake@redhat.com>
> ---
> qapi/block-core.json | 15 ++++++++++-----
> block/crypto.c | 2 +-
> block/qcow2.c | 29 ++++++++++++++++++++++++++++-
> block/raw-format.c | 2 +-
> qemu-img.c | 3 +++
> tests/qemu-iotests/190 | 15 +++++++++++++--
> tests/qemu-iotests/190.out | 13 ++++++++++++-
> 7 files changed, 68 insertions(+), 11 deletions(-)
>
> diff --git a/qapi/block-core.json b/qapi/block-core.json
> index 943df1926a91..b47c6d69ba27 100644
> --- a/qapi/block-core.json
> +++ b/qapi/block-core.json
> @@ -633,18 +633,23 @@
> # efficiently so file size may be smaller than virtual disk size.
> #
> # The values are upper bounds that are guaranteed to fit the new image file.
> -# Subsequent modification, such as internal snapshot or bitmap creation, may
> -# require additional space and is not covered here.
> +# Subsequent modification, such as internal snapshot or further bitmap
> +# creation, may require additional space and is not covered here.
> #
> -# @required: Size required for a new image file, in bytes.
> +# @required: Size required for a new image file, in bytes, when copying just
> +# guest-visible contents.
> #
> # @fully-allocated: Image file size, in bytes, once data has been written
> -# to all sectors.
> +# to all sectors, when copying just guest-visible contents.
> +#
> +# @bitmaps: Additional size required for bitmap metadata not directly used
> +# for guest contents,
Not sure how I feel about the “not directly used for guest contents”,
because it feels a bit superfluous, and it sounds like there might be
bitmap data that is directly used for guest contents.
> when that metadata can be copied in addition
> +# to guest contents. (since 5.1)
> #
> # Since: 2.10
> ##
> { 'struct': 'BlockMeasureInfo',
> - 'data': {'required': 'int', 'fully-allocated': 'int'} }
> + 'data': {'required': 'int', 'fully-allocated': 'int', '*bitmaps': 'int'} }
Why is @bitmaps optional? I.e., what does absence mean, besides “not
supported yet”?
Right now, absence means anything in “format doesn’t support bitmaps, so
nothing can be copied”, “no input image given, so there’s no data on
bitmaps”, to “0 bytes to copy”.
I think in the latter case we should emit it as 0, maybe even in the
former case, because I think the fact that there won’t be any bitmap
data to copy might be interesting. (And it’s also definitely 0, not
just “don’t know”.)
I suppose absence does make sense in case the user didn’t specify an
input image, because then we just really don’t know.
[...]
> diff --git a/block/qcow2.c b/block/qcow2.c
> index b524b0c53f84..9fd650928016 100644
> --- a/block/qcow2.c
> +++ b/block/qcow2.c
[...]
> @@ -4739,6 +4742,28 @@ static BlockMeasureInfo *qcow2_measure(QemuOpts *opts, BlockDriverState *in_bs,
> goto err;
> }
>
> + FOR_EACH_DIRTY_BITMAP(in_bs, bm) {
> + if (bdrv_dirty_bitmap_get_persistence(bm)) {
> + const char *name = bdrv_dirty_bitmap_name(bm);
> + uint32_t granularity = bdrv_dirty_bitmap_granularity(bm);
> + uint64_t bmbits = DIV_ROUND_UP(bdrv_dirty_bitmap_size(bm),
> + granularity);
> + uint64_t bmclusters = DIV_ROUND_UP(DIV_ROUND_UP(bmbits,
> + CHAR_BIT),
I suppose if we allowed CHAR_BIT to be anything but 8, it would be wrong
to use it here. So maybe just a plain 8 would be more correct; although
I suppose CHAR_BIT kind of describes what constant we want here.
> + cluster_size);
> +
> + /* Assume the entire bitmap is allocated */
> + bitmaps_size += bmclusters * cluster_size;
> + /* Also reserve space for the bitmap table entries */
> + bitmaps_size += ROUND_UP(bmclusters * sizeof(uint64_t),
> + cluster_size);
> + /* Guess at contribution to bitmap directory size */
> + bitmap_overhead += ROUND_UP(strlen(name) + 24,
> + sizeof(uint64_t));
Seems not just a guess to me, but actually correct. Maybe
bitmap_overhead should be called bitmap_directory_size (or
bitmap_dir_size, or bmap_dir_size, or whatever (but probably not bds!
:)), because that’s what it is.
> + }
> + }
> + bitmaps_size += ROUND_UP(bitmap_overhead, cluster_size);
> +
> virtual_size = ROUND_UP(ssize, cluster_size);
>
> if (has_backing_file) {
[...]
> diff --git a/tests/qemu-iotests/190.out b/tests/qemu-iotests/190.out
> index d001942002db..11962f972429 100644
> --- a/tests/qemu-iotests/190.out
> +++ b/tests/qemu-iotests/190.out
> @@ -1,5 +1,5 @@
> QA output created by 190
> -== Huge file ==
> +== Huge file without bitmaps ==
>
> Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=2199023255552
> required size: 2199023255552
> @@ -8,4 +8,15 @@ required size: 335806464
> fully allocated size: 2199359062016
> required size: 18874368
> fully allocated size: 2199042129920
> +
> +== Huge file with bitmaps ==
> +
> +required size: 2199023255552
> +fully allocated size: 2199023255552
> +required size: 335806464
> +fully allocated size: 2199359062016
> +bitmaps size: 537198592
> +required size: 18874368
> +fully allocated size: 2199042129920
> +bitmaps size: 545259520
Looks correct.
(It might be nicer to calculate the reference value in the script,
because this way I had to verify it by hand, but, well, now I did verify
it, so...)
Max
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v2 5/6] qemu-img: Add convert --bitmaps option
2020-04-21 21:20 ` [PATCH v2 5/6] qemu-img: Add convert --bitmaps option Eric Blake
@ 2020-05-04 12:14 ` Max Reitz
0 siblings, 0 replies; 23+ messages in thread
From: Max Reitz @ 2020-05-04 12:14 UTC (permalink / raw)
To: Eric Blake, qemu-devel; +Cc: kwolf, qemu-block
[-- Attachment #1.1: Type: text/plain, Size: 1699 bytes --]
On 21.04.20 23:20, Eric Blake wrote:
> Make it easier to copy all the persistent bitmaps of a source image
> along with the contents, by adding a boolean flag for use with
> qemu-img convert. This is basically shorthand, as the same effect
> could be accomplished with a series of 'qemu-img bitmap --add' and
> 'qemu-img bitmap --merge -b source' commands, or by QMP commands.
>
> See also https://bugzilla.redhat.com/show_bug.cgi?id=1779893
>
> Signed-off-by: Eric Blake <eblake@redhat.com>
> ---
> docs/tools/qemu-img.rst | 6 +++-
> qemu-img.c | 80 +++++++++++++++++++++++++++++++++++++++--
> qemu-img-cmds.hx | 4 +--
> 3 files changed, 84 insertions(+), 6 deletions(-)
[...]
> diff --git a/qemu-img.c b/qemu-img.c
> index e1127273f21e..6cfc1f52ef98 100644
> --- a/qemu-img.c
> +++ b/qemu-img.c
[...]
> @@ -2293,7 +2342,6 @@ static int img_convert(int argc, char **argv)
> goto fail_getopt;
> }
>
> -
> /* ret is still -EINVAL until here */
> ret = bdrv_parse_cache_mode(src_cache, &src_flags, &src_writethrough);
> if (ret < 0) {
[...]
> @@ -2461,9 +2531,7 @@ static int img_convert(int argc, char **argv)
> if (!skip_create) {
> open_opts = qdict_new();
> qemu_opt_foreach(opts, img_add_key_secrets, open_opts, &error_abort);
> - }
>
> - if (!skip_create) {
I mean, cleanups are always welcome, but I feel compelled[1] to complain
that it’s done in this patch without mentioning it in the commit message.
[1] By social norms, not by my personal opinion.
I feel good about all changes, though, so:
Reviewed-by: Max Reitz <mreitz@redhat.com>
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v2 6/6] iotests: Add test 291 to for qemu-img bitmap coverage
2020-04-21 21:20 ` [PATCH v2 6/6] iotests: Add test 291 to for qemu-img bitmap coverage Eric Blake
@ 2020-05-04 13:05 ` Max Reitz
2020-05-05 21:22 ` Eric Blake
0 siblings, 1 reply; 23+ messages in thread
From: Max Reitz @ 2020-05-04 13:05 UTC (permalink / raw)
To: Eric Blake, qemu-devel; +Cc: kwolf, qemu-block
[-- Attachment #1.1: Type: text/plain, Size: 3443 bytes --]
On 21.04.20 23:20, Eric Blake wrote:
> Add a new test covering the 'qemu-img bitmap' subcommand, as well as
> 'qemu-img convert --bitmaps', both added in recent patches.
>
> Signed-off-by: Eric Blake <eblake@redhat.com>
> ---
> tests/qemu-iotests/291 | 103 +++++++++++++++++++++++++++++++++++++
> tests/qemu-iotests/291.out | 78 ++++++++++++++++++++++++++++
> tests/qemu-iotests/group | 1 +
> 3 files changed, 182 insertions(+)
> create mode 100755 tests/qemu-iotests/291
> create mode 100644 tests/qemu-iotests/291.out
>
> diff --git a/tests/qemu-iotests/291 b/tests/qemu-iotests/291
> new file mode 100755
> index 000000000000..77713c0cfea7
> --- /dev/null
> +++ b/tests/qemu-iotests/291
[...]
> +echo
> +echo "=== Bitmap preservation not possible to non-qcow2 ==="
> +echo
> +
> +mv "$TEST_IMG" "$TEST_IMG.orig"
“mv” doesn’t work images with external data files.
(ORIG_IMG=$TEST_IMG; TEST_IMG="$TEST_IMG".orig should work)
> +$QEMU_IMG convert --bitmaps -O raw "$TEST_IMG.orig" "$TEST_IMG"
> +
> +echo
> +echo "=== Convert with bitmap preservation ==="
> +echo
> +
> +# Only bitmaps from the active layer are copied
That’s kind of obvious when you think about (whenever an image is
attached to a VM, only the active layer’s bitmaps are visible, not those
from the backing chain), but maybe this should be noted in the
documentation?
> +$QEMU_IMG convert --bitmaps -O qcow2 "$TEST_IMG.orig" "$TEST_IMG"
> +$QEMU_IMG info "$TEST_IMG" | _filter_img_info --format-specific
> +# But we can also merge in bitmaps from other layers
> +$QEMU_IMG bitmap --add --disabled -f $IMGFMT "$TEST_IMG" b0
> +$QEMU_IMG bitmap --add -f $IMGFMT "$TEST_IMG" tmp
> +$QEMU_IMG bitmap --merge b0 -b "$TEST_IMG.base" -F $IMGFMT "$TEST_IMG" tmp
> +$QEMU_IMG bitmap --merge tmp "$TEST_IMG" b0
> +$QEMU_IMG bitmap --remove -f $IMGFMT "$TEST_IMG" tmp
Why do we need tmp here? Can’t we just merge base’s b0 directly into
$TEST_IMG’s b0?
[...]
> diff --git a/tests/qemu-iotests/291.out b/tests/qemu-iotests/291.out
> new file mode 100644
> index 000000000000..d716c0c7cc0b
> --- /dev/null
> +++ b/tests/qemu-iotests/291.out
[...]
> +=== Check bitmap contents ===
> +
> +[{ "start": 0, "length": 3145728, "depth": 0, "zero": false, "data": true, "offset": OFFSET},
> +{ "start": 3145728, "length": 1048576, "depth": 0, "zero": false, "data": false},
> +{ "start": 4194304, "length": 6291456, "depth": 0, "zero": false, "data": true, "offset": OFFSET}]
> +[{ "start": 0, "length": 1048576, "depth": 0, "zero": false, "data": true, "offset": OFFSET},
> +{ "start": 1048576, "length": 1048576, "depth": 0, "zero": false, "data": false},
> +{ "start": 2097152, "length": 8388608, "depth": 0, "zero": false, "data": true, "offset": OFFSET}]
> +[{ "start": 0, "length": 2097152, "depth": 0, "zero": false, "data": true, "offset": OFFSET},
> +{ "start": 2097152, "length": 1048576, "depth": 0, "zero": false, "data": false},
> +{ "start": 3145728, "length": 7340032, "depth": 0, "zero": false, "data": true, "offset": OFFSET}]
Am I looking at this wrong or does the bitmap data seem to be inverted?
Everywhere where I’d expect the bitmaps to be cleared, this map reports
data=true, whereas where I’d expect them to be set, it reports data=false.
I suppose that’s intentional, but can you explain this behavior to me?
Max
> +*** done
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v2 3/6] qemu-img: Add bitmap sub-command
2020-05-04 10:01 ` Max Reitz
@ 2020-05-04 13:28 ` Eric Blake
0 siblings, 0 replies; 23+ messages in thread
From: Eric Blake @ 2020-05-04 13:28 UTC (permalink / raw)
To: Max Reitz, qemu-devel; +Cc: kwolf, qemu-block
On 5/4/20 5:01 AM, Max Reitz wrote:
>>>> +.. option:: bitmap {--add [-g GRANULARITY] [--disabled] | --remove |
>>>> --clear | --enable | --disable | --merge SOURCE_BITMAP [-b
>>>> SOURCE_FILE [-F SOURCE_FMT]]} [--object OBJECTDEF] [--image-opts] [-f
>>>> FMT] FILENAME BITMAP
>>>
>>> So I can do multiple operations in one roll, but they all use the same
>>> BITMAP? Sounds a bit weird. It actually took me a while to understands
>>> this, because I thought for sure that each command would take a bitmap
>>> name. (And was ready to complain that it looked like they don’t, but,
>>> well, that’s because they don’t.)
>>
>> All of the operations take one bitmap name (the final BITMAP).
>> Additionally, the --merge operation takes a second bitmap name
>> (SOURCE_BITMAP). None of the other operations need a second bitmap
>> name, so only --merge requires an option argument. As written, the { a
>> | b | c } implies that operations are mutually exclusive: you can only
>> request one operation per qemu-img invocation.
>
> Well, as I found out later it’s supposed to imply that. I always expect
> {} to mean repetition.
>> In command line syntax, I'm most used to seeing repetition as '...',
>> optional as [], and mutually-exclusive choice as {|}. Yes, that's
>> different than EBNF.
>
> It’s confusing is what it is, and unnecessarily so. The | already
> signifies exclusion: Say there are -a and -b, if they’re not mutually
> exclusive, then the doc describe them as “[-a] [-b]”; if they are, it’d
> be “-a | -b”. Maybe “(-a | -b)”.
Is s/{/(/ is all the more that would be needed to take this patch as-is?
Or should I really try and re-write it to take a list of operations,
and iterate through the list until the first failure?
(At any rate, I'll probably end up trying that anyways, just for
comparison in the lines of code...)
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v2 4/6] qcow2: Expose bitmaps' size during measure
2020-05-04 11:36 ` Max Reitz
@ 2020-05-04 13:44 ` Eric Blake
0 siblings, 0 replies; 23+ messages in thread
From: Eric Blake @ 2020-05-04 13:44 UTC (permalink / raw)
To: Max Reitz, qemu-devel; +Cc: kwolf, Markus Armbruster, qemu-block
On 5/4/20 6:36 AM, Max Reitz wrote:
> On 21.04.20 23:20, Eric Blake wrote:
>> It's useful to know how much space can be occupied by qcow2 persistent
>> bitmaps, even though such metadata is unrelated to the guest-visible
>> data. Report this value as an additional field. Update iotest 190 to
>> cover it and a portion of the just-added qemu-img bitmap command.
>>
>> The addition of a new field demonstrates why we should always
>> zero-initialize qapi C structs; while the qcow2 driver still fully
>> populates all fields, the raw and crypto drivers had to be tweaked.
>>
>> See also: https://bugzilla.redhat.com/1779904
>>
>> Reported-by: Nir Soffer <nsoffer@redhat.com>
>> Signed-off-by: Eric Blake <eblake@redhat.com>
>> ---
>> # @fully-allocated: Image file size, in bytes, once data has been written
>> -# to all sectors.
>> +# to all sectors, when copying just guest-visible contents.
>> +#
>> +# @bitmaps: Additional size required for bitmap metadata not directly used
>> +# for guest contents,
>
> Not sure how I feel about the “not directly used for guest contents”,
> because it feels a bit superfluous, and it sounds like there might be
> bitmap data that is directly used for guest contents.
Hmm. I was trying to make it obvious that bitmap size is separate from
fully-allocated (and not double-counted), but you may have a point that
just using "Additional size required for bitmap metadata, when that
metadata can be copied in addition..." would work.
>
>> when that metadata can be copied in addition
>> +# to guest contents. (since 5.1)
>> #
>> # Since: 2.10
>> ##
>> { 'struct': 'BlockMeasureInfo',
>> - 'data': {'required': 'int', 'fully-allocated': 'int'} }
>> + 'data': {'required': 'int', 'fully-allocated': 'int', '*bitmaps': 'int'} }
>
> Why is @bitmaps optional? I.e., what does absence mean, besides “not
> supported yet”?
>
> Right now, absence means anything in “format doesn’t support bitmaps, so
> nothing can be copied”, “no input image given, so there’s no data on
> bitmaps”, to “0 bytes to copy”.
>
> I think in the latter case we should emit it as 0, maybe even in the
> former case, because I think the fact that there won’t be any bitmap
> data to copy might be interesting. (And it’s also definitely 0, not
> just “don’t know”.)
>
> I suppose absence does make sense in case the user didn’t specify an
> input image, because then we just really don’t know.
The patch will require a tweak to report an explicit 0 (when there is
nothing to be copied), but doing that makes sense (explicit 0 for v3
qcow2 images, and maybe even for v2, but omitted for other formats that
have no bitmap support).
>> @@ -4739,6 +4742,28 @@ static BlockMeasureInfo *qcow2_measure(QemuOpts *opts, BlockDriverState *in_bs,
>> goto err;
>> }
>>
>> + FOR_EACH_DIRTY_BITMAP(in_bs, bm) {
>> + if (bdrv_dirty_bitmap_get_persistence(bm)) {
>> + const char *name = bdrv_dirty_bitmap_name(bm);
>> + uint32_t granularity = bdrv_dirty_bitmap_granularity(bm);
>> + uint64_t bmbits = DIV_ROUND_UP(bdrv_dirty_bitmap_size(bm),
>> + granularity);
>> + uint64_t bmclusters = DIV_ROUND_UP(DIV_ROUND_UP(bmbits,
>> + CHAR_BIT),
>
> I suppose if we allowed CHAR_BIT to be anything but 8, it would be wrong
> to use it here. So maybe just a plain 8 would be more correct; although
> I suppose CHAR_BIT kind of describes what constant we want here.
POSIX requires CHAR_BIT to be 8. (C99 allows some odd machines where
CHAR_BIT is 9, 16, or even 32, but we don't ever try to port to such
machines).
>> +== Huge file with bitmaps ==
>> +
>> +required size: 2199023255552
>> +fully allocated size: 2199023255552
>> +required size: 335806464
>> +fully allocated size: 2199359062016
>> +bitmaps size: 537198592
>> +required size: 18874368
>> +fully allocated size: 2199042129920
>> +bitmaps size: 545259520
>
> Looks correct.
>
> (It might be nicer to calculate the reference value in the script,
> because this way I had to verify it by hand, but, well, now I did verify
> it, so...)
Feels like duplicate work, which would require tweaking in two spots if
we ever change our implementation or the qcow2 format is further
enhanced, but it would also make it obvious that we are aware of the
impact of such future changes. Okay, I'll see what I can add.
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v2 6/6] iotests: Add test 291 to for qemu-img bitmap coverage
2020-05-04 13:05 ` Max Reitz
@ 2020-05-05 21:22 ` Eric Blake
0 siblings, 0 replies; 23+ messages in thread
From: Eric Blake @ 2020-05-05 21:22 UTC (permalink / raw)
To: Max Reitz, qemu-devel; +Cc: kwolf, qemu-block
On 5/4/20 8:05 AM, Max Reitz wrote:
> On 21.04.20 23:20, Eric Blake wrote:
>> Add a new test covering the 'qemu-img bitmap' subcommand, as well as
>> 'qemu-img convert --bitmaps', both added in recent patches.
>>
>> Signed-off-by: Eric Blake <eblake@redhat.com>
>> +echo
>> +echo "=== Bitmap preservation not possible to non-qcow2 ==="
>> +echo
>> +
>> +mv "$TEST_IMG" "$TEST_IMG.orig"
>
> “mv” doesn’t work images with external data files.
>
> (ORIG_IMG=$TEST_IMG; TEST_IMG="$TEST_IMG".orig should work)
Good idea.
>
>> +$QEMU_IMG convert --bitmaps -O raw "$TEST_IMG.orig" "$TEST_IMG"
>> +
>> +echo
>> +echo "=== Convert with bitmap preservation ==="
>> +echo
>> +
>> +# Only bitmaps from the active layer are copied
>
> That’s kind of obvious when you think about (whenever an image is
> attached to a VM, only the active layer’s bitmaps are visible, not those
> from the backing chain), but maybe this should be noted in the
> documentation?
As part of integrating bitmaps with external snapshots, libvirt actually
depends on being able to see bitmaps from the backing chain - but as
bitmaps are always referenced as a 'node name, bitmap name' tuple, this
is indeed doable.
>
>> +$QEMU_IMG convert --bitmaps -O qcow2 "$TEST_IMG.orig" "$TEST_IMG"
>> +$QEMU_IMG info "$TEST_IMG" | _filter_img_info --format-specific
>> +# But we can also merge in bitmaps from other layers
>> +$QEMU_IMG bitmap --add --disabled -f $IMGFMT "$TEST_IMG" b0
>> +$QEMU_IMG bitmap --add -f $IMGFMT "$TEST_IMG" tmp
>> +$QEMU_IMG bitmap --merge b0 -b "$TEST_IMG.base" -F $IMGFMT "$TEST_IMG" tmp
>> +$QEMU_IMG bitmap --merge tmp "$TEST_IMG" b0
>> +$QEMU_IMG bitmap --remove -f $IMGFMT "$TEST_IMG" tmp
>
> Why do we need tmp here? Can’t we just merge base’s b0 directly into
> $TEST_IMG’s b0?
Yes, we could. But then I wouldn't cover as many bitmap subcommands.
Adding a comment about why the example is contrived (for maximal
coverage) is a good idea.
>> +=== Check bitmap contents ===
>> +
>> +[{ "start": 0, "length": 3145728, "depth": 0, "zero": false, "data": true, "offset": OFFSET},
>> +{ "start": 3145728, "length": 1048576, "depth": 0, "zero": false, "data": false},
>> +{ "start": 4194304, "length": 6291456, "depth": 0, "zero": false, "data": true, "offset": OFFSET}]
>> +[{ "start": 0, "length": 1048576, "depth": 0, "zero": false, "data": true, "offset": OFFSET},
>> +{ "start": 1048576, "length": 1048576, "depth": 0, "zero": false, "data": false},
>> +{ "start": 2097152, "length": 8388608, "depth": 0, "zero": false, "data": true, "offset": OFFSET}]
>> +[{ "start": 0, "length": 2097152, "depth": 0, "zero": false, "data": true, "offset": OFFSET},
>> +{ "start": 2097152, "length": 1048576, "depth": 0, "zero": false, "data": false},
>> +{ "start": 3145728, "length": 7340032, "depth": 0, "zero": false, "data": true, "offset": OFFSET}]
>
> Am I looking at this wrong or does the bitmap data seem to be inverted?
> Everywhere where I’d expect the bitmaps to be cleared, this map reports
> data=true, whereas where I’d expect them to be set, it reports data=false.
>
> I suppose that’s intentional, but can you explain this behavior to me?
This is an artifact of how x-dirty-bitmap works (it has the x- prefix
because it is a hack, but we don't have anything better for reading out
bitmap contents). The NBD spec returns block status as a 32-bit value
for a 'metadata context'; normally, we use context 'base:allocation'
context where bit 0 is set for holes or clear for allocated, and bit 1
is set for reads-as-zero or clear for unknown contents (favoring all-0
as the most-common case). But with x-dirty-bitmap, we are instead
abusing NBD to query the 'qemu:dirty-bitmap:FOO' context, where bit 0 is
set for anywhere the bitmap has a 1, yet feed that information into the
pre-existing qemu code for handling block status. So qemu-img map is
reporting "data":true for what it thinks is the normal 0-for-allocated,
and "data":false for 1-for-sparse, and we just have to translate that
back into an understanding of what the bitmap reported. Yes, a comment
would be helpful.
I would _really_ love to enhance 'qemu-img map' to output image-specific
metadata _in addition_ to the existing "zero" and "data" fields (by
having qemu-img read two NBD contexts at once: both base:allocation and
qemu:dirty-bitmap:FOO), at which point we can drop the x- prefix and
avoid the abuse of qemu's internals by overwriting the block_status
code. But that's a bigger project.
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v2 2/6] blockdev: Split off basic bitmap operations for qemu-img
2020-04-30 14:50 ` Eric Blake
@ 2020-05-08 11:37 ` Kevin Wolf
2020-05-08 13:48 ` Eric Blake
0 siblings, 1 reply; 23+ messages in thread
From: Kevin Wolf @ 2020-05-08 11:37 UTC (permalink / raw)
To: Eric Blake; +Cc: Markus Armbruster, qemu-devel, qemu-block, Max Reitz
Am 30.04.2020 um 16:50 hat Eric Blake geschrieben:
> On 4/30/20 8:59 AM, Max Reitz wrote:
> > On 21.04.20 23:20, Eric Blake wrote:
> > > Upcoming patches want to add some basic bitmap manipulation abilities
> > > to qemu-img. But blockdev.o is too heavyweight to link into qemu-img
> > > (among other things, it would drag in block jobs and transaction
> > > support - qemu-img does offline manipulation, where atomicity is less
> > > important because there are no concurrent modifications to compete
> > > with), so it's time to split off the bare bones of what we will need
> > > into a new file blockbitmaps.o.
> > >
> > > In addition to exposing 6 QMP commands for use by qemu-img (add,
> > > remove, clear, enable, disable, merge), this also has to export three
> > > previously-static functions for use by blockdev.c transactions.
> > >
> > > Signed-off-by: Eric Blake <eblake@redhat.com>
> > > ---
> > > Makefile.objs | 2 +-
> > > include/sysemu/blockdev.h | 14 ++
> > > blockbitmaps.c | 324 ++++++++++++++++++++++++++++++++++++++
> >
> > Hm. Can we get a better name? blockdev-bitmaps.c, for example?
>
> Sure, I'm open to bike-shed suggestions. I'd also _really_ love to make the
> new file NOT live in the top-level, but that's a harder task that I'm not
> sure how to do (it's easy to tweak Makefile.objs for another file in the
> same directory, but harder to see through the magic to figure out how to
> relocate things).
Yes, please move it somewhere else. I'd suggest something like
block/monitor/bitmap-qmp-cmds.c for the QMP command handlers, and if
there are functions that are more generally useful, block/bitmaps.c.
Instead of modifying the top-level Makefile.objs, you would just edit
block/monitor/Makefile.objs instead and add the filename there. I don't
think you need to understand any magic apart from knowing that is exists
and does what you would expect.
Kevin
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v2 2/6] blockdev: Split off basic bitmap operations for qemu-img
2020-05-08 11:37 ` Kevin Wolf
@ 2020-05-08 13:48 ` Eric Blake
0 siblings, 0 replies; 23+ messages in thread
From: Eric Blake @ 2020-05-08 13:48 UTC (permalink / raw)
To: Kevin Wolf; +Cc: Markus Armbruster, qemu-devel, qemu-block, Max Reitz
On 5/8/20 6:37 AM, Kevin Wolf wrote:
>>>> ---
>>>> Makefile.objs | 2 +-
>>>> include/sysemu/blockdev.h | 14 ++
>>>> blockbitmaps.c | 324 ++++++++++++++++++++++++++++++++++++++
>>>
>>> Hm. Can we get a better name? blockdev-bitmaps.c, for example?
>>
>> Sure, I'm open to bike-shed suggestions. I'd also _really_ love to make the
>> new file NOT live in the top-level, but that's a harder task that I'm not
>> sure how to do (it's easy to tweak Makefile.objs for another file in the
>> same directory, but harder to see through the magic to figure out how to
>> relocate things).
>
> Yes, please move it somewhere else. I'd suggest something like
> block/monitor/bitmap-qmp-cmds.c for the QMP command handlers, and if
> there are functions that are more generally useful, block/bitmaps.c.
>
> Instead of modifying the top-level Makefile.objs, you would just edit
> block/monitor/Makefile.objs instead and add the filename there. I don't
> think you need to understand any magic apart from knowing that is exists
> and does what you would expect.
Well, you still have to modify the top-level Makefile.objs to tell it to
look in the right subdirectories, but I got it figured out finally. v3
will use block/monitor/bitmap-qmp-cmds.c.
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
^ permalink raw reply [flat|nested] 23+ messages in thread
end of thread, other threads:[~2020-05-08 13:49 UTC | newest]
Thread overview: 23+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-04-21 21:20 [PATCH v2 0/6] qemu-img: Add convert --bitmaps Eric Blake
2020-04-21 21:20 ` [PATCH v2 1/6] docs: Sort sections on qemu-img subcommand parameters Eric Blake
2020-04-30 12:50 ` Max Reitz
2020-04-21 21:20 ` [PATCH v2 2/6] blockdev: Split off basic bitmap operations for qemu-img Eric Blake
2020-04-30 13:59 ` Max Reitz
2020-04-30 14:50 ` Eric Blake
2020-05-08 11:37 ` Kevin Wolf
2020-05-08 13:48 ` Eric Blake
2020-04-21 21:20 ` [PATCH v2 3/6] qemu-img: Add bitmap sub-command Eric Blake
2020-04-30 14:55 ` Max Reitz
2020-04-30 15:21 ` Eric Blake
2020-05-04 10:01 ` Max Reitz
2020-05-04 13:28 ` Eric Blake
2020-04-21 21:20 ` [PATCH v2 4/6] qcow2: Expose bitmaps' size during measure Eric Blake
2020-05-04 11:36 ` Max Reitz
2020-05-04 13:44 ` Eric Blake
2020-04-21 21:20 ` [PATCH v2 5/6] qemu-img: Add convert --bitmaps option Eric Blake
2020-05-04 12:14 ` Max Reitz
2020-04-21 21:20 ` [PATCH v2 6/6] iotests: Add test 291 to for qemu-img bitmap coverage Eric Blake
2020-05-04 13:05 ` Max Reitz
2020-05-05 21:22 ` Eric Blake
2020-04-21 22:22 ` [PATCH v2 0/6] qemu-img: Add convert --bitmaps no-reply
2020-04-21 22:49 ` [PATCH] fixup! qemu-img: Add bitmap sub-command Eric Blake
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.