Re: [Qemu-devel] [PATCH V5 2/8] snapshot: distinguish id and name in snapshot delete

[Wenchao Xia]

于 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 <address@hidden>
> > Signed-off-by: Pavel Hrdina <address@hidden>
> > ---
> >   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


--
Best Regards

Wenchao Xia