| Message ID | 1384121021-24815-2-git-send-email-xiawenc@linux.vnet.ibm.com |
|---|---|
| State | New |
| Headers | show |
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 <xiawenc@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. > @@ -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/ > + * @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? > If @errp != NULL, it will always be filled on > + * failure. > + */ The rest looks good. Kevin
δΊ 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 <xiawenc@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 >
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); 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]; @@ -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 + * @name, return -ENOENT. If @bs does 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 1666066..8bbfb09 100644 --- a/include/block/block_int.h +++ b/include/block/block_int.h @@ -175,7 +175,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); ImageInfoSpecific *(*bdrv_get_specific_info)(BlockDriverState *bs); 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 bf3fb4f..fe28119 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; }
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 <xiawenc@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(-)