Patchwork [V3,1/7] snapshot: distinguish id and name in load_tmp

login
register
mail settings
Submitter Wayne Xia
Date Sept. 26, 2013, 12:16 a.m.
Message ID <1380154568-5339-2-git-send-email-xiawenc@linux.vnet.ibm.com>
Download mbox | patch
Permalink /patch/278048/
State New
Headers show

Comments

Wayne Xia - Sept. 26, 2013, 12:16 a.m.
Since later this function will be used so improve it. The only caller of it
now is qemu-img, and it is not impacted by introduce function
bdrv_snapshot_load_tmp_by_id_or_name() that call bdrv_snapshot_load_tmp()
twice to keep old search logic. bdrv_snapshot_load_tmp_by_id_or_name() return
int to let caller know the errno, and errno will be used later.

Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
---
 block/qcow2-snapshot.c    |   16 +++++++++++-
 block/qcow2.h             |    5 +++-
 block/snapshot.c          |   58 +++++++++++++++++++++++++++++++++++++++++++-
 include/block/block_int.h |    4 ++-
 include/block/snapshot.h  |    7 ++++-
 qemu-img.c                |    8 ++++-
 6 files changed, 89 insertions(+), 9 deletions(-)
Eric Blake - Oct. 1, 2013, 2:35 p.m.
On 09/25/2013 06:16 PM, Wenchao Xia wrote:
> Since later this function will be used so improve it. The only caller of it
> now is qemu-img, and it is not impacted by introduce function
> bdrv_snapshot_load_tmp_by_id_or_name() that call bdrv_snapshot_load_tmp()
> twice to keep old search logic. bdrv_snapshot_load_tmp_by_id_or_name() return
> int to let caller know the errno, and errno will be used later.
> 
> Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
> ---
>  block/qcow2-snapshot.c    |   16 +++++++++++-
>  block/qcow2.h             |    5 +++-
>  block/snapshot.c          |   58 +++++++++++++++++++++++++++++++++++++++++++-
>  include/block/block_int.h |    4 ++-
>  include/block/snapshot.h  |    7 ++++-
>  qemu-img.c                |    8 ++++-
>  6 files changed, 89 insertions(+), 9 deletions(-)
> 
> + *
> + * If both @snapshot_id and @name are specified, load the first one with
> + * id @snapshot_id and name @name.
> + * If only @snapshot_id is specified, load the first one with id
> + * @snapshot_id.
> + * If only @name is specified, load the first one with name @name.
> + * if none is specified, return -ENINVAL.

s/ENINVAL/EINVAL/

> + *
> + * Returns: 0 on success, -errno on fail. If @bs is not inserted, return
> + * -ENOMEDIUM. If @bs is not readonly, return -EINVAL. If @bs did not support
> + * internal snapshot, return -ENOTSUP. If qemu can't find one matching @id and
> + * @name, return -ENOENT. If @bs do not support parameter @snapshot_id or

s/do/does/
Wayne Xia - Oct. 10, 2013, 5:38 a.m.
于 2013/10/1 22:35, Eric Blake 写道:
> On 09/25/2013 06:16 PM, Wenchao Xia wrote:
>> Since later this function will be used so improve it. The only caller of it
>> now is qemu-img, and it is not impacted by introduce function
>> bdrv_snapshot_load_tmp_by_id_or_name() that call bdrv_snapshot_load_tmp()
>> twice to keep old search logic. bdrv_snapshot_load_tmp_by_id_or_name() return
>> int to let caller know the errno, and errno will be used later.
>>
>> Signed-off-by: Wenchao Xia<xiawenc@linux.vnet.ibm.com>
>> ---
>>   block/qcow2-snapshot.c    |   16 +++++++++++-
>>   block/qcow2.h             |    5 +++-
>>   block/snapshot.c          |   58 +++++++++++++++++++++++++++++++++++++++++++-
>>   include/block/block_int.h |    4 ++-
>>   include/block/snapshot.h  |    7 ++++-
>>   qemu-img.c                |    8 ++++-
>>   6 files changed, 89 insertions(+), 9 deletions(-)
>>
>> + *
>> + * If both @snapshot_id and @name are specified, load the first one with
>> + * id @snapshot_id and name @name.
>> + * If only @snapshot_id is specified, load the first one with id
>> + * @snapshot_id.
>> + * If only @name is specified, load the first one with name @name.
>> + * if none is specified, return -ENINVAL.
> s/ENINVAL/EINVAL/
>
>> + *
>> + * Returns: 0 on success, -errno on fail. If @bs is not inserted, return
>> + * -ENOMEDIUM. If @bs is not readonly, return -EINVAL. If @bs did not support
>> + * internal snapshot, return -ENOTSUP. If qemu can't find one matching @id and
>> + * @name, return -ENOENT. If @bs do not support parameter @snapshot_id or
> s/do/does/
>
will fix the grammar, thanks.

