Re: [Qemu-devel] [PATCH v4 09/10] block/qcow2-bitmap: fix and improve qcow2_reopen_bitmaps_rw

[John Snow]

On 8/7/19 10:12 AM, Vladimir Sementsov-Ogievskiy wrote:
> - Correct check for write access to file child, and in correct place
>   (only if we want to write).
> - Support reopen rw -> rw (which will be used in following commit),
>   for example, !bdrv_dirty_bitmap_readonly() is not a corruption if
>   bitmap is marked IN_USE in the image.
> - Consider unexpected bitmap as a corruption and check other
>   combinations of in-image and in-RAM bitmaps.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <address@hidden>
> ---
>  block/qcow2-bitmap.c | 56 +++++++++++++++++++++++++++++---------------
>  1 file changed, 37 insertions(+), 19 deletions(-)
> 
> diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c
> index a636dc50ca..e276a95154 100644
> --- a/block/qcow2-bitmap.c
> +++ b/block/qcow2-bitmap.c
> @@ -1108,18 +1108,14 @@ int qcow2_reopen_bitmaps_rw(BlockDriverState *bs, 
> Error **errp)
>      Qcow2BitmapList *bm_list;
>      Qcow2Bitmap *bm;
>      GSList *ro_dirty_bitmaps = NULL;
> -    int ret = 0;
> +    int ret = -EINVAL;
> +    bool need_header_update = false;
>  
>      if (s->nb_bitmaps == 0) {
>          /* No bitmaps - nothing to do */
>          return 0;
>      }
>  
> -    if (!can_write(bs)) {
> -        error_setg(errp, "Can't write to the image on reopening bitmaps rw");
> -        return -EINVAL;
> -    }
> -
>      bm_list = bitmap_list_load(bs, s->bitmap_directory_offset,
>                                 s->bitmap_directory_size, errp);
>      if (bm_list == NULL) {
> @@ -1128,32 +1124,54 @@ int qcow2_reopen_bitmaps_rw(BlockDriverState *bs, 
> Error **errp)
>  
>      QSIMPLEQ_FOREACH(bm, bm_list, entry) {
>          BdrvDirtyBitmap *bitmap = bdrv_find_dirty_bitmap(bs, bm->name);
> -        if (bitmap == NULL) {
> -            continue;
> -        }
>  
> -        if (!bdrv_dirty_bitmap_readonly(bitmap)) {
> -            error_setg(errp, "Bitmap %s was loaded prior to rw-reopen, but 
> was "
> -                       "not marked as readonly. This is a bug, something 
> went "
> -                       "wrong. All of the bitmaps may be corrupted", 
> bm->name);
> -            ret = -EINVAL;
> +        if (!bitmap) {
> +            error_setg(errp, "Unexpected bitmap '%s' in the image '%s'",
> +                       bm->name, bs->filename);
>              goto out;
>          }
>  

I think you can actually drop the definite article, because the image
name is the specifier.

"Unexpected bitmap '%s' in image '%s'" is sufficient.

> -        bm->flags |= BME_FLAG_IN_USE;
> -        ro_dirty_bitmaps = g_slist_append(ro_dirty_bitmaps, bitmap);
> +        if (!(bm->flags & BME_FLAG_IN_USE)) {
> +            if (!bdrv_dirty_bitmap_readonly(bitmap)) {
> +                error_setg(errp, "Corruption: bitmap '%s' is not marked 
> IN_USE "
> +                           "in the image '%s' and not marked readonly in 
> RAM",
> +                           bm->name, bs->filename);
> +                goto out;
> +            }
> +            if (bdrv_dirty_bitmap_inconsistent(bitmap)) {
> +                error_setg(errp, "Corruption: bitmap '%s' is inconsistent 
> but "
> +                           "is not marked IN_USE in the image '%s'", 
> bm->name,
> +                           bs->filename);
> +                goto out;
> +            }

We support RW --> RW now, but what happens if something is marked IN_USE
on RO --> RW? It's not obvious from this function alone why that's safe
to ignore.

> +
> +            bm->flags |= BME_FLAG_IN_USE;
> +            need_header_update = true;
> +        }
> +
> +        if (bdrv_dirty_bitmap_readonly(bitmap)) {
> +            ro_dirty_bitmaps = g_slist_append(ro_dirty_bitmaps, bitmap);
> +        }
>      }
>  
> -    if (ro_dirty_bitmaps != NULL) {
> +    if (need_header_update) {
> +        if (!can_write(bs->file->bs) || !(bs->file->perm & BLK_PERM_WRITE)) {
> +            error_setg(errp, "Failed to reopen bitmaps rw: no write access "
> +                       "the protocol file");
> +            goto out;
> +        }
> +
>          /* in_use flags must be updated */
>          ret = update_ext_header_and_dir_in_place(bs, bm_list);
>          if (ret < 0) {
> -            error_setg_errno(errp, -ret, "Can't update bitmap directory");
> +            error_setg_errno(errp, -ret, "Cannot update bitmap directory");
>              goto out;
>          }
> -        g_slist_foreach(ro_dirty_bitmaps, set_readonly_helper, false);
>      }
>  
> +    g_slist_foreach(ro_dirty_bitmaps, set_readonly_helper, false);
> +    ret = 0;
> +
>  out:
>      g_slist_free(ro_dirty_bitmaps);
>      bitmap_list_free(bm_list);
> 

Seems OK otherwise, but I just have that one doubt.