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

2013-05-03 Thread Kevin Wolf 
Am 24.04.2013 um 17:32 hat Pavel Hrdina geschrieben:
 @@ -2528,15 +2530,13 @@ void do_delvm(Monitor *mon, const QDict *qdict)
  bs1 = NULL;
  while ((bs1 = bdrv_next(bs1))) {
  if (bdrv_can_snapshot(bs1)) {
 -ret = bdrv_snapshot_delete(bs1, name);
 -if (ret  0) {
 -if (ret == -ENOTSUP)
 -monitor_printf(mon,
 -   Snapshots not supported on device 
 '%s'\n,
 -   bdrv_get_device_name(bs1));
 -else
 -monitor_printf(mon, Error %d while deleting snapshot on 
 
 -   '%s'\n, ret, bdrv_get_device_name(bs1));
 +bdrv_snapshot_delete(bs1, name, local_err);
 +if (error_is_set(local_err)) {
 +qerror_report(ERROR_CLASS_GENERIC_ERROR, Failed to delete 
 +  old snapshot on device '%s': %s,

Here in do_delvm() it doesn't make sense to talk about an old snapshot.
Probably some unchanged copy  paste from above?

 +  bdrv_get_device_name(bs),
 +  error_get_pretty(local_err));
 +error_free(local_err);
  }
  }
  }

Kevin

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

2013-04-25 Thread Stefan Hajnoczi 
On Wed, Apr 24, 2013 at 05:32:00PM +0200, Pavel Hrdina wrote:
 diff --git a/block/sheepdog.c b/block/sheepdog.c
 index 20b5d06..7e0610f 100644
 --- a/block/sheepdog.c
 +++ b/block/sheepdog.c
 @@ -1937,10 +1937,12 @@ out:
  return ret;
  }
  
 -static int sd_snapshot_delete(BlockDriverState *bs, const char *snapshot_id)
 +static void sd_snapshot_delete(BlockDriverState *bs,
 +   const char *snapshot_id,
 +   Error **errp)
  {
  /* FIXME: Delete specified snapshot id.  */
 -return 0;
 +error_setg(errp, Deleting snapshot is not supported);
  }

Careful, this could break existing tests or applications that expect
snapshot delete to lie.  I suggest *not* setting the error for now,
unless Kazutaka agrees it's okay to start erroring now.

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

2013-04-24 Thread Pavel Hrdina 
Signed-off-by: Pavel Hrdina phrd...@redhat.com
---
 block.c   | 21 +
 block/qcow2-snapshot.c| 19 +--
 block/qcow2.h |  4 +++-
 block/rbd.c   | 10 +++---
 block/sheepdog.c  |  6 --
 include/block/block.h |  4 +++-
 include/block/block_int.h |  4 +++-
 qemu-img.c|  8 +++-
 savevm.c  | 32 
 9 files changed, 65 insertions(+), 43 deletions(-)

diff --git a/block.c b/block.c
index aa9a533..3f7ee38 100644
--- a/block.c
+++ b/block.c
@@ -3438,16 +3438,21 @@ int bdrv_snapshot_goto(BlockDriverState *bs,
 return -ENOTSUP;
 }
 
-int bdrv_snapshot_delete(BlockDriverState *bs, const char *snapshot_id)
+void bdrv_snapshot_delete(BlockDriverState *bs,
+  const char *snapshot_id,
+  Error **errp)
 {
 BlockDriver *drv = bs-drv;
-if (!drv)
-return -ENOMEDIUM;
-if (drv-bdrv_snapshot_delete)
-return drv-bdrv_snapshot_delete(bs, snapshot_id);
-if (bs-file)
-return bdrv_snapshot_delete(bs-file, snapshot_id);
-return -ENOTSUP;
+
+if (!drv) {
+error_setg(errp, Device has no medium);
+} else if (drv-bdrv_snapshot_delete) {
+drv-bdrv_snapshot_delete(bs, snapshot_id, errp);
+} else if (bs-file) {
+bdrv_snapshot_delete(bs-file, snapshot_id, errp);
+} else {
+error_setg(errp, Snapshots are not supported);
+}
 }
 
 int bdrv_snapshot_list(BlockDriverState *bs,
diff --git a/block/qcow2-snapshot.c b/block/qcow2-snapshot.c
index 992a5c8..fd0e231 100644
--- a/block/qcow2-snapshot.c
+++ b/block/qcow2-snapshot.c
@@ -530,7 +530,9 @@ fail:
 return ret;
 }
 
