On 11/09/2016 01:17 PM, Vladimir Sementsov-Ogievskiy wrote:
Add dirty bitmap extension as specified in docs/specs/qcow2.txt.
For now, just mirror extension header into Qcow2 state and check
constraints.
For now, disable image resize if it has bitmaps. It will be fixed later.
Signed-off-by: Vladimir Sementsov-Ogievskiy <address@hidden>
---
block/qcow2.c | 101
++++++++++++++++++++++++++++++++++++++++++++++++++++++++--
block/qcow2.h | 24 ++++++++++++++
2 files changed, 123 insertions(+), 2 deletions(-)
diff --git a/block/qcow2.c b/block/qcow2.c
index 6d5689a..c6050de 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -63,6 +63,7 @@ typedef struct {
#define QCOW2_EXT_MAGIC_END 0
#define QCOW2_EXT_MAGIC_BACKING_FORMAT 0xE2792ACA
#define QCOW2_EXT_MAGIC_FEATURE_TABLE 0x6803f857
+#define QCOW2_EXT_MAGIC_DIRTY_BITMAPS 0x23852875
static int qcow2_probe(const uint8_t *buf, int buf_size, const char
*filename)
{
@@ -92,6 +93,7 @@ static int qcow2_read_extensions(BlockDriverState
*bs, uint64_t start_offset,
QCowExtension ext;
uint64_t offset;
int ret;
+ Qcow2BitmapHeaderExt bitmaps_ext;
#ifdef DEBUG_EXT
printf("qcow2_read_extensions: start=%ld end=%ld\n",
start_offset, end_offset);
Unrelated, the comment at the top of qcow2_read_extensions is sorely
lacking.
@@ -162,6 +164,75 @@ static int
qcow2_read_extensions(BlockDriverState *bs, uint64_t start_offset,
}
break;
+ case QCOW2_EXT_MAGIC_DIRTY_BITMAPS:
+ if (ext.len != sizeof(bitmaps_ext)) {
+ error_setg_errno(errp, -ret, "ERROR: bitmaps_ext: "
+ "Invalid extension length");
+ return -EINVAL;
+ }
+
+ if (!(s->autoclear_features &
QCOW2_AUTOCLEAR_DIRTY_BITMAPS)) {
+ fprintf(stderr,
+ "WARNING: bitmaps_ext: autoclear flag is not "
+ "set, all bitmaps will be considered as
inconsistent");
+ break;
+ }
+
I might drop the "as" and just say "WARNING: bitmaps_ext: [the]
autoclear flag is not set. All bitmaps will be considered inconsistent."
This may be a good place for Eric to check our English.
+ ret = bdrv_pread(bs->file, offset, &bitmaps_ext, ext.len);
+ if (ret < 0) {
+ error_setg_errno(errp, -ret, "ERROR: bitmaps_ext: "
+ "Could not read ext header");
+ return ret;
+ }
+
+ if (bitmaps_ext.reserved32 != 0) {
+ error_setg_errno(errp, -ret, "ERROR: bitmaps_ext: "
+ "Reserved field is not zero");
+ return -EINVAL;
+ }
+
+ be32_to_cpus(&bitmaps_ext.nb_bitmaps);
+ be64_to_cpus(&bitmaps_ext.bitmap_directory_size);
+ be64_to_cpus(&bitmaps_ext.bitmap_directory_offset);
+
+ if (bitmaps_ext.nb_bitmaps > QCOW_MAX_DIRTY_BITMAPS) {
+ error_setg(errp, "ERROR: bitmaps_ext: "
+ "too many dirty bitmaps");
I might opt for something more like "File %s has %d bitmaps, exceeding
the QEMU supported maximum of %d" to be a little more informative than
"too many." (How many is too many? How many do we have?)
+ return -EINVAL;
+ }
+
+ if (bitmaps_ext.nb_bitmaps == 0) {
+ error_setg(errp, "ERROR: bitmaps_ext: "
+ "found bitmaps extension with zero
bitmaps");
+ return -EINVAL;
+ }
+
+ if (bitmaps_ext.bitmap_directory_offset &
(s->cluster_size - 1)) {
+ error_setg(errp, "ERROR: bitmaps_ext: "
+ "invalid dirty bitmap directory
offset");
+ return -EINVAL;
+ }
+
Don't we have a helper for checking alignment? We've got
"validate_table_offset." Can we use that?
+ if (bitmaps_ext.bitmap_directory_size >
+ QCOW_MAX_DIRTY_BITMAP_DIRECTORY_SIZE) {
+ error_setg(errp, "ERROR: bitmaps_ext: "
+ "too large dirty bitmap directory");
+ return -EINVAL;
+ }
+
"Too large dirty bitmap" is an awkward phrasing, because it turns the
entire message into a large compound noun.
I suggest working in a verb into the message, like "is" or "exceeds,"
here are some suggestions:
"[the] dirty bitmap directory size is too large" or "[the] dirty
bitmap directory size (%zu) exceeds [the] maximum supported size (%zu)"
I can't decide if it's appropriate to include or exclude the article.
Luckily nobody else knows how English works either.
+ s->nb_bitmaps = bitmaps_ext.nb_bitmaps;
+ s->bitmap_directory_offset =
+ bitmaps_ext.bitmap_directory_offset;
+ s->bitmap_directory_size =
+ bitmaps_ext.bitmap_directory_size;
+
+#ifdef DEBUG_EXT
+ printf("Qcow2: Got dirty bitmaps extension:"
+ " offset=%" PRIu64 " nb_bitmaps=%" PRIu32 "\n",
+ s->bitmap_directory_offset, s->nb_bitmaps);
+#endif
+ break;
+
default:
/* unknown magic - save it in case we need to rewrite
the header */
{
@@ -1144,8 +1215,9 @@ static int qcow2_open(BlockDriverState *bs,
QDict *options, int flags,
}
/* Clear unknown autoclear feature bits */
- if (!bs->read_only && !(flags & BDRV_O_INACTIVE) &&
s->autoclear_features) {
- s->autoclear_features = 0;
+ if (!bs->read_only && !(flags & BDRV_O_INACTIVE) &&
+ (s->autoclear_features & ~QCOW2_AUTOCLEAR_MASK)) {
+ s->autoclear_features &= QCOW2_AUTOCLEAR_MASK;
ret = qcow2_update_header(bs);
if (ret < 0) {
error_setg_errno(errp, -ret, "Could not update qcow2
header");
@@ -1944,6 +2016,24 @@ int qcow2_update_header(BlockDriverState *bs)
buflen -= ret;
}
+ if (s->nb_bitmaps > 0) {
+ Qcow2BitmapHeaderExt bitmaps_header = {
+ .nb_bitmaps = cpu_to_be32(s->nb_bitmaps),
+ .bitmap_directory_size =
+ cpu_to_be64(s->bitmap_directory_size),
+ .bitmap_directory_offset =
+ cpu_to_be64(s->bitmap_directory_offset)
+ };
+ ret = header_ext_add(buf, QCOW2_EXT_MAGIC_DIRTY_BITMAPS,
+ &bitmaps_header, sizeof(bitmaps_header),
+ buflen);
+ if (ret < 0) {
+ goto fail;
+ }
+ buf += ret;
+ buflen -= ret;
+ }
+
/* Keep unknown header extensions */
QLIST_FOREACH(uext, &s->unknown_header_ext, next) {
ret = header_ext_add(buf, uext->magic, uext->data,
uext->len, buflen);
@@ -2514,6 +2604,13 @@ static int qcow2_truncate(BlockDriverState
*bs, int64_t offset)
return -ENOTSUP;
}
+ /* cannot proceed if image has bitmaps */
+ if (s->nb_bitmaps) {
+ /* TODO: resize bitmaps in the image */
+ error_report("Can't resize an image which has bitmaps");
+ return -ENOTSUP;
+ }
+
I guess this is an open problem and not one we address in this series.
/* shrinking is currently not supported */
if (offset < bs->total_sectors * 512) {
error_report("qcow2 doesn't support shrinking images yet");
diff --git a/block/qcow2.h b/block/qcow2.h
index 92203a8..be9ba32 100644
--- a/block/qcow2.h
+++ b/block/qcow2.h
@@ -52,6 +52,10 @@
* space for snapshot names and IDs */
#define QCOW_MAX_SNAPSHOTS_SIZE (1024 * QCOW_MAX_SNAPSHOTS)
+/* Bitmap header extension constraints */
+#define QCOW_MAX_DIRTY_BITMAPS 65535
+#define QCOW_MAX_DIRTY_BITMAP_DIRECTORY_SIZE (1024 *
QCOW_MAX_DIRTY_BITMAPS)
+
64MiB for just the headers seems like a really generous limit that we
will never, ever, ever exhaust.
(I look forward to future versions of QEMU where this feedback item is
laughed at playfully.)
/* indicate that the refcount of the referenced cluster is exactly
one. */
#define QCOW_OFLAG_COPIED (1ULL << 63)
/* indicate that the cluster is compressed (they never have the
copied flag) */
@@ -195,6 +199,15 @@ enum {
QCOW2_COMPAT_FEAT_MASK = QCOW2_COMPAT_LAZY_REFCOUNTS,
};
+/* Autoclear feature bits */
+enum {
+ QCOW2_AUTOCLEAR_DIRTY_BITMAPS_BITNR = 0,
+ QCOW2_AUTOCLEAR_DIRTY_BITMAPS =
+ 1 << QCOW2_AUTOCLEAR_DIRTY_BITMAPS_BITNR,
+
+ QCOW2_AUTOCLEAR_MASK =
QCOW2_AUTOCLEAR_DIRTY_BITMAPS,
+};
+
This seems like a strange construct to me -- the ranges for _BITNR and
mask variants will begin colliding as soon as we add just one more
constant to the list.
Enumerations do allow this, but I generally don't like using a single
enumeration to describe more than one thing.
Do we really need the BITNR variants at all? Why not the following:
enum {
QCOW2_AUTOCLEAR_DIRTY_BITMAPS = 1 << 0,
};
#define QCOW2_AUTOCLEAR_MASK QCOW2_AUTOCLEAR_DIRTY_BITMAPS