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

[John Snow <jsnow@redhat.com>]

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.

> +    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.