[Qemu-devel] [PATCH for-2.11 0/3] block: Error out on load_vm with active dirty bitmaps

[Qemu-devel] [PATCH for-2.11 0/3] block: Error out on load_vm with active dirty bitmaps

[Kevin Wolf]

Following the discussing at
https://lists.gnu.org/archive/html/qemu-devel/2017-11/msg03572.html
this implements the error return for loading a snapshot while dirty
bitmaps are active.

Kevin Wolf (3):
  block: Add errp to bdrv_snapshot_goto()
  block: Add errp to bdrv_all_goto_snapshot()
  block: Error out on load_vm with active dirty bitmaps

 include/block/snapshot.h |  6 ++++--
 block/snapshot.c         | 43 +++++++++++++++++++++++--------------------
 migration/savevm.c       |  6 +++---
 qemu-img.c               |  6 +++---
 4 files changed, 33 insertions(+), 28 deletions(-)

-- 
2.13.6

[Qemu-devel] [PATCH for-2.11 1/3] block: Add errp to bdrv_snapshot_goto()

[Kevin Wolf]

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 include/block/snapshot.h |  3 ++-
 block/snapshot.c         | 21 ++++++++++++++++-----
 qemu-img.c               |  6 +++---
 3 files changed, 21 insertions(+), 9 deletions(-)

