Re: [Qemu-devel] [PATCH V5 1/5] snapshot: distinguish id and name in load_tmp

[Wenchao Xia]

于 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 <address@hidden>
> > ---
> >   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