-int qcow2_snapshot_delete(BlockDriverState *bs, const char *snapshot_id)
+void qcow2_snapshot_delete(BlockDriverState *bs,
+   const char *snapshot_id,
+   Error **errp)
 {
 BDRVQcowState *s = bs-opaque;
 QCowSnapshot sn;
@@ -539,7 +541,9 @@ int qcow2_snapshot_delete(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];
 
@@ -550,7 +554,9 @@ int qcow2_snapshot_delete(BlockDriverState *bs, const char 
*snapshot_id)
 s-nb_snapshots--;
 ret = qcow2_write_snapshots(bs);
 if (ret  0) {
-return ret;
+error_setg_errno(errp, -ret, Failed to remove snapshot '%s' from 
+ snapshot list, sn.name);
+return;
 }
 
 /*
@@ -567,14 +573,16 @@ int qcow2_snapshot_delete(BlockDriverState *bs, const 
char *snapshot_id)
 ret = qcow2_update_snapshot_refcount(bs, sn.l1_table_offset,
  sn.l1_size, -1);
 if (ret  0) {
-return ret;
+error_setg_errno(errp, -ret, Failed to update refcounts);
+return;
 }
 qcow2_free_clusters(bs, sn.l1_table_offset, sn.l1_size * sizeof(uint64_t));
 
 /* must update the copied flag on the current cluster offsets */
 ret = qcow2_update_snapshot_refcount(bs, s-l1_table_offset, s-l1_size, 
0);
 if (ret  0) {
-return ret;
+error_setg_errno(errp, -ret, Failed to update cluster flags);
+return;
 }
 
 #ifdef DEBUG_ALLOC
@@ -583,7 +591,6 @@ int qcow2_snapshot_delete(BlockDriverState *bs, const char 
*snapshot_id)
 qcow2_check_refcounts(bs, result, 0);
 }
 #endif
-return 0;
 }
 
 int qcow2_snapshot_list(BlockDriverState *bs, QEMUSnapshotInfo **psn_tab)
diff --git a/block/qcow2.h b/block/qcow2.h
index 9421843..dbd332d 100644
--- a/block/qcow2.h
+++ b/block/qcow2.h
@@ -384,7 +384,9 @@ int qcow2_zero_clusters(BlockDriverState *bs, uint64_t 
offset, int nb_sectors);
 /* qcow2-snapshot.c functions */
 int qcow2_snapshot_create(BlockDriverState *bs, QEMUSnapshotInfo *sn_info);
 int qcow2_snapshot_goto(BlockDriverState *bs, const char *snapshot_id);
-int qcow2_snapshot_delete(BlockDriverState *bs, const char *snapshot_id);
+void qcow2_snapshot_delete(BlockDriverState *bs,
+   const char *snapshot_id,
+   Error **errp);
 int qcow2_snapshot_list(BlockDriverState *bs, QEMUSnapshotInfo **psn_tab);
 int qcow2_snapshot_load_tmp(BlockDriverState *bs, const char *snapshot_name);
 
diff --git a/block/rbd.c b/block/rbd.c
index 1826411..1e54c55 100644
--- a/block/rbd.c
+++ b/block/rbd.c
@@ -899,14 +899,18 @@ static int qemu_rbd_snap_create(BlockDriverState *bs,
 return 0;
 }
 
