Patchwork [04/11] snapshot: new function bdrv_snapshot_find_by_id_and_name()

login
register
mail settings
Submitter Wayne Xia
Date June 8, 2013, 6:58 a.m.
Message ID <1370674687-13849-5-git-send-email-xiawenc@linux.vnet.ibm.com>
Download mbox | patch
Permalink /patch/249927/
State New
Headers show

Comments

Wayne Xia - June 8, 2013, 6:58 a.m.
To make it clear about id and name in searching, add this API
to distinguish them. Caller can choose to search by id or name,
*errp will be set only for exception.

Some code are modified based on Pavel's patch.

Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
---
 block/snapshot.c         |   74 ++++++++++++++++++++++++++++++++++++++++++++++
 include/block/snapshot.h |    6 ++++
 2 files changed, 80 insertions(+), 0 deletions(-)
Fam Zheng - June 8, 2013, 7:31 a.m.
On Sat, 06/08 14:58, Wenchao Xia wrote:
> To make it clear about id and name in searching, add this API
> to distinguish them. Caller can choose to search by id or name,
> *errp will be set only for exception.
> 
> Some code are modified based on Pavel's patch.
> 
> Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
> Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
> ---
>  block/snapshot.c         |   74 ++++++++++++++++++++++++++++++++++++++++++++++
>  include/block/snapshot.h |    6 ++++
>  2 files changed, 80 insertions(+), 0 deletions(-)
> 
> diff --git a/block/snapshot.c b/block/snapshot.c
> index 6c6d9de..0a9af4e 100644
> --- a/block/snapshot.c
> +++ b/block/snapshot.c
> @@ -48,6 +48,80 @@ int bdrv_snapshot_find(BlockDriverState *bs, QEMUSnapshotInfo *sn_info,
>      return ret;
>  }
>  
> +/**
> + * Look up an internal snapshot by @id and @name.
> + * @bs: block device to search
> + * @id: unique snapshot ID, or NULL
> + * @name: snapshot name, or NULL
> + * @sn_info: location to store information on the snapshot found
> + * @errp: location to store error, will be set only for exception
> + *
> + * This function will traverse snapshot list in @bs to search the matching
> + * one, @id and @name are the matching condition:
> + * If both @id and @name are specified, find the first one with id @id and
> + * name @name.
> + * If only @id is specified, find the first one with id @id.
> + * If only @name is specified, find the first one with name @name.
> + * if none is specified, abort().
> + *
> + * Returns: true when a snapshot is found and @sn_info will be filled, false
> + * when error or not found. If all operation succeed but no matching one is
> + * found, @errp will NOT be set.
> + */
> +bool bdrv_snapshot_find_by_id_and_name(BlockDriverState *bs,
> +                                       const char *id,
> +                                       const char *name,
> +                                       QEMUSnapshotInfo *sn_info,
> +                                       Error **errp)
> +{
> +    QEMUSnapshotInfo *sn_tab, *sn;
> +    int nb_sns, i;
> +    bool ret = false;
> +
> +    nb_sns = bdrv_snapshot_list(bs, &sn_tab);
> +    if (nb_sns < 0) {
> +        error_setg_errno(errp, -nb_sns, "Failed to get a snapshot list");
> +        return false;
> +    } else if (nb_sns == 0) {
> +        return false;
> +    }
> +
> +    if (id && name) {
> +        for (i = 0; i < nb_sns; i++) {
> +            sn = &sn_tab[i];
> +            if (!strcmp(sn->id_str, id) && !strcmp(sn->name, name)) {
> +                *sn_info = *sn;
> +                ret = true;
> +                break;
> +            }
> +        }
> +    } else if (id) {
> +        for (i = 0; i < nb_sns; i++) {
> +            sn = &sn_tab[i];
> +            if (!strcmp(sn->id_str, id)) {
> +                *sn_info = *sn;
> +                ret = true;
> +                break;
> +            }
> +        }
> +    } else if (name) {
> +        for (i = 0; i < nb_sns; i++) {
> +            sn = &sn_tab[i];
> +            if (!strcmp(sn->name, name)) {
> +                *sn_info = *sn;
> +                ret = true;
> +                break;
> +            }
> +        }
> +    } else {
> +        /* program error */
> +        abort();
> +    }

Looks duplicated. How about:

    if (id || name) {
        for (i = 0; i < nb_sns; i++) {
            sn = &sn_tab[i];
            if ((!id || !strcmp(sn->id_str, id)) &&
                (!name || !strcmp(sn->name, name))) {
                *sn_info = *sn;
                ret = true;
                break;
            }
        }
    } else {
        abort();
    }

And why do we have to abort here? It is not completely nonsense to me to
return first snapshot with id == NULL and name == NULL.
Wayne Xia - June 8, 2013, 7:58 a.m.
于 2013-6-8 15:31, Fam Zheng 写道:
> On Sat, 06/08 14:58, Wenchao Xia wrote:
>> To make it clear about id and name in searching, add this API
>> to distinguish them. Caller can choose to search by id or name,
>> *errp will be set only for exception.
>>
>> Some code are modified based on Pavel's patch.
>>
>> Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
>> Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
>> ---
>>   block/snapshot.c         |   74 ++++++++++++++++++++++++++++++++++++++++++++++
>>   include/block/snapshot.h |    6 ++++
>>   2 files changed, 80 insertions(+), 0 deletions(-)
>>
>> diff --git a/block/snapshot.c b/block/snapshot.c
>> index 6c6d9de..0a9af4e 100644
>> --- a/block/snapshot.c
>> +++ b/block/snapshot.c
>> @@ -48,6 +48,80 @@ int bdrv_snapshot_find(BlockDriverState *bs, QEMUSnapshotInfo *sn_info,
>>       return ret;
>>   }
>>
>> +/**
>> + * Look up an internal snapshot by @id and @name.
>> + * @bs: block device to search
>> + * @id: unique snapshot ID, or NULL
>> + * @name: snapshot name, or NULL
>> + * @sn_info: location to store information on the snapshot found
>> + * @errp: location to store error, will be set only for exception
>> + *
>> + * This function will traverse snapshot list in @bs to search the matching
>> + * one, @id and @name are the matching condition:
>> + * If both @id and @name are specified, find the first one with id @id and
>> + * name @name.
>> + * If only @id is specified, find the first one with id @id.
>> + * If only @name is specified, find the first one with name @name.
>> + * if none is specified, abort().
>> + *
>> + * Returns: true when a snapshot is found and @sn_info will be filled, false
>> + * when error or not found. If all operation succeed but no matching one is
>> + * found, @errp will NOT be set.
>> + */
>> +bool bdrv_snapshot_find_by_id_and_name(BlockDriverState *bs,
>> +                                       const char *id,
>> +                                       const char *name,
>> +                                       QEMUSnapshotInfo *sn_info,
>> +                                       Error **errp)
>> +{
>> +    QEMUSnapshotInfo *sn_tab, *sn;
>> +    int nb_sns, i;
>> +    bool ret = false;
>> +
>> +    nb_sns = bdrv_snapshot_list(bs, &sn_tab);
>> +    if (nb_sns < 0) {
>> +        error_setg_errno(errp, -nb_sns, "Failed to get a snapshot list");
>> +        return false;
>> +    } else if (nb_sns == 0) {
>> +        return false;
>> +    }
>> +
>> +    if (id && name) {
>> +        for (i = 0; i < nb_sns; i++) {
>> +            sn = &sn_tab[i];
>> +            if (!strcmp(sn->id_str, id) && !strcmp(sn->name, name)) {
>> +                *sn_info = *sn;
>> +                ret = true;
>> +                break;
>> +            }
>> +        }
>> +    } else if (id) {
>> +        for (i = 0; i < nb_sns; i++) {
>> +            sn = &sn_tab[i];
>> +            if (!strcmp(sn->id_str, id)) {
>> +                *sn_info = *sn;
>> +                ret = true;
>> +                break;
>> +            }
>> +        }
>> +    } else if (name) {
>> +        for (i = 0; i < nb_sns; i++) {
>> +            sn = &sn_tab[i];
>> +            if (!strcmp(sn->name, name)) {
>> +                *sn_info = *sn;
>> +                ret = true;
>> +                break;
>> +            }
>> +        }
>> +    } else {
>> +        /* program error */
>> +        abort();
>> +    }
>
> Looks duplicated. How about:
>
>      if (id || name) {
>          for (i = 0; i < nb_sns; i++) {
>              sn = &sn_tab[i];
>              if ((!id || !strcmp(sn->id_str, id)) &&
>                  (!name || !strcmp(sn->name, name))) {
>                  *sn_info = *sn;
>                  ret = true;
>                  break;
>              }
>          }
>      } else {
>          abort();
>      }
>
   Less code, but slightly slower since more "if" inside "for". I think
three "for" also show more clear about judgement logic.

> And why do we have to abort here? It is not completely nonsense to me to
> return first snapshot with id == NULL and name == NULL.
>
   Just to tip program error. An snapshot with id == NULL and name ==
NULL is not possible, isn't it?.
Fam Zheng - June 8, 2013, 8:35 a.m.
On Sat, 06/08 15:58, Wenchao Xia wrote:
> 于 2013-6-8 15:31, Fam Zheng 写道:
> >On Sat, 06/08 14:58, Wenchao Xia wrote:
> >>To make it clear about id and name in searching, add this API
> >>to distinguish them. Caller can choose to search by id or name,
> >>*errp will be set only for exception.
> >>
> >>Some code are modified based on Pavel's patch.
> >>
> >>Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
> >>Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
> >>---
> >>  block/snapshot.c         |   74 ++++++++++++++++++++++++++++++++++++++++++++++
> >>  include/block/snapshot.h |    6 ++++
> >>  2 files changed, 80 insertions(+), 0 deletions(-)
> >>
> >>diff --git a/block/snapshot.c b/block/snapshot.c
> >>index 6c6d9de..0a9af4e 100644
> >>--- a/block/snapshot.c
> >>+++ b/block/snapshot.c
> >>@@ -48,6 +48,80 @@ int bdrv_snapshot_find(BlockDriverState *bs, QEMUSnapshotInfo *sn_info,
> >>      return ret;
> >>  }
> >>
> >>+/**
> >>+ * Look up an internal snapshot by @id and @name.
> >>+ * @bs: block device to search
> >>+ * @id: unique snapshot ID, or NULL
> >>+ * @name: snapshot name, or NULL
> >>+ * @sn_info: location to store information on the snapshot found
> >>+ * @errp: location to store error, will be set only for exception
> >>+ *
> >>+ * This function will traverse snapshot list in @bs to search the matching
> >>+ * one, @id and @name are the matching condition:
> >>+ * If both @id and @name are specified, find the first one with id @id and
> >>+ * name @name.
> >>+ * If only @id is specified, find the first one with id @id.
> >>+ * If only @name is specified, find the first one with name @name.
> >>+ * if none is specified, abort().
> >>+ *
> >>+ * Returns: true when a snapshot is found and @sn_info will be filled, false
> >>+ * when error or not found. If all operation succeed but no matching one is
> >>+ * found, @errp will NOT be set.
> >>+ */
> >>+bool bdrv_snapshot_find_by_id_and_name(BlockDriverState *bs,
> >>+                                       const char *id,
> >>+                                       const char *name,
> >>+                                       QEMUSnapshotInfo *sn_info,
> >>+                                       Error **errp)
> >>+{
> >>+    QEMUSnapshotInfo *sn_tab, *sn;
> >>+    int nb_sns, i;
> >>+    bool ret = false;
> >>+
> >>+    nb_sns = bdrv_snapshot_list(bs, &sn_tab);
> >>+    if (nb_sns < 0) {
> >>+        error_setg_errno(errp, -nb_sns, "Failed to get a snapshot list");
> >>+        return false;
> >>+    } else if (nb_sns == 0) {
> >>+        return false;
> >>+    }
> >>+
> >>+    if (id && name) {
> >>+        for (i = 0; i < nb_sns; i++) {
> >>+            sn = &sn_tab[i];
> >>+            if (!strcmp(sn->id_str, id) && !strcmp(sn->name, name)) {
> >>+                *sn_info = *sn;
> >>+                ret = true;
> >>+                break;
> >>+            }
> >>+        }
> >>+    } else if (id) {
> >>+        for (i = 0; i < nb_sns; i++) {
> >>+            sn = &sn_tab[i];
> >>+            if (!strcmp(sn->id_str, id)) {
> >>+                *sn_info = *sn;
> >>+                ret = true;
> >>+                break;
> >>+            }
> >>+        }
> >>+    } else if (name) {
> >>+        for (i = 0; i < nb_sns; i++) {
> >>+            sn = &sn_tab[i];
> >>+            if (!strcmp(sn->name, name)) {
> >>+                *sn_info = *sn;
> >>+                ret = true;
> >>+                break;
> >>+            }
> >>+        }
> >>+    } else {
> >>+        /* program error */
> >>+        abort();
> >>+    }
> >
> >Looks duplicated. How about:
> >
> >     if (id || name) {
> >         for (i = 0; i < nb_sns; i++) {
> >             sn = &sn_tab[i];
> >             if ((!id || !strcmp(sn->id_str, id)) &&
> >                 (!name || !strcmp(sn->name, name))) {
> >                 *sn_info = *sn;
> >                 ret = true;
> >                 break;
> >             }
> >         }
> >     } else {
> >         abort();
> >     }
> >
>   Less code, but slightly slower since more "if" inside "for". I think
> three "for" also show more clear about judgement logic.

No I don't think if-in-for or for-in-if *here* makes any meaningful
difference in performance, if we really need it fast, we'd better sort the
list it first and binary search. And I don't see it clearer to duplicate
the same logic for three times, If I want to understand it, I need to
compare if#1 and if#2 to get they are the same, and then compare #2 and
#3 again, just to know that the three are no different.

> 
> >And why do we have to abort here? It is not completely nonsense to me to
> >return first snapshot with id == NULL and name == NULL.
> >
>   Just to tip program error. An snapshot with id == NULL and name ==
> NULL is not possible, isn't it?.

OK.
Wayne Xia - June 9, 2013, 2:33 a.m.
于 2013-6-8 16:35, Fam Zheng 写道:
> On Sat, 06/08 15:58, Wenchao Xia wrote:
>> 于 2013-6-8 15:31, Fam Zheng 写道:
>>> On Sat, 06/08 14:58, Wenchao Xia wrote:
>>>> To make it clear about id and name in searching, add this API
>>>> to distinguish them. Caller can choose to search by id or name,
>>>> *errp will be set only for exception.
>>>>
>>>> Some code are modified based on Pavel's patch.
>>>>
>>>> Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
>>>> Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
>>>> ---
>>>>   block/snapshot.c         |   74 ++++++++++++++++++++++++++++++++++++++++++++++
>>>>   include/block/snapshot.h |    6 ++++
>>>>   2 files changed, 80 insertions(+), 0 deletions(-)
>>>>
>>>> diff --git a/block/snapshot.c b/block/snapshot.c
>>>> index 6c6d9de..0a9af4e 100644
>>>> --- a/block/snapshot.c
>>>> +++ b/block/snapshot.c
>>>> @@ -48,6 +48,80 @@ int bdrv_snapshot_find(BlockDriverState *bs, QEMUSnapshotInfo *sn_info,
>>>>       return ret;
>>>>   }
>>>>
>>>> +/**
>>>> + * Look up an internal snapshot by @id and @name.
>>>> + * @bs: block device to search
>>>> + * @id: unique snapshot ID, or NULL
>>>> + * @name: snapshot name, or NULL
>>>> + * @sn_info: location to store information on the snapshot found
>>>> + * @errp: location to store error, will be set only for exception
>>>> + *
>>>> + * This function will traverse snapshot list in @bs to search the matching
>>>> + * one, @id and @name are the matching condition:
>>>> + * If both @id and @name are specified, find the first one with id @id and
>>>> + * name @name.
>>>> + * If only @id is specified, find the first one with id @id.
>>>> + * If only @name is specified, find the first one with name @name.
>>>> + * if none is specified, abort().
>>>> + *
>>>> + * Returns: true when a snapshot is found and @sn_info will be filled, false
>>>> + * when error or not found. If all operation succeed but no matching one is
>>>> + * found, @errp will NOT be set.
>>>> + */
>>>> +bool bdrv_snapshot_find_by_id_and_name(BlockDriverState *bs,
>>>> +                                       const char *id,
>>>> +                                       const char *name,
>>>> +                                       QEMUSnapshotInfo *sn_info,
>>>> +                                       Error **errp)
>>>> +{
>>>> +    QEMUSnapshotInfo *sn_tab, *sn;
>>>> +    int nb_sns, i;
>>>> +    bool ret = false;
>>>> +
>>>> +    nb_sns = bdrv_snapshot_list(bs, &sn_tab);
>>>> +    if (nb_sns < 0) {
>>>> +        error_setg_errno(errp, -nb_sns, "Failed to get a snapshot list");
>>>> +        return false;
>>>> +    } else if (nb_sns == 0) {
>>>> +        return false;
>>>> +    }
>>>> +
>>>> +    if (id && name) {
>>>> +        for (i = 0; i < nb_sns; i++) {
>>>> +            sn = &sn_tab[i];
>>>> +            if (!strcmp(sn->id_str, id) && !strcmp(sn->name, name)) {
>>>> +                *sn_info = *sn;
>>>> +                ret = true;
>>>> +                break;
>>>> +            }
>>>> +        }
>>>> +    } else if (id) {
>>>> +        for (i = 0; i < nb_sns; i++) {
>>>> +            sn = &sn_tab[i];
>>>> +            if (!strcmp(sn->id_str, id)) {
>>>> +                *sn_info = *sn;
>>>> +                ret = true;
>>>> +                break;
>>>> +            }
>>>> +        }
>>>> +    } else if (name) {
>>>> +        for (i = 0; i < nb_sns; i++) {
>>>> +            sn = &sn_tab[i];
>>>> +            if (!strcmp(sn->name, name)) {
>>>> +                *sn_info = *sn;
>>>> +                ret = true;
>>>> +                break;
>>>> +            }
>>>> +        }
>>>> +    } else {
>>>> +        /* program error */
>>>> +        abort();
>>>> +    }
>>>
>>> Looks duplicated. How about:
>>>
>>>      if (id || name) {
>>>          for (i = 0; i < nb_sns; i++) {
>>>              sn = &sn_tab[i];
>>>              if ((!id || !strcmp(sn->id_str, id)) &&
>>>                  (!name || !strcmp(sn->name, name))) {
>>>                  *sn_info = *sn;
>>>                  ret = true;
>>>                  break;
>>>              }
>>>          }
>>>      } else {
>>>          abort();
>>>      }
>>>
>>    Less code, but slightly slower since more "if" inside "for". I think
>> three "for" also show more clear about judgement logic.
>
> No I don't think if-in-for or for-in-if *here* makes any meaningful
> difference in performance, if we really need it fast, we'd better sort the
   My instinct is forbid if in for, just my custom.

> list it first and binary search. And I don't see it clearer to duplicate
> the same logic for three times, If I want to understand it, I need to
   Three cases makes work flow clear, and it is easy when I want to
change a logic in one case.

> compare if#1 and if#2 to get they are the same, and then compare #2 and
> #3 again, just to know that the three are no different.
>
   There is difference requiring reader think and extend it out into
three cases in his mind:
               if ((!id || !strcmp(sn->id_str, id)) &&
                   (!name || !strcmp(sn->name, name)))

It is a coding style issue, usually I don't mind to write more C lines 
to make things simple. Hope to get maintainer's idea.

>>
>>> And why do we have to abort here? It is not completely nonsense to me to
>>> return first snapshot with id == NULL and name == NULL.
>>>
>>    Just to tip program error. An snapshot with id == NULL and name ==
>> NULL is not possible, isn't it?.
>
> OK.
>
Stefan Hajnoczi - June 11, 2013, 8:26 a.m.
On Sat, Jun 08, 2013 at 02:58:00PM +0800, Wenchao Xia wrote:
> +    if (id && name) {
> +        for (i = 0; i < nb_sns; i++) {
> +            sn = &sn_tab[i];
> +            if (!strcmp(sn->id_str, id) && !strcmp(sn->name, name)) {
> +                *sn_info = *sn;
> +                ret = true;
> +                break;
> +            }
> +        }
> +    } else if (id) {
> +        for (i = 0; i < nb_sns; i++) {
> +            sn = &sn_tab[i];
> +            if (!strcmp(sn->id_str, id)) {
> +                *sn_info = *sn;
> +                ret = true;
> +                break;
> +            }
> +        }
> +    } else if (name) {
> +        for (i = 0; i < nb_sns; i++) {
> +            sn = &sn_tab[i];
> +            if (!strcmp(sn->name, name)) {
> +                *sn_info = *sn;
> +                ret = true;
> +                break;
> +            }
> +        }
> +    } else {
> +        /* program error */
> +        abort();
> +    }

If you respin, this would be a little clearer:

assert(id || name);

if (id && name) {
    ...
} else if (id) {
    ...
} else if (name) {
    ...
}

The advantage is that the assert(3) condition is included in the error
message that gets printed.

Stefan
Wayne Xia - June 13, 2013, 3:34 a.m.
于 2013-6-11 16:26, Stefan Hajnoczi 写道:
> On Sat, Jun 08, 2013 at 02:58:00PM +0800, Wenchao Xia wrote:
>> +    if (id && name) {
>> +        for (i = 0; i < nb_sns; i++) {
>> +            sn = &sn_tab[i];
>> +            if (!strcmp(sn->id_str, id) && !strcmp(sn->name, name)) {
>> +                *sn_info = *sn;
>> +                ret = true;
>> +                break;
>> +            }
>> +        }
>> +    } else if (id) {
>> +        for (i = 0; i < nb_sns; i++) {
>> +            sn = &sn_tab[i];
>> +            if (!strcmp(sn->id_str, id)) {
>> +                *sn_info = *sn;
>> +                ret = true;
>> +                break;
>> +            }
>> +        }
>> +    } else if (name) {
>> +        for (i = 0; i < nb_sns; i++) {
>> +            sn = &sn_tab[i];
>> +            if (!strcmp(sn->name, name)) {
>> +                *sn_info = *sn;
>> +                ret = true;
>> +                break;
>> +            }
>> +        }
>> +    } else {
>> +        /* program error */
>> +        abort();
>> +    }
>
> If you respin, this would be a little clearer:
>
> assert(id || name);
>
> if (id && name) {
>      ...
> } else if (id) {
>      ...
> } else if (name) {
>      ...
> }
>
> The advantage is that the assert(3) condition is included in the error
> message that gets printed.
>
> Stefan
>
   OK, will do it in next version.

Patch

diff --git a/block/snapshot.c b/block/snapshot.c
index 6c6d9de..0a9af4e 100644
--- a/block/snapshot.c
+++ b/block/snapshot.c
@@ -48,6 +48,80 @@  int bdrv_snapshot_find(BlockDriverState *bs, QEMUSnapshotInfo *sn_info,
     return ret;
 }
 
+/**
+ * Look up an internal snapshot by @id and @name.
+ * @bs: block device to search
+ * @id: unique snapshot ID, or NULL
+ * @name: snapshot name, or NULL
+ * @sn_info: location to store information on the snapshot found
+ * @errp: location to store error, will be set only for exception
+ *
+ * This function will traverse snapshot list in @bs to search the matching
+ * one, @id and @name are the matching condition:
+ * If both @id and @name are specified, find the first one with id @id and
+ * name @name.
+ * If only @id is specified, find the first one with id @id.
+ * If only @name is specified, find the first one with name @name.
+ * if none is specified, abort().
+ *
+ * Returns: true when a snapshot is found and @sn_info will be filled, false
+ * when error or not found. If all operation succeed but no matching one is
+ * found, @errp will NOT be set.
+ */
+bool bdrv_snapshot_find_by_id_and_name(BlockDriverState *bs,
+                                       const char *id,
+                                       const char *name,
+                                       QEMUSnapshotInfo *sn_info,
+                                       Error **errp)
+{
+    QEMUSnapshotInfo *sn_tab, *sn;
+    int nb_sns, i;
+    bool ret = false;
+
+    nb_sns = bdrv_snapshot_list(bs, &sn_tab);
+    if (nb_sns < 0) {
+        error_setg_errno(errp, -nb_sns, "Failed to get a snapshot list");
+        return false;
+    } else if (nb_sns == 0) {
+        return false;
+    }
+
+    if (id && name) {
+        for (i = 0; i < nb_sns; i++) {
+            sn = &sn_tab[i];
+            if (!strcmp(sn->id_str, id) && !strcmp(sn->name, name)) {
+                *sn_info = *sn;
+                ret = true;
+                break;
+            }
+        }
+    } else if (id) {
+        for (i = 0; i < nb_sns; i++) {
+            sn = &sn_tab[i];
+            if (!strcmp(sn->id_str, id)) {
+                *sn_info = *sn;
+                ret = true;
+                break;
+            }
+        }
+    } else if (name) {
+        for (i = 0; i < nb_sns; i++) {
+            sn = &sn_tab[i];
+            if (!strcmp(sn->name, name)) {
+                *sn_info = *sn;
+                ret = true;
+                break;
+            }
+        }
+    } else {
+        /* program error */
+        abort();
+    }
+
+    g_free(sn_tab);
+    return ret;
+}
+
 int bdrv_can_snapshot(BlockDriverState *bs)
 {
     BlockDriver *drv = bs->drv;
diff --git a/include/block/snapshot.h b/include/block/snapshot.h
index eaf61f0..9d06dc1 100644
--- a/include/block/snapshot.h
+++ b/include/block/snapshot.h
@@ -26,6 +26,7 @@ 
 #define SNAPSHOT_H
 
 #include "qemu-common.h"
+#include "qapi/error.h"
 
 typedef struct QEMUSnapshotInfo {
     char id_str[128]; /* unique snapshot id */
@@ -40,6 +41,11 @@  typedef struct QEMUSnapshotInfo {
 
 int bdrv_snapshot_find(BlockDriverState *bs, QEMUSnapshotInfo *sn_info,
                        const char *name);
+bool bdrv_snapshot_find_by_id_and_name(BlockDriverState *bs,
+                                       const char *id,
+                                       const char *name,
+                                       QEMUSnapshotInfo *sn_info,
+                                       Error **errp);
 int bdrv_can_snapshot(BlockDriverState *bs);
 int bdrv_snapshot_create(BlockDriverState *bs,
                          QEMUSnapshotInfo *sn_info);