Re: [Qemu-block] [Qemu-devel] [PATCH 07/24] qcow2: add bitmaps extension

[John Snow]

On 02/03/2017 04:40 AM, Vladimir Sementsov-Ogievskiy wrote:
> Add 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 <address@hidden>
> ---
>  block/qcow2.c | 127 
> +++++++++++++++++++++++++++++++++++++++++++++++++++++++---
>  block/qcow2.h |  24 +++++++++++
>  2 files changed, 145 insertions(+), 6 deletions(-)
> 
> diff --git a/block/qcow2.c b/block/qcow2.c
> index 96fb8a8f16..d263ab1ed7 100644
> --- a/block/qcow2.c
> +++ b/block/qcow2.c
> @@ -63,6 +63,7 @@ typedef struct {
>  #define  QCOW2_EXT_MAGIC_END 0
>  #define  QCOW2_EXT_MAGIC_BACKING_FORMAT 0xE2792ACA
>  #define  QCOW2_EXT_MAGIC_FEATURE_TABLE 0x6803f857
> +#define  QCOW2_EXT_MAGIC_BITMAPS 0x23852875
>  
>  static int qcow2_probe(const uint8_t *buf, int buf_size, const char 
> *filename)
>  {
> @@ -86,12 +87,17 @@ static int qcow2_probe(const uint8_t *buf, int buf_size, 
> const char *filename)
>   */
>  static int qcow2_read_extensions(BlockDriverState *bs, uint64_t start_offset,
>                                   uint64_t end_offset, void **p_feature_table,
> -                                 Error **errp)
> +                                 bool *need_update_header, Error **errp)
>  {
>      BDRVQcow2State *s = bs->opaque;
>      QCowExtension ext;
>      uint64_t offset;
>      int ret;
> +    Qcow2BitmapHeaderExt bitmaps_ext;
> +
> +    if (need_update_header != NULL) {
> +        *need_update_header = false;
> +    }
>  
>  #ifdef DEBUG_EXT
>      printf("qcow2_read_extensions: start=%ld end=%ld\n", start_offset, 
> end_offset);
> @@ -162,6 +168,85 @@ static int qcow2_read_extensions(BlockDriverState *bs, 
> uint64_t start_offset,
>              }
>              break;
>  
> +        case QCOW2_EXT_MAGIC_BITMAPS:
> +            if (ext.len != sizeof(bitmaps_ext)) {
> +                error_setg_errno(errp, -ret, "bitmaps_ext: "
> +                                 "Invalid extension length");
> +                return -EINVAL;
> +            }
> +
> +            if (!(s->autoclear_features & QCOW2_AUTOCLEAR_BITMAPS)) {
> +                fprintf(stderr,
> +                        "WARNING: a program lacking bitmap support modified "
> +                        "this file, so all bitmaps are now considered "
> +                        "inconsistent. Some clusters may be leaked, run "
> +                        "'qemu-img check -r' on the image file to fix.");
> +                if (need_update_header != NULL) {
> +                    *need_update_header = true;
> +                }
> +                break;
> +            }
> +
> +            ret = bdrv_pread(bs->file, offset, &bitmaps_ext, ext.len);
> +            if (ret < 0) {
> +                error_setg_errno(errp, -ret, "bitmaps_ext: "
> +                                 "Could not read ext header");
> +                return ret;
> +            }
> +
> +            if (bitmaps_ext.reserved32 != 0) {
> +                error_setg_errno(errp, -ret, "bitmaps_ext: "
> +                                 "Reserved field is not zero");
> +                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 > QCOW2_MAX_BITMAPS) {
> +                error_setg(errp,
> +                           "bitmaps_ext: File %s has %" PRIu32 " bitmaps, "
> +                           "exceeding the QEMU supported maximum of %d",
> +                           bs->filename, bitmaps_ext.nb_bitmaps,
> +                           QCOW2_MAX_BITMAPS);
> +                return -EINVAL;
> +            }
> +
> +            if (bitmaps_ext.nb_bitmaps == 0) {
> +                error_setg(errp, "found bitmaps extension with zero 
> bitmaps");
> +                return -EINVAL;
> +            }
> +
> +            if (bitmaps_ext.bitmap_directory_offset & (s->cluster_size - 1)) 
> {
> +                error_setg(errp, "bitmaps_ext: "
> +                                 "invalid bitmap directory offset");
> +                return -EINVAL;
> +            }
> +
> +            if (bitmaps_ext.bitmap_directory_size >
> +                QCOW2_MAX_BITMAP_DIRECTORY_SIZE) {
> +                error_setg(errp, "bitmaps_ext: "
> +                                 "bitmap directory size (%" PRIu64 ") 
> exceeds "
> +                                 "the maximum supported size (%d)",
> +                                 bitmaps_ext.bitmap_directory_size,
> +                                 QCOW2_MAX_BITMAP_DIRECTORY_SIZE);
> +                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 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 
> */
>              {
> @@ -824,6 +909,7 @@ static int qcow2_open(BlockDriverState *bs, QDict 
> *options, int flags,
>      Error *local_err = NULL;
>      uint64_t ext_end;
>      uint64_t l1_vm_state_index;
> +    bool need_update_header = false;
>  
>      ret = bdrv_pread(bs->file, 0, &header, sizeof(header));
>      if (ret < 0) {
> @@ -929,7 +1015,7 @@ static int qcow2_open(BlockDriverState *bs, QDict 
> *options, int flags,
>      if (s->incompatible_features & ~QCOW2_INCOMPAT_MASK) {
>          void *feature_table = NULL;
>          qcow2_read_extensions(bs, header.header_length, ext_end,
> -                              &feature_table, NULL);
> +                              &feature_table, NULL, NULL);
>          report_unsupported_feature(errp, feature_table,
>                                     s->incompatible_features &
>                                     ~QCOW2_INCOMPAT_MASK);
> @@ -1116,7 +1202,7 @@ static int qcow2_open(BlockDriverState *bs, QDict 
> *options, int flags,
>  
>      /* read qcow2 extensions */
>      if (qcow2_read_extensions(bs, header.header_length, ext_end, NULL,
> -        &local_err)) {
> +        &need_update_header, &local_err)) {
>          error_propagate(errp, local_err);
>          ret = -EINVAL;
>          goto fail;
> @@ -1151,9 +1237,13 @@ static int qcow2_open(BlockDriverState *bs, QDict 
> *options, int flags,
>          goto fail;
>      }
>  
> -    /* Clear unknown autoclear feature bits */
> -    if (!bs->read_only && !(flags & BDRV_O_INACTIVE) && 
> s->autoclear_features) {
> -        s->autoclear_features = 0;
> +    if (!need_update_header) {
> +        /* Clear unknown autoclear feature bits */
> +        need_update_header = s->autoclear_features & ~QCOW2_AUTOCLEAR_MASK;
> +    }
> +
> +    if (need_update_header && !bs->read_only && !(flags & BDRV_O_INACTIVE)) {
> +        s->autoclear_features &= QCOW2_AUTOCLEAR_MASK;
>          ret = qcow2_update_header(bs);
>          if (ret < 0) {
>              error_setg_errno(errp, -ret, "Could not update qcow2 header");
> @@ -1953,6 +2043,24 @@ int qcow2_update_header(BlockDriverState *bs)
>          buflen -= ret;
>      }
>  
> +    if (s->nb_bitmaps > 0) {
> +        Qcow2BitmapHeaderExt bitmaps_header = {
> +            .nb_bitmaps = cpu_to_be32(s->nb_bitmaps),
> +            .bitmap_directory_size =
> +                    cpu_to_be64(s->bitmap_directory_size),
> +            .bitmap_directory_offset =
> +                    cpu_to_be64(s->bitmap_directory_offset)
> +        };
> +        ret = header_ext_add(buf, QCOW2_EXT_MAGIC_BITMAPS,
> +                             &bitmaps_header, sizeof(bitmaps_header),
> +                             buflen);
> +        if (ret < 0) {
> +            goto fail;
> +        }
> +        buf += ret;
> +        buflen -= ret;
> +    }
> +
>      /* Keep unknown header extensions */
>      QLIST_FOREACH(uext, &s->unknown_header_ext, next) {
>          ret = header_ext_add(buf, uext->magic, uext->data, uext->len, 
> buflen);
> @@ -2528,6 +2636,13 @@ static int qcow2_truncate(BlockDriverState *bs, 
> int64_t offset)
>          return -ENOTSUP;
>      }
>  
> +    /* cannot proceed if image has bitmaps */
> +    if (s->nb_bitmaps) {
> +        /* TODO: resize bitmaps in the image */
> +        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");
> diff --git a/block/qcow2.h b/block/qcow2.h
> index 182341483a..861b5011dd 100644
> --- a/block/qcow2.h
> +++ b/block/qcow2.h
> @@ -52,6 +52,10 @@
>   * space for snapshot names and IDs */
>  #define QCOW_MAX_SNAPSHOTS_SIZE (1024 * QCOW_MAX_SNAPSHOTS)
>  
> +/* Bitmap header extension constraints */
> +#define QCOW2_MAX_BITMAPS 65535
> +#define QCOW2_MAX_BITMAP_DIRECTORY_SIZE (1024 * QCOW2_MAX_BITMAPS)
> +
>  /* indicate that the refcount of the referenced cluster is exactly one. */
>  #define QCOW_OFLAG_COPIED     (1ULL << 63)
>  /* indicate that the cluster is compressed (they never have the copied flag) 
> */
> @@ -195,6 +199,15 @@ enum {
>      QCOW2_COMPAT_FEAT_MASK            = QCOW2_COMPAT_LAZY_REFCOUNTS,
>  };
>  
> +/* Autoclear feature bits */
> +enum {
> +    QCOW2_AUTOCLEAR_BITMAPS_BITNR = 0,
> +    QCOW2_AUTOCLEAR_BITMAPS       =
> +        1 << QCOW2_AUTOCLEAR_BITMAPS_BITNR,
> +
> +    QCOW2_AUTOCLEAR_MASK                = QCOW2_AUTOCLEAR_BITMAPS,
> +};
> +
>  enum qcow2_discard_type {
>      QCOW2_DISCARD_NEVER = 0,
>      QCOW2_DISCARD_ALWAYS,
> @@ -222,6 +235,13 @@ typedef uint64_t Qcow2GetRefcountFunc(const void 
> *refcount_array,
>  typedef void Qcow2SetRefcountFunc(void *refcount_array,
>                                    uint64_t index, uint64_t value);
>  
> +typedef struct Qcow2BitmapHeaderExt {
> +    uint32_t nb_bitmaps;
> +    uint32_t reserved32;
> +    uint64_t bitmap_directory_size;
> +    uint64_t bitmap_directory_offset;
> +} QEMU_PACKED Qcow2BitmapHeaderExt;
> +
>  typedef struct BDRVQcow2State {
>      int cluster_bits;
>      int cluster_size;
> @@ -263,6 +283,10 @@ typedef struct BDRVQcow2State {
>      unsigned int nb_snapshots;
>      QCowSnapshot *snapshots;
>  
> +    uint32_t nb_bitmaps;
> +    uint64_t bitmap_directory_size;
> +    uint64_t bitmap_directory_offset;
> +
>      int flags;
>      int qcow_version;
>      bool use_lazy_refcounts;
> 

Reviewed-by: John Snow <address@hidden>