On 05/05/2017 03:51 PM, Max Reitz wrote: > On 04.05.2017 05:07, Eric Blake wrote: >> Treat plain zero clusters differently from allocated ones, so that >> we can simplify the logic of checking whether an offset is present. >> Do this by splitting QCOW2_CLUSTER_ZERO into two new enums, >> QCOW2_CLUSTER_ZERO_PLAIN and QCOW2_CLUSTER_ZERO_ALLOC. >> >> I tried to arrange the enum so that we could use >> 'ret <= QCOW2_CLUSTER_ZERO_PLAIN' for all unallocated types, and >> 'ret >= QCOW2_CLUSTER_ZERO_ALLOC' for allocated types, although >> I didn't actually end up taking advantage of the layout. >> >> In many cases, this leads to simpler code, by properly combining >> cases (sometimes, both zero types pair together, other times, >> plain zero is more like unallocated while allocated zero is more >> like normal). >> >> Signed-off-by: Eric Blake <ebl...@redhat.com> >>
>> @@ -558,52 +557,32 @@ int qcow2_get_cluster_offset(BlockDriverState *bs,
>> uint64_t offset,
>> assert(nb_clusters <= INT_MAX);
>>
>> ret = qcow2_get_cluster_type(*cluster_offset);
>> + if (s->qcow_version < 3 && (ret == QCOW2_CLUSTER_ZERO_PLAIN ||
>> + ret == QCOW2_CLUSTER_ZERO_ALLOC)) {
>> + qcow2_signal_corruption(bs, true, -1, -1, "Zero cluster entry found"
>> + " in pre-v3 image (L2 offset: %#" PRIx64
>> + ", L2 index: %#x)", l2_offset, l2_index);
>> + ret = -EIO;
>> + goto fail;
>> + }
...
>> + case QCOW2_CLUSTER_ZERO_PLAIN:
>> case QCOW2_CLUSTER_UNALLOCATED:
>> /* how many empty clusters ? */
>> c = count_contiguous_clusters_unallocated(nb_clusters,
>> - &l2_table[l2_index],
>> -
>> QCOW2_CLUSTER_UNALLOCATED);
>> + &l2_table[l2_index], ret);
>
> Nit pick: Using ret here is a bit weird (because it's such a meaningless
> name). It would be good if we had a separate cluster_type variable.
qcow2_get_cluster_offset() returns the cluster type on success, and
-errno on failure. So 'ret' actually makes some sense: it really is the
value we are about to return. But it may also work to have a separate
variable up front, then assign ret = cluster_type at the end; I'll play
with it and see which one looks better.
>
>> *cluster_offset = 0;
>> break;
>> + case QCOW2_CLUSTER_ZERO_ALLOC:
>> case QCOW2_CLUSTER_NORMAL:
>> /* how many allocated clusters ? */
>> c = count_contiguous_clusters(nb_clusters, s->cluster_size,
>> - &l2_table[l2_index], QCOW_OFLAG_ZERO);
>> + &l2_table[l2_index], QCOW_OFLAG_ZERO);
>> *cluster_offset &= L2E_OFFSET_MASK;
>> if (offset_into_cluster(s, *cluster_offset)) {
>> qcow2_signal_corruption(bs, true, -1, -1, "Data cluster offset
>> %#"
>
> Well, preallocated zero clusters are not exactly data clusters... Not
> that any user cared, but s/Data cluster/Cluster allocation/ would be
> more correct.
Good idea.
>
> By the way, allow me to state just how much I love this hunk: Very much.
> Looks great! It gets a place on my list of favorite hunks of this year
> at least.
>
> [...]
>
>> @@ -1760,7 +1740,8 @@ static int expand_zero_clusters_in_l1(BlockDriverState
>> *bs, uint64_t *l1_table,
>> int cluster_type = qcow2_get_cluster_type(l2_entry);>
>> bool preallocated = offset != 0;
>
> I could get behind removing this variable and replacing all
> "if (!preallocated)" instances by
> "if (cluster_type == QCOW2_CLUSTER_ZERO_PLAIN)". Up to you, though.
Makes sense.
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org
signature.asc
Description: OpenPGP digital signature
