Re: [libvirt] [PATCHv2] snapshot: detect when qemu lacks disk-snapshot support

2011-10-20 Thread lvroyce 

Tested by: Royce Lv

Patch works as expected:
tested with cmd snapshot-create domain_name --disk-only
(1)when without hmp cmd support ,snapshot failed,0 size source file deleted
(2)when source file creating failed,no snapshot taken and no crash  with 
need_unlink flag set


--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

[libvirt] [PATCHv2] snapshot: detect when qemu lacks disk-snapshot support

2011-10-18 Thread Eric Blake 
Noticed when testing new libvirt against old qemu that lacked the
snapshot_blkdev HMP command.  Libvirt was mistakenly treating the
command as successful, and re-writing the domain XML to use the
just-created 0-byte file, rendering the domain broken on restart.

* src/qemu/qemu_monitor_text.c (qemuMonitorTextDiskSnapshot):
Notice another possible error message.
* src/qemu/qemu_driver.c
(qemuDomainSnapshotCreateSingleDiskActive): Don't keep 0-byte file
on failure.
---

v2: clean up on failure

 src/qemu/qemu_driver.c   |2 +-
 src/qemu/qemu_monitor_text.c |3 ++-
 2 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index f833655..84ef4dd 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -9027,7 +9027,6 @@ qemuDomainSnapshotCreateSingleDiskActive(struct 
qemud_driver *driver,
 VIR_WARN(Unable to release lock on %s, source);
 goto cleanup;
 }
-need_unlink = false;

 disk-src = origsrc;
 origsrc = NULL;
@@ -9041,6 +9040,7 @@ qemuDomainSnapshotCreateSingleDiskActive(struct 
qemud_driver *driver,
 goto cleanup;

 /* Update vm in place to match changes.  */
+need_unlink = false;
 VIR_FREE(disk-src);
 disk-src = source;
 source = NULL;
diff --git a/src/qemu/qemu_monitor_text.c b/src/qemu/qemu_monitor_text.c
index 2f31d99..4774df9 100644
--- a/src/qemu/qemu_monitor_text.c
+++ b/src/qemu/qemu_monitor_text.c
@@ -3064,7 +3064,8 @@ qemuMonitorTextDiskSnapshot(qemuMonitorPtr mon, const 
char *device,
 goto cleanup;
 }

-if (strstr(reply, error while creating qcow2) != NULL) {
+if (strstr(reply, error while creating qcow2) != NULL ||
+strstr(reply, unknown command:) != NULL) {
 qemuReportError(VIR_ERR_OPERATION_FAILED,
 _(Failed to take snapshot: %s), reply);
 goto cleanup;
-- 
1.7.4.4

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCHv2] snapshot: detect when qemu lacks disk-snapshot support

2011-10-18 Thread Eric Blake 

On 10/18/2011 05:31 PM, Dave Allan wrote:

On Tue, Oct 18, 2011 at 04:04:57PM -0600, Eric Blake wrote:

Noticed when testing new libvirt against old qemu that lacked the
snapshot_blkdev HMP command.  Libvirt was mistakenly treating the
command as successful, and re-writing the domain XML to use the
just-created 0-byte file, rendering the domain broken on restart.

* src/qemu/qemu_monitor_text.c (qemuMonitorTextDiskSnapshot):
Notice another possible error message.
* src/qemu/qemu_driver.c
(qemuDomainSnapshotCreateSingleDiskActive): Don't keep 0-byte file
on failure.


ACK, snapshot still works correctly when qemu does support it, and
after review with Eric, I think the code looks good.


Thanks; pushed.

--
Eric Blake   ebl...@redhat.com+1-801-349-2682
Libvirt virtualization library http://libvirt.org

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCHv2] snapshot: detect when qemu lacks disk-snapshot support

2011-10-18 Thread Dave Allan 
On Tue, Oct 18, 2011 at 04:04:57PM -0600, Eric Blake wrote:
 Noticed when testing new libvirt against old qemu that lacked the
 snapshot_blkdev HMP command.  Libvirt was mistakenly treating the
 command as successful, and re-writing the domain XML to use the
 just-created 0-byte file, rendering the domain broken on restart.
 
 * src/qemu/qemu_monitor_text.c (qemuMonitorTextDiskSnapshot):
 Notice another possible error message.
 * src/qemu/qemu_driver.c
 (qemuDomainSnapshotCreateSingleDiskActive): Don't keep 0-byte file
 on failure.
 ---
 
 v2: clean up on failure
 
  src/qemu/qemu_driver.c   |2 +-
  src/qemu/qemu_monitor_text.c |3 ++-
  2 files changed, 3 insertions(+), 2 deletions(-)
 
 diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
 index f833655..84ef4dd 100644
 --- a/src/qemu/qemu_driver.c
 +++ b/src/qemu/qemu_driver.c
 @@ -9027,7 +9027,6 @@ qemuDomainSnapshotCreateSingleDiskActive(struct 
 qemud_driver *driver,
  VIR_WARN(Unable to release lock on %s, source);
  goto cleanup;
  }
 -need_unlink = false;
 
  disk-src = origsrc;
  origsrc = NULL;
 @@ -9041,6 +9040,7 @@ qemuDomainSnapshotCreateSingleDiskActive(struct 
 qemud_driver *driver,
  goto cleanup;
 
  /* Update vm in place to match changes.  */
 +need_unlink = false;
  VIR_FREE(disk-src);
  disk-src = source;
  source = NULL;
 diff --git a/src/qemu/qemu_monitor_text.c b/src/qemu/qemu_monitor_text.c
 index 2f31d99..4774df9 100644
 --- a/src/qemu/qemu_monitor_text.c
 +++ b/src/qemu/qemu_monitor_text.c
 @@ -3064,7 +3064,8 @@ qemuMonitorTextDiskSnapshot(qemuMonitorPtr mon, const 
 char *device,
  goto cleanup;
  }
 
 -if (strstr(reply, error while creating qcow2) != NULL) {
 +if (strstr(reply, error while creating qcow2) != NULL ||
 +strstr(reply, unknown command:) != NULL) {
  qemuReportError(VIR_ERR_OPERATION_FAILED,
  _(Failed to take snapshot: %s), reply);
  goto cleanup;
 -- 
 1.7.4.4
 
 --
 libvir-list mailing list
 libvir-list@redhat.com
 https://www.redhat.com/mailman/listinfo/libvir-list

ACK, snapshot still works correctly when qemu does support it, and
after review with Eric, I think the code looks good.

Dave

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list