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 <vsement...@virtuozzo.com>
> ---
> 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.
