Hi,Crístian

I've read your patch, which has solved the snapshots restore problems more clearly. I agree with you that we've already fixed this issue in separate patches, and I need to make some changes for the formatting issues and test case.

My patch will be responsible for the url redirect problem, which modifies base.py and function 'revert' in vmsnapshots.py.

Besides, I found there are two points need to pay attention to:

1. snap.listAllChildren() in _backup_snapshots.py need flag in older libvirt versions, as I used "0.10.2", it raised "listAllChildren() takes exactly 2 arguments (1 given)" 

2. UI transfer param 'name' no matter you update vm name or not (of course while you edit a vm), so do we only need to bakup and restore the snapshots while vm name is really updated ?

Best regards,
Zongmei Gou

-------- Original message --------
From: Cr�stian Viana <vianac@linux.vnet.ibm.com>
Date: 2015-01-07 02:46
To: gouzongmei@ourfuture.cn;Kimchi Devel <kimchi-devel@ovirt.org>
Subject: Re:Re: [Kimchi-devel] [PATCH]


On 20-12-2014 05:49, gouzongmei@ourfuture.cn wrote:
> From: Zongmei Gou <gouzongmei@ourfuture.cn>
>
> ---
>   src/kimchi/control/base.py      |   13 ++++++++++---
>   src/kimchi/model/vms.py         |   24 ++++++++++++++++++------
>   src/kimchi/model/vmsnapshots.py |    8 ++++++--
>   src/kimchi/xmlutils/utils.py    |    5 +++++
>   4 files changed, 39 insertions(+), 11 deletions(-)

Thanks for the patch! It looks very nice written.

However, there are a few details which need to be fixed regarding
snapshots (e.g. the order the snapshots need to be recreated because of
parent/child relationship, setting back the current snapshot, among
other things). Also, we need to make sure the commands "make
check-local" (ensure formatting guidelines) and "make check" (run the
local tests) complete successfully before applying any new patch. There
are a few formatting issues with this patch which need to be updated,
and it's always good to add a test case which exploits the problem that
the patch fixes, so we don't hit it again in the future.

As I mentioned in a previous e-mail, I already have a patch which does
the snapshot handling but fails when reverting to a snapshot which was
created when the VM had a different name, due to the URL redirection
approach. AFAIK, that was the only problem with it and I didn't have to
look deeper to solve it. And your patch seems to fix that correctly (at
src/kimchi/control/base.py). Please, take a look at my patch at
https://github.com/cd1/kimchi/commit/20510e607e5f08ff6344fae3fbb0d84f324d16ce
and let me know what you think about it. I haven't sent it yet to this
mailing list because it's not completely finished, as I said.

Do you think we can fix this issue by submitting our patches separately
(i.e. you send the changes to src/kimchi/control/base.py and I send the
snapshot changes) or even by merging our patches together? It seems
we've already fixed this issue but in separate patches.