Patchwork [V8,08/20] block: add filter for vm snapshot in bdrv_query_snapshot_info_list()

login
register
mail settings
Submitter Wayne Xia
Date March 7, 2013, 6:07 a.m.
Message ID <1362636445-7188-9-git-send-email-xiawenc@linux.vnet.ibm.com>
Download mbox | patch
Permalink /patch/225732/
State New
Headers show

Comments

Wayne Xia - March 7, 2013, 6:07 a.m.
This patch adds a parameter to tell whether return valid snapshots
for whole VM only.

Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
---
 block/qapi.c         |   39 +++++++++++++++++++++++++++++++++++++--
 include/block/qapi.h |    1 +
 qemu-img.c           |    3 ++-
 3 files changed, 40 insertions(+), 3 deletions(-)
Eric Blake - March 8, 2013, 10:55 p.m.
On 03/06/2013 11:07 PM, Wenchao Xia wrote:
>   This patch adds a parameter to tell whether return valid snapshots
> for whole VM only.
> 
> Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
> ---
>  block/qapi.c         |   39 +++++++++++++++++++++++++++++++++++++--
>  include/block/qapi.h |    1 +
>  qemu-img.c           |    3 ++-
>  3 files changed, 40 insertions(+), 3 deletions(-)
> 

> + * @sn: snapshot info need to be checked.

s/need //

> + * return 0 if valid, it means load_vmstate() for it could succeed.

Reads awkwardly; how about:

it means load_vmstate() could load that snapshot.