Patch

diff --git a/block/qcow2-snapshot.c b/block/qcow2-snapshot.c
index 5e8a779..4911ebe 100644
--- a/block/qcow2-snapshot.c
+++ b/block/qcow2-snapshot.c
@@ -669,7 +669,10 @@  int qcow2_snapshot_list(BlockDriverState *bs, QEMUSnapshotInfo **psn_tab)
     return s->nb_snapshots;
 }
 
-int qcow2_snapshot_load_tmp(BlockDriverState *bs, const char *snapshot_name)
+int qcow2_snapshot_load_tmp(BlockDriverState *bs,
+                            const char *snapshot_id,
+                            const char *name,
+                            Error **errp)
 {
     int i, snapshot_index;
     BDRVQcowState *s = bs->opaque;
@@ -677,12 +680,17 @@  int qcow2_snapshot_load_tmp(BlockDriverState *bs, const char *snapshot_name)
     uint64_t *new_l1_table;
     int new_l1_bytes;
     int ret;
+    const char *device = bdrv_get_device_name(bs);
 
     assert(bs->read_only);
 
     /* Search the snapshot */
-    snapshot_index = find_snapshot_by_id_or_name(bs, snapshot_name);
+    snapshot_index = find_snapshot_by_id_and_name(bs, snapshot_id, name);
     if (snapshot_index < 0) {
+        error_setg(errp,
+                   "Can't find a snapshot with ID '%s' and name '%s' "
+                   "on device '%s'",
+                   STR_OR_NULL(snapshot_id), STR_OR_NULL(name), device);
         return -ENOENT;
     }
     sn = &s->snapshots[snapshot_index];
@@ -693,6 +701,10 @@  int qcow2_snapshot_load_tmp(BlockDriverState *bs, const char *snapshot_name)
 
     ret = bdrv_pread(bs->file, sn->l1_table_offset, new_l1_table, new_l1_bytes);
     if (ret < 0) {
+        error_setg(errp,
+                   "Failed to read l1 table for snapshot with ID '%s' and name "
+                   "'%s' on device '%s'",
+                   sn->id_str, sn->name, device);
         g_free(new_l1_table);
         return ret;
     }
diff --git a/block/qcow2.h b/block/qcow2.h
index c90e5d6..12cfeaf 100644
--- a/block/qcow2.h
+++ b/block/qcow2.h
@@ -472,7 +472,10 @@  int qcow2_snapshot_delete(BlockDriverState *bs,
                           const char *name,
                           Error **errp);
 int qcow2_snapshot_list(BlockDriverState *bs, QEMUSnapshotInfo **psn_tab);
-int qcow2_snapshot_load_tmp(BlockDriverState *bs, const char *snapshot_name);
+int qcow2_snapshot_load_tmp(BlockDriverState *bs,
+                            const char *snapshot_id,
+                            const char *name,
+                            Error **errp);
 
 void qcow2_free_snapshots(BlockDriverState *bs);
 int qcow2_read_snapshots(BlockDriverState *bs);
diff --git a/block/snapshot.c b/block/snapshot.c
index a05c0c0..2ae3099 100644
--- a/block/snapshot.c
+++ b/block/snapshot.c
@@ -265,18 +265,72 @@  int bdrv_snapshot_list(BlockDriverState *bs,
     return -ENOTSUP;
 }
 
+/**
+ * Temporarily load an internal snapshot by @snapshot_id and @name.
+ * @bs: block device used in the operation
+ * @snapshot_id: unique snapshot ID, or NULL
+ * @name: snapshot name, or NULL
+ * @errp: location to store error
+ *
+ * If both @snapshot_id and @name are specified, load the first one with
+ * id @snapshot_id and name @name.
+ * If only @snapshot_id is specified, load the first one with id
+ * @snapshot_id.
+ * If only @name is specified, load the first one with name @name.
+ * if none is specified, return -ENINVAL.
+ *
+ * Returns: 0 on success, -errno on fail. If @bs is not inserted, return
+ * -ENOMEDIUM. If @bs is not readonly, return -EINVAL. If @bs did not support
+ * internal snapshot, return -ENOTSUP. If qemu can't find one matching @id and
+ * @name, return -ENOENT. If @bs do not support parameter @snapshot_id or
+ * @name, return -EINVAL. If @errp != NULL, it will always be filled on
+ * failure.
+ */
 int bdrv_snapshot_load_tmp(BlockDriverState *bs,
-        const char *snapshot_name)
+                           const char *snapshot_id,
+                           const char *name,
+                           Error **errp)
 {
     BlockDriver *drv = bs->drv;
+    const char *device = bdrv_get_device_name(bs);
     if (!drv) {
+        error_set(errp, QERR_DEVICE_HAS_NO_MEDIUM, device);
         return -ENOMEDIUM;
     }
+    if (!snapshot_id && !name) {
+        error_setg(errp, "snapshot_id and name are both NULL");
+        return -EINVAL;
+    }
     if (!bs->read_only) {
+        error_setg(errp, "Device '%s' is not readonly", device);
         return -EINVAL;
     }
     if (drv->bdrv_snapshot_load_tmp) {
-        return drv->bdrv_snapshot_load_tmp(bs, snapshot_name);
+        return drv->bdrv_snapshot_load_tmp(bs, snapshot_id, name, errp);
     }
+    error_set(errp, QERR_BLOCK_FORMAT_FEATURE_NOT_SUPPORTED,
+              drv->format_name, device,
+              "temporarily load internal snapshot");
     return -ENOTSUP;
 }
