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/20510e607e5f08ff6344fae3fbb0d84f324d16c... 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.