Re: [Qemu-devel] [PATCH 12/22] qcow2-bitmap: add IN_USE flag

[Vladimir Sementsov-Ogievskiy]

07.10.2016 22:44, Max Reitz пишет:
> On 30.09.2016 12:53, Vladimir Sementsov-Ogievskiy wrote:
> > This flag means that the bitmap is now in use by the software or was not
> > successfully saved. In any way, with this flag set the bitmap data must
> > be considered inconsistent and should not be loaded.
> >
> > With current implementation this flag is never set, as we just remove
> > bitmaps from the image after loading. But it defined in qcow2 spec and
> > must be handled. Also, it can be used in future, if async schemes of
> > bitmap loading/saving are implemented.
> >
> > We also remove in-use bitmaps from the image on open.
> >
> > Signed-off-by: Vladimir Sementsov-Ogievskiy <address@hidden>
> > ---
> >   block/qcow2-bitmap.c | 17 ++++++++++++++++-
> >   1 file changed, 16 insertions(+), 1 deletion(-)
> Don't you want to make use of this flag? It would appear useful to me if
> you just marked autoload bitmaps as in_use instead of deleting them from
> the image when it's opened and then overwrite them when the image is closed.

And what is the use of it?

Any way, storing an invalid copy of online data is bad I think. 
Therefore I will have to free bitmap data clusters and zero bitmap table 
in file.

> > diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c
> > index a5be25a..8cf40f0 100644
> > --- a/block/qcow2-bitmap.c
> > +++ b/block/qcow2-bitmap.c
> > @@ -28,6 +28,7 @@
> >   #include "qemu/osdep.h"
> >   #include "qapi/error.h"
> >   #include "qemu/cutils.h"
> > +#include "exec/log.h"
> >   
> >   #include "block/block_int.h"
> >   #include "block/qcow2.h"
> > @@ -44,7 +45,8 @@
> >   #define BME_MAX_NAME_SIZE 1023
> >   
> >   /* Bitmap directory entry flags */
> > -#define BME_RESERVED_FLAGS 0xfffffffd
> > +#define BME_RESERVED_FLAGS 0xfffffffc
> > +#define BME_FLAG_IN_USE 1
> This should be (1U << 0) to be consistent with the definition of
> BME_FLAG_AUTO.
>
> >   #define BME_FLAG_AUTO   (1U << 1)
> >   
> >   /* bits [1, 8] U [56, 63] are reserved */
> > @@ -287,6 +289,11 @@ static BdrvDirtyBitmap *load_bitmap(BlockDriverState *bs,
> >       BdrvDirtyBitmap *bitmap = NULL;
> >       char *name = g_strndup(dir_entry_name_notcstr(entry), entry->name_size);
> >   
> > +    if (entry->flags & BME_FLAG_IN_USE) {
> > +        error_setg(errp, "Bitmap '%s' is in use", name);
> > +        goto fail;
> > +    }
> > +
> >       ret = bitmap_table_load(bs, entry, &bitmap_table);
> >       if (ret < 0) {
> >           error_setg_errno(errp, -ret,
> > @@ -533,6 +540,14 @@ int qcow2_read_bitmaps(BlockDriverState *bs, Error **errp)
> >       for_each_bitmap_dir_entry(e, dir, dir_size) {
> >           uint32_t fl = e->flags;
> >   
> > +        if (fl & BME_FLAG_IN_USE) {
> > +            qemu_log("qcow2 warning: "
> > +                     "removing in_use bitmap '%.*s' for image %s.\n",
> > +                     e->name_size, (char *)(e + 1), 
> > bdrv_get_device_or_node_name(bs));
> I'm not sure whether qemu_log() is the right way to do this. I think
> fprintf() to stderr might actually be more appropriate. qemu_log() looks
> like it's mostly used for debugging purposes.
>
> Also, this is not a warning. You are just doing it. You are not warning
> someone about a cliff when he's already fallen off.
>
> But you can actually make it a warning: Just leave the bitmap in the
> image (maybe someone can do something with it?) and instead let qemu-img
> check clean it up. The warning should then just be "Warning: Ignoring
> in_use bitmap %.*s, use qemu-img check -r all to delete it".
>
> Max
>
> > +
> > +            continue;
> > +        }
> > +
> >           if (fl & BME_FLAG_AUTO) {
> >               BdrvDirtyBitmap *bitmap = load_bitmap(bs, e, errp);
> >               if (bitmap == NULL) {


--
Best regards,
Vladimir