| Message ID | 1388950991-30105-5-git-send-email-xiawenc@linux.vnet.ibm.com |
|---|---|
| State | New |
| Headers | show |
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>
δΊ 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.
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);
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(-)