Re: [Qemu-devel] [PATCH v2 05/12] block: update error reporting for bdrv_snapshot_goto() and related functions

2013-05-03 Thread Kevin Wolf 
Am 24.04.2013 um 17:32 hat Pavel Hrdina geschrieben:
 Signed-off-by: Pavel Hrdina phrd...@redhat.com

 --- a/block/sheepdog.c
 +++ b/block/sheepdog.c
 @@ -1867,7 +1867,9 @@ cleanup:
  return ret;
  }
  
 -static int sd_snapshot_goto(BlockDriverState *bs, const char *snapshot_id)
 +static void sd_snapshot_goto(BlockDriverState *bs,
 + const char *snapshot_id,
 + Error **errp)
  {
  BDRVSheepdogState *s = bs-opaque;
  BDRVSheepdogState *old_s;
 @@ -1892,13 +1894,13 @@ static int sd_snapshot_goto(BlockDriverState *bs, 
 const char *snapshot_id)
  
  ret = find_vdi_name(s, vdi, snapid, tag, vid, 1);
  if (ret) {
 -error_report(Failed to find_vdi_name);
 +error_setg_errno(errp, -ret, Failed to find VDI snapshot '%s', 
 vdi);

Isn't snapid what identifies the snapshot? Or maybe the combination of
vdi and snapid.

  goto out;
  }
  
  fd = connect_to_sdog(s);
  if (fd  0) {
 -ret = fd;
 +error_setg_errno(errp, -fd, Failed to connect to sdog);
  goto out;
  }
  
 @@ -1909,14 +1911,14 @@ static int sd_snapshot_goto(BlockDriverState *bs, 
 const char *snapshot_id)
  closesocket(fd);
  
  if (ret) {
 +error_setg_errno(errp, -ret, Failed to open VDI snapshot '%s', 
 vdi);
  goto out;
  }
  
  memcpy(s-inode, buf, sizeof(s-inode));
  
  if (!s-inode.vm_state_size) {
 -error_report(Invalid snapshot);
 -ret = -ENOENT;
 +error_setg(errp, Invalid snapshot '%s', snapshot_id);

Here as well.

  goto out;
  }
  
 @@ -1925,16 +1927,12 @@ static int sd_snapshot_goto(BlockDriverState *bs, 
 const char *snapshot_id)
  g_free(buf);
  g_free(old_s);
  
 -return 0;
 +return;
  out:
  /* recover bdrv_sd_state */
  memcpy(s, old_s, sizeof(BDRVSheepdogState));
  g_free(buf);
  g_free(old_s);
 -
 -error_report(failed to open. recover old bdrv_sd_state.);
 -
 -return ret;
  }
  
  static void sd_snapshot_delete(BlockDriverState *bs,

 --- a/savevm.c
 +++ b/savevm.c
 @@ -2529,11 +2529,11 @@ int load_vmstate(const char *name)
  bs = NULL;
  while ((bs = bdrv_next(bs))) {
  if (bdrv_can_snapshot(bs)) {
 -ret = bdrv_snapshot_goto(bs, name);
 -if (ret  0) {
 -error_report(Error %d while activating snapshot '%s' on 
 '%s',
 - ret, name, bdrv_get_device_name(bs));
 -return ret;
 +bdrv_snapshot_goto(bs, name, local_err);
 +if (error_is_set(local_err)) {
 +qerror_report_err(local_err);

Shouldn't we add the device name on which the failure occurred?

 +error_free(local_err);
 +return -EIO;
  }
  }
  }

Kevin

Re: [Qemu-devel] [PATCH v2 05/12] block: update error reporting for bdrv_snapshot_goto() and related functions

2013-04-25 Thread Eric Blake 
On 04/24/2013 09:32 AM, Pavel Hrdina wrote:
 Signed-off-by: Pavel Hrdina phrd...@redhat.com
 ---
  block.c   | 34 ++
  block/qcow2-snapshot.c| 24 +---
  block/qcow2.h |  4 +++-
  block/rbd.c   | 10 +++---
  block/sheepdog.c  | 18 --
  include/block/block.h |  5 +++--
  include/block/block_int.h |  5 +++--
  qemu-img.c|  7 ++-
  savevm.c  | 10 +-
  9 files changed, 66 insertions(+), 51 deletions(-)

Reviewed-by: Eric Blake ebl...@redhat.com

-- 
Eric Blake   eblake redhat com+1-919-301-3266
Libvirt virtualization library http://libvirt.org



signature.asc
Description: OpenPGP digital signature

[Qemu-devel] [PATCH v2 05/12] block: update error reporting for bdrv_snapshot_goto() and related functions

2013-04-24 Thread Pavel Hrdina 
Signed-off-by: Pavel Hrdina phrd...@redhat.com
---
 block.c   | 34 ++
 block/qcow2-snapshot.c| 24 +---
 block/qcow2.h |  4 +++-
 block/rbd.c   | 10 +++---
 block/sheepdog.c  | 18 --
 include/block/block.h |  5 +++--
 include/block/block_int.h |  5 +++--
 qemu-img.c|  7 ++-
 savevm.c  | 10 +-
 9 files changed, 66 insertions(+), 51 deletions(-)

diff --git a/block.c b/block.c
index 3f7ee38..c36d3d5 100644
--- a/block.c
+++ b/block.c
@@ -3412,30 +3412,32 @@ int bdrv_snapshot_create(BlockDriverState *bs,
 return -ENOTSUP;
 }
 
-int bdrv_snapshot_goto(BlockDriverState *bs,
-   const char *snapshot_id)
+void bdrv_snapshot_goto(BlockDriverState *bs,
+const char *snapshot_id,
+Error **errp)
 {
 BlockDriver *drv = bs-drv;
-int ret, open_ret;
-
-if (!drv)
-return -ENOMEDIUM;
-if (drv-bdrv_snapshot_goto)
-return drv-bdrv_snapshot_goto(bs, snapshot_id);
+int ret;
 
-if (bs-file) {
+if (!drv) {
+error_setg(errp, Device has no medium);
+} else if (drv-bdrv_snapshot_goto) {
+drv-bdrv_snapshot_goto(bs, snapshot_id, errp);
+} else if (bs-file) {
 drv-bdrv_close(bs);
-ret = bdrv_snapshot_goto(bs-file, snapshot_id);
-open_ret = drv-bdrv_open(bs, NULL, bs-open_flags);
-if (open_ret  0) {
+bdrv_snapshot_goto(bs-file, snapshot_id, errp);
+ret = drv-bdrv_open(bs, NULL, bs-open_flags);
+if (ret  0) {
 bdrv_delete(bs-file);
 bs-drv = NULL;
-return open_ret;
+if (!error_is_set(errp)) {
+error_setg_errno(errp, -ret, Failed to open '%s',
+ bdrv_get_device_name(bs));
+}
 }
-return ret;
+} else {
+error_setg(errp, Snapshots are not supported);
 }
-
-return -ENOTSUP;
 }
 
 void bdrv_snapshot_delete(BlockDriverState *bs,
diff --git a/block/qcow2-snapshot.c b/block/qcow2-snapshot.c
index fd0e231..67a57fc 100644
--- a/block/qcow2-snapshot.c
+++ b/block/qcow2-snapshot.c
@@ -417,7 +417,9 @@ fail:
 }
 
 /* copy the snapshot 'snapshot_name' into the current disk image */
-int qcow2_snapshot_goto(BlockDriverState *bs, const char *snapshot_id)
+void qcow2_snapshot_goto(BlockDriverState *bs,
+ const char *snapshot_id,
+ Error **errp)
 {
 BDRVQcowState *s = bs-opaque;
 QCowSnapshot *sn;
@@ -429,14 +431,15 @@ int qcow2_snapshot_goto(BlockDriverState *bs, const char 
*snapshot_id)
 /* Search the snapshot */
 snapshot_index = find_snapshot_by_id_or_name(bs, snapshot_id);
 if (snapshot_index  0) {
-return -ENOENT;
+error_setg(errp, Failed to find snapshot '%s' by id or name,
+   snapshot_id);
+return;
 }
 sn = s-snapshots[snapshot_index];
 
 if (sn-disk_size != bs-total_sectors * BDRV_SECTOR_SIZE) {
-error_report(qcow2: Loading snapshots with different disk 
-size is not implemented);
-ret = -ENOTSUP;
+error_setg(errp, Loading snapshots with different disk size is not 
+   implemented);
 goto fail;
 }
 
@@ -447,6 +450,7 @@ int qcow2_snapshot_goto(BlockDriverState *bs, const char 
*snapshot_id)
  */
 ret = qcow2_grow_l1_table(bs, sn-l1_size, true);
 if (ret  0) {
+error_setg_errno(errp, -ret, Failed to grow L1 table);
 goto fail;
 }
 
@@ -465,18 +469,22 @@ int qcow2_snapshot_goto(BlockDriverState *bs, const char 
*snapshot_id)
 
 ret = bdrv_pread(bs-file, sn-l1_table_offset, sn_l1_table, sn_l1_bytes);
 if (ret  0) {
+error_setg_errno(errp, -ret, Failed to load data into L1 table);
 goto fail;
 }
 
 ret = qcow2_update_snapshot_refcount(bs, sn-l1_table_offset,
  sn-l1_size, 1);
 if (ret  0) {
+error_setg_errno(errp, -ret, Failed to increase the cluster refcounts 

+ of the snapshot to load);
 goto fail;
 }
 
 ret = bdrv_pwrite_sync(bs-file, s-l1_table_offset, sn_l1_table,
cur_l1_bytes);
 if (ret  0) {
+error_setg_errno(errp, -ret, Failed to save L1 table);
 goto fail;
 }
 
@@ -502,6 +510,8 @@ int qcow2_snapshot_goto(BlockDriverState *bs, const char 
*snapshot_id)
 }
 
 if (ret  0) {
+error_setg_errno(errp, -ret, Failed to decrease the cluster refcounts 

+ of the old snapshot);
 goto fail;
 }
 
@@ -514,6 +524,7 @@ int qcow2_snapshot_goto(BlockDriverState *bs, const char 
*snapshot_id)
  */
 ret = qcow2_update_snapshot_refcount(bs, s-l1_table_offset, s-l1_size, 
0);
 if (ret  0) {
+