| Message ID | 1478715476-132280-8-git-send-email-vsementsov@virtuozzo.com |
|---|---|
| State | New |
| Headers | show |
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 <vsementsov@virtuozzo.com> > --- > 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 > enum qcow2_discard_type { > QCOW2_DISCARD_NEVER = 0, > QCOW2_DISCARD_ALWAYS, > @@ -222,6 +235,13 @@ typedef uint64_t Qcow2GetRefcountFunc(const void *refcount_array, > typedef void Qcow2SetRefcountFunc(void *refcount_array, > uint64_t index, uint64_t value); > > +typedef struct Qcow2BitmapHeaderExt { > + uint32_t nb_bitmaps; > + uint32_t reserved32; > + uint64_t bitmap_directory_size; > + uint64_t bitmap_directory_offset; > +} QEMU_PACKED Qcow2BitmapHeaderExt; > + > typedef struct BDRVQcow2State { > int cluster_bits; > int cluster_size; > @@ -263,6 +283,10 @@ typedef struct BDRVQcow2State { > unsigned int nb_snapshots; > QCowSnapshot *snapshots; > > + uint32_t nb_bitmaps; > + uint64_t bitmap_directory_size; > + uint64_t bitmap_directory_offset; > + > int flags; > int qcow_version; > bool use_lazy_refcounts; > Only style issues from my POV... Reviewed-by: John Snow <jsnow@redhat.com>
On 11/15/2016 03:42 PM, John Snow wrote: > > > 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 <vsementsov@virtuozzo.com> >> --- >> + 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." Even the 'bitmaps_ext:' prefix seems a bit redundant, since the rest of the message talks about bitmaps. > > This may be a good place for Eric to check our English. > Maybe take the message from a different angle: WARNING: all bitmaps are considered inconsistent since the autoclear flag was cleared or WARNING: the autoclear flag was cleared, so all bitmaps are considered inconsistent or even skip the technical details, and report it with a longer message but while still sounding legible: WARNING: a program lacking bitmap support modified this file, so all bitmaps are now considered inconsistent >> + >> + 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?) > The use of ERROR: in error_setg() seems over the top. John's proposed wording is nice, here. >> + return -EINVAL; >> + } >> + >> + if (bitmaps_ext.nb_bitmaps == 0) { >> + error_setg(errp, "ERROR: bitmaps_ext: " >> + "found bitmaps extension with zero >> bitmaps"); So why is it an error to have a bitmaps extension but no bitmaps allocated? Is that too strict? Again, the ERROR: prefix is a bit much in error_setg(). > >> + 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)" The latter is the most informative. > > I can't decide if it's appropriate to include or exclude the article. Yep, choosing when to use articles is sometimes subjective. the/blank sounds odd - it's the only combo I'd avoid blank/blank seems reasonable, and has the benefit of being short blank/the seems reasonable the/the seems rather formal, but still works > > Luckily nobody else knows how English works either. What, there's rules to follow? :)
On 11/15/2016 05:39 PM, Eric Blake wrote: > On 11/15/2016 03:42 PM, John Snow wrote: >> >> >> 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 <vsementsov@virtuozzo.com> >>> --- > >>> + 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." > > Even the 'bitmaps_ext:' prefix seems a bit redundant, since the rest of > the message talks about bitmaps. > >> >> This may be a good place for Eric to check our English. >> > > Maybe take the message from a different angle: > > WARNING: all bitmaps are considered inconsistent since the autoclear > flag was cleared > > or > > WARNING: the autoclear flag was cleared, so all bitmaps are considered > inconsistent > > or even skip the technical details, and report it with a longer message > but while still sounding legible: > > WARNING: a program lacking bitmap support modified this file, so all > bitmaps are now considered inconsistent > > >>> + >>> + 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?) >> > > The use of ERROR: in error_setg() seems over the top. > > John's proposed wording is nice, here. > >>> + return -EINVAL; >>> + } >>> + >>> + if (bitmaps_ext.nb_bitmaps == 0) { >>> + error_setg(errp, "ERROR: bitmaps_ext: " >>> + "found bitmaps extension with zero >>> bitmaps"); > > So why is it an error to have a bitmaps extension but no bitmaps > allocated? Is that too strict? > > Again, the ERROR: prefix is a bit much in error_setg(). > We wrote it that way in the spec: ```` The fields of the bitmaps extension are: Byte 0 - 3: nb_bitmaps The number of bitmaps contained in the image. Must be greater than or equal to 1. ```` So if we have no bitmaps, we must have no header. > >> >>> + 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)" > > The latter is the most informative. > >> >> I can't decide if it's appropriate to include or exclude the article. > > Yep, choosing when to use articles is sometimes subjective. > > the/blank sounds odd - it's the only combo I'd avoid > blank/blank seems reasonable, and has the benefit of being short > blank/the seems reasonable > the/the seems rather formal, but still works > 'Dirty bitmap directory size (%zu) exceeds the maximum supported size (%zu)' is probably my favorite upon review. >> >> Luckily nobody else knows how English works either. > > What, there's rules to follow? :) > Not that I am aware of. Thanks for the proofreading.
16.11.2016 00:42, John Snow wrote: > > > 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 <vsementsov@virtuozzo.com> >> --- >> 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 I've just copied the same structure for 'Compatible feature bits'. And it was even already discussed). Anyway, this code should be refactored with separate patch, before or after these series. > > >> enum qcow2_discard_type { >> QCOW2_DISCARD_NEVER = 0, >> QCOW2_DISCARD_ALWAYS, >> @@ -222,6 +235,13 @@ typedef uint64_t Qcow2GetRefcountFunc(const void >> *refcount_array, >> typedef void Qcow2SetRefcountFunc(void *refcount_array, >> uint64_t index, uint64_t value); >> >> +typedef struct Qcow2BitmapHeaderExt { >> + uint32_t nb_bitmaps; >> + uint32_t reserved32; >> + uint64_t bitmap_directory_size; >> + uint64_t bitmap_directory_offset; >> +} QEMU_PACKED Qcow2BitmapHeaderExt; >> + >> typedef struct BDRVQcow2State { >> int cluster_bits; >> int cluster_size; >> @@ -263,6 +283,10 @@ typedef struct BDRVQcow2State { >> unsigned int nb_snapshots; >> QCowSnapshot *snapshots; >> >> + uint32_t nb_bitmaps; >> + uint64_t bitmap_directory_size; >> + uint64_t bitmap_directory_offset; >> + >> int flags; >> int qcow_version; >> bool use_lazy_refcounts; >> > > Only style issues from my POV... > > Reviewed-by: John Snow <jsnow@redhat.com>
16.11.2016 00:42, John Snow wrote: > > > 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 <vsementsov@virtuozzo.com> >> --- >> 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? function validate_table_offset() is strange, it actually validates not offset, but offset, size and entry-size.. It should be probably renamed to vailidate_table().. And I have no "entry size" for bitmap directory, as it's entries are of different sizes. > >> + 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 > > >> enum qcow2_discard_type { >> QCOW2_DISCARD_NEVER = 0, >> QCOW2_DISCARD_ALWAYS, >> @@ -222,6 +235,13 @@ typedef uint64_t Qcow2GetRefcountFunc(const void >> *refcount_array, >> typedef void Qcow2SetRefcountFunc(void *refcount_array, >> uint64_t index, uint64_t value); >> >> +typedef struct Qcow2BitmapHeaderExt { >> + uint32_t nb_bitmaps; >> + uint32_t reserved32; >> + uint64_t bitmap_directory_size; >> + uint64_t bitmap_directory_offset; >> +} QEMU_PACKED Qcow2BitmapHeaderExt; >> + >> typedef struct BDRVQcow2State { >> int cluster_bits; >> int cluster_size; >> @@ -263,6 +283,10 @@ typedef struct BDRVQcow2State { >> unsigned int nb_snapshots; >> QCowSnapshot *snapshots; >> >> + uint32_t nb_bitmaps; >> + uint64_t bitmap_directory_size; >> + uint64_t bitmap_directory_offset; >> + >> int flags; >> int qcow_version; >> bool use_lazy_refcounts; >> > > Only style issues from my POV... > > Reviewed-by: John Snow <jsnow@redhat.com>
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); @@ -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; + } + + 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"); + 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; + } + + 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; + } + + 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; + } + /* 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) + /* 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, +}; + enum qcow2_discard_type { QCOW2_DISCARD_NEVER = 0, QCOW2_DISCARD_ALWAYS, @@ -222,6 +235,13 @@ typedef uint64_t Qcow2GetRefcountFunc(const void *refcount_array, typedef void Qcow2SetRefcountFunc(void *refcount_array, uint64_t index, uint64_t value); +typedef struct Qcow2BitmapHeaderExt { + uint32_t nb_bitmaps; + uint32_t reserved32; + uint64_t bitmap_directory_size; + uint64_t bitmap_directory_offset; +} QEMU_PACKED Qcow2BitmapHeaderExt; + typedef struct BDRVQcow2State { int cluster_bits; int cluster_size; @@ -263,6 +283,10 @@ typedef struct BDRVQcow2State { unsigned int nb_snapshots; QCowSnapshot *snapshots; + uint32_t nb_bitmaps; + uint64_t bitmap_directory_size; + uint64_t bitmap_directory_offset; + int flags; int qcow_version; bool use_lazy_refcounts;
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 <vsementsov@virtuozzo.com> --- block/qcow2.c | 101 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++-- block/qcow2.h | 24 ++++++++++++++ 2 files changed, 123 insertions(+), 2 deletions(-)