On 25.4.2013 08:31, Wenchao Xia wrote:
>> Finding snapshot by a name which could also be an id isn't best way
>> how to do it. There will be rewrite of savevm, loadvm and delvm to
>> improve the behavior of these commands. The savevm and loadvm will
>> have their own patch series.
>>
>> Now bdrv_snapshot_find takes more parameters. The name parameter will
>> be matched only against the name of the snapshot and the same applies
>> to id parameter.
>>
>> There is one exception. If you set the last parameter, the name parameter
>> will be matched against the name or the id of a snapshot. This exception
>> is only for backward compatibility for other commands and it will be
>> dropped after all commands will be rewritten.
>>
>> We only need to know if that snapshot exists or not. We don't care
>> about any error message. If snapshot exists it returns TRUE otherwise
>> it returns FALSE.
>>
>> There is also new Error parameter which will containt error messeage if
>> something goes wrong.
>>
>> Signed-off-by: Pavel Hrdina <phrd...@redhat.com>
>> ---
>> savevm.c | 93
>> ++++++++++++++++++++++++++++++++++++++++++++++------------------
>> 1 file changed, 67 insertions(+), 26 deletions(-)
>>
>> diff --git a/savevm.c b/savevm.c
>> index ba97c41..1622c55 100644
>> --- a/savevm.c
>> +++ b/savevm.c
>> @@ -2262,26 +2262,66 @@ out:
>> return ret;
>> }
>>
>> -static int bdrv_snapshot_find(BlockDriverState *bs, QEMUSnapshotInfo
>> *sn_info,
>> - const char *name)
>> +static bool bdrv_snapshot_find(BlockDriverState *bs, QEMUSnapshotInfo
>> *sn_info,
>> + const char *name, const char *id, Error
>> **errp,
>> + bool old_match)
> suggest directly drop old_match parameter here, or squash patch 12
> into this one, mark the change in commit message, then the logic will
> be clearer.
> Personally hope to place parameter *id before *name.
That make sense, I'll change it.
>
>> {
>> QEMUSnapshotInfo *sn_tab, *sn;
>> - int nb_sns, i, ret;
>> + int nb_sns, i;
>> + bool found = false;
>> +
>> + assert(name || id);
>>
>> - ret = -ENOENT;
>> nb_sns = bdrv_snapshot_list(bs, &sn_tab);
>> - if (nb_sns < 0)
>> - return ret;
>> - for(i = 0; i < nb_sns; i++) {
>> + if (nb_sns < 0) {
>> + error_setg_errno(errp, -nb_sns, "Failed to get a snapshot list");
>> + return found;
>> + }
>> +
>> + if (nb_sns == 0) {
>> + error_setg(errp, "Device has no snapshots");
>> + return found;
>> + }
> suggest not set errp here, which can be used to tip exception, but
> having not found one is a normal case.
You don't have to use the error message. It is set as the error message
but actually is more like an announcement.
>
>> +
>> + for (i = 0; i < nb_sns; i++) {
>> sn = &sn_tab[i];
>> - if (!strcmp(sn->id_str, name) || !strcmp(sn->name, name)) {
>> - *sn_info = *sn;
>> - ret = 0;
>> - break;
>> + if (name && id) {
>> + if (!strcmp(sn->id_str, id) && !strcmp(sn->name, name)) {
>> + *sn_info = *sn;
>> + found = true;
>> + break;
>> + }
>> + } else if (name) {
>> + /* for compatibility for old bdrv_snapshot_find call
>> + * will be removed */
>> + if (old_match) {
>> + if (!strcmp(sn->id_str, name) || !strcmp(sn->name, name)) {
>> + *sn_info = *sn;
>> + found = true;
>> + break;
>> + }
>> + } else {
>> + if (!strcmp(sn->name, name)) {
>> + *sn_info = *sn;
>> + found = true;
>> + break;
>> + }
>> + }
>> + } else if (id) {
>> + if (!strcmp(sn->id_str, id)) {
>> + *sn_info = *sn;
>> + found = true;
>> + break;
>> + }
>> }
>> }
> suggest change the sequence:
>
> if (name && id) {
> for () {
> }
> } else if (name){
> for () {
> }
> } else if (id) {
> for () {
> }
> }
> slightly faster, and make logic more clear.
Thanks for suggest, I'll change the logic.
>
>> +
>> + if (!found) {
>> + error_setg(errp, "Failed to find snapshot '%s'", name ? name : id);
> suggest not to set error, since it is a normal case.
The same comment as for "Device has no snapshots".
Pavel
