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 

> --- 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 
> ---
>  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 

-- 
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 
---
 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,