Re: [Qemu-devel] [PATCH V9 4/8] qcow2: return int for qcow2_free_clusters()

[Wenchao Xia]

于 2014/1/12 7:59, Max Reitz 写道:

On 05.01.2014 20:43, Wenchao Xia wrote:

The return value can help caller check whether error happens,
and it does not need to have *errp since the return value already tips
what happend.

Signed-off-by: Wenchao Xia xiaw...@linux.vnet.ibm.com
---
  block/qcow2-refcount.c |8 +---
  block/qcow2.h  |6 +++---
  2 files changed, 8 insertions(+), 6 deletions(-)


I'm not sure if we actually need this since it's never really bad to
have an error occur in qcow2_free_clusters(); at least, there's nothing
the caller can do about that and it never blocks any subsequent
operation, so most callers just don't care. But it won't hurt, either, so:

Reviewed-by: Max Reitz mre...@redhat.com


  In snapshot, L1 table - snapshot_list, if snapshot_list cluster
is not freed successfully, L1 talble should be kepted to avoid dangling
pointer, So I added this patch to enable the check.

Re: [Qemu-devel] [PATCH V9 4/8] qcow2: return int for qcow2_free_clusters()

[Max Reitz]

On 05.01.2014 20:43, Wenchao Xia wrote:

The return value can help caller check whether error happens,
and it does not need to have *errp since the return value already tips
what happend.

Signed-off-by: Wenchao Xia xiaw...@linux.vnet.ibm.com
---
  block/qcow2-refcount.c |8 +---
  block/qcow2.h  |6 +++---
  2 files changed, 8 insertions(+), 6 deletions(-)


I'm not sure if we actually need this since it's never really bad to 
have an error occur in qcow2_free_clusters(); at least, there's nothing 
the caller can do about that and it never blocks any subsequent 
operation, so most callers just don't care. But it won't hurt, either, so:


Reviewed-by: Max Reitz mre...@redhat.com

[Qemu-devel] [PATCH V9 4/8] qcow2: return int for qcow2_free_clusters()

[Wenchao Xia]

The return value can help caller check whether error happens,
and it does not need to have *errp since the return value already tips
what happend.

Signed-off-by: Wenchao Xia xiaw...@linux.vnet.ibm.com
---
 block/qcow2-refcount.c |8 +---
 block/qcow2.h  |6 +++---
 2 files changed, 8 insertions(+), 6 deletions(-)

diff --git a/block/qcow2-refcount.c b/block/qcow2-refcount.c
index c974abe..4eb413a 100644
--- a/block/qcow2-refcount.c
+++ b/block/qcow2-refcount.c
@@ -761,9 +761,9 @@ int64_t qcow2_alloc_bytes(BlockDriverState *bs, int size)
 return offset;
 }
 
-void qcow2_free_clusters(BlockDriverState *bs,
-  int64_t offset, int64_t size,
-  enum qcow2_discard_type type)
+int qcow2_free_clusters(BlockDriverState *bs,
+int64_t offset, int64_t size,
+enum qcow2_discard_type type)
 {
 int ret;
 
@@ -773,6 +773,8 @@ void qcow2_free_clusters(BlockDriverState *bs,
 fprintf(stderr, qcow2_free_clusters failed: %s\n, strerror(-ret));
 /* TODO Remember the clusters to free them later and avoid leaking */
 }
+
+return ret;
 }
 
 /*
diff --git a/block/qcow2.h b/block/qcow2.h
index c56a5b6..161549a 100644
--- a/block/qcow2.h
+++ b/block/qcow2.h
@@ -435,9 +435,9 @@ int64_t qcow2_alloc_clusters(BlockDriverState *bs, int64_t 
size);
 int qcow2_alloc_clusters_at(BlockDriverState *bs, uint64_t offset,
 int nb_clusters);
 int64_t qcow2_alloc_bytes(BlockDriverState *bs, int size);
-void qcow2_free_clusters(BlockDriverState *bs,
-  int64_t offset, int64_t size,
-  enum qcow2_discard_type type);
+int qcow2_free_clusters(BlockDriverState *bs,
+int64_t offset, int64_t size,
+enum qcow2_discard_type type);
 void qcow2_free_any_clusters(BlockDriverState *bs, uint64_t l2_entry,
  int nb_clusters, enum qcow2_discard_type type);
 
-- 
1.7.1