[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v2] qcow2: Rewrite qcow2_alloc_bytes()
From: |
Eric Blake |
Subject: |
Re: [Qemu-devel] [PATCH v2] qcow2: Rewrite qcow2_alloc_bytes() |
Date: |
Thu, 05 Feb 2015 10:53:22 -0700 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.4.0 |
On 02/05/2015 08:58 AM, Max Reitz wrote:
> qcow2_alloc_bytes() is a function with insufficient error handling and
> an unnecessary goto. This patch rewrites it.
>
> Signed-off-by: Max Reitz <address@hidden>
> ---
> v2:
> - s/free_cluster_index/free_byte_index/ [Eric]
> - added an assertion at the start of the function that
> s->free_byte_offset is either 0 or points to the tail of a cluster
> (but never to the start)
> - use ROUND_UP() instead of start_of_cluster() + cluster_size [Eric]
> - added an assertion that s->free_byte_offset is set before using it
> [Eric]
Looks nicer.
> + cluster_end = ROUND_UP(s->free_byte_offset, s->cluster_size);
> + if (!s->free_byte_offset || cluster_end != new_cluster) {
Here, I can prove that the left side of the || makes no difference to
the outcome (using just the right half is sufficient, since
!s->free_byte_offset implies cluster_end == 0, but new_cluster will be
non-zero because we never allocate the header cluster). I'd be fine if
you wanted to respin that and add the comment for the micro-optimized
shorter code; but for maintenance purposes, it's easier to rely on the
explicit condition (even if redundant) than to think about whether a
proof listed in a comment is correct, so I'm also fine leaving things as is.
> + s->free_byte_offset = new_cluster;
At this point, s->free_byte_offset violates the precondition if we just
allocated a cluster, so we have to make sure that we restore the
precondition before exiting...
> + if (offset_into_cluster(s, s->free_byte_offset)) {
> + int ret = qcow2_update_cluster_refcount(bs,
> + s->free_byte_offset >> s->cluster_bits, 1,
> + QCOW2_DISCARD_NEVER);
> + if (ret < 0) {
> + if (new_cluster > 0) {
> + qcow2_free_clusters(bs, new_cluster, s->cluster_size,
> + QCOW2_DISCARD_OTHER);
> + }
> + return ret;
...this early exit only happens if s->free_byte_offset was pointing to a
tail, and there are no other early exits, with the final exit properly
restoring the precondition.
Reviewed-by: Eric Blake <address@hidden>
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
signature.asc
Description: OpenPGP digital signature