Patchwork [V12,07/18] block: change VM snapshot checking logic

login
register
mail settings
Submitter Wayne Xia
Date April 13, 2013, 8:56 a.m.
Message ID <1365843407-16504-8-git-send-email-xiawenc@linux.vnet.ibm.com>
Download mbox | patch
Permalink /patch/236313/
State New
Headers show

Comments

Wayne Xia - April 13, 2013, 8:56 a.m.
Original logic is different with load_vmstate(), this patch change it
to be exactly the same with load_vmstate(), so any VM snapshot shown in
qmp/hmp should succeed in load_vmstate().
  Note that, runtime snapshot info maybe different with what is got
in "qemu-img info" as static snapshot info, and this patch clearly
tips it.

Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
---
 block/qapi.c |   13 ++++++++++++-
 1 files changed, 12 insertions(+), 1 deletions(-)
Eric Blake - April 17, 2013, 9:58 p.m.
On 04/13/2013 02:56 AM, Wenchao Xia wrote:
>   Original logic is different with load_vmstate(), this patch change it

s/with/from/
s/change/changes/

> to be exactly the same with load_vmstate(), so any VM snapshot shown in

s/with/as/

> qmp/hmp should succeed in load_vmstate().

Looking through git logs, most people use blank lines and start at
column 1, rather than your idiom of 2-space indent and no blank line,
when starting a new paragraph.

>   Note that, runtime snapshot info maybe different with what is got

s/that,/that consistent/
s/maybe different with/may be a subset of/
s/got/listed/

> in "qemu-img info" as static snapshot info, and this patch clearly
> tips it.

s/tips/identifies/

> 
> Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
> ---
>  block/qapi.c |   13 ++++++++++++-
>  1 files changed, 12 insertions(+), 1 deletions(-)

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

> 
> diff --git a/block/qapi.c b/block/qapi.c
> index 97c5cd4..49c0eb0 100644
> --- a/block/qapi.c
> +++ b/block/qapi.c
> @@ -46,7 +46,18 @@ static bool snapshot_valid_for_vm(const QEMUSnapshotInfo *sn,
>         take snapshot, for example, readonly ones, will be ignored in
>         load_vmstate(). */
>      while ((bs1 = bdrv_next(bs1))) {
> -        if (bs1 != bs && bdrv_can_snapshot(bs1)) {
> +

A blank line after a while(){ is not common, but as it is only
whitespace, and checkpatch didn't complain, it doesn't hold up my review.

> +        if (!bdrv_is_inserted(bs1) || bdrv_is_read_only(bs1)) {
> +            continue;
> +        }
> +
> +        if (!bdrv_can_snapshot(bs1)) {
> +            /* Device is writable but does not support snapshots, will be
> +               rejected by load_vmstate(). */
> +            return false;
> +        }
> +
> +        if (bs1 != bs) {
>              ret = bdrv_snapshot_find(bs1, sn_info, sn->id_str, NULL);
>              if (ret < 0) {
>                  return false;
>

Patch

diff --git a/block/qapi.c b/block/qapi.c
index 97c5cd4..49c0eb0 100644
--- a/block/qapi.c
+++ b/block/qapi.c
@@ -46,7 +46,18 @@  static bool snapshot_valid_for_vm(const QEMUSnapshotInfo *sn,
        take snapshot, for example, readonly ones, will be ignored in
        load_vmstate(). */
     while ((bs1 = bdrv_next(bs1))) {
-        if (bs1 != bs && bdrv_can_snapshot(bs1)) {
+
+        if (!bdrv_is_inserted(bs1) || bdrv_is_read_only(bs1)) {
+            continue;
+        }
+
+        if (!bdrv_can_snapshot(bs1)) {
+            /* Device is writable but does not support snapshots, will be
+               rejected by load_vmstate(). */
+            return false;
+        }
+
+        if (bs1 != bs) {
             ret = bdrv_snapshot_find(bs1, sn_info, sn->id_str, NULL);
             if (ret < 0) {
                 return false;