Message ID | 1373521624-4380-3-git-send-email-xiawenc@linux.vnet.ibm.com |
---|---|
State | New |
Headers | show |
On Thu, Jul 11, 2013 at 01:46:58PM +0800, Wenchao Xia wrote: > diff --git a/include/qemu-common.h b/include/qemu-common.h > index f439738..06c777f 100644 > --- a/include/qemu-common.h > +++ b/include/qemu-common.h > @@ -191,6 +191,9 @@ int64_t strtosz_suffix(const char *nptr, char **end, const char default_suffix); > int64_t strtosz_suffix_unit(const char *nptr, char **end, > const char default_suffix, int64_t unit); > > +/* used to print char* safely */ > +#define STR_PRINT_CHAR(str) ((str) ? (str) : "null") When I saw the name I thought it would filter out non-printable characters. Maybe STR_OR_NULL() is a better name? BTW the evil gcc shortcut is pretty quick to type: str ?: "null". Besides this I'm pretty happy with this version.
Am 11.07.2013 um 07:46 hat Wenchao Xia geschrieben: > Snapshot creation actually already distinguish id and name since it take > a structured parameter *sn, but delete can't. Later an accurate delete > is needed in qmp_transaction abort and blockdev-snapshot-delete-sync, > so change its prototype. Also *errp is added to tip error, but return > value is kepted to let caller check what kind of error happens. Existing > caller for it are savevm, delvm and qemu-img, they are not impacted by > check the return value and do the operations again. > > Before this patch: > For qcow2, it search id first then name to find the one to delete. > For rbd, it search name. > For sheepdog, it does nothing. > > After this patch: > For qcow2, logic is the same by call it twice in caller. > For rbd, it always fails in delete with id, but still search for name > in second try, no change for user. > > Some code for *errp is based on Pavel's patch. > > Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com> > Signed-off-by: Pavel Hrdina <phrdina@redhat.com> > --- > block/qcow2-snapshot.c | 66 ++++++++++++++++++++++++++++++++++----------- > block/qcow2.h | 5 +++- > block/rbd.c | 23 +++++++++++++++- > block/sheepdog.c | 5 +++- > block/snapshot.c | 36 ++++++++++++++++++++++-- > include/block/block_int.h | 5 +++- > include/block/snapshot.h | 5 +++- > include/qemu-common.h | 3 ++ > qemu-img.c | 5 +++- > savevm.c | 10 +++++- > 10 files changed, 136 insertions(+), 27 deletions(-) > @@ -531,15 +547,23 @@ fail: > return ret; > } > > -int qcow2_snapshot_delete(BlockDriverState *bs, const char *snapshot_id) > +int qcow2_snapshot_delete(BlockDriverState *bs, > + const char *snapshot_id, > + const char *name, > + Error **errp) > { > BDRVQcowState *s = bs->opaque; > QCowSnapshot sn; > int snapshot_index, ret; > + const char *device = bdrv_get_device_name(bs); > > /* Search the snapshot */ > - snapshot_index = find_snapshot_by_id_or_name(bs, snapshot_id); > + 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_PRINT_CHAR(snapshot_id), STR_PRINT_CHAR(name), device); At least the "on device '%s'" part should be removed from the error messages. It is something the caller can add if there is any ambiguity. > --- a/qemu-img.c > +++ b/qemu-img.c > @@ -1883,7 +1883,10 @@ static int img_snapshot(int argc, char **argv) > break; > > case SNAPSHOT_DELETE: > - ret = bdrv_snapshot_delete(bs, snapshot_name); > + ret = bdrv_snapshot_delete(bs, snapshot_name, NULL, NULL); > + if (ret == -ENOENT || ret == -EINVAL) { > + ret = bdrv_snapshot_delete(bs, NULL, snapshot_name, NULL); > + } > if (ret) { > error_report("Could not delete snapshot '%s': %d (%s)", > snapshot_name, ret, strerror(-ret)); Why don't you use the new error messages? > diff --git a/savevm.c b/savevm.c > index e0491e7..56afebb 100644 > --- a/savevm.c > +++ b/savevm.c > @@ -2332,7 +2332,10 @@ static int del_existing_snapshots(Monitor *mon, const char *name) > if (bdrv_can_snapshot(bs) && > bdrv_snapshot_find(bs, snapshot, name) >= 0) > { > - ret = bdrv_snapshot_delete(bs, name); > + ret = bdrv_snapshot_delete(bs, name, NULL, NULL); > + if (ret == -ENOENT || ret == -EINVAL) { > + ret = bdrv_snapshot_delete(bs, NULL, name, NULL); > + } > if (ret < 0) { > monitor_printf(mon, > "Error while deleting snapshot on '%s'\n", > @@ -2562,7 +2565,10 @@ void do_delvm(Monitor *mon, const QDict *qdict) > bs1 = NULL; > while ((bs1 = bdrv_next(bs1))) { > if (bdrv_can_snapshot(bs1)) { > - ret = bdrv_snapshot_delete(bs1, name); > + ret = bdrv_snapshot_delete(bs1, name, NULL, NULL); > + if (ret == -ENOENT || ret == -EINVAL) { > + ret = bdrv_snapshot_delete(bs, NULL, name, NULL); > + } > if (ret < 0) { > if (ret == -ENOTSUP) > monitor_printf(mon, Same thing here. Kevin
于 2013-7-18 19:07, Stefan Hajnoczi 写道: > On Thu, Jul 11, 2013 at 01:46:58PM +0800, Wenchao Xia wrote: >> diff --git a/include/qemu-common.h b/include/qemu-common.h >> index f439738..06c777f 100644 >> --- a/include/qemu-common.h >> +++ b/include/qemu-common.h >> @@ -191,6 +191,9 @@ int64_t strtosz_suffix(const char *nptr, char **end, const char default_suffix); >> int64_t strtosz_suffix_unit(const char *nptr, char **end, >> const char default_suffix, int64_t unit); >> >> +/* used to print char* safely */ >> +#define STR_PRINT_CHAR(str) ((str) ? (str) : "null") > > When I saw the name I thought it would filter out non-printable > characters. Maybe STR_OR_NULL() is a better name? > I'll use STR_OR_NULL(), looks better. > BTW the evil gcc shortcut is pretty quick to type: str ?: "null". > > Besides this I'm pretty happy with this version. >
于 2013-7-18 19:48, Kevin Wolf 写道: > Am 11.07.2013 um 07:46 hat Wenchao Xia geschrieben: >> Snapshot creation actually already distinguish id and name since it take >> a structured parameter *sn, but delete can't. Later an accurate delete >> is needed in qmp_transaction abort and blockdev-snapshot-delete-sync, >> so change its prototype. Also *errp is added to tip error, but return >> value is kepted to let caller check what kind of error happens. Existing >> caller for it are savevm, delvm and qemu-img, they are not impacted by >> check the return value and do the operations again. >> >> Before this patch: >> For qcow2, it search id first then name to find the one to delete. >> For rbd, it search name. >> For sheepdog, it does nothing. >> >> After this patch: >> For qcow2, logic is the same by call it twice in caller. >> For rbd, it always fails in delete with id, but still search for name >> in second try, no change for user. >> >> Some code for *errp is based on Pavel's patch. >> >> Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com> >> Signed-off-by: Pavel Hrdina <phrdina@redhat.com> >> --- >> block/qcow2-snapshot.c | 66 ++++++++++++++++++++++++++++++++++----------- >> block/qcow2.h | 5 +++- >> block/rbd.c | 23 +++++++++++++++- >> block/sheepdog.c | 5 +++- >> block/snapshot.c | 36 ++++++++++++++++++++++-- >> include/block/block_int.h | 5 +++- >> include/block/snapshot.h | 5 +++- >> include/qemu-common.h | 3 ++ >> qemu-img.c | 5 +++- >> savevm.c | 10 +++++- >> 10 files changed, 136 insertions(+), 27 deletions(-) > >> @@ -531,15 +547,23 @@ fail: >> return ret; >> } >> >> -int qcow2_snapshot_delete(BlockDriverState *bs, const char *snapshot_id) >> +int qcow2_snapshot_delete(BlockDriverState *bs, >> + const char *snapshot_id, >> + const char *name, >> + Error **errp) >> { >> BDRVQcowState *s = bs->opaque; >> QCowSnapshot sn; >> int snapshot_index, ret; >> + const char *device = bdrv_get_device_name(bs); >> >> /* Search the snapshot */ >> - snapshot_index = find_snapshot_by_id_or_name(bs, snapshot_id); >> + 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_PRINT_CHAR(snapshot_id), STR_PRINT_CHAR(name), device); > > At least the "on device '%s'" part should be removed from the error > messages. It is something the caller can add if there is any ambiguity. > will remove. >> --- a/qemu-img.c >> +++ b/qemu-img.c >> @@ -1883,7 +1883,10 @@ static int img_snapshot(int argc, char **argv) >> break; >> >> case SNAPSHOT_DELETE: >> - ret = bdrv_snapshot_delete(bs, snapshot_name); >> + ret = bdrv_snapshot_delete(bs, snapshot_name, NULL, NULL); >> + if (ret == -ENOENT || ret == -EINVAL) { >> + ret = bdrv_snapshot_delete(bs, NULL, snapshot_name, NULL); >> + } >> if (ret) { >> error_report("Could not delete snapshot '%s': %d (%s)", >> snapshot_name, ret, strerror(-ret)); > > Why don't you use the new error messages? > >> diff --git a/savevm.c b/savevm.c >> index e0491e7..56afebb 100644 >> --- a/savevm.c >> +++ b/savevm.c >> @@ -2332,7 +2332,10 @@ static int del_existing_snapshots(Monitor *mon, const char *name) >> if (bdrv_can_snapshot(bs) && >> bdrv_snapshot_find(bs, snapshot, name) >= 0) >> { >> - ret = bdrv_snapshot_delete(bs, name); >> + ret = bdrv_snapshot_delete(bs, name, NULL, NULL); >> + if (ret == -ENOENT || ret == -EINVAL) { >> + ret = bdrv_snapshot_delete(bs, NULL, name, NULL); >> + } >> if (ret < 0) { >> monitor_printf(mon, >> "Error while deleting snapshot on '%s'\n", >> @@ -2562,7 +2565,10 @@ void do_delvm(Monitor *mon, const QDict *qdict) >> bs1 = NULL; >> while ((bs1 = bdrv_next(bs1))) { >> if (bdrv_can_snapshot(bs1)) { >> - ret = bdrv_snapshot_delete(bs1, name); >> + ret = bdrv_snapshot_delete(bs1, name, NULL, NULL); >> + if (ret == -ENOENT || ret == -EINVAL) { >> + ret = bdrv_snapshot_delete(bs, NULL, name, NULL); >> + } >> if (ret < 0) { >> if (ret == -ENOTSUP) >> monitor_printf(mon, > > Same thing here. > > Kevin >
diff --git a/block/qcow2-snapshot.c b/block/qcow2-snapshot.c index 0caac90..47db6ad 100644 --- a/block/qcow2-snapshot.c +++ b/block/qcow2-snapshot.c @@ -288,31 +288,47 @@ static void find_new_snapshot_id(BlockDriverState *bs, snprintf(id_str, id_str_size, "%d", id_max + 1); } -static int find_snapshot_by_id(BlockDriverState *bs, const char *id_str) +static int find_snapshot_by_id_and_name(BlockDriverState *bs, + const char *id, + const char *name) { BDRVQcowState *s = bs->opaque; int i; - for(i = 0; i < s->nb_snapshots; i++) { - if (!strcmp(s->snapshots[i].id_str, id_str)) - return i; + if (id && name) { + for (i = 0; i < s->nb_snapshots; i++) { + if (!strcmp(s->snapshots[i].id_str, id) && + !strcmp(s->snapshots[i].name, name)) { + return i; + } + } + } else if (id) { + for (i = 0; i < s->nb_snapshots; i++) { + if (!strcmp(s->snapshots[i].id_str, id)) { + return i; + } + } + } else if (name) { + for (i = 0; i < s->nb_snapshots; i++) { + if (!strcmp(s->snapshots[i].name, name)) { + return i; + } + } } + return -1; } -static int find_snapshot_by_id_or_name(BlockDriverState *bs, const char *name) +static int find_snapshot_by_id_or_name(BlockDriverState *bs, + const char *id_or_name) { - BDRVQcowState *s = bs->opaque; - int i, ret; + int ret; - ret = find_snapshot_by_id(bs, name); - if (ret >= 0) + ret = find_snapshot_by_id_and_name(bs, id_or_name, NULL); + if (ret >= 0) { return ret; - for(i = 0; i < s->nb_snapshots; i++) { - if (!strcmp(s->snapshots[i].name, name)) - return i; } - return -1; + return find_snapshot_by_id_and_name(bs, NULL, id_or_name); } /* if no id is provided, a new one is constructed */ @@ -334,7 +350,7 @@ int qcow2_snapshot_create(BlockDriverState *bs, QEMUSnapshotInfo *sn_info) } /* Check that the ID is unique */ - if (find_snapshot_by_id(bs, sn_info->id_str) >= 0) { + if (find_snapshot_by_id_and_name(bs, sn_info->id_str, NULL) >= 0) { return -EEXIST; } @@ -531,15 +547,23 @@ fail: return ret; } -int qcow2_snapshot_delete(BlockDriverState *bs, const char *snapshot_id) +int qcow2_snapshot_delete(BlockDriverState *bs, + const char *snapshot_id, + const char *name, + Error **errp) { BDRVQcowState *s = bs->opaque; QCowSnapshot sn; int snapshot_index, ret; + const char *device = bdrv_get_device_name(bs); /* Search the snapshot */ - snapshot_index = find_snapshot_by_id_or_name(bs, snapshot_id); + 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_PRINT_CHAR(snapshot_id), STR_PRINT_CHAR(name), device); return -ENOENT; } sn = s->snapshots[snapshot_index]; @@ -551,6 +575,10 @@ int qcow2_snapshot_delete(BlockDriverState *bs, const char *snapshot_id) s->nb_snapshots--; ret = qcow2_write_snapshots(bs); if (ret < 0) { + error_setg(errp, + "Failed to remove snapshot with ID '%s' and name '%s' from " + "the snapshot list on device '%s'", + STR_PRINT_CHAR(snapshot_id), STR_PRINT_CHAR(name), device); return ret; } @@ -568,6 +596,9 @@ int qcow2_snapshot_delete(BlockDriverState *bs, const char *snapshot_id) ret = qcow2_update_snapshot_refcount(bs, sn.l1_table_offset, sn.l1_size, -1); if (ret < 0) { + error_setg(errp, + "Failed to free the cluster and L1 table on device '%s'", + device); return ret; } qcow2_free_clusters(bs, sn.l1_table_offset, sn.l1_size * sizeof(uint64_t), @@ -576,6 +607,9 @@ int qcow2_snapshot_delete(BlockDriverState *bs, const char *snapshot_id) /* must update the copied flag on the current cluster offsets */ ret = qcow2_update_snapshot_refcount(bs, s->l1_table_offset, s->l1_size, 0); if (ret < 0) { + error_setg(errp, + "Failed to update snapshot status in disk on device '%s'", + device); return ret; } diff --git a/block/qcow2.h b/block/qcow2.h index 3b2d5cd..2e6c471 100644 --- a/block/qcow2.h +++ b/block/qcow2.h @@ -411,7 +411,10 @@ int qcow2_zero_clusters(BlockDriverState *bs, uint64_t offset, int nb_sectors); /* qcow2-snapshot.c functions */ int qcow2_snapshot_create(BlockDriverState *bs, QEMUSnapshotInfo *sn_info); int qcow2_snapshot_goto(BlockDriverState *bs, const char *snapshot_id); -int qcow2_snapshot_delete(BlockDriverState *bs, const char *snapshot_id); +int qcow2_snapshot_delete(BlockDriverState *bs, + const char *snapshot_id, + 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); diff --git a/block/rbd.c b/block/rbd.c index cb71751..688b571 100644 --- a/block/rbd.c +++ b/block/rbd.c @@ -903,12 +903,33 @@ static int qemu_rbd_snap_create(BlockDriverState *bs, } static int qemu_rbd_snap_remove(BlockDriverState *bs, - const char *snapshot_name) + const char *snapshot_id, + const char *snapshot_name, + Error **errp) { BDRVRBDState *s = bs->opaque; int r; + const char *device = bdrv_get_device_name(bs); + + if (snapshot_id && snapshot_id[0] != '\0') { + error_setg(errp, + "rbd do not support snapshot id on device '%s'", device); + return -EINVAL; + } + /* then snapshot_id == NULL or it contain an empty line, ignore it */ + + if (!snapshot_name) { + error_setg(errp, + "rbd need a valid snapshot name on device '%s'", device); + return -EINVAL; + } r = rbd_snap_remove(s->image, snapshot_name); + if (r < 0) { + error_setg_errno(errp, -r, + "Failed to remove snapshot '%s' on device '%s'", + snapshot_name, device); + } return r; } diff --git a/block/sheepdog.c b/block/sheepdog.c index b397b5b..7fdae71 100644 --- a/block/sheepdog.c +++ b/block/sheepdog.c @@ -2089,7 +2089,10 @@ out: return ret; } -static int sd_snapshot_delete(BlockDriverState *bs, const char *snapshot_id) +static int sd_snapshot_delete(BlockDriverState *bs, + const char *snapshot_id, + const char *name, + Error **errp) { /* FIXME: Delete specified snapshot id. */ return 0; diff --git a/block/snapshot.c b/block/snapshot.c index 481a3ee..cf2f4ca 100644 --- a/block/snapshot.c +++ b/block/snapshot.c @@ -182,18 +182,48 @@ int bdrv_snapshot_goto(BlockDriverState *bs, return -ENOTSUP; } -int bdrv_snapshot_delete(BlockDriverState *bs, const char *snapshot_id) +/** + * Delete 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, delete the first one with + * id @snapshot_id and name @name. + * 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. + * + * Returns: 0 on success, -errno on fail. If @bs is not inserted, return + * -ENOMEDIUM. If can't find one matching @id and @name, return -ENOENT. + * If @bs did not support internal snapshot, return -ENOTSUP. If @bs do not + * support parameter @snapshot_id or @name, return -EINVAL. + */ +int bdrv_snapshot_delete(BlockDriverState *bs, + const char *snapshot_id, + const char *name, + Error **errp) { BlockDriver *drv = bs->drv; if (!drv) { + error_set(errp, QERR_DEVICE_HAS_NO_MEDIUM, bdrv_get_device_name(bs)); return -ENOMEDIUM; } + if (!snapshot_id && !name) { + error_setg(errp, "snapshot_id and name are both NULL"); + return -EINVAL; + } if (drv->bdrv_snapshot_delete) { - return drv->bdrv_snapshot_delete(bs, snapshot_id); + return drv->bdrv_snapshot_delete(bs, snapshot_id, name, errp); } if (bs->file) { - return bdrv_snapshot_delete(bs->file, snapshot_id); + return bdrv_snapshot_delete(bs->file, snapshot_id, name, errp); } + error_set(errp, QERR_BLOCK_FORMAT_FEATURE_NOT_SUPPORTED, + drv->format_name, bdrv_get_device_name(bs), + "internal snapshot"); return -ENOTSUP; } diff --git a/include/block/block_int.h b/include/block/block_int.h index c6ac871..4499b8b 100644 --- a/include/block/block_int.h +++ b/include/block/block_int.h @@ -166,7 +166,10 @@ struct BlockDriver { QEMUSnapshotInfo *sn_info); int (*bdrv_snapshot_goto)(BlockDriverState *bs, const char *snapshot_id); - int (*bdrv_snapshot_delete)(BlockDriverState *bs, const char *snapshot_id); + int (*bdrv_snapshot_delete)(BlockDriverState *bs, + const char *snapshot_id, + const char *name, + Error **errp); int (*bdrv_snapshot_list)(BlockDriverState *bs, QEMUSnapshotInfo **psn_info); int (*bdrv_snapshot_load_tmp)(BlockDriverState *bs, diff --git a/include/block/snapshot.h b/include/block/snapshot.h index 9d06dc1..e47e78d 100644 --- a/include/block/snapshot.h +++ b/include/block/snapshot.h @@ -51,7 +51,10 @@ int bdrv_snapshot_create(BlockDriverState *bs, QEMUSnapshotInfo *sn_info); int bdrv_snapshot_goto(BlockDriverState *bs, const char *snapshot_id); -int bdrv_snapshot_delete(BlockDriverState *bs, const char *snapshot_id); +int bdrv_snapshot_delete(BlockDriverState *bs, + const char *snapshot_id, + const char *name, + Error **errp); int bdrv_snapshot_list(BlockDriverState *bs, QEMUSnapshotInfo **psn_info); int bdrv_snapshot_load_tmp(BlockDriverState *bs, diff --git a/include/qemu-common.h b/include/qemu-common.h index f439738..06c777f 100644 --- a/include/qemu-common.h +++ b/include/qemu-common.h @@ -191,6 +191,9 @@ int64_t strtosz_suffix(const char *nptr, char **end, const char default_suffix); int64_t strtosz_suffix_unit(const char *nptr, char **end, const char default_suffix, int64_t unit); +/* used to print char* safely */ +#define STR_PRINT_CHAR(str) ((str) ? (str) : "null") + /* path.c */ void init_paths(const char *prefix); const char *path(const char *pathname); diff --git a/qemu-img.c b/qemu-img.c index f8c97d3..05564d3 100644 --- a/qemu-img.c +++ b/qemu-img.c @@ -1883,7 +1883,10 @@ static int img_snapshot(int argc, char **argv) break; case SNAPSHOT_DELETE: - ret = bdrv_snapshot_delete(bs, snapshot_name); + ret = bdrv_snapshot_delete(bs, snapshot_name, NULL, NULL); + if (ret == -ENOENT || ret == -EINVAL) { + ret = bdrv_snapshot_delete(bs, NULL, snapshot_name, NULL); + } if (ret) { error_report("Could not delete snapshot '%s': %d (%s)", snapshot_name, ret, strerror(-ret)); diff --git a/savevm.c b/savevm.c index e0491e7..56afebb 100644 --- a/savevm.c +++ b/savevm.c @@ -2332,7 +2332,10 @@ static int del_existing_snapshots(Monitor *mon, const char *name) if (bdrv_can_snapshot(bs) && bdrv_snapshot_find(bs, snapshot, name) >= 0) { - ret = bdrv_snapshot_delete(bs, name); + ret = bdrv_snapshot_delete(bs, name, NULL, NULL); + if (ret == -ENOENT || ret == -EINVAL) { + ret = bdrv_snapshot_delete(bs, NULL, name, NULL); + } if (ret < 0) { monitor_printf(mon, "Error while deleting snapshot on '%s'\n", @@ -2562,7 +2565,10 @@ void do_delvm(Monitor *mon, const QDict *qdict) bs1 = NULL; while ((bs1 = bdrv_next(bs1))) { if (bdrv_can_snapshot(bs1)) { - ret = bdrv_snapshot_delete(bs1, name); + ret = bdrv_snapshot_delete(bs1, name, NULL, NULL); + if (ret == -ENOENT || ret == -EINVAL) { + ret = bdrv_snapshot_delete(bs, NULL, name, NULL); + } if (ret < 0) { if (ret == -ENOTSUP) monitor_printf(mon,