> + */
> +static int snapshot_filter_vm(const QEMUSnapshotInfo *sn, BlockDriverState *bs)
> +{
> +    BlockDriverState *bs1 = NULL;
> +    QEMUSnapshotInfo s, *sn_info = &s;
> +    int ret = 0;
> +
> +    /* Check logic is connected with load_vmstate():
> +       Only check the devices that can snapshot, other devices that can't
> +       take snapshot, for example, readonly ones, will be ignored in
> +       load_vmstate(). */
> +    while ((bs1 = bdrv_next(bs1))) {
> +        if (bdrv_can_snapshot(bs1) && bs1 != bs) {
> +            ret = bdrv_snapshot_find(bs1, sn_info, sn->id_str, NULL);
> +            if (ret < 0) {
> +                ret = -1;

This function only returns 0 or -1...

> +/* return 0 on success, and @p_list will be set only on success. If
> +   @vm_snapshot is true, only the snapshot valid for whole vm will be got. */

grammar

If @vm_snapshot is true, limit the results to snapshots valid for the
whole vm.

> -
> +        if (vm_snapshot && snapshot_filter_vm(&sn_tab[i], bs)) {

...yet you are only ever calling it in a boolean context.  Would it be
smarter to have the function return bool (true for valid vm snapshot,
false if the image snapshot is not usable as a vm snapshot)?

Also, it might be nicer to name it snapshot_valid_for_vm, as in:

if (vm_snapshot && !snapshot_valid_for_vm(...)) {
    continue;
}
Wayne Xia - March 9, 2013, 4:24 a.m.
于 2013-3-9 6:55, Eric Blake 写道:
> On 03/06/2013 11:07 PM, Wenchao Xia wrote:
>>    This patch adds a parameter to tell whether return valid snapshots
>> for whole VM only.
>>
>> Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
>> ---
>>   block/qapi.c         |   39 +++++++++++++++++++++++++++++++++++++--
>>   include/block/qapi.h |    1 +
>>   qemu-img.c           |    3 ++-
>>   3 files changed, 40 insertions(+), 3 deletions(-)
>>
>
>> + * @sn: snapshot info need to be checked.
>
> s/need //
>
   OK.

>> + * return 0 if valid, it means load_vmstate() for it could succeed.
>
> Reads awkwardly; how about:
>
> it means load_vmstate() could load that snapshot.
>
   I forgot it may not have vmstate() but only only block snapshot,
It should be:
    return 0 if valid, it means the snapshot is consistent for the VM.

>> + */
>> +static int snapshot_filter_vm(const QEMUSnapshotInfo *sn, BlockDriverState *bs)
>> +{
>> +    BlockDriverState *bs1 = NULL;
>> +    QEMUSnapshotInfo s, *sn_info = &s;
>> +    int ret = 0;
>> +
>> +    /* Check logic is connected with load_vmstate():
>> +       Only check the devices that can snapshot, other devices that can't
>> +       take snapshot, for example, readonly ones, will be ignored in
>> +       load_vmstate(). */
>> +    while ((bs1 = bdrv_next(bs1))) {
>> +        if (bdrv_can_snapshot(bs1) && bs1 != bs) {
>> +            ret = bdrv_snapshot_find(bs1, sn_info, sn->id_str, NULL);
>> +            if (ret < 0) {
>> +                ret = -1;
>
> This function only returns 0 or -1...
>
   OK, it should be bool.

>> +/* return 0 on success, and @p_list will be set only on success. If
>> +   @vm_snapshot is true, only the snapshot valid for whole vm will be got. */
>
> grammar
>
> If @vm_snapshot is true, limit the results to snapshots valid for the
> whole vm.
>
   Looks better, thanks.

>> -
>> +        if (vm_snapshot && snapshot_filter_vm(&sn_tab[i], bs)) {
>
> ...yet you are only ever calling it in a boolean context.  Would it be
> smarter to have the function return bool (true for valid vm snapshot,
> false if the image snapshot is not usable as a vm snapshot)?
>
> Also, it might be nicer to name it snapshot_valid_for_vm, as in:
>
> if (vm_snapshot && !snapshot_valid_for_vm(...)) {
>      continue;
> }
   OK.

>

Patch

diff --git a/block/qapi.c b/block/qapi.c
index b503cfa..a90b4c7 100644
--- a/block/qapi.c
+++ b/block/qapi.c
@@ -12,11 +12,44 @@ 
  */
 
 #include "block/qapi.h"
+#include "block/snapshot.h"
 #include "block/block_int.h"
 
-/* return 0 on success, and @p_list will be set only on success. */
+/*
+ * check whether the snapshot is valid for whole vm.
+ *
+ * @sn: snapshot info need to be checked.
+ * @bs: where @sn was found.
+ *
+ * return 0 if valid, it means load_vmstate() for it could succeed.
+ */
+static int snapshot_filter_vm(const QEMUSnapshotInfo *sn, BlockDriverState *bs)
+{
+    BlockDriverState *bs1 = NULL;
+    QEMUSnapshotInfo s, *sn_info = &s;
+    int ret = 0;
+
+    /* Check logic is connected with load_vmstate():
+       Only check the devices that can snapshot, other devices that can't
+       take snapshot, for example, readonly ones, will be ignored in
+       load_vmstate(). */
+    while ((bs1 = bdrv_next(bs1))) {
+        if (bdrv_can_snapshot(bs1) && bs1 != bs) {
+            ret = bdrv_snapshot_find(bs1, sn_info, sn->id_str, NULL);
+            if (ret < 0) {
+                ret = -1;
+                break;
+            }
+        }
+    }
+    return ret;
+}
+
+/* return 0 on success, and @p_list will be set only on success. If
+   @vm_snapshot is true, only the snapshot valid for whole vm will be got. */
 int bdrv_query_snapshot_info_list(BlockDriverState *bs,
                                   SnapshotInfoList **p_list,
+                                  bool vm_snapshot,
                                   Error **errp)
 {
     int i, sn_count;
@@ -45,7 +78,9 @@  int bdrv_query_snapshot_info_list(BlockDriverState *bs,
     }
 
     for (i = 0; i < sn_count; i++) {
-
+        if (vm_snapshot && snapshot_filter_vm(&sn_tab[i], bs)) {
+            continue;
+        }
         info = g_new0(SnapshotInfo, 1);
         info->id            = g_strdup(sn_tab[i].id_str);
         info->name          = g_strdup(sn_tab[i].name);
diff --git a/include/block/qapi.h b/include/block/qapi.h
index 030964b..4842c12 100644
--- a/include/block/qapi.h
+++ b/include/block/qapi.h
@@ -6,6 +6,7 @@ 
 
 int bdrv_query_snapshot_info_list(BlockDriverState *bs,
                                   SnapshotInfoList **p_list,
+                                  bool vm_snapshot,
                                   Error **errp);
 void bdrv_collect_image_info(BlockDriverState *bs,
                              ImageInfo *info,
diff --git a/qemu-img.c b/qemu-img.c
index 32a72f5..7786953 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -1735,7 +1735,8 @@  static ImageInfoList *collect_image_info_list(const char *filename,
 
         info = g_new0(ImageInfo, 1);
         bdrv_collect_image_info(bs, info, filename, fmt);
-        if (!bdrv_query_snapshot_info_list(bs, &info->snapshots, NULL)) {
+        if (!bdrv_query_snapshot_info_list(bs, &info->snapshots,
+                                           false, NULL)) {
             info->has_snapshots = true;
         }