Re: [Qemu-block] [PATCH 06/22] qcow2: add dirty bitmaps extension

[Vladimir Sementsov-Ogievskiy]

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 <address@hidden>
> > ---
> >   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