| Message ID | 1433776886-27239-7-git-send-email-vsementsov@virtuozzo.com |
|---|---|
| State | New |
| Headers | show |
On Mon, Jun 08, 2015 at 06:21:24PM +0300, Vladimir Sementsov-Ogievskiy wrote: > @@ -928,8 +936,9 @@ static int qcow2_open(BlockDriverState *bs, QDict *options, int flags, > } > > /* Clear unknown autoclear feature bits */ > - if (!bs->read_only && !(flags & BDRV_O_INCOMING) && s->autoclear_features) { > - s->autoclear_features = 0; > + if (!bs->read_only && !(flags & BDRV_O_INCOMING) && > + (s->autoclear_features & ~QCOW2_AUTOCLEAR_MASK)) { > + s->autoclear_features |= QCOW2_AUTOCLEAR_MASK; This should be bitwise-and instead of bitwise-or: s->autoclear_features &= QCOW2_AUTOCLEAR_MASK Otherwise we set features that happen to be in QCOW2_AUTOCLEAR_MASK but were not enabled by the user. Right now that's not fatal but if other features are added to QCOW2_AUTOCLEAR_MASK it could introduce a bug, depending on the feature semantics.
On Mon, Jun 08, 2015 at 06:21:24PM +0300, Vladimir Sementsov-Ogievskiy wrote: > diff --git a/block/qcow2.c b/block/qcow2.c > index 406e55d..f85a55a 100644 > --- a/block/qcow2.c > +++ b/block/qcow2.c > @@ -182,6 +182,14 @@ static int qcow2_read_extensions(BlockDriverState *bs, uint64_t start_offset, > return ret; > } > > + if (!(s->autoclear_features & QCOW2_AUTOCLEAR_DIRTY_BITMAPS) && > + s->nb_dirty_bitmaps > 0) { > + ret = qcow2_delete_all_dirty_bitmaps(bs, errp); > + if (ret < 0) { > + return ret; > + } > + } > + What if the file is read-only? We shouldn't modify the file in qcow2_read_extensions().
On 06/08/2015 11:21 AM, Vladimir Sementsov-Ogievskiy wrote: > From: Vladimir Sementsov-Ogievskiy <vsementsov@parallels.com> > > Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> > --- > block/qcow2-dirty-bitmap.c | 5 +++++ > block/qcow2.c | 13 +++++++++++-- > block/qcow2.h | 9 +++++++++ > 3 files changed, 25 insertions(+), 2 deletions(-) > > diff --git a/block/qcow2-dirty-bitmap.c b/block/qcow2-dirty-bitmap.c > index db83112..686a121 100644 > --- a/block/qcow2-dirty-bitmap.c > +++ b/block/qcow2-dirty-bitmap.c > @@ -188,6 +188,11 @@ static int qcow2_write_dirty_bitmaps(BlockDriverState *bs) > > s->dirty_bitmaps_offset = dirty_bitmaps_offset; > s->dirty_bitmaps_size = dirty_bitmaps_size; > + if (s->nb_dirty_bitmaps > 0) { > + s->autoclear_features |= QCOW2_AUTOCLEAR_DIRTY_BITMAPS; > + } else { > + s->autoclear_features &= ~QCOW2_AUTOCLEAR_DIRTY_BITMAPS; > + } > ret = qcow2_update_header(bs); > if (ret < 0) { > fprintf(stderr, "Could not update qcow2 header\n"); > diff --git a/block/qcow2.c b/block/qcow2.c > index 406e55d..f85a55a 100644 > --- a/block/qcow2.c > +++ b/block/qcow2.c > @@ -182,6 +182,14 @@ static int qcow2_read_extensions(BlockDriverState *bs, uint64_t start_offset, > return ret; > } > > + if (!(s->autoclear_features & QCOW2_AUTOCLEAR_DIRTY_BITMAPS) && > + s->nb_dirty_bitmaps > 0) { > + ret = qcow2_delete_all_dirty_bitmaps(bs, errp); > + if (ret < 0) { > + return ret; > + } > + } > + > #ifdef DEBUG_EXT > printf("Qcow2: Got dirty bitmaps extension:" > " offset=%" PRIu64 " nb_bitmaps=%" PRIu32 "\n", > @@ -928,8 +936,9 @@ static int qcow2_open(BlockDriverState *bs, QDict *options, int flags, > } > > /* Clear unknown autoclear feature bits */ > - if (!bs->read_only && !(flags & BDRV_O_INCOMING) && s->autoclear_features) { > - s->autoclear_features = 0; > + if (!bs->read_only && !(flags & BDRV_O_INCOMING) && > + (s->autoclear_features & ~QCOW2_AUTOCLEAR_MASK)) { > + s->autoclear_features |= QCOW2_AUTOCLEAR_MASK; Like Stefan already mentioned, fixing this |= to &= will fix iotest 036, which is otherwise broken by this patch. > ret = qcow2_update_header(bs); > if (ret < 0) { > error_setg_errno(errp, -ret, "Could not update qcow2 header"); > diff --git a/block/qcow2.h b/block/qcow2.h > index b5e576c..14bd6f9 100644 > --- a/block/qcow2.h > +++ b/block/qcow2.h > @@ -215,6 +215,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, > +}; > + I find it a little awkward to have an enum with three different kinds of data in it, unless I am reading this incorrectly. (bit position, bit masks, and accumulated bit mask.) Just enumerating the indices is probably sufficient: enum { QCOW2_AUTOCLEAR_BEGIN = 0, QCOW2_AUTOCLEAR_DIRTY_BITMAPS = QCOW2_AUTOCLEAR_BEGIN, ..., QCOW2_AUTOCLEAR_END } and then the QCOW2_AUTOCLEAR_MASK can either be programmatically defined via a function, or just pre-computed as a #define. If you still want the mask definitions, you could do something cheeky like this: #define AUTOCLEAR_MASK(X) (1 << QCOW2_AUTOCLEAR_ ## X) and then you can use things like AUTOCLEAR_MASK(DIRTY_BITMAPS) without having to create and maintain two separate tables if you want both forms easily available. > enum qcow2_discard_type { > QCOW2_DISCARD_NEVER = 0, > QCOW2_DISCARD_ALWAYS, >
Am 11.06.2015 um 01:42 hat John Snow geschrieben: > > > On 06/08/2015 11:21 AM, Vladimir Sementsov-Ogievskiy wrote: > > From: Vladimir Sementsov-Ogievskiy <vsementsov@parallels.com> > > > > Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> > > --- > > block/qcow2-dirty-bitmap.c | 5 +++++ > > block/qcow2.c | 13 +++++++++++-- > > block/qcow2.h | 9 +++++++++ > > 3 files changed, 25 insertions(+), 2 deletions(-) > > > > diff --git a/block/qcow2-dirty-bitmap.c b/block/qcow2-dirty-bitmap.c > > index db83112..686a121 100644 > > --- a/block/qcow2-dirty-bitmap.c > > +++ b/block/qcow2-dirty-bitmap.c > > @@ -188,6 +188,11 @@ static int qcow2_write_dirty_bitmaps(BlockDriverState *bs) > > > > s->dirty_bitmaps_offset = dirty_bitmaps_offset; > > s->dirty_bitmaps_size = dirty_bitmaps_size; > > + if (s->nb_dirty_bitmaps > 0) { > > + s->autoclear_features |= QCOW2_AUTOCLEAR_DIRTY_BITMAPS; > > + } else { > > + s->autoclear_features &= ~QCOW2_AUTOCLEAR_DIRTY_BITMAPS; > > + } > > ret = qcow2_update_header(bs); > > if (ret < 0) { > > fprintf(stderr, "Could not update qcow2 header\n"); > > diff --git a/block/qcow2.c b/block/qcow2.c > > index 406e55d..f85a55a 100644 > > --- a/block/qcow2.c > > +++ b/block/qcow2.c > > @@ -182,6 +182,14 @@ static int qcow2_read_extensions(BlockDriverState *bs, uint64_t start_offset, > > return ret; > > } > > > > + if (!(s->autoclear_features & QCOW2_AUTOCLEAR_DIRTY_BITMAPS) && > > + s->nb_dirty_bitmaps > 0) { > > + ret = qcow2_delete_all_dirty_bitmaps(bs, errp); > > + if (ret < 0) { > > + return ret; > > + } > > + } > > + > > #ifdef DEBUG_EXT > > printf("Qcow2: Got dirty bitmaps extension:" > > " offset=%" PRIu64 " nb_bitmaps=%" PRIu32 "\n", > > @@ -928,8 +936,9 @@ static int qcow2_open(BlockDriverState *bs, QDict *options, int flags, > > } > > > > /* Clear unknown autoclear feature bits */ > > - if (!bs->read_only && !(flags & BDRV_O_INCOMING) && s->autoclear_features) { > > - s->autoclear_features = 0; > > + if (!bs->read_only && !(flags & BDRV_O_INCOMING) && > > + (s->autoclear_features & ~QCOW2_AUTOCLEAR_MASK)) { > > + s->autoclear_features |= QCOW2_AUTOCLEAR_MASK; > > Like Stefan already mentioned, fixing this |= to &= will fix iotest 036, > which is otherwise broken by this patch. > > > ret = qcow2_update_header(bs); > > if (ret < 0) { > > error_setg_errno(errp, -ret, "Could not update qcow2 header"); > > diff --git a/block/qcow2.h b/block/qcow2.h > > index b5e576c..14bd6f9 100644 > > --- a/block/qcow2.h > > +++ b/block/qcow2.h > > @@ -215,6 +215,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, > > +}; > > + > > I find it a little awkward to have an enum with three different kinds of > data in it, unless I am reading this incorrectly. (bit position, bit > masks, and accumulated bit mask.) This is only consistent with the enums for incompatible and compatible feature flags. If we were to change that, we should change it everywhere. > Just enumerating the indices is probably sufficient: > > enum { > QCOW2_AUTOCLEAR_BEGIN = 0, > QCOW2_AUTOCLEAR_DIRTY_BITMAPS = QCOW2_AUTOCLEAR_BEGIN, > ..., > QCOW2_AUTOCLEAR_END > } I don't mind the colour of the bikeshed, as long as all constants are explicitly defined. Letting the compiler assign integers when these integers are part of an external interface is too easy to break accidentally. Kevin
On 11.06.2015 02:42, John Snow wrote: > > On 06/08/2015 11:21 AM, Vladimir Sementsov-Ogievskiy wrote: >> From: Vladimir Sementsov-Ogievskiy <vsementsov@parallels.com> >> >> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> >> --- >> block/qcow2-dirty-bitmap.c | 5 +++++ >> block/qcow2.c | 13 +++++++++++-- >> block/qcow2.h | 9 +++++++++ >> 3 files changed, 25 insertions(+), 2 deletions(-) >> >> diff --git a/block/qcow2-dirty-bitmap.c b/block/qcow2-dirty-bitmap.c >> index db83112..686a121 100644 >> --- a/block/qcow2-dirty-bitmap.c >> +++ b/block/qcow2-dirty-bitmap.c >> @@ -188,6 +188,11 @@ static int qcow2_write_dirty_bitmaps(BlockDriverState *bs) >> >> s->dirty_bitmaps_offset = dirty_bitmaps_offset; >> s->dirty_bitmaps_size = dirty_bitmaps_size; >> + if (s->nb_dirty_bitmaps > 0) { >> + s->autoclear_features |= QCOW2_AUTOCLEAR_DIRTY_BITMAPS; >> + } else { >> + s->autoclear_features &= ~QCOW2_AUTOCLEAR_DIRTY_BITMAPS; >> + } >> ret = qcow2_update_header(bs); >> if (ret < 0) { >> fprintf(stderr, "Could not update qcow2 header\n"); >> diff --git a/block/qcow2.c b/block/qcow2.c >> index 406e55d..f85a55a 100644 >> --- a/block/qcow2.c >> +++ b/block/qcow2.c >> @@ -182,6 +182,14 @@ static int qcow2_read_extensions(BlockDriverState *bs, uint64_t start_offset, >> return ret; >> } >> >> + if (!(s->autoclear_features & QCOW2_AUTOCLEAR_DIRTY_BITMAPS) && >> + s->nb_dirty_bitmaps > 0) { >> + ret = qcow2_delete_all_dirty_bitmaps(bs, errp); >> + if (ret < 0) { >> + return ret; >> + } >> + } >> + >> #ifdef DEBUG_EXT >> printf("Qcow2: Got dirty bitmaps extension:" >> " offset=%" PRIu64 " nb_bitmaps=%" PRIu32 "\n", >> @@ -928,8 +936,9 @@ static int qcow2_open(BlockDriverState *bs, QDict *options, int flags, >> } >> >> /* Clear unknown autoclear feature bits */ >> - if (!bs->read_only && !(flags & BDRV_O_INCOMING) && s->autoclear_features) { >> - s->autoclear_features = 0; >> + if (!bs->read_only && !(flags & BDRV_O_INCOMING) && >> + (s->autoclear_features & ~QCOW2_AUTOCLEAR_MASK)) { >> + s->autoclear_features |= QCOW2_AUTOCLEAR_MASK; > Like Stefan already mentioned, fixing this |= to &= will fix iotest 036, > which is otherwise broken by this patch. > >> ret = qcow2_update_header(bs); >> if (ret < 0) { >> error_setg_errno(errp, -ret, "Could not update qcow2 header"); >> diff --git a/block/qcow2.h b/block/qcow2.h >> index b5e576c..14bd6f9 100644 >> --- a/block/qcow2.h >> +++ b/block/qcow2.h >> @@ -215,6 +215,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, >> +}; >> + > I find it a little awkward to have an enum with three different kinds of > data in it, unless I am reading this incorrectly. (bit position, bit > masks, and accumulated bit mask.) > > Just enumerating the indices is probably sufficient: > > enum { > QCOW2_AUTOCLEAR_BEGIN = 0, > QCOW2_AUTOCLEAR_DIRTY_BITMAPS = QCOW2_AUTOCLEAR_BEGIN, > ..., > QCOW2_AUTOCLEAR_END > } > > and then the QCOW2_AUTOCLEAR_MASK can either be programmatically defined > via a function, or just pre-computed as a #define. > > If you still want the mask definitions, you could do something cheeky > like this: > > #define AUTOCLEAR_MASK(X) (1 << QCOW2_AUTOCLEAR_ ## X) > > and then you can use things like AUTOCLEAR_MASK(DIRTY_BITMAPS) without > having to create and maintain two separate tables if you want both forms > easily available. This enum is made like enums for QCOW2_INCOMPAT_* and QCOW2_COMPAT_*, which are already in the code... Then, may I make a patch for them too? I agree, it is strange solution to put things of different nature to one enum. > >> enum qcow2_discard_type { >> QCOW2_DISCARD_NEVER = 0, >> QCOW2_DISCARD_ALWAYS, >>
On 06/11/2015 06:49 AM, Vladimir Sementsov-Ogievskiy wrote: > On 11.06.2015 02:42, John Snow wrote: >> >> On 06/08/2015 11:21 AM, Vladimir Sementsov-Ogievskiy wrote: >>> From: Vladimir Sementsov-Ogievskiy <vsementsov@parallels.com> >>> >>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> >>> --- >>> block/qcow2-dirty-bitmap.c | 5 +++++ >>> block/qcow2.c | 13 +++++++++++-- >>> block/qcow2.h | 9 +++++++++ >>> 3 files changed, 25 insertions(+), 2 deletions(-) >>> >>> diff --git a/block/qcow2-dirty-bitmap.c b/block/qcow2-dirty-bitmap.c >>> index db83112..686a121 100644 >>> --- a/block/qcow2-dirty-bitmap.c >>> +++ b/block/qcow2-dirty-bitmap.c >>> @@ -188,6 +188,11 @@ static int >>> qcow2_write_dirty_bitmaps(BlockDriverState *bs) >>> s->dirty_bitmaps_offset = dirty_bitmaps_offset; >>> s->dirty_bitmaps_size = dirty_bitmaps_size; >>> + if (s->nb_dirty_bitmaps > 0) { >>> + s->autoclear_features |= QCOW2_AUTOCLEAR_DIRTY_BITMAPS; >>> + } else { >>> + s->autoclear_features &= ~QCOW2_AUTOCLEAR_DIRTY_BITMAPS; >>> + } >>> ret = qcow2_update_header(bs); >>> if (ret < 0) { >>> fprintf(stderr, "Could not update qcow2 header\n"); >>> diff --git a/block/qcow2.c b/block/qcow2.c >>> index 406e55d..f85a55a 100644 >>> --- a/block/qcow2.c >>> +++ b/block/qcow2.c >>> @@ -182,6 +182,14 @@ static int >>> qcow2_read_extensions(BlockDriverState *bs, uint64_t start_offset, >>> return ret; >>> } >>> + if (!(s->autoclear_features & >>> QCOW2_AUTOCLEAR_DIRTY_BITMAPS) && >>> + s->nb_dirty_bitmaps > 0) { >>> + ret = qcow2_delete_all_dirty_bitmaps(bs, errp); >>> + if (ret < 0) { >>> + return ret; >>> + } >>> + } >>> + >>> #ifdef DEBUG_EXT >>> printf("Qcow2: Got dirty bitmaps extension:" >>> " offset=%" PRIu64 " nb_bitmaps=%" PRIu32 "\n", >>> @@ -928,8 +936,9 @@ static int qcow2_open(BlockDriverState *bs, QDict >>> *options, int flags, >>> } >>> /* Clear unknown autoclear feature bits */ >>> - if (!bs->read_only && !(flags & BDRV_O_INCOMING) && >>> s->autoclear_features) { >>> - s->autoclear_features = 0; >>> + if (!bs->read_only && !(flags & BDRV_O_INCOMING) && >>> + (s->autoclear_features & ~QCOW2_AUTOCLEAR_MASK)) { >>> + s->autoclear_features |= QCOW2_AUTOCLEAR_MASK; >> Like Stefan already mentioned, fixing this |= to &= will fix iotest 036, >> which is otherwise broken by this patch. >> >>> ret = qcow2_update_header(bs); >>> if (ret < 0) { >>> error_setg_errno(errp, -ret, "Could not update qcow2 >>> header"); >>> diff --git a/block/qcow2.h b/block/qcow2.h >>> index b5e576c..14bd6f9 100644 >>> --- a/block/qcow2.h >>> +++ b/block/qcow2.h >>> @@ -215,6 +215,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, >>> +}; >>> + >> I find it a little awkward to have an enum with three different kinds of >> data in it, unless I am reading this incorrectly. (bit position, bit >> masks, and accumulated bit mask.) >> >> Just enumerating the indices is probably sufficient: >> >> enum { >> QCOW2_AUTOCLEAR_BEGIN = 0, >> QCOW2_AUTOCLEAR_DIRTY_BITMAPS = QCOW2_AUTOCLEAR_BEGIN, >> ..., >> QCOW2_AUTOCLEAR_END >> } >> >> and then the QCOW2_AUTOCLEAR_MASK can either be programmatically defined >> via a function, or just pre-computed as a #define. >> >> If you still want the mask definitions, you could do something cheeky >> like this: >> >> #define AUTOCLEAR_MASK(X) (1 << QCOW2_AUTOCLEAR_ ## X) >> >> and then you can use things like AUTOCLEAR_MASK(DIRTY_BITMAPS) without >> having to create and maintain two separate tables if you want both forms >> easily available. > > > This enum is made like enums for QCOW2_INCOMPAT_* and QCOW2_COMPAT_*, > which are already in the code... Then, may I make a patch for them too? > I agree, it is strange solution to put things of different nature to one > enum. > Follow Kevin's lead, here -- It looked strange to me, but it _is_ best to follow the existing style. I didn't look at the surrounding code too carefully. > >> >>> enum qcow2_discard_type { >>> QCOW2_DISCARD_NEVER = 0, >>> QCOW2_DISCARD_ALWAYS, >>> > >
On 09.06.2015 18:50, Stefan Hajnoczi wrote: > On Mon, Jun 08, 2015 at 06:21:24PM +0300, Vladimir Sementsov-Ogievskiy wrote: >> diff --git a/block/qcow2.c b/block/qcow2.c >> index 406e55d..f85a55a 100644 >> --- a/block/qcow2.c >> +++ b/block/qcow2.c >> @@ -182,6 +182,14 @@ static int qcow2_read_extensions(BlockDriverState *bs, uint64_t start_offset, >> return ret; >> } >> >> + if (!(s->autoclear_features & QCOW2_AUTOCLEAR_DIRTY_BITMAPS) && >> + s->nb_dirty_bitmaps > 0) { >> + ret = qcow2_delete_all_dirty_bitmaps(bs, errp); >> + if (ret < 0) { >> + return ret; >> + } >> + } >> + > What if the file is read-only? > > We shouldn't modify the file in qcow2_read_extensions(). But where? In qcow2_open? Or nowhere? I think auto clear extensions should be cleared automatically..
On 27.08.2015 10:45, Vladimir Sementsov-Ogievskiy wrote: > On 09.06.2015 18:50, Stefan Hajnoczi wrote: >> On Mon, Jun 08, 2015 at 06:21:24PM +0300, Vladimir >> Sementsov-Ogievskiy wrote: >>> diff --git a/block/qcow2.c b/block/qcow2.c >>> index 406e55d..f85a55a 100644 >>> --- a/block/qcow2.c >>> +++ b/block/qcow2.c >>> @@ -182,6 +182,14 @@ static int >>> qcow2_read_extensions(BlockDriverState *bs, uint64_t start_offset, >>> return ret; >>> } >>> + if (!(s->autoclear_features & >>> QCOW2_AUTOCLEAR_DIRTY_BITMAPS) && >>> + s->nb_dirty_bitmaps > 0) { >>> + ret = qcow2_delete_all_dirty_bitmaps(bs, errp); >>> + if (ret < 0) { >>> + return ret; >>> + } >>> + } >>> + >> What if the file is read-only? >> >> We shouldn't modify the file in qcow2_read_extensions(). > But where? In qcow2_open? Or nowhere? I think auto clear extensions > should be cleared automatically.. > May be, move clearing to qemu-img, and just warn here?
On 08/27/2015 01:45 AM, Vladimir Sementsov-Ogievskiy wrote: > On 09.06.2015 18:50, Stefan Hajnoczi wrote: >> On Mon, Jun 08, 2015 at 06:21:24PM +0300, Vladimir Sementsov-Ogievskiy >> wrote: >>> diff --git a/block/qcow2.c b/block/qcow2.c >>> index 406e55d..f85a55a 100644 >>> --- a/block/qcow2.c >>> +++ b/block/qcow2.c >>> @@ -182,6 +182,14 @@ static int >>> qcow2_read_extensions(BlockDriverState *bs, uint64_t start_offset, >>> return ret; >>> } >>> + if (!(s->autoclear_features & >>> QCOW2_AUTOCLEAR_DIRTY_BITMAPS) && >>> + s->nb_dirty_bitmaps > 0) { >>> + ret = qcow2_delete_all_dirty_bitmaps(bs, errp); >>> + if (ret < 0) { >>> + return ret; >>> + } >>> + } >>> + >> What if the file is read-only? >> >> We shouldn't modify the file in qcow2_read_extensions(). > But where? In qcow2_open? Or nowhere? I think auto clear extensions > should be cleared automatically.. Autoclear bits should be cleared ONLY when opening a file for writing, and ONLY if the version of qemu[-img] opening the file does not recognize what the bit controls (or if it does recognize the bit, but is about to perform a semantic action that violates what the bit represents). We should already be clearing all unrecognized autoclear bits upon opening a file for writing (if not, that's a bug in the current implementation), when executing older qemu[-img]. And after your patch series, we know how to handle dirty bitmaps, so the dirty bitmap autoclear bit should no longer be cleared automatically (it is no longer in the mask of unrecognized autoclear bits). So all we have to do now that we are new-enough qemu[-img] is: 1. be sure to set the autoclear bit any time we write a dirty bitmap (the image can no longer be safely written by an older qemu[-img], because those older executables don't know how to interpret the dirty bitmap extension header and might try to overwrite a cluster that we have tied up in a dirty bitmap) 2. clear the bit if we are removing the last dirty bitmap from an image (optimization that is not strictly necessary; but lets older qemu[-img] once again be able to write to the file without the risk of corrupting it) 3. add in error reporting in case the autoclear bit is clear but the dirty bitmap header extension is present with a non-zero number of bitmaps (the autoclear bit served its purpose: an older qemu[-img] has opened the file for writing since new qemu last handled it, and may have broken our bitmaps) Since opening a file read-only cannot (further) corrupt the image, it also does not need to clear any autoclear bits.
On 08/31/2015 04:39 PM, Eric Blake wrote: >>>> +++ b/block/qcow2.c >>>> @@ -182,6 +182,14 @@ static int >>>> qcow2_read_extensions(BlockDriverState *bs, uint64_t start_offset, >>>> return ret; >>>> } >>>> + if (!(s->autoclear_features & >>>> QCOW2_AUTOCLEAR_DIRTY_BITMAPS) && >>>> + s->nb_dirty_bitmaps > 0) { >>>> + ret = qcow2_delete_all_dirty_bitmaps(bs, errp); >>>> + if (ret < 0) { >>>> + return ret; >>>> + } >>>> + } >>>> + >>> What if the file is read-only? >>> >>> We shouldn't modify the file in qcow2_read_extensions(). >> But where? In qcow2_open? Or nowhere? I think auto clear extensions >> should be cleared automatically.. > > 3. add in error reporting in case the autoclear bit is clear but the > dirty bitmap header extension is present with a non-zero number of > bitmaps (the autoclear bit served its purpose: an older qemu[-img] has > opened the file for writing since new qemu last handled it, and may have > broken our bitmaps) This code is attempting to do the error recovery if an older qemu opened the file for writing and thus cleared the unknown bit. But silently dropping the probably-corrupt bitmaps is not nice; an error message would be nicer, as well as requiring an explicit 'qemu-img check -r' as the way to recover the space occupied by the bitmaps. And thinking about it a bit more, I wonder if we should (independently) add a new safety flag into qemu and/or qemu-img, which allows the user the option to refuse to open a file read-write if the file contains an autoclear flag that is not recognized, rather than the current default of opening the file anyways and clearing the bit. The default behavior is safe but may cause data loss (where presumably the lost data is not that important, or we would have made it an incompatible feature bit instead of an autoclear bit), so the safety flag would give users a bit more control on whether they are okay with modifying a file knowing that the modifications will clear the feature. But I guess 'qemu-img info' already knows how to report unknown autoclear bits (thanks to the feature name table extension header) in a read-only manner, so it is already possible to do a read-only probe of a file to see if it contains unknown autoclear bits before doing a read-write open; and maybe we don't need a safety flag after all.
diff --git a/block/qcow2-dirty-bitmap.c b/block/qcow2-dirty-bitmap.c index db83112..686a121 100644 --- a/block/qcow2-dirty-bitmap.c +++ b/block/qcow2-dirty-bitmap.c @@ -188,6 +188,11 @@ static int qcow2_write_dirty_bitmaps(BlockDriverState *bs) s->dirty_bitmaps_offset = dirty_bitmaps_offset; s->dirty_bitmaps_size = dirty_bitmaps_size; + if (s->nb_dirty_bitmaps > 0) { + s->autoclear_features |= QCOW2_AUTOCLEAR_DIRTY_BITMAPS; + } else { + s->autoclear_features &= ~QCOW2_AUTOCLEAR_DIRTY_BITMAPS; + } ret = qcow2_update_header(bs); if (ret < 0) { fprintf(stderr, "Could not update qcow2 header\n"); diff --git a/block/qcow2.c b/block/qcow2.c index 406e55d..f85a55a 100644 --- a/block/qcow2.c +++ b/block/qcow2.c @@ -182,6 +182,14 @@ static int qcow2_read_extensions(BlockDriverState *bs, uint64_t start_offset, return ret; } + if (!(s->autoclear_features & QCOW2_AUTOCLEAR_DIRTY_BITMAPS) && + s->nb_dirty_bitmaps > 0) { + ret = qcow2_delete_all_dirty_bitmaps(bs, errp); + if (ret < 0) { + return ret; + } + } + #ifdef DEBUG_EXT printf("Qcow2: Got dirty bitmaps extension:" " offset=%" PRIu64 " nb_bitmaps=%" PRIu32 "\n", @@ -928,8 +936,9 @@ static int qcow2_open(BlockDriverState *bs, QDict *options, int flags, } /* Clear unknown autoclear feature bits */ - if (!bs->read_only && !(flags & BDRV_O_INCOMING) && s->autoclear_features) { - s->autoclear_features = 0; + if (!bs->read_only && !(flags & BDRV_O_INCOMING) && + (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"); diff --git a/block/qcow2.h b/block/qcow2.h index b5e576c..14bd6f9 100644 --- a/block/qcow2.h +++ b/block/qcow2.h @@ -215,6 +215,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,