Re: [Qemu-devel] [PATCH 05/17] qcow2-dirty-bitmap: read dirty bitmap directory

[Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>]

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 <vsementsov@virtuozzo.com>
>> ---
>>   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