On 2014-11-20 at 22:43, Eric Blake wrote:
On 11/20/2014 10:06 AM, Max Reitz wrote:
Add a helper function for reallocating a refcount array, independent of
the refcount order. The newly allocated space is zeroed and the function
handles failed reallocations gracefully.
The helper function will always align the buffer size to a cluster
boundary; if storing the refcounts in such an array in big endian byte
order, this makes it possible to write parts of the array directly as
refcount blocks into the image file.
Signed-off-by: Max Reitz <mre...@redhat.com>
Reviewed-by: Eric Blake <ebl...@redhat.com>
Perhaps the changes since v2 warranted removing my earlier R-b to make
sure I review closely?
Well, normally I would not have taken the R-b. But you explicitly wrote:
Code looks correct as written, whether or not you also add more
comments, asserts, and/or shortcuts for no-op situations.
So I took that to mean "You may change the commit message as proposed
(s/independently/independent/ and add a note about the cluster
alignment), add a comment to realloc_refcount_array() about its calling
contract, an assert(new_byte_size > 0) and/or an early-out for when the
byte size of the refcount array does not increase", which is pretty
broad, but that's what you wrote. That's why I kept the R-b in.
---
block/qcow2-refcount.c | 135 +++++++++++++++++++++++++++++++------------------
1 file changed, 86 insertions(+), 49 deletions(-)
+static int realloc_refcount_array(BDRVQcowState *s, uint16_t **array,
+ int64_t *size, int64_t new_size)
+{
+ /* Round to clusters so the array can be directly written to disk */
+ size_t old_byte_size = ROUND_UP(refcount_array_byte_size(s, *size),
+ s->cluster_size);
+ size_t new_byte_size = ROUND_UP(refcount_array_byte_size(s, new_size),
+ s->cluster_size);
+ uint16_t *new_ptr;
+
+ if (new_byte_size <= old_byte_size) {
+ *size = new_size;
+ return 0;
+ }
+
+ assert(new_byte_size > 0);
Should this assert be moved before the early exit?
Could do, but it should not matter. It's important that new_byte_size >
0 after the early-out; the caller should not use new_size == 0 at all,
but I'd rather not add an assertion against a contract breach somewhere
where the code can handle that breach actually just fine.
Hmm, that's my only finding, and you did incorporate improvements mostly
to comments or asserts as compared to v2. Moving the assert is small
enough, and not a show-stopper if you leave it in place, so:
Reviewed-by: Eric Blake <ebl...@redhat.com>
Phew :-)
Thanks!
Max
