| Message ID | 1475232808-4852-7-git-send-email-vsementsov@virtuozzo.com |
|---|---|
| State | New |
| Headers | show |
On 30.09.2016 12:53, 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 | 83 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ > block/qcow2.h | 4 +++ > 2 files changed, 87 insertions(+) > > diff --git a/block/qcow2.c b/block/qcow2.c > index c079aa8..08c4ef9 100644 > --- a/block/qcow2.c > +++ b/block/qcow2.c [...] > @@ -162,6 +164,62 @@ static int qcow2_read_extensions(BlockDriverState *bs, uint64_t start_offset, > } > break; > > + case QCOW2_EXT_MAGIC_DIRTY_BITMAPS: > + ret = bdrv_pread(bs->file, offset, &bitmaps_ext, ext.len); Overflows with ext.len > sizeof(bitmaps_ext). (ext.len < sizeof(bitmaps_ext) is also wrong, but less dramatically so.) > + 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."); Please drop the full stop at the end. > + 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: " > + "wrong dirty bitmap directory offset"); s/wrong/invalid/ > + 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 */ > { [...] > @@ -2509,6 +2585,13 @@ static int qcow2_truncate(BlockDriverState *bs, int64_t offset) > return -ENOTSUP; > } > > + /* cannot proceed if image has bitmaps */ > + if (s->nb_bitmaps) { > + /* FIXME */ I'd call it a TODO, but that's probably a matter of taste. > + 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"); Max
On 01.10.2016 17:46, Max Reitz wrote: > On 30.09.2016 12:53, 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 | 83 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ >> block/qcow2.h | 4 +++ >> 2 files changed, 87 insertions(+) >> >> diff --git a/block/qcow2.c b/block/qcow2.c >> index c079aa8..08c4ef9 100644 >> --- a/block/qcow2.c >> +++ b/block/qcow2.c > [...] > >> @@ -162,6 +164,62 @@ static int qcow2_read_extensions(BlockDriverState *bs, uint64_t start_offset, >> } >> break; >> >> + case QCOW2_EXT_MAGIC_DIRTY_BITMAPS: >> + ret = bdrv_pread(bs->file, offset, &bitmaps_ext, ext.len); > Overflows with ext.len > sizeof(bitmaps_ext). > > (ext.len < sizeof(bitmaps_ext) is also wrong, but less dramatically so.) > >> + 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."); > Please drop the full stop at the end. what do you mean? goto to fail: here? or not stop at all, just print error? > >> + 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: " >> + "wrong dirty bitmap directory offset"); > s/wrong/invalid/ > >> + 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 */ >> { > [...] > >> @@ -2509,6 +2585,13 @@ static int qcow2_truncate(BlockDriverState *bs, int64_t offset) >> return -ENOTSUP; >> } >> >> + /* cannot proceed if image has bitmaps */ >> + if (s->nb_bitmaps) { >> + /* FIXME */ > I'd call it a TODO, but that's probably a matter of taste. > >> + 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"); > Max >
On 11.10.2016 14:09, Vladimir Sementsov-Ogievskiy wrote: > On 01.10.2016 17:46, Max Reitz wrote: >> On 30.09.2016 12:53, 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 | 83 >>> +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ >>> block/qcow2.h | 4 +++ >>> 2 files changed, 87 insertions(+) >>> >>> diff --git a/block/qcow2.c b/block/qcow2.c >>> index c079aa8..08c4ef9 100644 >>> --- a/block/qcow2.c >>> +++ b/block/qcow2.c >> [...] >> >>> @@ -162,6 +164,62 @@ static int >>> qcow2_read_extensions(BlockDriverState *bs, uint64_t start_offset, >>> } >>> break; >>> + case QCOW2_EXT_MAGIC_DIRTY_BITMAPS: >>> + ret = bdrv_pread(bs->file, offset, &bitmaps_ext, ext.len); >> Overflows with ext.len > sizeof(bitmaps_ext). >> >> (ext.len < sizeof(bitmaps_ext) is also wrong, but less dramatically so.) >> >>> + 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."); >> Please drop the full stop at the end. > > what do you mean? goto to fail: here? or not stop at all, just print error? The "." at the end of the message. :-) (https://en.wikipedia.org/wiki/Full_stop) Max
On 12.10.2016 21:21, Max Reitz wrote: > On 11.10.2016 14:09, Vladimir Sementsov-Ogievskiy wrote: >> On 01.10.2016 17:46, Max Reitz wrote: >>> On 30.09.2016 12:53, 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 | 83 >>>> +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ >>>> block/qcow2.h | 4 +++ >>>> 2 files changed, 87 insertions(+) >>>> >>>> diff --git a/block/qcow2.c b/block/qcow2.c >>>> index c079aa8..08c4ef9 100644 >>>> --- a/block/qcow2.c >>>> +++ b/block/qcow2.c >>> [...] >>> >>>> @@ -162,6 +164,62 @@ static int >>>> qcow2_read_extensions(BlockDriverState *bs, uint64_t start_offset, >>>> } >>>> break; >>>> + case QCOW2_EXT_MAGIC_DIRTY_BITMAPS: >>>> + ret = bdrv_pread(bs->file, offset, &bitmaps_ext, ext.len); >>> Overflows with ext.len > sizeof(bitmaps_ext). >>> >>> (ext.len < sizeof(bitmaps_ext) is also wrong, but less dramatically so.) >>> >>>> + 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."); >>> Please drop the full stop at the end. >> what do you mean? goto to fail: here? or not stop at all, just print error? > The "." at the end of the message. :-) > > (https://en.wikipedia.org/wiki/Full_stop) > > Max > aha, cool. didn't know this. )
diff --git a/block/qcow2.c b/block/qcow2.c index c079aa8..08c4ef9 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,62 @@ static int qcow2_read_extensions(BlockDriverState *bs, uint64_t start_offset, } break; + case QCOW2_EXT_MAGIC_DIRTY_BITMAPS: + 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: " + "wrong 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 */ { @@ -1939,6 +1997,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); @@ -2509,6 +2585,13 @@ static int qcow2_truncate(BlockDriverState *bs, int64_t offset) return -ENOTSUP; } + /* cannot proceed if image has bitmaps */ + if (s->nb_bitmaps) { + /* FIXME */ + 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 0480b8b..c068b2c 100644 --- a/block/qcow2.h +++ b/block/qcow2.h @@ -292,6 +292,10 @@ typedef struct BDRVQcow2State { unsigned int nb_snapshots; QCowSnapshot *snapshots; + uint64_t bitmap_directory_offset; + uint64_t bitmap_directory_size; + uint32_t nb_bitmaps; + 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 | 83 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ block/qcow2.h | 4 +++ 2 files changed, 87 insertions(+)