δΊ 2013/11/19 19:20, Kevin Wolf ει:
Am 10.11.2013 um 23:03 hat Wenchao Xia geschrieben:
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.
Also fix a typo in comments of bdrv_snapshot_delete().
Signed-off-by: Wenchao Xia <xiaw...@linux.vnet.ibm.com>
---
block/qcow2-snapshot.c | 16 ++++++++++-
block/qcow2.h | 5 +++-
block/snapshot.c | 60 ++++++++++++++++++++++++++++++++++++++++++--
include/block/block_int.h | 4 ++-
include/block/snapshot.h | 7 ++++-
qemu-img.c | 8 ++++-
6 files changed, 90 insertions(+), 10 deletions(-)
diff --git a/block/qcow2-snapshot.c b/block/qcow2-snapshot.c
index 3529c68..aeb5efd 100644
--- a/block/qcow2-snapshot.c
+++ b/block/qcow2-snapshot.c
@@ -675,7 +675,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;
@@ -683,12 +686,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);
This is wrong, low-level functions shouldn't need the device name for
anything.
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];
I think we already discussed the same thing in the context of a
different series: The caller knows which device and which snapshot name
or ID he used. The only information he really needs is:
error_setg(errp, "Can't find snapshot");
If in the context of the caller's caller this isn't enough, the caller
can add this information. I doubt that it's even necessary in this case.
I remember that, will follow this rule.
@@ -699,6 +707,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 922e190..303eb26 100644
--- a/block/qcow2.h
+++ b/block/qcow2.h
@@ -488,7 +488,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..e51a7db 100644
--- a/block/snapshot.c
+++ b/block/snapshot.c
@@ -194,7 +194,7 @@ int bdrv_snapshot_goto(BlockDriverState *bs,
* If only @snapshot_id is specified, delete the first one with id
* @snapshot_id.
* If only @name is specified, delete the first one with name @name.
- * if none is specified, return -ENINVAL.
+ * if none is specified, return -EINVAL.
*
* Returns: 0 on success, -errno on failure. If @bs is not inserted, return
* -ENOMEDIUM. If @snapshot_id and @name are both NULL, return -EINVAL. If @bs
@@ -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 -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
s/one/a/
OK.
+ * @name, return -ENOENT. If @bs does not support parameter @snapshot_id or
+ * @name, return -EINVAL.
What do you mean by "bs does not support parameter"? Is this when you
specify a name, but the block driver doesn't use names, but only IDs?
likely, I mean rbd doesn't support ID. But rbd and snapshot doesn't
implement load_tmp, so will remove this comments.
If @errp != NULL, it will always be filled on
+ * failure.
+ */
The rest looks good.
Kevin