From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:52960) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aW3eM-0003yf-45 for qemu-devel@nongnu.org; Wed, 17 Feb 2016 10:04:22 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1aW3eG-0001dD-Cu for qemu-devel@nongnu.org; Wed, 17 Feb 2016 10:04:18 -0500 Received: from mx2.parallels.com ([199.115.105.18]:50636) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aW3eG-0001cO-42 for qemu-devel@nongnu.org; Wed, 17 Feb 2016 10:04:12 -0500 Message-ID: <56C48BDE.3000202@virtuozzo.com> Date: Wed, 17 Feb 2016 18:03:58 +0300 From: Vladimir Sementsov-Ogievskiy MIME-Version: 1.0 References: <1441471439-6157-1-git-send-email-vsementsov@virtuozzo.com> <1441471439-6157-6-git-send-email-vsementsov@virtuozzo.com> <56143CBE.8000908@redhat.com> In-Reply-To: <56143CBE.8000908@redhat.com> Content-Type: text/plain; charset="windows-1252"; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH 05/17] qcow2-dirty-bitmap: read dirty bitmap directory List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: John Snow , qemu-devel@nongnu.org Cc: kwolf@redhat.com, pbonzini@redhat.com, stefanha@redhat.com, den@openvz.org On 07.10.2015 00:27, John Snow wrote: > > On 09/05/2015 12:43 PM, Vladimir Sementsov-Ogievskiy wrote: >> Adds qcow2_read_dirty_bitmaps, reading Dirty Bitmap Directory as >> specified in docs/specs/qcow2.txt >> >> Signed-off-by: Vladimir Sementsov-Ogievskiy >> --- >> block/qcow2-dirty-bitmap.c | 155 +++++++++++++++++++++++++++++++++++++++++++++ >> block/qcow2.h | 10 +++ >> 2 files changed, 165 insertions(+) >> >> diff --git a/block/qcow2-dirty-bitmap.c b/block/qcow2-dirty-bitmap.c >> index fd4e0ef..1260d1d 100644 >> --- a/block/qcow2-dirty-bitmap.c >> +++ b/block/qcow2-dirty-bitmap.c >> @@ -25,6 +25,9 @@ >> * THE SOFTWARE. >> */ >> >> +#include "block/block_int.h" >> +#include "block/qcow2.h" >> + >> /* NOTICE: DBM here means Dirty Bitmap and used as a namespace for _internal_ >> * constants. Please do not use this _internal_ abbreviation for other needs >> * and/or outside of this file. */ >> @@ -40,3 +43,155 @@ >> >> /* bits [0, 8] U [56, 63] are reserved */ >> #define DBM_TABLE_ENTRY_RESERVED_MASK 0xff000000000001ff >> + >> +void qcow2_free_dirty_bitmaps(BlockDriverState *bs) >> +{ >> + BDRVQcowState *s = bs->opaque; > BDRVQcow2State here and everywhere else in this patch, now. > >> + int i; >> + >> + for (i = 0; i < s->nb_dirty_bitmaps; i++) { >> + g_free(s->dirty_bitmaps[i].name); >> + } >> + g_free(s->dirty_bitmaps); >> + s->dirty_bitmaps = NULL; >> + s->nb_dirty_bitmaps = 0; >> + >> + g_free(s->dirty_bitmap_directory); >> + s->dirty_bitmap_directory = NULL; >> +} >> + >> +static void bitmap_header_to_cpu(QCowDirtyBitmapHeader *h) >> +{ >> + be64_to_cpus(&h->dirty_bitmap_table_offset); >> + be64_to_cpus(&h->nb_virtual_bits); >> + be32_to_cpus(&h->dirty_bitmap_table_size); >> + be32_to_cpus(&h->granularity_bits); >> + be32_to_cpus(&h->flags); >> + be16_to_cpus(&h->name_size); > I realize you probably got these functions by example from the other > qcow2 files, but what exactly is cpu*s* here? What does the *s* stand for? > > I guess it refers to the in-place swapping variants that the Linux > kernel defines? > > hmm, just a curiosity on my part ... > > the function looks correct, anyway. :) > >> +} >> + >> +static int calc_dir_entry_size(size_t name_size) >> +{ >> + return align_offset(sizeof(QCowDirtyBitmapHeader) + name_size, 8); > Matches spec. > >> +} >> + >> +static int dir_entry_size(QCowDirtyBitmapHeader *h) >> +{ >> + return calc_dir_entry_size(h->name_size); > OK. > >> +} >> + >> +static int check_constraints(int cluster_size, >> + QCowDirtyBitmapHeader *h) >> +{ >> + uint64_t phys_bitmap_bytes = >> + (uint64_t)h->dirty_bitmap_table_size * cluster_size; >> + uint64_t max_virtual_bits = (phys_bitmap_bytes * 8) << h->granularity_bits; >> + >> + int fail = >> + (h->dirty_bitmap_table_offset % cluster_size) || >> + (h->dirty_bitmap_table_size > DBM_MAX_TABLE_SIZE) || >> + (phys_bitmap_bytes > DBM_MAX_PHYS_SIZE) || >> + (h->nb_virtual_bits > max_virtual_bits) || >> + (h->granularity_bits > DBM_MAX_GRANULARITY_BITS) || >> + (h->flags & DBM_RESERVED_FLAGS) || >> + (h->name_size > DBM_MAX_NAME_SIZE); >> + > Function is a little dense, but appears to be correct -- apart from the > DMB_RESERVED_FLAGS issue I mentioned earlier. > >> + return fail ? -EINVAL : 0; >> +} >> + >> +static int directory_read(BlockDriverState *bs) >> +{ >> + int ret; >> + BDRVQcowState *s = bs->opaque; >> + uint8_t *entry, *end; >> + >> + if (s->dirty_bitmap_directory != NULL) { >> + /* already read */ >> + return -EEXIST; >> + } >> + >> + s->dirty_bitmap_directory = g_try_malloc0(s->dirty_bitmap_directory_size); >> + if (s->dirty_bitmap_directory == NULL) { >> + return -ENOMEM; >> + } >> + > I assume we're trying here in case the directory size is garbage, as a > method of preventing garbage from crashing our program. Since > dirty_bitmap_directory_size was in theory already read in (by a function > checked in later in this series), did we not validate that input value? > >> + ret = bdrv_pread(bs->file, >> + s->dirty_bitmap_directory_offset, >> + s->dirty_bitmap_directory, >> + s->dirty_bitmap_directory_size); >> + if (ret < 0) { >> + goto fail; >> + } >> + > Alright, so we read the entire directory into memory... which can be as > large as 64K * 1024, or 64MiB. A non-trivial size. > >> + entry = s->dirty_bitmap_directory; >> + end = s->dirty_bitmap_directory + s->dirty_bitmap_directory_size; >> + while (entry < end) { >> + QCowDirtyBitmapHeader *h = (QCowDirtyBitmapHeader *)entry; >> + bitmap_header_to_cpu(h); >> + > OK, so we're interpreting the values in-place in memory, but leaving > them in the table. > >> + ret = check_constraints(s->cluster_size, h); >> + if (ret < 0) { >> + goto fail; >> + } >> + >> + entry += dir_entry_size(h); >> + } >> + >> + return 0; >> + >> +fail: >> + g_free(s->dirty_bitmap_directory); >> + s->dirty_bitmap_directory = NULL; >> + >> + return ret; >> +} >> + >> +int qcow2_read_dirty_bitmaps(BlockDriverState *bs) >> +{ >> + int ret; >> + BDRVQcowState *s = bs->opaque; >> + size_t offset; >> + QCowDirtyBitmap *bm, *end; >> + >> + if (s->dirty_bitmap_directory != NULL || s->dirty_bitmaps != NULL) { >> + /* already read */ >> + return -EEXIST; >> + } >> + >> + if (s->nb_dirty_bitmaps == 0) { >> + /* No bitmaps - nothing to do */ >> + return 0; >> + } >> + > OK, so this assumes that the extension header has been read, but that > code comes later in this series. > >> + ret = directory_read(bs); >> + if (ret < 0) { >> + return ret; >> + } >> + > At the end of this call we have interpreted the header into a CPU native > format, but not performed any processing on it whatsoever. bitmap directory is store in ram in cpu native format > >> + s->dirty_bitmaps = g_try_new0(QCowDirtyBitmap, s->nb_dirty_bitmaps); >> + if (s->dirty_bitmaps == NULL) { >> + ret = -ENOMEM; >> + goto out; >> + } >> + > I think we could actually allocate this block of memory sooner (we > already have read and validated nb_dirty_bitmaps) and then during the > initial read, after validation, we can just fill the QcowDirtyBitmap > structures as we go. > > If we keep "int n" as we parse bitmaps in the header, we can just unwind > on failure with: > > for (i = n; i >= 0; i--) { > bm = s->dirty_bitmaps[i]; > g_free(bm->name); > } > g_free(s->dirty_bitmaps); > > Then we don't have to re-crawl through the structure looking for names, > getting sizes again, etc. It should be a little faster. > >> + offset = 0; >> + end = s->dirty_bitmaps + s->nb_dirty_bitmaps; >> + for (bm = s->dirty_bitmaps; bm < end; ++bm) { >> + QCowDirtyBitmapHeader *h = >> + (QCowDirtyBitmapHeader *)(s->dirty_bitmap_directory + offset); >> + >> + bm->offset = offset; >> + bm->name = g_malloc(h->name_size + 1); >> + memcpy(bm->name, h + 1, h->name_size); >> + bm->name[h->name_size] = '\0'; > You can replace the last three lines if you want with just: > > bm->name = g_strndup(h + 1, h->name_size); > >> + >> + offset += dir_entry_size(h); >> + } >> + ret = 0; >> + >> +out: >> + if (ret < 0) { >> + qcow2_free_dirty_bitmaps(bs); >> + } >> + return ret; >> +} >> diff --git a/block/qcow2.h b/block/qcow2.h >> index a2a5d4a..5016fa1 100644 >> --- a/block/qcow2.h >> +++ b/block/qcow2.h >> @@ -288,6 +288,12 @@ typedef struct BDRVQcowState { >> unsigned int nb_snapshots; >> QCowSnapshot *snapshots; >> >> + uint64_t dirty_bitmap_directory_offset; >> + size_t dirty_bitmap_directory_size; > I guess these two are from the extension header. > >> + uint8_t *dirty_bitmap_directory; >> + unsigned int nb_dirty_bitmaps; > This one is also from the extension header. Pointing out only for review > purposes that these values are set "elsewhere" in future patches. > >> + QCowDirtyBitmap *dirty_bitmaps; >> + >> int flags; >> int qcow_version; >> bool use_lazy_refcounts; >> @@ -598,6 +604,10 @@ int qcow2_snapshot_load_tmp(BlockDriverState *bs, >> void qcow2_free_snapshots(BlockDriverState *bs); >> int qcow2_read_snapshots(BlockDriverState *bs); >> >> +/* qcow2-dirty-bitmap.c functions */ >> +void qcow2_free_dirty_bitmaps(BlockDriverState *bs); >> +int qcow2_read_dirty_bitmaps(BlockDriverState *bs); >> + >> /* qcow2-cache.c functions */ >> Qcow2Cache *qcow2_cache_create(BlockDriverState *bs, int num_tables); >> int qcow2_cache_destroy(BlockDriverState* bs, Qcow2Cache *c); >> > Patch order is a little strange in that we expect to have parsed the > header already, but nothing criminal if this was just the easiest way to > do it. I'll defer to your judgment. > -- Best regards, Vladimir