From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:46655) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1btvsH-00011H-RN for qemu-devel@nongnu.org; Tue, 11 Oct 2016 08:09:41 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1btvsF-0001p1-IK for qemu-devel@nongnu.org; Tue, 11 Oct 2016 08:09:36 -0400 Received: from mailhub.sw.ru ([195.214.232.25]:26619 helo=relay.sw.ru) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1btvsF-0001oX-4V for qemu-devel@nongnu.org; Tue, 11 Oct 2016 08:09:35 -0400 References: <1475232808-4852-1-git-send-email-vsementsov@virtuozzo.com> <1475232808-4852-7-git-send-email-vsementsov@virtuozzo.com> <617c1183-a483-7fb6-5e00-8d6fa5e7394f@redhat.com> From: Vladimir Sementsov-Ogievskiy Message-ID: <57FCD66F.9090704@virtuozzo.com> Date: Tue, 11 Oct 2016 15:09:19 +0300 MIME-Version: 1.0 In-Reply-To: <617c1183-a483-7fb6-5e00-8d6fa5e7394f@redhat.com> Content-Type: text/plain; charset=iso-8859-15; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH 06/22] qcow2: add dirty bitmaps extension List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Max Reitz , qemu-block@nongnu.org, qemu-devel@nongnu.org Cc: kwolf@redhat.com, armbru@redhat.com, eblake@redhat.com, jsnow@redhat.com, famz@redhat.com, den@openvz.org, stefanha@redhat.com, pbonzini@redhat.com 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 >> --- >> 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 > -- Best regards, Vladimir