Re: [Qemu-devel] [PATCH 05/10] snapshot: create bdrv_all_find_snapshot helper

[Denis V. Lunev]

On 11/17/2015 10:22 AM, Stefan Hajnoczi wrote:

On Mon, Nov 16, 2015 at 06:24:36PM +0300, Denis V. Lunev wrote:

+int bdrv_all_find_snapshot(const char *name, bool skip_read_only,
+   BlockDriverState **first_bad_bs)
+{
+QEMUSnapshotInfo sn;
+int err = 0;
+BlockDriverState *bs = NULL;
+
+while (err == 0 && (bs = bdrv_next(bs))) {
+AioContext *ctx = bdrv_get_aio_context(bs);
+
+if (skip_read_only &&
+(!bdrv_is_inserted(bs) || bdrv_is_read_only(bs))) {

These must be called with AioContext acquired.



AFAIK no. this is called without that in bdrv jobs code.

bdrv_is_read_only is a simple flag, which is filled on open
bdrv_is_inserted is defined for CDROM only and does
not rely on AIO context stuff.


+continue;
+}
+
+aio_context_acquire(ctx);
+if (bdrv_can_snapshot(bs)) {

The !bdrv_is_inserted(bs) || bdrv_is_read_only(bs) checks above are
redundant since bdrv_can_snapshot() checks too:

   int bdrv_can_snapshot(BlockDriverState *bs)
   {
   BlockDriver *drv = bs->drv;
   if (!drv || !bdrv_is_inserted(bs) || bdrv_is_read_only(bs)) {
   return 0;
   }

It also means that the "skip_read_only" name is inaccurate.  Read-only
drives are always skipped, regardless of skip_read_only's value.

The skip_read_only argument can be dropped and the earlier
!bdrv_is_inserted(bs) || bdrv_is_read_only(bs) check can be dropped too.

I have though on this several times and once again I am wrong :(
This looks like a punishment for my sins. OK :(



@@ -2168,21 +2157,7 @@ void hmp_info_snapshots(Monitor *mon, const QDict *qdict)
  available_snapshots = g_new0(int, nb_sns);
  total = 0;
  for (i = 0; i < nb_sns; i++) {
-sn = _tab[i];
-available = 1;
-bs1 = NULL;
-
-while ((bs1 = bdrv_next(bs1))) {
-if (bdrv_can_snapshot(bs1) && bs1 != bs) {
-ret = bdrv_snapshot_find(bs1, sn_info, sn->id_str);
-if (ret < 0) {
-available = 0;
-break;
-}
-}
-}
-
-if (available) {
+if (bdrv_all_find_snapshot(sn_tab[i].id_str, false, ) == 0) {

bdrv_all_find_snapshot() doesn't do the bs1 != bs exclusion so the new
code behaves differently from the old code.  That seems like a bug.

no. The result will be the same. This is a minor optimisation:
- we get the first block device we can make snapshot on
- we get snapshot list on that device
- after that we start iterations over the list and removing not 
available snapshots on other devices

This means that if bs == bs1 the check will be "always true"

Re: [Qemu-devel] [PATCH 05/10] snapshot: create bdrv_all_find_snapshot helper

[Stefan Hajnoczi]

On Tue, Nov 17, 2015 at 11:10:26AM +0300, Denis V. Lunev wrote:
> On 11/17/2015 10:22 AM, Stefan Hajnoczi wrote:
> >On Mon, Nov 16, 2015 at 06:24:36PM +0300, Denis V. Lunev wrote:
> >>@@ -2168,21 +2157,7 @@ void hmp_info_snapshots(Monitor *mon, const QDict 
> >>*qdict)
> >>  available_snapshots = g_new0(int, nb_sns);
> >>  total = 0;
> >>  for (i = 0; i < nb_sns; i++) {
> >>-sn = _tab[i];
> >>-available = 1;
> >>-bs1 = NULL;
> >>-
> >>-while ((bs1 = bdrv_next(bs1))) {
> >>-if (bdrv_can_snapshot(bs1) && bs1 != bs) {
> >>-ret = bdrv_snapshot_find(bs1, sn_info, sn->id_str);
> >>-if (ret < 0) {
> >>-available = 0;
> >>-break;
> >>-}
> >>-}
> >>-}
> >>-
> >>-if (available) {
> >>+if (bdrv_all_find_snapshot(sn_tab[i].id_str, false, ) == 0) {
> >bdrv_all_find_snapshot() doesn't do the bs1 != bs exclusion so the new
> >code behaves differently from the old code.  That seems like a bug.
> no. The result will be the same. This is a minor optimisation:
> - we get the first block device we can make snapshot on
> - we get snapshot list on that device
> - after that we start iterations over the list and removing not available
> snapshots on other devices
> This means that if bs == bs1 the check will be "always true"

Ah, I see.  Thanks!

Stefan


signature.asc
Description: PGP signature

Re: [Qemu-devel] [PATCH 05/10] snapshot: create bdrv_all_find_snapshot helper

[Denis V. Lunev]

On 11/16/2015 12:31 PM, Stefan Hajnoczi wrote:

On Tue, Nov 10, 2015 at 05:25:30PM +0300, Denis V. Lunev wrote:

+int bdrv_all_find_snapshot(const char *name, bool read_only,
+   BlockDriverState **first_bad_bs)
+{
+QEMUSnapshotInfo sn;
+int err = 0;
+BlockDriverState *bs = NULL;
+
+while (err == 0 && (bs = bdrv_next(bs))) {
+AioContext *ctx = bdrv_get_aio_context(bs);
+
+aio_context_acquire(ctx);
+if (read_only || (bdrv_is_inserted(bs) && !bdrv_is_read_only(bs))) {
+err = bdrv_snapshot_find(bs, , name);
+}
+aio_context_release(ctx);
+}
+
+*first_bad_bs = bs;
+return err;
+}

It's difficult to see how bdrv_all_find_snapshot(read_only=true) is
equivalent to what you replaced below:


@@ -1500,21 +1489,7 @@ void hmp_info_snapshots(Monitor *mon, const QDict *qdict)
  available_snapshots = g_new0(int, nb_sns);
  total = 0;
  for (i = 0; i < nb_sns; i++) {
-sn = _tab[i];
-available = 1;
-bs1 = NULL;
-
-while ((bs1 = bdrv_next(bs1))) {
-if (bdrv_can_snapshot(bs1) && bs1 != bs) {
-ret = bdrv_snapshot_find(bs1, sn_info, sn->id_str);
-if (ret < 0) {
-available = 0;
-break;
-}
-}
-}
-
-if (available) {
+if (bdrv_all_find_snapshot(sn_tab[i].id_str, true, ) == 0) {

The original loop skips drives that cannot snapshot and the vmstate
drive.  The new code tries to find a snapshot all devices.

To me it seems the semantics are changed.  Can you explain why this
change is safe?
argh... This code is used for 'virsh qemu-monitor-command  --hmp rhel7 
info snapshots'
and NOT used for 'virsh snapshot-list rhel7' or used in a very different 
way.


This is the root of the problem.

You are right, the command is broken completely. Some refactoring is 
necessary

here.

Den

Re: [Qemu-devel] [PATCH 05/10] snapshot: create bdrv_all_find_snapshot helper

[Stefan Hajnoczi]

On Tue, Nov 10, 2015 at 05:25:30PM +0300, Denis V. Lunev wrote:
> +int bdrv_all_find_snapshot(const char *name, bool read_only,
> +   BlockDriverState **first_bad_bs)
> +{
> +QEMUSnapshotInfo sn;
> +int err = 0;
> +BlockDriverState *bs = NULL;
> +
> +while (err == 0 && (bs = bdrv_next(bs))) {
> +AioContext *ctx = bdrv_get_aio_context(bs);
> +
> +aio_context_acquire(ctx);
> +if (read_only || (bdrv_is_inserted(bs) && !bdrv_is_read_only(bs))) {
> +err = bdrv_snapshot_find(bs, , name);
> +}
> +aio_context_release(ctx);
> +}
> +
> +*first_bad_bs = bs;
> +return err;
> +}

It's difficult to see how bdrv_all_find_snapshot(read_only=true) is
equivalent to what you replaced below:

> @@ -1500,21 +1489,7 @@ void hmp_info_snapshots(Monitor *mon, const QDict 
> *qdict)
>  available_snapshots = g_new0(int, nb_sns);
>  total = 0;
>  for (i = 0; i < nb_sns; i++) {
> -sn = _tab[i];
> -available = 1;
> -bs1 = NULL;
> -
> -while ((bs1 = bdrv_next(bs1))) {
> -if (bdrv_can_snapshot(bs1) && bs1 != bs) {
> -ret = bdrv_snapshot_find(bs1, sn_info, sn->id_str);
> -if (ret < 0) {
> -available = 0;
> -break;
> -}
> -}
> -}
> -
> -if (available) {
> +if (bdrv_all_find_snapshot(sn_tab[i].id_str, true, ) == 0) {

The original loop skips drives that cannot snapshot and the vmstate
drive.  The new code tries to find a snapshot all devices.

To me it seems the semantics are changed.  Can you explain why this
change is safe?


signature.asc
Description: PGP signature

[Qemu-devel] [PATCH 05/10] snapshot: create bdrv_all_find_snapshot helper

[Denis V. Lunev]

to check that snapshot is available for all loaded block drivers. The
ability to switch to snapshot is verified separately using
bdrv_all_can_snapshot.

The patch also ensures proper locking.

Signed-off-by: Denis V. Lunev 
Reviewed-by: Fam Zheng 
CC: Juan Quintela 
CC: Stefan Hajnoczi 
CC: Kevin Wolf 
---
 block/snapshot.c | 26 +++
 include/block/snapshot.h |  2 ++
 migration/savevm.c   | 55 +---
 3 files changed, 43 insertions(+), 40 deletions(-)

diff --git a/block/snapshot.c b/block/snapshot.c
index 9f07a63..eb82873 100644
--- a/block/snapshot.c
+++ b/block/snapshot.c
@@ -423,3 +423,29 @@ int bdrv_all_goto_snapshot(const char *name, 
BlockDriverState **first_bad_bs)
 *first_bad_bs = bs;
 return err;
 }
+
+int bdrv_all_find_snapshot(const char *name, bool skip_read_only,
+   BlockDriverState **first_bad_bs)
+{
+QEMUSnapshotInfo sn;
+int err = 0;
+BlockDriverState *bs = NULL;
+
+while (err == 0 && (bs = bdrv_next(bs))) {
+AioContext *ctx = bdrv_get_aio_context(bs);
+
+if (skip_read_only &&
+(!bdrv_is_inserted(bs) || bdrv_is_read_only(bs))) {
+continue;
+}
+
+aio_context_acquire(ctx);
+if (bdrv_can_snapshot(bs)) {
+err = bdrv_snapshot_find(bs, , name);
+}
+aio_context_release(ctx);
+}
+
+*first_bad_bs = bs;
+return err;
+}
diff --git a/include/block/snapshot.h b/include/block/snapshot.h
index 0a176c7..431360a 100644
--- a/include/block/snapshot.h
+++ b/include/block/snapshot.h
@@ -85,5 +85,7 @@ 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_find_snapshot(const char *name, bool skip_read_only,
+   BlockDriverState **first_bad_bs);
 
 #endif
diff --git a/migration/savevm.c b/migration/savevm.c
index 254e51d..a391b3a 100644
--- a/migration/savevm.c
+++ b/migration/savevm.c
@@ -2051,6 +2051,18 @@ int load_vmstate(const char *name)
 QEMUFile *f;
 int ret;
 
+if (!bdrv_all_can_snapshot()) {
+error_report("Device '%s' is writable but does not support snapshots.",
+ bdrv_get_device_name(bs));
+return -ENOTSUP;
+}
+ret = bdrv_all_find_snapshot(name, true, );
+if (ret < 0) {
+error_report("Device '%s' does not have the requested snapshot '%s'",
+ bdrv_get_device_name(bs), name);
+return ret;
+}
+
 bs_vm_state = find_vmstate_bs();
 if (!bs_vm_state) {
 error_report("No block device supports snapshots");
@@ -2067,29 +2079,6 @@ int load_vmstate(const char *name)
 return -EINVAL;
 }
 
-/* Verify if there is any device that doesn't support snapshots and is
-writable and check if the requested snapshot is available too. */
-bs = NULL;
-while ((bs = bdrv_next(bs))) {
-
-if (!bdrv_is_inserted(bs) || bdrv_is_read_only(bs)) {
-continue;
-}
-
-if (!bdrv_can_snapshot(bs)) {
-error_report("Device '%s' is writable but does not support 
snapshots.",
-   bdrv_get_device_name(bs));
-return -ENOTSUP;
-}
-
-ret = bdrv_snapshot_find(bs, , name);
-if (ret < 0) {
-error_report("Device '%s' does not have the requested snapshot 
'%s'",
-   bdrv_get_device_name(bs), name);
-return ret;
-}
-}
-
 /* Flush all IO requests so they don't interfere with the new state.  */
 bdrv_drain_all();
 
@@ -2143,8 +2132,8 @@ void hmp_delvm(Monitor *mon, const QDict *qdict)
 void hmp_info_snapshots(Monitor *mon, const QDict *qdict)
 {
 BlockDriverState *bs, *bs1;
-QEMUSnapshotInfo *sn_tab, *sn, s, *sn_info = 
-int nb_sns, i, ret, available;
+QEMUSnapshotInfo *sn_tab, *sn;
+int nb_sns, i;
 int total;
 int *available_snapshots;
 
@@ -2168,21 +2157,7 @@ void hmp_info_snapshots(Monitor *mon, const QDict *qdict)
 available_snapshots = g_new0(int, nb_sns);
 total = 0;
 for (i = 0; i < nb_sns; i++) {
-sn = _tab[i];
-available = 1;
-bs1 = NULL;
-
-while ((bs1 = bdrv_next(bs1))) {
-if (bdrv_can_snapshot(bs1) && bs1 != bs) {
-ret = bdrv_snapshot_find(bs1, sn_info, sn->id_str);
-if (ret < 0) {
-available = 0;
-break;
-}
-}
-}
-
-if (available) {
+if (bdrv_all_find_snapshot(sn_tab[i].id_str, false, ) == 0) {
 available_snapshots[total] = i;
 total++;
 

Re: [Qemu-devel] [PATCH 05/10] snapshot: create bdrv_all_find_snapshot helper

[Denis V. Lunev]

On 11/16/2015 12:31 PM, Stefan Hajnoczi wrote:

On Tue, Nov 10, 2015 at 05:25:30PM +0300, Denis V. Lunev wrote:

+int bdrv_all_find_snapshot(const char *name, bool read_only,
+   BlockDriverState **first_bad_bs)
+{
+QEMUSnapshotInfo sn;
+int err = 0;
+BlockDriverState *bs = NULL;
+
+while (err == 0 && (bs = bdrv_next(bs))) {
+AioContext *ctx = bdrv_get_aio_context(bs);
+
+aio_context_acquire(ctx);
+if (read_only || (bdrv_is_inserted(bs) && !bdrv_is_read_only(bs))) {
+err = bdrv_snapshot_find(bs, , name);
+}
+aio_context_release(ctx);
+}
+
+*first_bad_bs = bs;
+return err;
+}

It's difficult to see how bdrv_all_find_snapshot(read_only=true) is
equivalent to what you replaced below:


@@ -1500,21 +1489,7 @@ void hmp_info_snapshots(Monitor *mon, const QDict *qdict)
  available_snapshots = g_new0(int, nb_sns);
  total = 0;
  for (i = 0; i < nb_sns; i++) {
-sn = _tab[i];
-available = 1;
-bs1 = NULL;
-
-while ((bs1 = bdrv_next(bs1))) {
-if (bdrv_can_snapshot(bs1) && bs1 != bs) {
-ret = bdrv_snapshot_find(bs1, sn_info, sn->id_str);
-if (ret < 0) {
-available = 0;
-break;
-}
-}
-}
-
-if (available) {
+if (bdrv_all_find_snapshot(sn_tab[i].id_str, true, ) == 0) {

The original loop skips drives that cannot snapshot and the vmstate
drive.  The new code tries to find a snapshot all devices.

To me it seems the semantics are changed.  Can you explain why this
change is safe?

yep. you are once again right... OK.

Re: [Qemu-devel] [PATCH 05/10] snapshot: create bdrv_all_find_snapshot helper

[Stefan Hajnoczi]

On Mon, Nov 16, 2015 at 06:24:36PM +0300, Denis V. Lunev wrote:
> +int bdrv_all_find_snapshot(const char *name, bool skip_read_only,
> +   BlockDriverState **first_bad_bs)
> +{
> +QEMUSnapshotInfo sn;
> +int err = 0;
> +BlockDriverState *bs = NULL;
> +
> +while (err == 0 && (bs = bdrv_next(bs))) {
> +AioContext *ctx = bdrv_get_aio_context(bs);
> +
> +if (skip_read_only &&
> +(!bdrv_is_inserted(bs) || bdrv_is_read_only(bs))) {

These must be called with AioContext acquired.

> +continue;
> +}
> +
> +aio_context_acquire(ctx);
> +if (bdrv_can_snapshot(bs)) {

The !bdrv_is_inserted(bs) || bdrv_is_read_only(bs) checks above are
redundant since bdrv_can_snapshot() checks too:

  int bdrv_can_snapshot(BlockDriverState *bs)
  {
  BlockDriver *drv = bs->drv;
  if (!drv || !bdrv_is_inserted(bs) || bdrv_is_read_only(bs)) {
  return 0;
  }

It also means that the "skip_read_only" name is inaccurate.  Read-only
drives are always skipped, regardless of skip_read_only's value.

The skip_read_only argument can be dropped and the earlier
!bdrv_is_inserted(bs) || bdrv_is_read_only(bs) check can be dropped too.

> @@ -2168,21 +2157,7 @@ void hmp_info_snapshots(Monitor *mon, const QDict 
> *qdict)
>  available_snapshots = g_new0(int, nb_sns);
>  total = 0;
>  for (i = 0; i < nb_sns; i++) {
> -sn = _tab[i];
> -available = 1;
> -bs1 = NULL;
> -
> -while ((bs1 = bdrv_next(bs1))) {
> -if (bdrv_can_snapshot(bs1) && bs1 != bs) {
> -ret = bdrv_snapshot_find(bs1, sn_info, sn->id_str);
> -if (ret < 0) {
> -available = 0;
> -break;
> -}
> -}
> -}
> -
> -if (available) {
> +if (bdrv_all_find_snapshot(sn_tab[i].id_str, false, ) == 0) {

bdrv_all_find_snapshot() doesn't do the bs1 != bs exclusion so the new
code behaves differently from the old code.  That seems like a bug.


signature.asc
Description: PGP signature

[Qemu-devel] [PATCH 05/10] snapshot: create bdrv_all_find_snapshot helper

[Denis V. Lunev]

to check that snapshot is available for all loaded block drivers. The
ability to switch to snapshot is verified separately using
bdrv_all_can_snapshot.

The patch also ensures proper locking.

Signed-off-by: Denis V. Lunev 
CC: Juan Quintela 
CC: Stefan Hajnoczi 
CC: Kevin Wolf 
---
 block/snapshot.c | 21 ++
 include/block/snapshot.h |  2 ++
 migration/savevm.c   | 55 +---
 3 files changed, 38 insertions(+), 40 deletions(-)

diff --git a/block/snapshot.c b/block/snapshot.c
index 9f07a63..97dc315 100644
--- a/block/snapshot.c
+++ b/block/snapshot.c
@@ -423,3 +423,24 @@ int bdrv_all_goto_snapshot(const char *name, 
BlockDriverState **first_bad_bs)
 *first_bad_bs = bs;
 return err;
 }
+
+int bdrv_all_find_snapshot(const char *name, bool read_only,
+   BlockDriverState **first_bad_bs)
+{
+QEMUSnapshotInfo sn;
+int err = 0;
+BlockDriverState *bs = NULL;
+
+while (err == 0 && (bs = bdrv_next(bs))) {
+AioContext *ctx = bdrv_get_aio_context(bs);
+
+aio_context_acquire(ctx);
+if (read_only || (bdrv_is_inserted(bs) && !bdrv_is_read_only(bs))) {
+err = bdrv_snapshot_find(bs, , name);
+}
+aio_context_release(ctx);
+}
+
+*first_bad_bs = bs;
+return err;
+}
diff --git a/include/block/snapshot.h b/include/block/snapshot.h
index 0a176c7..0fae32b 100644
--- a/include/block/snapshot.h
+++ b/include/block/snapshot.h
@@ -85,5 +85,7 @@ 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_find_snapshot(const char *name, bool read_only,
+   BlockDriverState **first_bad_bs);
 
 #endif
diff --git a/migration/savevm.c b/migration/savevm.c
index d18ff13..90aa565 100644
--- a/migration/savevm.c
+++ b/migration/savevm.c
@@ -1383,6 +1383,18 @@ int load_vmstate(const char *name)
 QEMUFile *f;
 int ret;
 
+if (!bdrv_all_can_snapshot()) {
+error_report("Device '%s' is writable but does not support snapshots.",
+ bdrv_get_device_name(bs));
+return -ENOTSUP;
+}
+ret = bdrv_all_find_snapshot(name, false, );
+if (ret < 0) {
+error_report("Device '%s' does not have the requested snapshot '%s'",
+ bdrv_get_device_name(bs), name);
+return ret;
+}
+
 bs_vm_state = find_vmstate_bs();
 if (!bs_vm_state) {
 error_report("No block device supports snapshots");
@@ -1399,29 +1411,6 @@ int load_vmstate(const char *name)
 return -EINVAL;
 }
 
-/* Verify if there is any device that doesn't support snapshots and is
-writable and check if the requested snapshot is available too. */
-bs = NULL;
-while ((bs = bdrv_next(bs))) {
-
-if (!bdrv_is_inserted(bs) || bdrv_is_read_only(bs)) {
-continue;
-}
-
-if (!bdrv_can_snapshot(bs)) {
-error_report("Device '%s' is writable but does not support 
snapshots.",
-   bdrv_get_device_name(bs));
-return -ENOTSUP;
-}
-
-ret = bdrv_snapshot_find(bs, , name);
-if (ret < 0) {
-error_report("Device '%s' does not have the requested snapshot 
'%s'",
-   bdrv_get_device_name(bs), name);
-return ret;
-}
-}
-
 /* Flush all IO requests so they don't interfere with the new state.  */
 bdrv_drain_all();
 
@@ -1475,8 +1464,8 @@ void hmp_delvm(Monitor *mon, const QDict *qdict)
 void hmp_info_snapshots(Monitor *mon, const QDict *qdict)
 {
 BlockDriverState *bs, *bs1;
-QEMUSnapshotInfo *sn_tab, *sn, s, *sn_info = 
-int nb_sns, i, ret, available;
+QEMUSnapshotInfo *sn_tab, *sn;
+int nb_sns, i;
 int total;
 int *available_snapshots;
 
@@ -1500,21 +1489,7 @@ void hmp_info_snapshots(Monitor *mon, const QDict *qdict)
 available_snapshots = g_new0(int, nb_sns);
 total = 0;
 for (i = 0; i < nb_sns; i++) {
-sn = _tab[i];
-available = 1;
-bs1 = NULL;
-
-while ((bs1 = bdrv_next(bs1))) {
-if (bdrv_can_snapshot(bs1) && bs1 != bs) {
-ret = bdrv_snapshot_find(bs1, sn_info, sn->id_str);
-if (ret < 0) {
-available = 0;
-break;
-}
-}
-}
-
-if (available) {
+if (bdrv_all_find_snapshot(sn_tab[i].id_str, true, ) == 0) {
 available_snapshots[total] = i;
 total++;
 }
-- 
2.5.0

Re: [Qemu-devel] [PATCH 05/10] snapshot: create bdrv_all_find_snapshot helper

[Stefan Hajnoczi]

On Sat, Nov 07, 2015 at 06:54:55PM +0300, Denis V. Lunev wrote:

This:

> +int bdrv_all_find_snapshot(const char *name, BlockDriverState **first_bad_bs)
> +{
> +QEMUSnapshotInfo sn;
> +int err = 0;
> +BlockDriverState *bs = NULL;
> +
> +while (err == 0 && (bs = bdrv_next(bs))) {
> +AioContext *ctx = bdrv_get_aio_context(bs);
> +
> +aio_context_acquire(ctx);
> +if (bdrv_is_inserted(bs) && !bdrv_is_read_only(bs)) {
> +err = bdrv_snapshot_find(bs, , name);
> +}
> +aio_context_release(ctx);
> +}
> +
> +*first_bad_bs = bs;
> +return err;
> +}

and this:

> @@ -1500,21 +1489,7 @@ void hmp_info_snapshots(Monitor *mon, const QDict 
> *qdict)
>  available_snapshots = g_new0(int, nb_sns);
>  total = 0;
>  for (i = 0; i < nb_sns; i++) {
> -sn = _tab[i];
> -available = 1;
> -bs1 = NULL;
> -
> -while ((bs1 = bdrv_next(bs1))) {
> -if (bdrv_can_snapshot(bs1) && bs1 != bs) {
> -ret = bdrv_snapshot_find(bs1, sn_info, sn->id_str);
> -if (ret < 0) {
> -available = 0;
> -break;
> -}
> -}
> -}
> -
> -if (available) {
> +if (bdrv_all_find_snapshot(sn_tab[i].id_str, ) == 0) {
>  available_snapshots[total] = i;
>  total++;
>  }

is not equivalent.  hmp_info_snapshots() skips devices that are inserted
and writeable but do not support snapshots.  The new code will fail in
that case.


signature.asc
Description: PGP signature

[Qemu-devel] [PATCH 05/10] snapshot: create bdrv_all_find_snapshot helper

[Denis V. Lunev]

to check that snapshot is available for all loaded block drivers. The
ability to switch to snapshot is verified separately using
bdrv_all_can_snapshot.

The patch also ensures proper locking.

Signed-off-by: Denis V. Lunev 
CC: Juan Quintela 
CC: Stefan Hajnoczi 
CC: Kevin Wolf 
---
 block/snapshot.c | 20 ++
 include/block/snapshot.h |  1 +
 migration/savevm.c   | 55 +---
 3 files changed, 36 insertions(+), 40 deletions(-)

diff --git a/block/snapshot.c b/block/snapshot.c
index 9f07a63..a7f1f8d 100644
--- a/block/snapshot.c
+++ b/block/snapshot.c
@@ -423,3 +423,23 @@ int bdrv_all_goto_snapshot(const char *name, 
BlockDriverState **first_bad_bs)
 *first_bad_bs = bs;
 return err;
 }
+
+int bdrv_all_find_snapshot(const char *name, BlockDriverState **first_bad_bs)
+{
+QEMUSnapshotInfo sn;
+int err = 0;
+BlockDriverState *bs = NULL;
+
+while (err == 0 && (bs = bdrv_next(bs))) {
+AioContext *ctx = bdrv_get_aio_context(bs);
+
+aio_context_acquire(ctx);
+if (bdrv_is_inserted(bs) && !bdrv_is_read_only(bs)) {
+err = bdrv_snapshot_find(bs, , name);
+}
+aio_context_release(ctx);
+}
+
+*first_bad_bs = bs;
+return err;
+}
diff --git a/include/block/snapshot.h b/include/block/snapshot.h
index 0a176c7..10ee582 100644
--- a/include/block/snapshot.h
+++ b/include/block/snapshot.h
@@ -85,5 +85,6 @@ 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_find_snapshot(const char *name, BlockDriverState **first_bad_bs);
 
 #endif
diff --git a/migration/savevm.c b/migration/savevm.c
index d18ff13..a9c1130 100644
--- a/migration/savevm.c
+++ b/migration/savevm.c
@@ -1383,6 +1383,18 @@ int load_vmstate(const char *name)
 QEMUFile *f;
 int ret;
 
+if (!bdrv_all_can_snapshot()) {
+error_report("Device '%s' is writable but does not support snapshots.",
+ bdrv_get_device_name(bs));
+return -ENOTSUP;
+}
+ret = bdrv_all_find_snapshot(name, );
+if (ret < 0) {
+error_report("Device '%s' does not have the requested snapshot '%s'",
+ bdrv_get_device_name(bs), name);
+return ret;
+}
+
 bs_vm_state = find_vmstate_bs();
 if (!bs_vm_state) {
 error_report("No block device supports snapshots");
@@ -1399,29 +1411,6 @@ int load_vmstate(const char *name)
 return -EINVAL;
 }
 
-/* Verify if there is any device that doesn't support snapshots and is
-writable and check if the requested snapshot is available too. */
-bs = NULL;
-while ((bs = bdrv_next(bs))) {
-
-if (!bdrv_is_inserted(bs) || bdrv_is_read_only(bs)) {
-continue;
-}
-
-if (!bdrv_can_snapshot(bs)) {
-error_report("Device '%s' is writable but does not support 
snapshots.",
-   bdrv_get_device_name(bs));
-return -ENOTSUP;
-}
-
-ret = bdrv_snapshot_find(bs, , name);
-if (ret < 0) {
-error_report("Device '%s' does not have the requested snapshot 
'%s'",
-   bdrv_get_device_name(bs), name);
-return ret;
-}
-}
-
 /* Flush all IO requests so they don't interfere with the new state.  */
 bdrv_drain_all();
 
@@ -1475,8 +1464,8 @@ void hmp_delvm(Monitor *mon, const QDict *qdict)
 void hmp_info_snapshots(Monitor *mon, const QDict *qdict)
 {
 BlockDriverState *bs, *bs1;
-QEMUSnapshotInfo *sn_tab, *sn, s, *sn_info = 
-int nb_sns, i, ret, available;
+QEMUSnapshotInfo *sn_tab, *sn;
+int nb_sns, i;
 int total;
 int *available_snapshots;
 
@@ -1500,21 +1489,7 @@ void hmp_info_snapshots(Monitor *mon, const QDict *qdict)
 available_snapshots = g_new0(int, nb_sns);
 total = 0;
 for (i = 0; i < nb_sns; i++) {
-sn = _tab[i];
-available = 1;
-bs1 = NULL;
-
-while ((bs1 = bdrv_next(bs1))) {
-if (bdrv_can_snapshot(bs1) && bs1 != bs) {
-ret = bdrv_snapshot_find(bs1, sn_info, sn->id_str);
-if (ret < 0) {
-available = 0;
-break;
-}
-}
-}
-
-if (available) {
+if (bdrv_all_find_snapshot(sn_tab[i].id_str, ) == 0) {
 available_snapshots[total] = i;
 total++;
 }
-- 
2.5.0