Re: [Qemu-devel] [PATCH V3 07/11] block: export function bdrv_find_snapshot()

[Wenchao Xia]

于 2013-1-16 1:57, Eric Blake 写道:
> On 01/15/2013 03:24 AM, Wenchao Xia wrote:
>
> > > > +int bdrv_snapshot_find(BlockDriverState *bs, QEMUSnapshotInfo *sn_info,
> > > > +                       const char *name)
>
> > > > +        if (!strcmp(sn->id_str, name) || !strcmp(sn->name, name)) {
>
> > > This code comparison favors ids over names; so if I request to delvm 2,
> > > I end up removing the second snapshot, not the first.  This is okay, but
> > > probably worth documenting,
>
> > how about:
> > /*  if id is not NULL, try find it with id, if not exist, return NULL
> >   *  if id is NULL and name is not NULL, try find it with name.
> >   */ if id and name is NULL, direct return fail.
> > int bdrv_snapshot_find(BlockDriverState *bs, QEMUSnapshotInfo *sn_info,
> >                         const char *id, const char *name)
>
> That would be pushing the burden onto the callers to decide whether they
> are doing an id lookup, a name lookup, or both.  In QMP terms, that
> means that your QMP command to delete a snapshot would now need an
> additional optional argument to decide whether the associated name is
> only an id, only a name, or can match either.  But I'm not sure you want
> that.
>
> What I was trying to get at is that given a single string "2", it does
> seem nicer to do both an id and a name lookup, and return the first hit;
> you just need to document that ids take preference over names (and thus,
> naming a snapshot "2" may make the snapshot become invisible by name,
> but not by id, if a later snapshot creation causes id 2 to be used).
> Then your QMP command for deleting a snapshot no longer needs to care
> whether "2" is an id or a name, just whether it matches.
  OK, I guess following is better and clear:

 /* Try to find the snapshot with @id and @name, @id have higher
  * priority in searching. Both @id and @name can be NULL.
  */
int bdrv_snapshot_find(BlockDriverState *bs, QEMUSnapshotInfo *sn_info,
                        const char *id, const char *name)

Then caller at hmp/qmp layer can decide how to use it. For info of
snapshot retrieving in this serial, things are simple. for create/delete
of snapshot in future, I think a check with id_wellformed() and an
interface like the proposal from Pavel is needed.
http://lists.gnu.org/archive/html/qemu-devel/2013-01/msg01551.html

> Hmm, while typing this, I thought of another snag.  Suppose you have a
> VM with two disks, but where only the first disk previously had a
> snapshot with id 1.  If I create a new snapshot across both disks, does
> that mean disk 1 gets id 2 while disk 2 gets id 1, or do both disks get
> id 2, even though that means disk 2 skips over id 1?  As long as the
> snapshot is named, you can refer to the name to get the same snapshot
> across both disks, regardless of what id it has.  But if the name is
> numeric, and id takes preference over name when doing a lookup, we could
> get ourselves into the situation where a snapshot created with name "2"
> can eventually never be restored in one piece, because the individual
> disks have different ids for the same snapshot name.  So maybe we DO
> need a way after all for QMP to specify whether a name lookup is for id,
> name, or both.


--
Best Regards

Wenchao Xia