From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:56188) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Z2xxp-0001Sg-MG for qemu-devel@nongnu.org; Thu, 11 Jun 2015 04:36:14 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Z2xxW-00007c-JQ for qemu-devel@nongnu.org; Thu, 11 Jun 2015 04:35:53 -0400 Received: from mx1.redhat.com ([209.132.183.28]:40730) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Z2xxW-00005l-9B for qemu-devel@nongnu.org; Thu, 11 Jun 2015 04:35:34 -0400 Date: Thu, 11 Jun 2015 10:35:30 +0200 From: Kevin Wolf Message-ID: <20150611083530.GB4818@noname.redhat.com> References: <1433776886-27239-1-git-send-email-vsementsov@virtuozzo.com> <1433776886-27239-7-git-send-email-vsementsov@virtuozzo.com> <5578CB52.5030504@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <5578CB52.5030504@redhat.com> Subject: Re: [Qemu-devel] [PATCH 6/8] qcow2: add autoclear bit for dirty bitmaps List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: John Snow Cc: Vladimir Sementsov-Ogievskiy , qemu-devel@nongnu.org, Vladimir Sementsov-Ogievskiy , stefanha@redhat.com, den@openvz.org, pbonzini@redhat.com 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 > > > > Signed-off-by: Vladimir Sementsov-Ogievskiy > > --- > > 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