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

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

> 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) {
>

signature.asc
Description: OpenPGP digital signature