diff --git a/include/block/snapshot.h b/include/block/snapshot.h
index e5c0553115..aeb80405e8 100644
--- a/include/block/snapshot.h
+++ b/include/block/snapshot.h
@@ -57,7 +57,8 @@ int bdrv_can_snapshot(BlockDriverState *bs);
 int bdrv_snapshot_create(BlockDriverState *bs,
                          QEMUSnapshotInfo *sn_info);
 int bdrv_snapshot_goto(BlockDriverState *bs,
-                       const char *snapshot_id);
+                       const char *snapshot_id,
+                       Error **errp);
 int bdrv_snapshot_delete(BlockDriverState *bs,
                          const char *snapshot_id,
                          const char *name,
diff --git a/block/snapshot.c b/block/snapshot.c
index be0743abac..9c941e638d 100644
--- a/block/snapshot.c
+++ b/block/snapshot.c
@@ -177,18 +177,21 @@ int bdrv_snapshot_create(BlockDriverState *bs,
 }
 
 int bdrv_snapshot_goto(BlockDriverState *bs,
-                       const char *snapshot_id)
+                       const char *snapshot_id,
+                       Error **errp)
 {
     BlockDriver *drv = bs->drv;
     int ret, open_ret;
     int64_t len;
 
     if (!drv) {
+        error_setg(errp, "Block driver is closed");
         return -ENOMEDIUM;
     }
 
     len = bdrv_getlength(bs);
     if (len < 0) {
+        error_setg_errno(errp, -len, "Cannot get block device size");
         return len;
     }
     /* We should set all bits in all enabled dirty bitmaps, because dirty
@@ -200,13 +203,18 @@ int bdrv_snapshot_goto(BlockDriverState *bs,
     bdrv_set_dirty(bs, 0, len);
 
     if (drv->bdrv_snapshot_goto) {
-        return drv->bdrv_snapshot_goto(bs, snapshot_id);
+        ret = drv->bdrv_snapshot_goto(bs, snapshot_id);
+        if (ret < 0) {
+            error_setg_errno(errp, -ret, "Failed to load snapshot");
+        }
+        return ret;
     }
 
     if (bs->file) {
         BlockDriverState *file;
         QDict *options = qdict_clone_shallow(bs->options);
         QDict *file_options;
+        Error *local_err = NULL;
 
         file = bs->file->bs;
         /* Prevent it from getting deleted when detached from bs */
@@ -220,12 +228,14 @@ int bdrv_snapshot_goto(BlockDriverState *bs,
         bdrv_unref_child(bs, bs->file);
         bs->file = NULL;
 
-        ret = bdrv_snapshot_goto(file, snapshot_id);
-        open_ret = drv->bdrv_open(bs, options, bs->open_flags, NULL);
+        ret = bdrv_snapshot_goto(file, snapshot_id, errp);
+        open_ret = drv->bdrv_open(bs, options, bs->open_flags, &local_err);
         QDECREF(options);
         if (open_ret < 0) {
             bdrv_unref(file);
             bs->drv = NULL;
+            /* A bdrv_snapshot_goto() error takes precedence */
+            error_propagate(errp, local_err);
             return open_ret;
         }
 
@@ -234,6 +244,7 @@ int bdrv_snapshot_goto(BlockDriverState *bs,
         return ret;
     }
 
+    error_setg(errp, "Block driver does not support snapshots");
     return -ENOTSUP;
 }
 
@@ -467,7 +478,7 @@ int bdrv_all_goto_snapshot(const char *name, BlockDriverState **first_bad_bs)
 
         aio_context_acquire(ctx);
         if (bdrv_can_snapshot(bs)) {
-            err = bdrv_snapshot_goto(bs, name);
+            err = bdrv_snapshot_goto(bs, name, NULL);
         }
         aio_context_release(ctx);
         if (err < 0) {
diff --git a/qemu-img.c b/qemu-img.c
index 02a6e27beb..68b375f998 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -2989,10 +2989,10 @@ static int img_snapshot(int argc, char **argv)
         break;
 
     case SNAPSHOT_APPLY:
-        ret = bdrv_snapshot_goto(bs, snapshot_name);
+        ret = bdrv_snapshot_goto(bs, snapshot_name, &err);
         if (ret) {
-            error_report("Could not apply snapshot '%s': %d (%s)",
-                snapshot_name, ret, strerror(-ret));
+            error_reportf_err(err, "Could not apply snapshot '%s': ",
+                              snapshot_name);
         }
         break;
 
-- 
2.13.6

[Qemu-devel] [PATCH for-2.11 2/3] block: Add errp to bdrv_all_goto_snapshot()

[Kevin Wolf]

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 include/block/snapshot.h |  3 ++-
 block/snapshot.c         | 11 ++++++-----
 migration/savevm.c       |  6 +++---
 3 files changed, 11 insertions(+), 9 deletions(-)

diff --git a/include/block/snapshot.h b/include/block/snapshot.h
index aeb80405e8..9407799941 100644
--- a/include/block/snapshot.h
+++ b/include/block/snapshot.h
@@ -84,7 +84,8 @@ int bdrv_snapshot_load_tmp_by_id_or_name(BlockDriverState *bs,
 bool bdrv_all_can_snapshot(BlockDriverState **first_bad_bs);
 int bdrv_all_delete_snapshot(const char *name, BlockDriverState **first_bsd_bs,
                              Error **err);
-int bdrv_all_goto_snapshot(const char *name, BlockDriverState **first_bsd_bs);
+int bdrv_all_goto_snapshot(const char *name, BlockDriverState **first_bad_bs,
+                           Error **errp);
 int bdrv_all_find_snapshot(const char *name, BlockDriverState **first_bad_bs);
 int bdrv_all_create_snapshot(QEMUSnapshotInfo *sn,
                              BlockDriverState *vm_state_bs,
diff --git a/block/snapshot.c b/block/snapshot.c
index 9c941e638d..13ec3b1c8c 100644
--- a/block/snapshot.c
+++ b/block/snapshot.c
@@ -467,9 +467,10 @@ fail:
 }
 
 
-int bdrv_all_goto_snapshot(const char *name, BlockDriverState **first_bad_bs)
+int bdrv_all_goto_snapshot(const char *name, BlockDriverState **first_bad_bs,
+                           Error **errp)
 {
-    int err = 0;
+    int ret = 0;
     BlockDriverState *bs;
     BdrvNextIterator it;
 
@@ -478,10 +479,10 @@ int bdrv_all_goto_snapshot(const char *name, BlockDriverState **first_bad_bs)
 
         aio_context_acquire(ctx);
         if (bdrv_can_snapshot(bs)) {
-            err = bdrv_snapshot_goto(bs, name, NULL);
+            ret = bdrv_snapshot_goto(bs, name, errp);
         }
         aio_context_release(ctx);
-        if (err < 0) {
+        if (ret < 0) {
             bdrv_next_cleanup(&it);
             goto fail;
         }
@@ -489,7 +490,7 @@ int bdrv_all_goto_snapshot(const char *name, BlockDriverState **first_bad_bs)
 
 fail:
     *first_bad_bs = bs;
-    return err;
+    return ret;
 }
 
 int bdrv_all_find_snapshot(const char *name, BlockDriverState **first_bad_bs)
diff --git a/migration/savevm.c b/migration/savevm.c
index 4a88228614..192f2d82cd 100644
--- a/migration/savevm.c
+++ b/migration/savevm.c
@@ -2346,10 +2346,10 @@ int load_snapshot(const char *name, Error **errp)
     /* Flush all IO requests so they don't interfere with the new state.  */
     bdrv_drain_all_begin();
 
-    ret = bdrv_all_goto_snapshot(name, &bs);
+    ret = bdrv_all_goto_snapshot(name, &bs, errp);
     if (ret < 0) {
-        error_setg(errp, "Error %d while activating snapshot '%s' on '%s'",
-                     ret, name, bdrv_get_device_name(bs));
+        error_prepend(errp, "Could not load snapshot '%s' on '%s': ",
+                      name, bdrv_get_device_name(bs));
         goto err_drain;
     }
 
-- 
2.13.6

[Qemu-devel] [PATCH for-2.11 3/3] block: Error out on load_vm with active dirty bitmaps

[Kevin Wolf]

Loading a snapshot invalidates the bitmap. Just marking all blocks dirty
is not a useful response in practice, instead the user needs to be aware
that we switch to a completely different state. If they are okay with
losing the dirty bitmap, they can just explicitly delete it.

This effectively reverts commit 04dec3c3ae5.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block/snapshot.c | 15 +++------------
 1 file changed, 3 insertions(+), 12 deletions(-)

diff --git a/block/snapshot.c b/block/snapshot.c
index 13ec3b1c8c..6b338978c5 100644
--- a/block/snapshot.c
+++ b/block/snapshot.c
@@ -182,25 +182,16 @@ int bdrv_snapshot_goto(BlockDriverState *bs,
 {
     BlockDriver *drv = bs->drv;
     int ret, open_ret;
-    int64_t len;
 
     if (!drv) {
         error_setg(errp, "Block driver is closed");
         return -ENOMEDIUM;
     }
 
-    len = bdrv_getlength(bs);
-    if (len < 0) {
-        error_setg_errno(errp, -len, "Cannot get block device size");
-        return len;
+    if (!QLIST_EMPTY(&bs->dirty_bitmaps)) {
+        error_setg(errp, "Device has active dirty bitmaps");
+        return -EBUSY;
     }
-    /* We should set all bits in all enabled dirty bitmaps, because dirty
-     * bitmaps reflect active state of disk and snapshot switch operation
-     * actually dirties active state.
-     * TODO: It may make sense not to set all bits but analyze block status of
-     * current state and destination snapshot and do not set bits corresponding
-     * to both-zero or both-unallocated areas. */
-    bdrv_set_dirty(bs, 0, len);
 
     if (drv->bdrv_snapshot_goto) {
         ret = drv->bdrv_snapshot_goto(bs, snapshot_id);
-- 
2.13.6

Re: [Qemu-devel] [PATCH for-2.11 0/3] block: Error out on load_vm with active dirty bitmaps

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 860 bytes --]

On 11/20/2017 08:50 AM, Kevin Wolf wrote:
> Following the discussing at
> https://lists.gnu.org/archive/html/qemu-devel/2017-11/msg03572.html
> this implements the error return for loading a snapshot while dirty
> bitmaps are active.
> 
> Kevin Wolf (3):
>   block: Add errp to bdrv_snapshot_goto()
>   block: Add errp to bdrv_all_goto_snapshot()
>   block: Error out on load_vm with active dirty bitmaps

Reviewed-by: Eric Blake <eblake@redhat.com>

> 
>  include/block/snapshot.h |  6 ++++--
>  block/snapshot.c         | 43 +++++++++++++++++++++++--------------------
>  migration/savevm.c       |  6 +++---
>  qemu-img.c               |  6 +++---
>  4 files changed, 33 insertions(+), 28 deletions(-)
> 

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 619 bytes --]

Re: [Qemu-devel] [PATCH for-2.11 0/3] block: Error out on load_vm with active dirty bitmaps

[Denis V. Lunev]

On 11/20/2017 05:50 PM, Kevin Wolf wrote:
> Following the discussing at
> https://lists.gnu.org/archive/html/qemu-devel/2017-11/msg03572.html
> this implements the error return for loading a snapshot while dirty
> bitmaps are active.
>
> Kevin Wolf (3):
>   block: Add errp to bdrv_snapshot_goto()
>   block: Add errp to bdrv_all_goto_snapshot()
>   block: Error out on load_vm with active dirty bitmaps
>
>  include/block/snapshot.h |  6 ++++--
>  block/snapshot.c         | 43 +++++++++++++++++++++++--------------------
>  migration/savevm.c       |  6 +++---
>  qemu-img.c               |  6 +++---
>  4 files changed, 33 insertions(+), 28 deletions(-)
>
Reviewed-by: Denis V. Lunev <den@openvz.org>

Re: [Qemu-devel] [PATCH for-2.11 1/3] block: Add errp to bdrv_snapshot_goto()

[Vladimir Sementsov-Ogievskiy]

20.11.2017 17:50, Kevin Wolf wrote:
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>   include/block/snapshot.h |  3 ++-
>   block/snapshot.c         | 21 ++++++++++++++++-----
>   qemu-img.c               |  6 +++---
>   3 files changed, 21 insertions(+), 9 deletions(-)
>
> diff --git a/include/block/snapshot.h b/include/block/snapshot.h
> index e5c0553115..aeb80405e8 100644
> --- a/include/block/snapshot.h
> +++ b/include/block/snapshot.h
> @@ -57,7 +57,8 @@ int bdrv_can_snapshot(BlockDriverState *bs);
>   int bdrv_snapshot_create(BlockDriverState *bs,
>                            QEMUSnapshotInfo *sn_info);
>   int bdrv_snapshot_goto(BlockDriverState *bs,
> -                       const char *snapshot_id);
> +                       const char *snapshot_id,
> +                       Error **errp);
>   int bdrv_snapshot_delete(BlockDriverState *bs,
>                            const char *snapshot_id,
>                            const char *name,
> diff --git a/block/snapshot.c b/block/snapshot.c
> index be0743abac..9c941e638d 100644
> --- a/block/snapshot.c
> +++ b/block/snapshot.c
> @@ -177,18 +177,21 @@ int bdrv_snapshot_create(BlockDriverState *bs,
>   }
>   
>   int bdrv_snapshot_goto(BlockDriverState *bs,
> -                       const char *snapshot_id)
> +                       const char *snapshot_id,
> +                       Error **errp)
>   {
>       BlockDriver *drv = bs->drv;
>       int ret, open_ret;
>       int64_t len;
>   
>       if (!drv) {
> +        error_setg(errp, "Block driver is closed");
>           return -ENOMEDIUM;
>       }
>   
>       len = bdrv_getlength(bs);
>       if (len < 0) {
> +        error_setg_errno(errp, -len, "Cannot get block device size");
>           return len;
>       }
>       /* We should set all bits in all enabled dirty bitmaps, because dirty
> @@ -200,13 +203,18 @@ int bdrv_snapshot_goto(BlockDriverState *bs,
>       bdrv_set_dirty(bs, 0, len);
>   
>       if (drv->bdrv_snapshot_goto) {
> -        return drv->bdrv_snapshot_goto(bs, snapshot_id);
> +        ret = drv->bdrv_snapshot_goto(bs, snapshot_id);
> +        if (ret < 0) {
> +            error_setg_errno(errp, -ret, "Failed to load snapshot");
> +        }
> +        return ret;
>       }
>   
>       if (bs->file) {
>           BlockDriverState *file;
>           QDict *options = qdict_clone_shallow(bs->options);
>           QDict *file_options;
> +        Error *local_err = NULL;
>   
>           file = bs->file->bs;
>           /* Prevent it from getting deleted when detached from bs */
> @@ -220,12 +228,14 @@ int bdrv_snapshot_goto(BlockDriverState *bs,
>           bdrv_unref_child(bs, bs->file);
>           bs->file = NULL;
>   
> -        ret = bdrv_snapshot_goto(file, snapshot_id);
> -        open_ret = drv->bdrv_open(bs, options, bs->open_flags, NULL);
> +        ret = bdrv_snapshot_goto(file, snapshot_id, errp);
> +        open_ret = drv->bdrv_open(bs, options, bs->open_flags, &local_err);
>           QDECREF(options);
>           if (open_ret < 0) {
>               bdrv_unref(file);
>               bs->drv = NULL;
> +            /* A bdrv_snapshot_goto() error takes precedence */

you return open_ret from bdrv_open and err msg from bdrv_snapshot_goto.
the may not correspond to each other.

> +            error_propagate(errp, local_err);
>               return open_ret;
>           }
>   
> @@ -234,6 +244,7 @@ int bdrv_snapshot_goto(BlockDriverState *bs,
>           return ret;
>       }
>   
> +    error_setg(errp, "Block driver does not support snapshots");
>       return -ENOTSUP;
>   }
>   
> @@ -467,7 +478,7 @@ int bdrv_all_goto_snapshot(const char *name, BlockDriverState **first_bad_bs)
>   
>           aio_context_acquire(ctx);
>           if (bdrv_can_snapshot(bs)) {
> -            err = bdrv_snapshot_goto(bs, name);
> +            err = bdrv_snapshot_goto(bs, name, NULL);
>           }
>           aio_context_release(ctx);
>           if (err < 0) {
> diff --git a/qemu-img.c b/qemu-img.c
> index 02a6e27beb..68b375f998 100644
> --- a/qemu-img.c
> +++ b/qemu-img.c
> @@ -2989,10 +2989,10 @@ static int img_snapshot(int argc, char **argv)
>           break;
>   
>       case SNAPSHOT_APPLY:
> -        ret = bdrv_snapshot_goto(bs, snapshot_name);
> +        ret = bdrv_snapshot_goto(bs, snapshot_name, &err);
>           if (ret) {
> -            error_report("Could not apply snapshot '%s': %d (%s)",
> -                snapshot_name, ret, strerror(-ret));
> +            error_reportf_err(err, "Could not apply snapshot '%s': ",
> +                              snapshot_name);
>           }
>           break;
>   


-- 
Best regards,
Vladimir

Re: [Qemu-devel] [PATCH for-2.11 2/3] block: Add errp to bdrv_all_goto_snapshot()

[Vladimir Sementsov-Ogievskiy]

20.11.2017 17:50, Kevin Wolf wrote:
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>   include/block/snapshot.h |  3 ++-
>   block/snapshot.c         | 11 ++++++-----
>   migration/savevm.c       |  6 +++---
>   3 files changed, 11 insertions(+), 9 deletions(-)
>
> diff --git a/include/block/snapshot.h b/include/block/snapshot.h
> index aeb80405e8..9407799941 100644
> --- a/include/block/snapshot.h
> +++ b/include/block/snapshot.h
> @@ -84,7 +84,8 @@ int bdrv_snapshot_load_tmp_by_id_or_name(BlockDriverState *bs,
>   bool bdrv_all_can_snapshot(BlockDriverState **first_bad_bs);
>   int bdrv_all_delete_snapshot(const char *name, BlockDriverState **first_bsd_bs,
>                                Error **err);
> -int bdrv_all_goto_snapshot(const char *name, BlockDriverState **first_bsd_bs);
> +int bdrv_all_goto_snapshot(const char *name, BlockDriverState **first_bad_bs,
> +                           Error **errp);
>   int bdrv_all_find_snapshot(const char *name, BlockDriverState **first_bad_bs);
>   int bdrv_all_create_snapshot(QEMUSnapshotInfo *sn,
>                                BlockDriverState *vm_state_bs,
> diff --git a/block/snapshot.c b/block/snapshot.c
> index 9c941e638d..13ec3b1c8c 100644
> --- a/block/snapshot.c
> +++ b/block/snapshot.c
> @@ -467,9 +467,10 @@ fail:
>   }
>   
>   
> -int bdrv_all_goto_snapshot(const char *name, BlockDriverState **first_bad_bs)
> +int bdrv_all_goto_snapshot(const char *name, BlockDriverState **first_bad_bs,
> +                           Error **errp)
>   {
> -    int err = 0;
> +    int ret = 0;
>       BlockDriverState *bs;
>       BdrvNextIterator it;
>   
> @@ -478,10 +479,10 @@ int bdrv_all_goto_snapshot(const char *name, BlockDriverState **first_bad_bs)
>   
>           aio_context_acquire(ctx);
>           if (bdrv_can_snapshot(bs)) {
> -            err = bdrv_snapshot_goto(bs, name, NULL);
> +            ret = bdrv_snapshot_goto(bs, name, errp);
>           }
>           aio_context_release(ctx);
> -        if (err < 0) {
> +        if (ret < 0) {
>               bdrv_next_cleanup(&it);
>               goto fail;
>           }
> @@ -489,7 +490,7 @@ int bdrv_all_goto_snapshot(const char *name, BlockDriverState **first_bad_bs)
>   
>   fail:
>       *first_bad_bs = bs;
> -    return err;
> +    return ret;
>   }
>   
>   int bdrv_all_find_snapshot(const char *name, BlockDriverState **first_bad_bs)
> diff --git a/migration/savevm.c b/migration/savevm.c
> index 4a88228614..192f2d82cd 100644
> --- a/migration/savevm.c
> +++ b/migration/savevm.c
> @@ -2346,10 +2346,10 @@ int load_snapshot(const char *name, Error **errp)
>       /* Flush all IO requests so they don't interfere with the new state.  */
>       bdrv_drain_all_begin();
>   
> -    ret = bdrv_all_goto_snapshot(name, &bs);
> +    ret = bdrv_all_goto_snapshot(name, &bs, errp);
>       if (ret < 0) {
> -        error_setg(errp, "Error %d while activating snapshot '%s' on '%s'",
> -                     ret, name, bdrv_get_device_name(bs));
> +        error_prepend(errp, "Could not load snapshot '%s' on '%s': ",
> +                      name, bdrv_get_device_name(bs));

possible resulting msg: "Could not load snapshot 'snap0' on 'dev0': 
Failed to load snapshot". a bit weird. with this fixed or not:

Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>

>           goto err_drain;
>       }
>   


-- 
Best regards,
Vladimir

Re: [Qemu-devel] [PATCH for-2.11 3/3] block: Error out on load_vm with active dirty bitmaps

[Vladimir Sementsov-Ogievskiy]

20.11.2017 17:50, Kevin Wolf wrote:
> Loading a snapshot invalidates the bitmap. Just marking all blocks dirty
> is not a useful response in practice, instead the user needs to be aware
> that we switch to a completely different state. If they are okay with
> losing the dirty bitmap, they can just explicitly delete it.
>
> This effectively reverts commit 04dec3c3ae5.
>
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>


Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>

> ---
>   block/snapshot.c | 15 +++------------
>   1 file changed, 3 insertions(+), 12 deletions(-)
>
> diff --git a/block/snapshot.c b/block/snapshot.c
> index 13ec3b1c8c..6b338978c5 100644
> --- a/block/snapshot.c
> +++ b/block/snapshot.c
> @@ -182,25 +182,16 @@ int bdrv_snapshot_goto(BlockDriverState *bs,
>   {
>       BlockDriver *drv = bs->drv;
>       int ret, open_ret;
> -    int64_t len;
>   
>       if (!drv) {
>           error_setg(errp, "Block driver is closed");
>           return -ENOMEDIUM;
>       }
>   
> -    len = bdrv_getlength(bs);
> -    if (len < 0) {
> -        error_setg_errno(errp, -len, "Cannot get block device size");
> -        return len;
> +    if (!QLIST_EMPTY(&bs->dirty_bitmaps)) {
> +        error_setg(errp, "Device has active dirty bitmaps");
> +        return -EBUSY;
>       }
> -    /* We should set all bits in all enabled dirty bitmaps, because dirty
> -     * bitmaps reflect active state of disk and snapshot switch operation
> -     * actually dirties active state.
> -     * TODO: It may make sense not to set all bits but analyze block status of
> -     * current state and destination snapshot and do not set bits corresponding
> -     * to both-zero or both-unallocated areas. */
> -    bdrv_set_dirty(bs, 0, len);
>   
>       if (drv->bdrv_snapshot_goto) {
>           ret = drv->bdrv_snapshot_goto(bs, snapshot_id);


-- 
Best regards,
Vladimir

Re: [Qemu-devel] [PATCH for-2.11 1/3] block: Add errp to bdrv_snapshot_goto()

[Kevin Wolf]

Am 20.11.2017 um 17:07 hat Vladimir Sementsov-Ogievskiy geschrieben:
> 20.11.2017 17:50, Kevin Wolf wrote:
> > Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> > ---
> >   include/block/snapshot.h |  3 ++-
> >   block/snapshot.c         | 21 ++++++++++++++++-----
> >   qemu-img.c               |  6 +++---
> >   3 files changed, 21 insertions(+), 9 deletions(-)
> > 
> > diff --git a/include/block/snapshot.h b/include/block/snapshot.h
> > index e5c0553115..aeb80405e8 100644
> > --- a/include/block/snapshot.h
> > +++ b/include/block/snapshot.h
> > @@ -57,7 +57,8 @@ int bdrv_can_snapshot(BlockDriverState *bs);
> >   int bdrv_snapshot_create(BlockDriverState *bs,
> >                            QEMUSnapshotInfo *sn_info);
> >   int bdrv_snapshot_goto(BlockDriverState *bs,
> > -                       const char *snapshot_id);
> > +                       const char *snapshot_id,
> > +                       Error **errp);
> >   int bdrv_snapshot_delete(BlockDriverState *bs,
> >                            const char *snapshot_id,
> >                            const char *name,
> > diff --git a/block/snapshot.c b/block/snapshot.c
> > index be0743abac..9c941e638d 100644
> > --- a/block/snapshot.c
> > +++ b/block/snapshot.c
> > @@ -177,18 +177,21 @@ int bdrv_snapshot_create(BlockDriverState *bs,
> >   }
> >   int bdrv_snapshot_goto(BlockDriverState *bs,
> > -                       const char *snapshot_id)
> > +                       const char *snapshot_id,
> > +                       Error **errp)
> >   {
> >       BlockDriver *drv = bs->drv;
> >       int ret, open_ret;
> >       int64_t len;
> >       if (!drv) {
> > +        error_setg(errp, "Block driver is closed");
> >           return -ENOMEDIUM;
> >       }
> >       len = bdrv_getlength(bs);
> >       if (len < 0) {
> > +        error_setg_errno(errp, -len, "Cannot get block device size");
> >           return len;
> >       }
> >       /* We should set all bits in all enabled dirty bitmaps, because dirty
> > @@ -200,13 +203,18 @@ int bdrv_snapshot_goto(BlockDriverState *bs,
> >       bdrv_set_dirty(bs, 0, len);
> >       if (drv->bdrv_snapshot_goto) {
> > -        return drv->bdrv_snapshot_goto(bs, snapshot_id);
> > +        ret = drv->bdrv_snapshot_goto(bs, snapshot_id);
> > +        if (ret < 0) {
> > +            error_setg_errno(errp, -ret, "Failed to load snapshot");
> > +        }
> > +        return ret;
> >       }
> >       if (bs->file) {
> >           BlockDriverState *file;
> >           QDict *options = qdict_clone_shallow(bs->options);
> >           QDict *file_options;
> > +        Error *local_err = NULL;
> >           file = bs->file->bs;
> >           /* Prevent it from getting deleted when detached from bs */
> > @@ -220,12 +228,14 @@ int bdrv_snapshot_goto(BlockDriverState *bs,
> >           bdrv_unref_child(bs, bs->file);
> >           bs->file = NULL;
> > -        ret = bdrv_snapshot_goto(file, snapshot_id);
> > -        open_ret = drv->bdrv_open(bs, options, bs->open_flags, NULL);
> > +        ret = bdrv_snapshot_goto(file, snapshot_id, errp);
> > +        open_ret = drv->bdrv_open(bs, options, bs->open_flags, &local_err);
> >           QDECREF(options);
> >           if (open_ret < 0) {
> >               bdrv_unref(file);
> >               bs->drv = NULL;
> > +            /* A bdrv_snapshot_goto() error takes precedence */
> 
> you return open_ret from bdrv_open and err msg from bdrv_snapshot_goto.
> the may not correspond to each other.

Hm, yes. Is this a problem, though?

If it is, I guess we can change the return value below:

> > +            error_propagate(errp, local_err);
> >               return open_ret;

return ret < 0 ? ret : open_ret;

Would people prefer this?

Kevin

Re: [Qemu-devel] [PATCH for-2.11 1/3] block: Add errp to bdrv_snapshot_goto()

[Vladimir Sementsov-Ogievskiy]

20.11.2017 19:23, Kevin Wolf wrote:
> Am 20.11.2017 um 17:07 hat Vladimir Sementsov-Ogievskiy geschrieben:
>> 20.11.2017 17:50, Kevin Wolf wrote:
>>> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
>>> ---
>>>    include/block/snapshot.h |  3 ++-
>>>    block/snapshot.c         | 21 ++++++++++++++++-----
>>>    qemu-img.c               |  6 +++---
>>>    3 files changed, 21 insertions(+), 9 deletions(-)
>>>
>>> diff --git a/include/block/snapshot.h b/include/block/snapshot.h
>>> index e5c0553115..aeb80405e8 100644
>>> --- a/include/block/snapshot.h
>>> +++ b/include/block/snapshot.h
>>> @@ -57,7 +57,8 @@ int bdrv_can_snapshot(BlockDriverState *bs);
>>>    int bdrv_snapshot_create(BlockDriverState *bs,
>>>                             QEMUSnapshotInfo *sn_info);
>>>    int bdrv_snapshot_goto(BlockDriverState *bs,
>>> -                       const char *snapshot_id);
>>> +                       const char *snapshot_id,
>>> +                       Error **errp);
>>>    int bdrv_snapshot_delete(BlockDriverState *bs,
>>>                             const char *snapshot_id,
>>>                             const char *name,
>>> diff --git a/block/snapshot.c b/block/snapshot.c
>>> index be0743abac..9c941e638d 100644
>>> --- a/block/snapshot.c
>>> +++ b/block/snapshot.c
>>> @@ -177,18 +177,21 @@ int bdrv_snapshot_create(BlockDriverState *bs,
>>>    }
>>>    int bdrv_snapshot_goto(BlockDriverState *bs,
>>> -                       const char *snapshot_id)
>>> +                       const char *snapshot_id,
>>> +                       Error **errp)
>>>    {
>>>        BlockDriver *drv = bs->drv;
>>>        int ret, open_ret;
>>>        int64_t len;
>>>        if (!drv) {
>>> +        error_setg(errp, "Block driver is closed");
>>>            return -ENOMEDIUM;
>>>        }
>>>        len = bdrv_getlength(bs);
>>>        if (len < 0) {
>>> +        error_setg_errno(errp, -len, "Cannot get block device size");
>>>            return len;
>>>        }
>>>        /* We should set all bits in all enabled dirty bitmaps, because dirty
>>> @@ -200,13 +203,18 @@ int bdrv_snapshot_goto(BlockDriverState *bs,
>>>        bdrv_set_dirty(bs, 0, len);
>>>        if (drv->bdrv_snapshot_goto) {
>>> -        return drv->bdrv_snapshot_goto(bs, snapshot_id);
>>> +        ret = drv->bdrv_snapshot_goto(bs, snapshot_id);
>>> +        if (ret < 0) {
>>> +            error_setg_errno(errp, -ret, "Failed to load snapshot");
>>> +        }
>>> +        return ret;
>>>        }
>>>        if (bs->file) {
>>>            BlockDriverState *file;
>>>            QDict *options = qdict_clone_shallow(bs->options);
>>>            QDict *file_options;
>>> +        Error *local_err = NULL;
>>>            file = bs->file->bs;
>>>            /* Prevent it from getting deleted when detached from bs */
>>> @@ -220,12 +228,14 @@ int bdrv_snapshot_goto(BlockDriverState *bs,
>>>            bdrv_unref_child(bs, bs->file);
>>>            bs->file = NULL;
>>> -        ret = bdrv_snapshot_goto(file, snapshot_id);
>>> -        open_ret = drv->bdrv_open(bs, options, bs->open_flags, NULL);
>>> +        ret = bdrv_snapshot_goto(file, snapshot_id, errp);
>>> +        open_ret = drv->bdrv_open(bs, options, bs->open_flags, &local_err);
>>>            QDECREF(options);
>>>            if (open_ret < 0) {
>>>                bdrv_unref(file);
>>>                bs->drv = NULL;
>>> +            /* A bdrv_snapshot_goto() error takes precedence */
>> you return open_ret from bdrv_open and err msg from bdrv_snapshot_goto.
>> the may not correspond to each other.
> Hm, yes. Is this a problem, though?
>
> If it is, I guess we can change the return value below:
>
>>> +            error_propagate(errp, local_err);
>>>                return open_ret;
> return ret < 0 ? ret : open_ret;
>
> Would people prefer this?

looks good,

Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>

>
> Kevin


-- 
Best regards,
Vladimir

Re: [Qemu-devel] [PATCH for-2.11 0/3] block: Error out on load_vm with active dirty bitmaps

[John Snow]

On 11/20/2017 09:50 AM, Kevin Wolf wrote:
> Following the discussing at
> https://lists.gnu.org/archive/html/qemu-devel/2017-11/msg03572.html
> this implements the error return for loading a snapshot while dirty
> bitmaps are active.
> 
> Kevin Wolf (3):
>   block: Add errp to bdrv_snapshot_goto()
>   block: Add errp to bdrv_all_goto_snapshot()
>   block: Error out on load_vm with active dirty bitmaps
> 
>  include/block/snapshot.h |  6 ++++--
>  block/snapshot.c         | 43 +++++++++++++++++++++++--------------------
>  migration/savevm.c       |  6 +++---
>  qemu-img.c               |  6 +++---
>  4 files changed, 33 insertions(+), 28 deletions(-)
> 

You have enough reviews, but just so it's unambiguous, I'm fine with
this approach in lieu of Vladimir's; so:

Reviewed-by: John Snow <jsnow@redhat.com>