-static int qemu_rbd_snap_remove(BlockDriverState *bs,
-const char *snapshot_name)
+static void qemu_rbd_snap_remove(BlockDriverState *bs,
+ const

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

2013-04-24 Thread Eric Blake 
On 04/24/2013 09:32 AM, Pavel Hrdina wrote:
 Signed-off-by: Pavel Hrdina phrd...@redhat.com
 ---
  block.c   | 21 +
  block/qcow2-snapshot.c| 19 +--
  block/qcow2.h |  4 +++-
  block/rbd.c   | 10 +++---
  block/sheepdog.c  |  6 --
  include/block/block.h |  4 +++-
  include/block/block_int.h |  4 +++-
  qemu-img.c|  8 +++-
  savevm.c  | 32 
  9 files changed, 65 insertions(+), 43 deletions(-)
 

 +++ b/block/sheepdog.c
 @@ -1937,10 +1937,12 @@ out:
  return ret;
  }
  
 -static int sd_snapshot_delete(BlockDriverState *bs, const char *snapshot_id)
 +static void sd_snapshot_delete(BlockDriverState *bs,
 +   const char *snapshot_id,
 +   Error **errp)
  {
  /* FIXME: Delete specified snapshot id.  */
 -return 0;
 +error_setg(errp, Deleting snapshot is not supported);

Commit message should mention this bug fix.

But the code looked okay to me, so:

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

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

2013-04-24 Thread Wenchao Xia 
  bdrv_snapshot_delete() do not return error number now, and I checked
original caller code, they used it only for error reporting, so it is
safe to change.

Reviewed-by: Wenchao Xia xiaw...@linux.vnet.ibm.com


 Signed-off-by: Pavel Hrdina phrd...@redhat.com
 ---
   block.c   | 21 +
   block/qcow2-snapshot.c| 19 +--
   block/qcow2.h |  4 +++-
   block/rbd.c   | 10 +++---
   block/sheepdog.c  |  6 --
   include/block/block.h |  4 +++-
   include/block/block_int.h |  4 +++-
   qemu-img.c|  8 +++-
   savevm.c  | 32 
   9 files changed, 65 insertions(+), 43 deletions(-)
 
 diff --git a/block.c b/block.c
 index aa9a533..3f7ee38 100644
 --- a/block.c
 +++ b/block.c
 @@ -3438,16 +3438,21 @@ int bdrv_snapshot_goto(BlockDriverState *bs,
   return -ENOTSUP;
   }
 
 -int bdrv_snapshot_delete(BlockDriverState *bs, const char *snapshot_id)
 +void bdrv_snapshot_delete(BlockDriverState *bs,
 +  const char *snapshot_id,
 +  Error **errp)
   {
   BlockDriver *drv = bs-drv;
 -if (!drv)
 -return -ENOMEDIUM;
 -if (drv-bdrv_snapshot_delete)
 -return drv-bdrv_snapshot_delete(bs, snapshot_id);
 -if (bs-file)
 -return bdrv_snapshot_delete(bs-file, snapshot_id);
 -return -ENOTSUP;
 +
 +if (!drv) {
 +error_setg(errp, Device has no medium);
 +} else if (drv-bdrv_snapshot_delete) {
 +drv-bdrv_snapshot_delete(bs, snapshot_id, errp);
 +} else if (bs-file) {
 +bdrv_snapshot_delete(bs-file, snapshot_id, errp);
 +} else {
 +error_setg(errp, Snapshots are not supported);
 +}
   }
 
   int bdrv_snapshot_list(BlockDriverState *bs,
 diff --git a/block/qcow2-snapshot.c b/block/qcow2-snapshot.c
 index 992a5c8..fd0e231 100644
 --- a/block/qcow2-snapshot.c
 +++ b/block/qcow2-snapshot.c
 @@ -530,7 +530,9 @@ fail:
   return ret;
   }
 
 -int qcow2_snapshot_delete(BlockDriverState *bs, const char *snapshot_id)
 +void qcow2_snapshot_delete(BlockDriverState *bs,
 +   const char *snapshot_id,
 +   Error **errp)
   {
   BDRVQcowState *s = bs-opaque;
   QCowSnapshot sn;
 @@ -539,7 +541,9 @@ int qcow2_snapshot_delete(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];
 
 @@ -550,7 +554,9 @@ int qcow2_snapshot_delete(BlockDriverState *bs, const 
 char *snapshot_id)
   s-nb_snapshots--;
   ret = qcow2_write_snapshots(bs);
   if (ret  0) {
 -return ret;
 +error_setg_errno(errp, -ret, Failed to remove snapshot '%s' from 
 + snapshot list, sn.name);
 +return;
   }
 
   /*
 @@ -567,14 +573,16 @@ int qcow2_snapshot_delete(BlockDriverState *bs, const 
 char *snapshot_id)
   ret = qcow2_update_snapshot_refcount(bs, sn.l1_table_offset,
sn.l1_size, -1);
   if (ret  0) {
 -return ret;
 +error_setg_errno(errp, -ret, Failed to update refcounts);
 +return;
   }
   qcow2_free_clusters(bs, sn.l1_table_offset, sn.l1_size * 
 sizeof(uint64_t));
 
   /* must update the copied flag on the current cluster offsets */
   ret = qcow2_update_snapshot_refcount(bs, s-l1_table_offset, 
 s-l1_size, 0);
   if (ret  0) {
 -return ret;
 +error_setg_errno(errp, -ret, Failed to update cluster flags);
 +return;
   }
 
   #ifdef DEBUG_ALLOC
 @@ -583,7 +591,6 @@ int qcow2_snapshot_delete(BlockDriverState *bs, const 
 char *snapshot_id)
   qcow2_check_refcounts(bs, result, 0);
   }
   #endif
 -return 0;
   }
 
   int qcow2_snapshot_list(BlockDriverState *bs, QEMUSnapshotInfo **psn_tab)
 diff --git a/block/qcow2.h b/block/qcow2.h
 index 9421843..dbd332d 100644
 --- a/block/qcow2.h
 +++ b/block/qcow2.h
 @@ -384,7 +384,9 @@ int qcow2_zero_clusters(BlockDriverState *bs, uint64_t 
 offset, int nb_sectors);
   /* qcow2-snapshot.c functions */
   int qcow2_snapshot_create(BlockDriverState *bs, QEMUSnapshotInfo *sn_info);
   int qcow2_snapshot_goto(BlockDriverState *bs, const char *snapshot_id);
 -int qcow2_snapshot_delete(BlockDriverState *bs, const char *snapshot_id);
 +void qcow2_snapshot_delete(BlockDriverState *bs,
 +   const char *snapshot_id,
 +   Error **errp);
   int qcow2_snapshot_list(BlockDriverState *bs, QEMUSnapshotInfo **psn_tab);
   int qcow2_snapshot_load_tmp(BlockDriverState *bs, const char 
 *snapshot_name);
 
 diff --git a/block/rbd.c