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 <vsement...@parallels.com> >>> >>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsement...@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, >>> > >