Patchwork [V9,4/8] qcow2: return int for qcow2_free_clusters()

login
register
mail settings
Submitter Wayne Xia
Date Jan. 5, 2014, 7:43 p.m.
Message ID <1388950991-30105-5-git-send-email-xiawenc@linux.vnet.ibm.com>
Download mbox | patch
Permalink /patch/307086/
State New
Headers show

Comments

Wayne Xia - Jan. 5, 2014, 7:43 p.m.
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 <xiawenc@linux.vnet.ibm.com>
---
 block/qcow2-refcount.c |    8 +++++---
 block/qcow2.h          |    6 +++---
 2 files changed, 8 insertions(+), 6 deletions(-)
Max Reitz - Jan. 11, 2014, 11:59 p.m.
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 <xiawenc@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 <mreitz@redhat.com>
Wayne Xia - Jan. 13, 2014, 2:11 a.m.
于 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 <xiawenc@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 <mreitz@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.

Patch

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