+
+int bdrv_snapshot_load_tmp_by_id_or_name(BlockDriverState *bs,
+                                         const char *id_or_name,
+                                         Error **errp)
+{
+    int ret;
+    Error *local_err = NULL;
+
+    ret = bdrv_snapshot_load_tmp(bs, id_or_name, NULL, &local_err);
+    if (ret == -ENOENT || ret == -EINVAL) {
+        error_free(local_err);
+        local_err = NULL;
+        ret = bdrv_snapshot_load_tmp(bs, NULL, id_or_name, &local_err);
+    }
+
+    if (error_is_set(&local_err)) {
+        error_propagate(errp, local_err);
+    }
+
+    return ret;
+}
diff --git a/include/block/block_int.h b/include/block/block_int.h
index 3eeb6fe..554cadd 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -160,7 +160,9 @@  struct BlockDriver {
     int (*bdrv_snapshot_list)(BlockDriverState *bs,
                               QEMUSnapshotInfo **psn_info);
     int (*bdrv_snapshot_load_tmp)(BlockDriverState *bs,
-                                  const char *snapshot_name);
+                                  const char *snapshot_id,
+                                  const char *name,
+                                  Error **errp);
     int (*bdrv_get_info)(BlockDriverState *bs, BlockDriverInfo *bdi);
 
     int (*bdrv_save_vmstate)(BlockDriverState *bs, QEMUIOVector *qiov,
diff --git a/include/block/snapshot.h b/include/block/snapshot.h
index 012bf22..d05bea7 100644
--- a/include/block/snapshot.h
+++ b/include/block/snapshot.h
@@ -61,5 +61,10 @@  void bdrv_snapshot_delete_by_id_or_name(BlockDriverState *bs,
 int bdrv_snapshot_list(BlockDriverState *bs,
                        QEMUSnapshotInfo **psn_info);
 int bdrv_snapshot_load_tmp(BlockDriverState *bs,
-                           const char *snapshot_name);
+                           const char *snapshot_id,
+                           const char *name,
+                           Error **errp);
+int bdrv_snapshot_load_tmp_by_id_or_name(BlockDriverState *bs,
+                                         const char *id_or_name,
+                                         Error **errp);
 #endif
diff --git a/qemu-img.c b/qemu-img.c
index 926f0a0..6df58ed 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -1260,8 +1260,12 @@  static int img_convert(int argc, char **argv)
             ret = -1;
             goto out;
         }
-        if (bdrv_snapshot_load_tmp(bs[0], snapshot_name) < 0) {
-            error_report("Failed to load snapshot");
+
+        bdrv_snapshot_load_tmp_by_id_or_name(bs[0], snapshot_name, &local_err);
+        if (error_is_set(&local_err)) {
+            error_report("Failed to load snapshot: %s",
+                         error_get_pretty(local_err));
+            error_free(local_err);
             ret = -1;
             goto out;
         }